feat(condo): DOMA-13059 register external payments#7433
Open
vovaaxeapolla wants to merge 13 commits intomainfrom
Open
feat(condo): DOMA-13059 register external payments#7433vovaaxeapolla wants to merge 13 commits intomainfrom
vovaaxeapolla wants to merge 13 commits intomainfrom
Conversation
vovaaxeapolla
added
the
🔬 WIP
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new GraphQL mutation Changes
sequenceDiagram
actor Client
participant Mutation as RegisterExternalPayments<br/>Mutation
participant Access as Access<br/>Rule
participant DB as Database
participant Validator as Validation<br/>Logic
Client->>Mutation: registerExternalPayments(data)
Mutation->>Access: canRegisterExternalPayments(user, data)
Access->>DB: Load AcquiringIntegrationContext & Integration
DB-->>Access: Context + Integration
Access-->>Mutation: Allowed / Denied
alt Allowed
Mutation->>Validator: Validate dv, sender, count <= 100
Validator-->>Mutation: Valid
Mutation->>Validator: Check incoming duplicate transactionIds
Validator-->>Mutation: No duplicates
Mutation->>DB: Query existing MultiPayments for org/integration
DB-->>Mutation: Existing records (if any)
alt No conflicts
Mutation->>Validator: Validate currency, period, dates, amounts
Validator-->>Mutation: Valid
Mutation->>DB: Create Payment records (PAYMENT_DONE)
DB-->>Mutation: Payments created
Mutation->>DB: Create MultiPayment records (MULTIPAYMENT_DONE)
DB-->>Mutation: MultiPayments created
Mutation-->>Client: { status: 'ok' }
else Conflicts
Mutation-->>Client: DUPLICATED_PAYMENTS error
end
else Denied
Mutation-->>Client: Access / Authentication Error
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dkoviazin
reviewed
Apr 8, 2026
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js
Outdated
Show resolved
Hide resolved
vovaaxeapolla
added
✋🙂 Review please
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9048281e14
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js
Outdated
Show resolved
Hide resolved
| query: REGISTER_EXTERNAL_PAYMENTS_MUTATION, | ||
| variables: { data: { dv: 1, ...data } }, | ||
| errorMessage: '[error] Unable to registerExternalPayments', | ||
| dataPath: 'obj', |
There was a problem hiding this comment.
This helper asks execGqlWithoutAccess for dataPath: 'obj', but the mutation is aliased as result in REGISTER_EXTERNAL_PAYMENTS_MUTATION. Any caller of registerExternalPayments() will therefore receive undefined instead of the mutation payload, making the new server utility unusable.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js (1)
35-40:⚠️ Potential issue | 🟠 MajorGuard
toISOString()withisValid()before comparisonCalling
toISOString()directly on an invalid parsed date throws aRangeError, bypassing your domainGQLErrorpath. UseisValid()to validate before callingtoISOString().Proposed fix
- if (dayjs(transactionDate).toISOString() !== transactionDate) { + const parsedTransactionDate = dayjs(transactionDate) + if (!parsedTransactionDate.isValid() || parsedTransactionDate.toISOString() !== transactionDate) { throw new GQLError({ ...ERRORS.INVALID_DATE_FORMAT, messageInterpolation: { date: transactionDate, transactionId } }, context) } - if (depositedDate && dayjs(depositedDate).toISOString() !== depositedDate) { - throw new GQLError({ ...ERRORS.INVALID_DATE_FORMAT, messageInterpolation: { date: depositedDate, transactionId } }, context) + if (depositedDate) { + const parsedDepositedDate = dayjs(depositedDate) + if (!parsedDepositedDate.isValid() || parsedDepositedDate.toISOString() !== depositedDate) { + throw new GQLError({ ...ERRORS.INVALID_DATE_FORMAT, messageInterpolation: { date: depositedDate, transactionId } }, context) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js` around lines 35 - 40, The date validation currently calls dayjs(...).toISOString() which throws on invalid dates; update the checks in RegisterExternalPaymentsService to first call dayjs(transactionDate).isValid() and dayjs(depositedDate).isValid() respectively, and if invalid throw the existing GQLError({ ...ERRORS.INVALID_DATE_FORMAT, messageInterpolation: { date: <value>, transactionId } }, context); only call toISOString() and compare when isValid() returns true (use the same transactionDate, depositedDate variables and keep GQLError, ERRORS.INVALID_DATE_FORMAT and context intact).
🧹 Nitpick comments (4)
apps/condo/domains/acquiring/utils/testSchema/index.js (1)
655-655: Use the project’s function declaration spacing.At Line 655,
registerExternalPaymentsByTestClient(client, ...)should keep a space before(for consistency with repository rules.♻️ Proposed fix
-async function registerExternalPaymentsByTestClient(client, extraAttrs = {}) { +async function registerExternalPaymentsByTestClient (client, extraAttrs = {}) {As per coding guidelines: "Always require space before function parentheses: function example () instead of function example()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/utils/testSchema/index.js` at line 655, Function declaration spacing is inconsistent: update the declaration of registerExternalPaymentsByTestClient to include a space before the parameter list (i.e., change "function registerExternalPaymentsByTestClient(client, ..." to "function registerExternalPaymentsByTestClient (client, ...)") so it follows the project's "space before function parentheses" rule; locate the function by the name registerExternalPaymentsByTestClient and adjust the spacing in its declaration accordingly throughout the file.apps/condo/schema.graphql (1)
91965-91967: Prefer enum over free-formstatusstring in mutation output.
status: String!makes client handling brittle. Consider a GraphQL enum for a stricter, self-documented contract.💡 Suggested schema adjustment
+enum RegisterExternalPaymentsStatus { + OK + PARTIAL + FAILED +} + type RegisterExternalPaymentsOutput { - status: String! + status: RegisterExternalPaymentsStatus! }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/schema.graphql` around lines 91965 - 91967, The output type RegisterExternalPaymentsOutput uses a free-form status: String!, which should be a GraphQL enum for stricter typing; add a new enum (e.g., RegisterExternalPaymentsStatus with values like SUCCESS, FAILED, PENDING — choose canonical names your app uses) and change the field on RegisterExternalPaymentsOutput to status: RegisterExternalPaymentsStatus!; then update any resolver/serializer logic that constructs RegisterExternalPaymentsOutput to return enum values instead of strings and update client/typegen usages accordingly.apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js (1)
23-54: SplitvalidatePaymentsinto focused validatorsThis block is over the cognitive complexity threshold from Sonar. Extracting
validateCurrency,validatePeriod,validateDates, andvalidateAmountswill make this easier to maintain and less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js` around lines 23 - 54, Split validatePayments into smaller validator functions: extract validateCurrency(payments, context) to check ISO_CODES and throw the same GQLError with ERRORS.INVALID_CURRENCY_CODE; validatePeriod(payments, context) to validate period via dayjs('YYYY-MM-01', true) and throw ERRORS.INVALID_PERIOD_FORMAT; validateDates(payments, context) to validate transactionDate and optional depositedDate with dayjs().toISOString() and throw ERRORS.INVALID_DATE_FORMAT; and validateAmounts(payments, context) to parse amounts/fees with Big, rethrowing ERRORS.INVALID_PAYMENT_AMOUNT on parse errors and ERRORS.NEGATIVE_PAYMENT_AMOUNT when Big(amount).lte(0). Then simplify validatePayments to iterate payments and call these helpers (or have helpers accept a single payment) ensuring you preserve the same messageInterpolation (transactionId, amount, date, period) and context values and continue using the existing symbols Big, dayjs, ISO_CODES, and the ERRORS and GQLError usage.apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js (1)
148-148: AvoidArray.fill()with a single payment objectLine 148 repeats the same object reference (and
transactionId) for every element. This makes the test implicitly depend on validation order. Generate unique payments instead.Proposed fix
- payments: Array(PAYMENTS_LIMIT + 1).fill(getExternalPayment()), + payments: Array.from({ length: PAYMENTS_LIMIT + 1 }, () => getExternalPayment()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js` at line 148, The test currently uses Array(PAYMENTS_LIMIT + 1).fill(getExternalPayment()), which duplicates the same object (and transactionId) for every entry; replace this with a generator that creates distinct payment objects so each element has a unique transactionId. For example, build the array with Array.from({length: PAYMENTS_LIMIT + 1}, (_, i) => getExternalPayment(i)) or map over a range and ensure getExternalPayment (or a small wrapper) accepts the index and returns a fresh object/unique transactionId for each payment used in the payments field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.js`:
- Around line 13-20: The function canRegisterExternalPayments accesses
acquiringIntegrationContext.id without checking for null; add a defensive check
before calling getByCondition: verify acquiringIntegrationContext exists (and
ideally acquiringIntegrationContext.id is present) and return false (or throw a
controlled error) if missing, then call getByCondition with
acquiringIntegrationContext.id; update the null-check near the top of
canRegisterExternalPayments to prevent the TypeError when
args.data.acquiringIntegrationContext is undefined.
In `@apps/condo/domains/acquiring/constants/errors.js`:
- Around line 71-72: The two error constants MULTIPAYMENT_NON_DONE_PAYMENTS and
MULTIPAYMENT_SEVERAL_PAYMENTS are missing trailing periods and
MULTIPAYMENT_NON_DONE_PAYMENTS also has a missing closing quote after the
${PAYMENT_DONE_STATUS} interpolation; update the string templates to end with a
period and correct the interpolation to `"${PAYMENT_DONE_STATUS}"` while keeping
the ${ACQUIRING_INTEGRATION_EXTERNAL_IMPORT_TYPE} interpolation intact so both
messages are consistent with other error strings.
In `@apps/condo/domains/acquiring/schema/MultiPayment.js`:
- Line 283: Remove the debug console.log call that prints
externalImportIntegration and resolvedData in MultiPayment.js; specifically
delete the line "console.log(externalImportIntegration, 'resolvedData',
resolvedData)" (or replace it with a safe, non-sensitive logger call at an
appropriate level if you need retention) to avoid logging sensitive payment
information in production and ensure no other nearby code depends on that
side-effect.
In `@apps/condo/domains/acquiring/schema/MultiPayment.test.js`:
- Around line 355-361: The test "Should pass with valid data" uses
PAYMENT_DONE_STATUS when creating a MultiPayment via createTestMultiPayment;
replace that with the MultiPayment-specific constant MULTIPAYMENT_DONE_STATUS in
the payload to avoid coupling to payment status semantics—update the
createTestMultiPayment call (and any related assertions if needed) to pass {
status: MULTIPAYMENT_DONE_STATUS } instead of { status: PAYMENT_DONE_STATUS }.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js`:
- Line 90: The test name "Service User with access rights: can register payments
for context linked to his integration" contradicts its assertion of
AccessDenied—rename the test description in the test(...) call to accurately
reflect denial (e.g., "Service User with access rights: cannot register payments
for context linked to his integration" or "is denied registering payments") so
CI output isn't misleading; update only the string in the test declaration that
wraps the assertions in this spec (the test(...) call) and leave the assertion
logic (AccessDenied) unchanged.
---
Duplicate comments:
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`:
- Around line 35-40: The date validation currently calls
dayjs(...).toISOString() which throws on invalid dates; update the checks in
RegisterExternalPaymentsService to first call dayjs(transactionDate).isValid()
and dayjs(depositedDate).isValid() respectively, and if invalid throw the
existing GQLError({ ...ERRORS.INVALID_DATE_FORMAT, messageInterpolation: { date:
<value>, transactionId } }, context); only call toISOString() and compare when
isValid() returns true (use the same transactionDate, depositedDate variables
and keep GQLError, ERRORS.INVALID_DATE_FORMAT and context intact).
---
Nitpick comments:
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`:
- Around line 23-54: Split validatePayments into smaller validator functions:
extract validateCurrency(payments, context) to check ISO_CODES and throw the
same GQLError with ERRORS.INVALID_CURRENCY_CODE; validatePeriod(payments,
context) to validate period via dayjs('YYYY-MM-01', true) and throw
ERRORS.INVALID_PERIOD_FORMAT; validateDates(payments, context) to validate
transactionDate and optional depositedDate with dayjs().toISOString() and throw
ERRORS.INVALID_DATE_FORMAT; and validateAmounts(payments, context) to parse
amounts/fees with Big, rethrowing ERRORS.INVALID_PAYMENT_AMOUNT on parse errors
and ERRORS.NEGATIVE_PAYMENT_AMOUNT when Big(amount).lte(0). Then simplify
validatePayments to iterate payments and call these helpers (or have helpers
accept a single payment) ensuring you preserve the same messageInterpolation
(transactionId, amount, date, period) and context values and continue using the
existing symbols Big, dayjs, ISO_CODES, and the ERRORS and GQLError usage.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js`:
- Line 148: The test currently uses Array(PAYMENTS_LIMIT +
1).fill(getExternalPayment()), which duplicates the same object (and
transactionId) for every entry; replace this with a generator that creates
distinct payment objects so each element has a unique transactionId. For
example, build the array with Array.from({length: PAYMENTS_LIMIT + 1}, (_, i) =>
getExternalPayment(i)) or map over a range and ensure getExternalPayment (or a
small wrapper) accepts the index and returns a fresh object/unique transactionId
for each payment used in the payments field.
In `@apps/condo/domains/acquiring/utils/testSchema/index.js`:
- Line 655: Function declaration spacing is inconsistent: update the declaration
of registerExternalPaymentsByTestClient to include a space before the parameter
list (i.e., change "function registerExternalPaymentsByTestClient(client, ..."
to "function registerExternalPaymentsByTestClient (client, ...)") so it follows
the project's "space before function parentheses" rule; locate the function by
the name registerExternalPaymentsByTestClient and adjust the spacing in its
declaration accordingly throughout the file.
In `@apps/condo/schema.graphql`:
- Around line 91965-91967: The output type RegisterExternalPaymentsOutput uses a
free-form status: String!, which should be a GraphQL enum for stricter typing;
add a new enum (e.g., RegisterExternalPaymentsStatus with values like SUCCESS,
FAILED, PENDING — choose canonical names your app uses) and change the field on
RegisterExternalPaymentsOutput to status: RegisterExternalPaymentsStatus!; then
update any resolver/serializer logic that constructs
RegisterExternalPaymentsOutput to return enum values instead of strings and
update client/typegen usages accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18705df3-e041-4dda-9fdc-02b503075bf2
📒 Files selected for processing (15)
apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/constants/errors.jsapps/condo/domains/acquiring/constants/payment.jsapps/condo/domains/acquiring/constants/registerExternalPayments.jsapps/condo/domains/acquiring/gql.jsapps/condo/domains/acquiring/schema/MultiPayment.jsapps/condo/domains/acquiring/schema/MultiPayment.test.jsapps/condo/domains/acquiring/schema/Payment.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.jsapps/condo/domains/acquiring/schema/index.jsapps/condo/domains/acquiring/utils/serverSchema/index.jsapps/condo/domains/acquiring/utils/testSchema/index.jsapps/condo/schema.graphqlapps/condo/schema.ts
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js
Outdated
Show resolved
Hide resolved
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`:
- Around line 82-89: The validateDepositedDate function currently throws when
the date is valid because the condition uses dayjs(depositedDate).isValid()
instead of negating it; update the conditional in validateDepositedDate to throw
a GQLError (using ERRORS.INVALID_DATE_FORMAT and the same messageInterpolation
with date and transactionId) only when depositedDate is present AND
dayjs(depositedDate).isValid() returns false (i.e., use
!dayjs(depositedDate).isValid()), leaving the rest of the error construction and
context parameter unchanged.
- Around line 178-212: The loop creating Payment and MultiPayment must be
wrapped in a DB transaction to guarantee atomicity: wrap the for-loop over
paymentsInput in context.prisma.$transaction(async (tx) => { ... }) and ensure
the model creates use the transaction client (either call the Prisma client on
tx or create a new context that sets context.prisma = tx before calling
Payment.create and MultiPayment.create). Specifically modify the block that
calls Payment.create and MultiPayment.create to run inside the $transaction and
use tx for all DB operations so partial writes cannot persist if any create
fails.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js`:
- Around line 144-153: The test creates duplicate payment objects because
Array(PAYMENTS_LIMIT + 1).fill(getExternalPayment()) calls getExternalPayment()
once; change the payments array to generate a fresh object per entry (e.g., use
Array.from({length: PAYMENTS_LIMIT + 1}, () => getExternalPayment()) or new
Array(PAYMENTS_LIMIT + 1).fill().map(() => getExternalPayment())) so each
payment (and its transactionId) is unique; update the test in the 'Should throw
error when payments count exceeds limit' case that calls
registerExternalPaymentsByTestClient(admin, payload) and uses PAYMENTS_LIMIT and
getExternalPayment to ensure you trigger PAYMENTS_LIMIT_EXCEEDED rather than
DUPLICATED_PAYMENTS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98d57ce3-dd8e-442b-b928-6244276f4fc1
📒 Files selected for processing (5)
apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/schema/MultiPayment.jsapps/condo/domains/acquiring/schema/MultiPayment.test.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js
✅ Files skipped from review due to trivial changes (1)
- apps/condo/domains/acquiring/schema/MultiPayment.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.js
Comment on lines
+178
to
+212
| for (const input of paymentsInput) { | ||
| const payment = await Payment.create(context, { | ||
| dv: 1, sender, | ||
| status: PAYMENT_DONE_STATUS, | ||
| amount: Big(input.amount).toString(), | ||
| explicitFee: Big(input.explicitFee || 0).toString(), | ||
| implicitFee: Big(input.implicitFee || 0).toString(), | ||
| currencyCode: input.currencyCode, | ||
| accountNumber: input.accountNumber, | ||
| period: input.period, | ||
| recipientBic: input.routingNumber, | ||
| recipientBankAccount: input.bankAccount, | ||
| advancedAt: dayjs(input.transactionDate).toISOString(), | ||
| depositedDate: input.depositedDate ? dayjs(input.depositedDate).toISOString() : dayjs(input.transactionDate).toISOString(), | ||
| rawAddress: input.address, | ||
| order: input.paymentOrder, | ||
| organization: { connect: { id: acquiringContext.organization } }, | ||
| context: { connect: { id: acquiringContext.id } }, | ||
| }) | ||
|
|
||
| await MultiPayment.create(context, { | ||
| dv: 1, sender, | ||
| status: MULTIPAYMENT_DONE_STATUS, | ||
| amountWithoutExplicitFee: Big(input.amount).toString(), | ||
| explicitFee: Big(input.explicitFee || 0).toString(), | ||
| implicitFee: Big(input.implicitFee || 0).toString(), | ||
| explicitServiceCharge: Big(0).toString(), | ||
| currencyCode: input.currencyCode, | ||
| integration: { connect: { id: acquiringIntegration.id } }, | ||
| payments: { connect: [{ id: payment.id }] }, | ||
| serviceCategory: DEFAULT_MULTIPAYMENT_SERVICE_CATEGORY, | ||
| transactionId: input.transactionId, | ||
| withdrawnAt: dayjs(input.transactionDate).toISOString(), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a transaction utility available in the codebase
rg -n --type=js 'transaction|withTransaction|runInTransaction' apps/condo/domains/acquiringRepository: open-condo-software/condo
Length of output: 1777
🏁 Script executed:
#!/bin/bash
# Try to search for transaction utilities and patterns
# Search broadly for transaction-related code patterns
rg -i 'transaction' --type js --max-count=20 apps/condo/domains/acquiring/ 2>/dev/null || echo "Search in acquiring domain failed"Repository: open-condo-software/condo
Length of output: 1862
🏁 Script executed:
#!/bin/bash
# Look for transaction patterns across the condo apps
fd -e js -e ts apps/condo/domains | head -20Repository: open-condo-software/condo
Length of output: 1862
🌐 Web query:
Keystone ORM transaction handling createMany atomic operations
💡 Result:
- Keystone
context.db.<List>.createMany()runs through Keystone’s internal GraphQL resolvers (so access control + hooks apply). Keystone docs don’t state that this API wraps the whole batch in a single DB transaction; if you need an explicit “all succeed or all rollback” guarantee across multiple operations, use Prisma transactions viacontext.prisma. [1] - If you just need “bulk insert is atomic” for one model: Prisma treats bulk operations as transactions, including
createMany()(and it’s typically executed as a single multi-rowINSERT ...wrapped inBEGIN/COMMIT). [2], [3] - If you need an atomic unit spanning multiple operations/models: use Prisma’s
$transaction, which is explicitly “all-or-nothing” (rolls back if any operation fails). [2] - If you need atomic dependent writes (parent + related children): use Prisma nested writes, which run “in a single transaction.” [2]
Practical Keystone pattern (atomic): run everything with context.prisma.$transaction(...) (sequential array form or interactive callback) instead of relying on context.db.* batching when atomicity matters. [1], [2]
Sources
[1] Keystone 6 docs – Database API (context.db.*) and using Prisma Client via context.prisma https://keystonejs.com/docs/context/db-items
[2] Prisma docs – Transactions, nested writes, batch operations, $transaction https://www.prisma.io/docs/orm/prisma-client/queries/transactions
[3] Prisma docs – createMany() shown as BEGIN ... INSERT ... COMMIT https://docs.prisma.io/docs/v6/orm/prisma-client/queries/crud
🌐 Web query:
Node.js Prisma transaction patterns batch operations
💡 Result:
Prisma + Node.js: transaction patterns for batch operations
1) Prefer bulk operations for true “batch writes”
Use createMany, updateMany, deleteMany (and *AndReturn variants where supported). Prisma documents that these bulk operations automatically run as transactions (all succeed or all roll back). [1]
await prisma.user.createMany({ data: users, skipDuplicates: true })
await prisma.user.updateMany({ where: { active: false }, data: { archived: true } })
await prisma.user.deleteMany({ where: { lastLogin: { lt: cutoff } } })2) Use nested writes when changes are related (single logical write)
If you’re creating/updating a parent plus related children, nested writes keep it atomic without manually composing many queries. Prisma lists nested writes as a key transaction approach. [2]
await prisma.order.create({
data: {
customerId,
items: { createMany: { data: lineItems } },
},
})3) Use $transaction([...]) for N independent queries you need to be atomic
Prisma’s transactions guide positions $transaction([]) (array form) as a way to run multiple operations in one atomic unit, and notes that many use cases are increasingly covered by bulk ops / nested writes. [2]
await prisma.$transaction([
prisma.account.update({ where: { id: fromId }, data: { balance: { decrement: amt } } }),
prisma.account.update({ where: { id: toId }, data: { balance: { increment: amt } } }),
])4) Use interactive transactions for read-modify-write / control flow
When later steps depend on earlier reads (or you need conditional logic), use the callback form prisma.$transaction(async (tx) => { ... }). [2]
await prisma.$transaction(async (tx) => {
const row = await tx.inventory.findUnique({ where: { sku } })
if (!row || row.qty < qty) throw new Error("Insufficient stock")
await tx.inventory.update({ where: { sku }, data: { qty: { decrement: qty } } })
})Prisma also explicitly advises: prefer array transactions over interactive transactions when queries don’t depend on each other. [3]
5) If you need “upsert many”
Prisma has createMany, updateMany, deleteMany, but no upsertMany; common workarounds are (a) grouping updates, (b) multiple upserts inside $transaction, or (c) raw SQL for a single statement (DB-specific). [4]
Sources
[1] Prisma Docs – Best practices (bulk operations run as transactions): (prisma.io)
[2] Prisma Docs – Transactions and batch queries reference (nested writes, bulk, $transaction forms): (prisma.io)
[3] Prisma Docs – Prisma Postgres FAQ (prefer array transactions when independent): (prisma.io)
[4] Stack Overflow – No upsertMany in Prisma (discussion/workarounds): (stackoverflow.com)
Citations:
- 1: https://www.prisma.io/docs/orm/more/best-practices?utm_source=openai
- 2: https://www.prisma.io/docs/v6/orm/prisma-client/queries/transactions?utm_source=openai
- 3: https://www.prisma.io/docs/v6/postgres/faq?utm_source=openai
- 4: https://stackoverflow.com/questions/70821501/how-to-upsert-many-fields-in-prisma-orm?utm_source=openai
Wrap Payment/MultiPayment creation loop in a transaction for atomicity.
The current code executes sequential create() calls in a loop without transaction wrapping. If an error occurs mid-loop, previously created Payment and MultiPayment records will persist while remaining operations fail, leaving the database in an inconsistent state. Use context.prisma.$transaction() to ensure all-or-nothing semantics:
await context.prisma.$transaction(async (tx) => {
for (const input of paymentsInput) {
const payment = await Payment.create(context, {
// ... existing fields
})
await MultiPayment.create(context, {
// ... existing fields
})
}
})Alternatively, refactor to use bulk operations where available for better performance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`
around lines 178 - 212, The loop creating Payment and MultiPayment must be
wrapped in a DB transaction to guarantee atomicity: wrap the for-loop over
paymentsInput in context.prisma.$transaction(async (tx) => { ... }) and ensure
the model creates use the transaction client (either call the Prisma client on
tx or create a new context that sets context.prisma = tx before calling
Payment.create and MultiPayment.create). Specifically modify the block that
calls Payment.create and MultiPayment.create to run inside the $transaction and
use tx for all DB operations so partial writes cannot persist if any create
fails.
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js (1)
178-212:⚠️ Potential issue | 🟠 MajorMake the batch write atomic.
This loop can leave partial imports behind if one
Payment.create()succeeds and a laterMultiPayment.create()fails. Retrying is especially awkward here because duplicate detection only checks existingMultiPayment.transactionIdvalues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js` around lines 178 - 212, The loop creating Payment.create and MultiPayment.create can leave partial state; wrap the batch write (the entire paymentsInput loop or at least each Payment.create + corresponding MultiPayment.create pair) in a single database transaction so either all records are persisted or none are (use the project's transaction API instead of calling Payment.create and MultiPayment.create separately), and ensure errors roll back the transaction and surface so retries won’t produce inconsistent partial imports; update the code around the paymentsInput loop in RegisterExternalPaymentsService.js to use the transaction wrapper when invoking Payment.create and MultiPayment.create.apps/condo/domains/acquiring/constants/errors.js (1)
71-71:⚠️ Potential issue | 🟡 MinorFix the broken interpolation in
MULTIPAYMENT_NON_DONE_PAYMENTS.The message is missing the closing quote after
${PAYMENT_DONE_STATUS}, so the exported error text is malformed and exact-message assertions will drift.Suggested fix
-const MULTIPAYMENT_NON_DONE_PAYMENTS = `[multiPayment:payments:status:not:done] MultiPayment cannot be created if any of payments has status not equal to "${PAYMENT_DONE_STATUS} for acquiring integration with type "${ACQUIRING_INTEGRATION_EXTERNAL_IMPORT_TYPE}".` +const MULTIPAYMENT_NON_DONE_PAYMENTS = `[multiPayment:payments:status:not:done] MultiPayment cannot be created if any of payments has status not equal to "${PAYMENT_DONE_STATUS}" for acquiring integration with type "${ACQUIRING_INTEGRATION_EXTERNAL_IMPORT_TYPE}".`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/acquiring/constants/errors.js` at line 71, The template string for MULTIPAYMENT_NON_DONE_PAYMENTS is malformed because the closing quote after ${PAYMENT_DONE_STATUS} is missing; update the MULTIPAYMENT_NON_DONE_PAYMENTS constant to properly close the interpolated expression and surrounding quotes so it reads something like including "${PAYMENT_DONE_STATUS}" and still references ${ACQUIRING_INTEGRATION_EXTERNAL_IMPORT_TYPE}, ensuring the final exported string is valid and matches exact-message assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`:
- Around line 91-95: validateNumericValues currently skips
explicitFee/implicitFee when they're empty strings and later code coerces them
with "|| 0", causing '' to be normalized to 0 instead of failing validation;
update validateNumericValues (and other places that use the same "|| 0" coercion
for explicitFee/implicitFee) to include empty-string values in valuesToValidate
and validate them as invalid (i.e., treat '' as non-numeric), and remove the "||
0" normalization—instead explicitly check for undefined/null (allowing omitted
fees if intended) and use Number.isFinite(Number(value)) or parseFloat+isNaN to
assert numeric input before converting/storing the value so malformed payloads
fail validation rather than being rewritten to zero.
- Around line 26-45: In validatePayments, add a strict non-empty check for
transactionId (reject null, empty string, or whitespace-only values using
transactionId.trim() === '') before any deduplication or other validations so
blank IDs cannot pass through; throw or call the existing validation error flow
(same pattern as validateCurrencyCode/validateDateFormat) referencing
transactionId and the current context. Apply the same non-blank transactionId
check to the other validation sites mentioned (the blocks around the same logic
at the other locations) to ensure MultiPayment.transactionId is never persisted
as blank.
---
Duplicate comments:
In `@apps/condo/domains/acquiring/constants/errors.js`:
- Line 71: The template string for MULTIPAYMENT_NON_DONE_PAYMENTS is malformed
because the closing quote after ${PAYMENT_DONE_STATUS} is missing; update the
MULTIPAYMENT_NON_DONE_PAYMENTS constant to properly close the interpolated
expression and surrounding quotes so it reads something like including
"${PAYMENT_DONE_STATUS}" and still references
${ACQUIRING_INTEGRATION_EXTERNAL_IMPORT_TYPE}, ensuring the final exported
string is valid and matches exact-message assertions.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`:
- Around line 178-212: The loop creating Payment.create and MultiPayment.create
can leave partial state; wrap the batch write (the entire paymentsInput loop or
at least each Payment.create + corresponding MultiPayment.create pair) in a
single database transaction so either all records are persisted or none are (use
the project's transaction API instead of calling Payment.create and
MultiPayment.create separately), and ensure errors roll back the transaction and
surface so retries won’t produce inconsistent partial imports; update the code
around the paymentsInput loop in RegisterExternalPaymentsService.js to use the
transaction wrapper when invoking Payment.create and MultiPayment.create.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7116576b-d39e-4b53-8bd0-9682526685f7
📒 Files selected for processing (6)
apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/constants/errors.jsapps/condo/domains/acquiring/schema/MultiPayment.jsapps/condo/domains/acquiring/schema/MultiPayment.test.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.jsapps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js
✅ Files skipped from review due to trivial changes (1)
- apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/condo/domains/acquiring/schema/MultiPayment.js
- apps/condo/domains/acquiring/access/RegisterExternalPaymentsService.js
Comment on lines
+26
to
+45
| function validatePayments (payments, context) { | ||
| for (const payment of payments) { | ||
| const { | ||
| transactionId, | ||
| amount, | ||
| period, | ||
| transactionDate, | ||
| depositedDate, | ||
| currencyCode, | ||
| explicitFee, | ||
| implicitFee, | ||
| } = payment | ||
|
|
||
| validateCurrencyCode(currencyCode, transactionId, context) | ||
| validatePeriodFormat(period, transactionId, context) | ||
| validateDateFormat(transactionDate, transactionId, context) | ||
| validateDepositedDate(depositedDate, transactionId, context) | ||
| validateNumericValues(amount, explicitFee, implicitFee, transactionId, context) | ||
| validatePositiveAmount(amount, transactionId, context) | ||
| } |
Contributor
There was a problem hiding this comment.
Reject blank transactionId values before deduplicating.
A blank or whitespace-only transactionId currently passes validation, participates in the duplicate check, and is then persisted to MultiPayment.transactionId. That leaves you with imports that can't be traced or retried reliably.
Also applies to: 161-163, 209-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`
around lines 26 - 45, In validatePayments, add a strict non-empty check for
transactionId (reject null, empty string, or whitespace-only values using
transactionId.trim() === '') before any deduplication or other validations so
blank IDs cannot pass through; throw or call the existing validation error flow
(same pattern as validateCurrencyCode/validateDateFormat) referencing
transactionId and the current context. Apply the same non-blank transactionId
check to the other validation sites mentioned (the blocks around the same logic
at the other locations) to ensure MultiPayment.transactionId is never persisted
as blank.
Comment on lines
+91
to
+95
| function validateNumericValues (amount, explicitFee, implicitFee, transactionId, context) { | ||
| const valuesToValidate = [amount] | ||
| if (explicitFee) valuesToValidate.push(explicitFee) | ||
| if (implicitFee) valuesToValidate.push(implicitFee) | ||
|
|
Contributor
There was a problem hiding this comment.
Don't normalize empty-string fees to zero.
validateNumericValues() skips explicitFee / implicitFee when they are '', and the later || 0 coercion stores them as 0. Malformed payloads should fail validation instead of being silently rewritten.
Suggested fix
function validateNumericValues (amount, explicitFee, implicitFee, transactionId, context) {
const valuesToValidate = [amount]
- if (explicitFee) valuesToValidate.push(explicitFee)
- if (implicitFee) valuesToValidate.push(implicitFee)
+ if (explicitFee !== undefined && explicitFee !== null) valuesToValidate.push(explicitFee)
+ if (implicitFee !== undefined && implicitFee !== null) valuesToValidate.push(implicitFee)
@@
dv: 1, sender,
status: PAYMENT_DONE_STATUS,
amount: Big(input.amount).toString(),
- explicitFee: Big(input.explicitFee || 0).toString(),
- implicitFee: Big(input.implicitFee || 0).toString(),
+ explicitFee: Big(input.explicitFee ?? 0).toString(),
+ implicitFee: Big(input.implicitFee ?? 0).toString(),
@@
status: MULTIPAYMENT_DONE_STATUS,
amountWithoutExplicitFee: Big(input.amount).toString(),
- explicitFee: Big(input.explicitFee || 0).toString(),
- implicitFee: Big(input.implicitFee || 0).toString(),
+ explicitFee: Big(input.explicitFee ?? 0).toString(),
+ implicitFee: Big(input.implicitFee ?? 0).toString(),Also applies to: 183-184, 202-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/condo/domains/acquiring/schema/RegisterExternalPaymentsService.js`
around lines 91 - 95, validateNumericValues currently skips
explicitFee/implicitFee when they're empty strings and later code coerces them
with "|| 0", causing '' to be normalized to 0 instead of failing validation;
update validateNumericValues (and other places that use the same "|| 0" coercion
for explicitFee/implicitFee) to include empty-string values in valuesToValidate
and validate them as invalid (i.e., treat '' as non-numeric), and remove the "||
0" normalization—instead explicitly check for undefined/null (allowing omitted
fees if intended) and use Number.isFinite(Number(value)) or parseFloat+isNaN to
assert numeric input before converting/storing the value so malformed payloads
fail validation rather than being rewritten to zero.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests