feat(condo): DOMA-13345 card binding handlers - #7905
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe acquiring domain adds GraphQL operations to retrieve and delete saved card bindings. It adds provider requests, deduplication, authorization, integration endpoint fields, database migration changes, server utilities, test helpers, generated schema types, and coverage. ChangesCard Binding Operations
Subproject References
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant AllCardBindingsService
participant AcquiringIntegrations
participant CardProvider
participant DeleteCardBindingService
Client->>AllCardBindingsService: Request card bindings
AllCardBindingsService->>AcquiringIntegrations: Find eligible integrations
AcquiringIntegrations-->>AllCardBindingsService: Integration endpoints
AllCardBindingsService->>CardProvider: Fetch cards concurrently
CardProvider-->>AllCardBindingsService: Card tokens
AllCardBindingsService-->>Client: Deduplicated card tokens
Client->>DeleteCardBindingService: Delete card binding
DeleteCardBindingService->>CardProvider: Delete card across integrations
CardProvider-->>DeleteCardBindingService: Deletion results
DeleteCardBindingService-->>Client: Status "ok"
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
vovaaxeapolla
added
🔬 WIP
![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: 0f4f2afdfc
ℹ️ 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".
| if (user.deletedAt) return false | ||
| if (user.isAdmin) return true | ||
|
|
||
| return !!userId |
There was a problem hiding this comment.
For non-admins this access check only verifies that the request contains some user.id, so any signed-in user who knows or can obtain another user's id can call allCardBindings for that id and receive that user's saved card tokens from every acquiring integration. This should compare the requested id with authentication.item.id (or otherwise enforce an explicit permission) before allowing the query.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah, we think that it is impossible to guess another user's id. The second reason is that old flow will use service user and auth userId will not be equal to userId from request
| if (user.deletedAt) return false | ||
| if (user.isAdmin) return true | ||
|
|
||
| return !!(userId && cardId) |
There was a problem hiding this comment.
For non-admins this only checks that user.id and cardId are present, so any authenticated user can submit another user's id and delete that user's saved card binding across acquiring integrations. The mutation should ensure the requested user is the authenticated user (or require a dedicated permission) before allowing deletion.
Useful? React with 👍 / 👎.
| for (const result of results) { | ||
| if (result.status === 'fulfilled') { | ||
| continue |
There was a problem hiding this comment.
When an acquiring endpoint returns a non-2xx response or times out, deleteUserCard catches the error and resolves false, so Promise.allSettled marks the attempt as fulfilled and this loop treats it as success. In that scenario the GraphQL mutation still returns { status: 'ok' } even though the card remains bound in at least one integration, leaving callers with a false success signal.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/condo/domains/acquiring/utils/testSchema/index.js (1)
81-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one single-quoted import for this module.
Combine these imports from
@condo/domains/acquiring/gqland use single quotes.Proposed fix
-const { DELETE_CARD_BINDING_MUTATION } = require('`@condo/domains/acquiring/gql`') -const { ALL_CARD_BINDINGS_QUERY } = require("`@condo/domains/acquiring/gql`"); +const { + ALL_CARD_BINDINGS_QUERY, + DELETE_CARD_BINDING_MUTATION, +} = require('`@condo/domains/acquiring/gql`')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/acquiring/utils/testSchema/index.js` around lines 81 - 82, Combine the DELETE_CARD_BINDING_MUTATION and ALL_CARD_BINDINGS_QUERY imports from `@condo/domains/acquiring/gql` into one require statement, using single quotes consistently.Source: Coding guidelines
apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js (1)
1-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep this unit spec independent of Keystone schema utilities.
This
*.spec.jsfile creates Keystone clients and initializes schema test state. Keep schema integration coverage inDeleteCardBindingService.test.js. Make this spec exercise isolated behavior with mocked dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js` around lines 1 - 38, Remove Keystone client creation and schema test-state initialization from the DeleteCardBindingService spec, including makeLoggedInAdminClient, makeClientWithNewRegisteredAndLoggedInUser, TestUtils, AcquiringTestMixin, setFakeClientMode, and the beforeAll setup. Keep schema integration coverage in DeleteCardBindingService.test.js and leave this spec focused on isolated behavior with mocked dependencies.Source: Coding guidelines
apps/condo/domains/acquiring/schema/AllCardBindingsService.spec.js (2)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrder imports by the configured groups.
@app/condo/indexis an internal import. It must not precede external and@open-condoimports. Place external imports first, then@open-condo, then internal imports with the required blank lines.As per coding guidelines, import groups must follow builtin → external →
@open-condo→ internal, with blank lines between groups.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/acquiring/schema/AllCardBindingsService.spec.js` around lines 1 - 15, Reorder the imports in the test file according to the configured groups: place the external `@faker-js/faker` import first, followed by `@open-condo/keystone/test.utils`, then the internal `@app/condo/index` and remaining `@condo` imports. Preserve blank lines between each group.Source: Coding guidelines
41-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove these schema integration scenarios to a
.test.jsfile.These tests initialize Keystone test infrastructure, create integrations, and execute GraphQL requests. Keep this behavior in
AllCardBindingsService.test.js, or replace the infrastructure with isolated dependency mocks before retaining the.spec.jssuffix.As per coding guidelines,
*.spec.jstests must mock dependencies rather than rely on Keystone schema test utilities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/acquiring/schema/AllCardBindingsService.spec.js` around lines 41 - 230, Move the integration scenarios from the AllCardBindingsService spec suite into AllCardBindingsService.test.js, preserving the existing Keystone setup, integration creation, and GraphQL request behavior. Keep the current tests unchanged in coverage and assertions while reserving the .spec.js suffix for tests using isolated dependency mocks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/acquiring/access/AllCardBindingsService.js`:
- Around line 6-11: Update canAllCardBindings so non-admin authenticated users
are authorized only when userId matches user.id, while preserving the existing
authentication, deleted-user, and admin checks. Add coverage verifying a
non-admin request with a different user ID is rejected.
In `@apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js`:
- Around line 28-39: Update canDeleteCardBinding and the delete flow to ignore
or reject a caller-supplied user.id for non-admin users, deriving the target
user ID from the authenticated context instead. Preserve admin behavior for
authorized cross-user deletion, and extend the user: execute test to verify
another user’s ID is rejected.
In `@apps/condo/domains/acquiring/utils/serverSchema/allCardBindings/utils.js`:
- Around line 10-14: Update the acquiringIntegrations query in fetchCardTokens
to filter only on getUserCardsUrl_not when retrieving cards, removing the
deleteUserCardUrl_not requirement. Keep the deletion-endpoint condition
restricted to the delete flow.
In `@apps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.js`:
- Around line 45-56: Update the card-deletion POST in the fetch call to disable
retries by setting maxRetries to 0, unless a provider-supported stable
idempotency key is added to the request. Keep the existing request method,
payload, and timeout behavior unchanged.
In `@apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js`:
- Around line 28-37: Update deleteCardBinding to propagate rejected provider
deletions instead of only logging them and resolving successfully. After
processing results, return a non-success result or rethrow an error that
includes the failed integrations; ensure DeleteCardBindingService does not
return { status: 'ok' } when any deleteUserCard call fails.
---
Nitpick comments:
In `@apps/condo/domains/acquiring/schema/AllCardBindingsService.spec.js`:
- Around line 1-15: Reorder the imports in the test file according to the
configured groups: place the external `@faker-js/faker` import first, followed by
`@open-condo/keystone/test.utils`, then the internal `@app/condo/index` and
remaining `@condo` imports. Preserve blank lines between each group.
- Around line 41-230: Move the integration scenarios from the
AllCardBindingsService spec suite into AllCardBindingsService.test.js,
preserving the existing Keystone setup, integration creation, and GraphQL
request behavior. Keep the current tests unchanged in coverage and assertions
while reserving the .spec.js suffix for tests using isolated dependency mocks.
In `@apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js`:
- Around line 1-38: Remove Keystone client creation and schema test-state
initialization from the DeleteCardBindingService spec, including
makeLoggedInAdminClient, makeClientWithNewRegisteredAndLoggedInUser, TestUtils,
AcquiringTestMixin, setFakeClientMode, and the beforeAll setup. Keep schema
integration coverage in DeleteCardBindingService.test.js and leave this spec
focused on isolated behavior with mocked dependencies.
In `@apps/condo/domains/acquiring/utils/testSchema/index.js`:
- Around line 81-82: Combine the DELETE_CARD_BINDING_MUTATION and
ALL_CARD_BINDINGS_QUERY imports from `@condo/domains/acquiring/gql` into one
require statement, using single quotes consistently.
🪄 Autofix
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 Plus
Run ID: cc5663f8-9ee6-46e5-a0ef-2bf3d7b33379
📒 Files selected for processing (20)
apps/condo/domains/acquiring/access/AllCardBindingsService.jsapps/condo/domains/acquiring/access/DeleteCardBindingService.jsapps/condo/domains/acquiring/gql.jsapps/condo/domains/acquiring/schema/AcquiringIntegration.jsapps/condo/domains/acquiring/schema/AllCardBindingsService.jsapps/condo/domains/acquiring/schema/AllCardBindingsService.spec.jsapps/condo/domains/acquiring/schema/AllCardBindingsService.test.jsapps/condo/domains/acquiring/schema/DeleteCardBindingService.jsapps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.jsapps/condo/domains/acquiring/schema/DeleteCardBindingService.test.jsapps/condo/domains/acquiring/schema/index.jsapps/condo/domains/acquiring/utils/serverSchema/allCardBindings/utils.jsapps/condo/domains/acquiring/utils/serverSchema/allCardBindings/utils.spec.jsapps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.jsapps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.jsapps/condo/domains/acquiring/utils/serverSchema/index.jsapps/condo/domains/acquiring/utils/testSchema/index.jsapps/condo/migrations/20260804174816-0545_acquiringintegration_deleteusercardurl_and_more.jsapps/epsapps/rb
| async function canAllCardBindings ({ args: { data: { user: { id: userId } } }, authentication: { item: user } }) { | ||
| if (!user) return throwAuthenticationError() | ||
| if (user.deletedAt) return false | ||
| if (user.isAdmin) return true | ||
|
|
||
| return !!userId |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'AllCardBindingsService' . | sed 's#^\./##'
echo '--- outline/service ---'
ast-grep outline apps/condo/domains/acquiring/access/AllCardBindingsService.js || true
echo '--- service ---'
cat -n apps/condo/domains/acquiring/access/AllCardBindingsService.js
echo '--- tests ---'
ast-grep outline apps/condo/domains/acquiring/access/AllCardBindingsService.test.js || true
cat -n apps/condo/domains/acquiring/access/AllCardBindingsService.test.js
echo '--- usages ---'
rg -n "canAllCardBindings|AllCardBindingsService|data\.user\.id|allCardBindings|user:\s*\{" apps/condo/domains/acquiring -SRepository: open-condo-software/condo
Length of output: 1747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- acquired access/schem tests ---'
for f in apps/condo/domains/acquiring/schema/AllCardBindingsService.test.js apps/condo/domains/acquiring/schema/AllCardBindingsService.spec.js; do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f"
fi
done
echo '--- schema service files outline ---'
ast-grep outline apps/condo/domains/acquiring/schema/AllCardBindingsService.js || true
echo '--- schema service file ---'
cat -n apps/condo/domains/acquiring/schema/AllCardBindingsService.js
echo '--- usages ---'
rg -n "canAllCardBindings|AllCardBindingsService|allCardBindings|data\.user\.id|user:\s*\{" apps/condo/domains/acquiring -SRepository: open-condo-software/condo
Length of output: 27844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- allCardBindings implementation ---'
cat -n apps/condo/domains/acquiring/utils/serverSchema/index.js | sed -n '130,185p'
echo '--- fetchCardTokens helpers ---'
cat -n apps/condo/domains/acquiring/utils/serverSchema/allCardBindings/utils.js | sed -n '1,220p'
echo '--- getUserCards usages ---'
rg -n "getUserCards|fetchCardTokens|deduplicateCardTokens|allCardBindings" apps/condo/domains/acquiring/utils/serverSchema apps/condo/domains/acquiring -S
echo '--- delete card binding access test for comparison ---'
rg -n "canDeleteCardBinding|DeleteCardBindingService" apps/condo/domains/acquiring -S
cat -n apps/condo/domains/acquiring/access/DeleteCardBindingService.js
cat -n apps/condo/domains/acquiring/utils/serverSchema/DeleteCardBindingService.jsRepository: open-condo-software/condo
Length of output: 16092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- getUserCards implementation ---'
cat -n apps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.js | sed -n '1,90p'
echo '--- exact service files access pattern diff context ---'
wc -l apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js apps/condo/domains/acquiring/access/DeleteCardBindingService.js apps/condo/domains/acquiring/schema/DeleteCardBindingService.js
cat -n apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js | sed -n '1,90p'
cat -n apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js | sed -n '1,90p'Repository: open-condo-software/condo
Length of output: 7322
🌐 Web query:
open condo Deleting of card binding service tests user execute should reject unauthorized user id
💡 Result:
In the Open Condo (condo) platform, testing unauthorized access for sensitive services—such as card binding, payment registration, or other administrative actions—follows a standardized pattern within the project's testing framework [1][2][3]. Tests for rejecting unauthorized users are typically implemented as integration tests using the project's Keystone-based testing utilities [3]. To verify that a service correctly rejects an unauthorized user ID, developers follow these practices: 1. Setup Isolated Clients: Developers use helpers such as makeClientWithServiceUser or similar authentication fixtures to create distinct client contexts [1][2]. 2. Define Access Rights: The test cases define both authorized and unauthorized states. For example, a test may configure a service user with an access-right set that explicitly has a permission flag (e.g., canExecuteRegisterExternalPayments) set to false [1]. 3. Assert Rejection: The test attempts to execute the service method with the unauthorized client context and asserts that the operation fails with an access-denied error (often mapping to a 403 Forbidden status or a specific GraphQL/API access-denied response) [1][2][4]. Because Open Condo utilizes role-based access control (RBAC) scoped to organizations [5], these tests verify that the system correctly validates the user's permissions within the specific organizational context of the request, ensuring that a user ID without the necessary privileges cannot trigger the deletion or registration of bound cards or other restricted entities [1][5][2]. Internal security enforcement is validated by ensuring that the access-control middleware is triggered before the service logic executes [5][6].
Citations:
- 1: feat(condo): DOMA-13328 allow b2b service users to use RegisterExternalPaymentsService #7703
- 2: feat(condo): DOMA-13059 register external payments #7433
- 3: https://www.mintlify.com/open-condo-software/condo/developer/testing
- 4: https://www.mintlify.com/open-condo-software/condo/miniapps/bridge
- 5: https://deepwiki.com/open-condo-software/condo/3.2-access-control-framework
- 6: https://github.com/kici-dev/kici-public/blob/main/packages/orchestrator/src/routes/admin-event-log.parameter-binding.test.ts
IDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External
Restrict non-admin reads to the authenticated user.
canAllCardBindings() returns true for any truthy data.user.id. The resolver uses that ID to fetch card bindings, so an authenticated non-admin can request another user’s saved card metadata. Require userId === user.id for non-admin users, and add a test that rejects a different user ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/acquiring/access/AllCardBindingsService.js` around lines 6
- 11, Update canAllCardBindings so non-admin authenticated users are authorized
only when userId matches user.id, while preserving the existing authentication,
deleted-user, and admin checks. Add coverage verifying a non-admin request with
a different user ID is rejected.
| test('user: execute', async () => { | ||
| const client = await makeClientWithNewRegisteredAndLoggedInUser() | ||
|
|
||
| const [result] = await deleteCardBindingByTestClient(client, { | ||
| user: { | ||
| id: faker.datatype.uuid(), | ||
| }, | ||
| cardId: faker.datatype.uuid(), | ||
| }) | ||
|
|
||
| expect(result.status).toBe('ok') | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'DeleteCardBindingService|DeleteCardBinding|cardBinding|acquiring' . | sed 's#^\./##' | head -200
echo "== git diff stat =="
git diff --stat || true
echo "== relevant search =="
rg -n "deleteCardBinding|DeleteCardBindingService|cardBinding|DeleteCardBinding" apps/condo/domains/acquiring -S || trueRepository: open-condo-software/condo
Length of output: 6073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
apps/condo/domains/acquiring/schema/DeleteCardBindingService.js \
apps/condo/domains/acquiring/access/DeleteCardBindingService.js \
apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js \
apps/condo/domains/acquiring/utils/serverSchema/index.js \
apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js \
apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js \
apps/condo/domains/acquiring/gql.js
do
echo "== $f =="
cat -n "$f"
done
echo "== structural/context usages near delete operations =="
python3 - <<'PY'
from pathlib import Path
paths = [
'apps/condo/domains/acquiring/schema/DeleteCardBindingService.js',
'apps/condo/domains/acquiring/access/DeleteCardBindingService.js',
'apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.py'
]
for path in ['apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js','apps/condo/domains/acquiring/utils/serverSchema/index.js']:
p=Path(path)
if p.exists():
txt=p.read_text()
print(f'--- {p} contains id fields ---')
for i,line in enumerate(txt.splitlines(),1):
if 'id' in line and ('user' in line or 'context' in line or 'deleteCardBinding' in line or 'authentication' in line or 'requireContext' in line):
print(f'{i}: {line}')
PYRepository: open-condo-software/condo
Length of output: 28631
IDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: External
Reject deletion requests for another user.
canDeleteCardBinding does not prevent non-admin callers from passing an arbitrary user.id; the delete flow then forwards that value. Derive the user ID from the authenticated context for non-admin requests and add coverage that another user’s ID is rejected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js` around
lines 28 - 39, Update canDeleteCardBinding and the delete flow to ignore or
reject a caller-supplied user.id for non-admin users, deriving the target user
ID from the authenticated context instead. Preserve admin behavior for
authorized cross-user deletion, and extend the user: execute test to verify
another user’s ID is rejected.
| const response = await fetch(url, { | ||
| maxRetries: 5, | ||
| timeoutBetweenRequests: 1000, | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| userId, | ||
| cardId, | ||
| }), | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'deleteUserCard|deleteUserCardUrl|maxRetries|idempot' apps packages -g '*.js'Repository: open-condo-software/condo
Length of output: 34002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the delete call path and retry implementation without executing repo code.
sed -n '35,75p' apps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.js
sed -n '160,210p' packages/keystone/fetch.js
rg -n -C 3 'acquiringCard.*delete|CardBinding|deleteCardBinding' apps/condo/domains/acquiring -g '*.js'Repository: open-condo-software/condo
Length of output: 36211
Do not retry the card-deletion POST without idempotency.
fetchWithRetriesAndLogger retries after a failed response or exception, and this call sets maxRetries: 5 with only userId and cardId in the body. If the acquiring endpoint deletes the card but the response is lost, a later retry can repeat the deletion. Set maxRetries = 0 for this mutation, or add a provider-supported stable idempotency key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.js`
around lines 45 - 56, Update the card-deletion POST in the fetch call to disable
retries by setting maxRetries to 0, unless a provider-supported stable
idempotency key is added to the request. Keep the existing request method,
payload, and timeout behavior unchanged.
| for (const result of results) { | ||
| if (result.status === 'fulfilled') { | ||
| continue | ||
| } | ||
|
|
||
| logger.error({ | ||
| msg: 'failed to delete card binding', | ||
| err: result.reason, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching relevant paths:"
fd -a 'utils\.js$|DeleteCardBindingService\.js$|deleteCardBinding' . | sed 's#^\./##' | head -100
echo
echo "target utils outline:"
ast-grep outline apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js || true
echo
echo "target utils content:"
cat -n apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js
echo
echo "service overview:"
cat -n apps/condo/domains/acquiring/schema/DeleteCardBindingService.js | sed -n '1,220p'Repository: open-condo-software/condo
Length of output: 7396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for deleteCardBinding usages and return handling:"
rg -n "deleteCardBinding|DeleteCardBinding|cardBinding|card binding" apps/condo/domains/acquiring -g '*.js' -g '*.ts' | head -250
echo
echo "Relevant schema/service files list:"
git ls-files 'apps/condo/domains/acquiring/**' | rg 'DeleteCardBinding|deleteCardBinding|schema|utils' | sortRepository: open-condo-software/condo
Length of output: 11425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "deleteUserCard implementation:"
cat -n apps/condo/domains/acquiring/utils/serverSchema/cardsOnlineInteraction.js | sed -n '1,130p'
echo
echo "DeleteCardBindingService tests/specs relevant sections:"
cat -n apps/condo/domains/acquiring/schema/DeleteCardBindingService.test.js | sed -n '1,140p'
echo
cat -n apps/condo/domains/acquiring/schema/DeleteCardBindingService.spec.js | sed -n '1,120p'
echo
echo "testSchema delete helper and serverSchema delete wrapper:"
cat -n apps/condo/domains/acquiring/utils/testSchema/index.js | sed -n '648,710p'
echo
cat -n apps/condo/domains/acquiring/utils/serverSchema/index.js | sed -n '145,185p'
echo
echo "Programmatic model equivalent:"
node - <<'JS'
async function promiseAllSettledAndResolve() {
const results = await Promise.allSettled([
Promise.resolve({}),
Promise.reject(new Error('provider failed')),
Promise.resolve({}),
])
for (const result of results) {
if (result.status === 'fulfilled') continue
console.log('logged rejected result')
}
return undefined
}
promiseAllSettledAndResolve().then(result => {
console.log('function resolved to:', result)
const serviceOutput = { status: 'ok' }
console.log('service output:', serviceOutput)
})
JSRepository: open-condo-software/condo
Length of output: 11893
Propagate failed provider deletions.
deleteUserCard logs and returns false when a provider call fails, and deleteCardBinding still resolves. DeleteCardBindingService then returns { status: 'ok' } after that resolve. If any provider deletion fails, return a non-success result containing the failed integrations or rethrow after collecting rejected results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/acquiring/utils/serverSchema/deleteCardBinding/utils.js`
around lines 28 - 37, Update deleteCardBinding to propagate rejected provider
deletions instead of only logging them and resolving successfully. After
processing results, return a non-success result or rethrow an error that
includes the failed integrations; ensure DeleteCardBindingService does not
return { status: 'ok' } when any deleteUserCard call fails.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/condo/schema.ts (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse four-space indentation in the changed TypeScript declarations.
If this file is generated, update its generator or formatter and regenerate the file.
apps/condo/schema.ts#L66-L74: Indent the added fields with four spaces.apps/condo/schema.ts#L1166-L1170: Indent the added input fields with four spaces.apps/condo/schema.ts#L1202-L1206: Indent the added history fields with four spaces.apps/condo/schema.ts#L1232-L1236: Indent the added history input fields with four spaces.apps/condo/schema.ts#L1267-L1271: Indent the added update input fields with four spaces.apps/condo/schema.ts#L1326-L1343: Indent the filter fields with four spaces.apps/condo/schema.ts#L1364-L1381: Indent the filter fields with four spaces.apps/condo/schema.ts#L1589-L1593: Indent the added input fields with four spaces.apps/condo/schema.ts#L1635-L1652: Indent the filter fields with four spaces.apps/condo/schema.ts#L1673-L1690: Indent the filter fields with four spaces.apps/condo/schema.ts#L2062-L2069: Indent type members with four spaces.apps/condo/schema.ts#L24978-L24988: Indent type members with four spaces.apps/condo/schema.ts#L28830-L28838: Indent type members with four spaces.apps/condo/schema.ts#L48740-L48740: Indent the mutation member with four spaces.apps/condo/schema.ts#L58280-L58282: Indent the argument member with four spaces.apps/condo/schema.ts#L79683-L79683: Indent the query member with four spaces.apps/condo/schema.ts#L85988-L85990: Indent the argument member with four spaces.apps/condo/schema.ts#L92841-L92848: Indent enum members with four spaces.apps/condo/schema.ts#L92890-L92897: Indent enum members with four spaces.As per coding guidelines,
**/*.{js,jsx,ts,tsx}files must use four-space indentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/schema.ts` around lines 66 - 74, Update the generated TypeScript declarations in apps/condo/schema.ts so all changed members use four-space indentation: lines 66-74, 1166-1170, 1202-1206, 1232-1236, 1267-1271, 1326-1343, 1364-1381, 1589-1593, 1635-1652, 1673-1690, 2062-2069, 24978-24988, 28830-28838, 48740, 58280-58282, 79683, 85988-85990, 92841-92848, and 92890-92897. If schema.ts is generated, update its generator or formatter and regenerate it rather than applying only manual edits.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/condo/schema.ts`:
- Around line 66-74: Update the generated TypeScript declarations in
apps/condo/schema.ts so all changed members use four-space indentation: lines
66-74, 1166-1170, 1202-1206, 1232-1236, 1267-1271, 1326-1343, 1364-1381,
1589-1593, 1635-1652, 1673-1690, 2062-2069, 24978-24988, 28830-28838, 48740,
58280-58282, 79683, 85988-85990, 92841-92848, and 92890-92897. If schema.ts is
generated, update its generator or formatter and regenerate it rather than
applying only manual edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f643fd8-f14d-4a87-b410-fe329def0cdc
📒 Files selected for processing (4)
apps/condo/domains/acquiring/schema/index.jsapps/condo/schema.graphqlapps/condo/schema.tsapps/eps
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/condo/domains/acquiring/schema/index.js
|
vovaaxeapolla
added
✋🙂 Review please
1 participant
Summary by CodeRabbit
New Features
Access Control
Bug Fixes
Tests