fix(PMNT-111): fix pagination on MangoPay #458
Conversation
WalkthroughThe changes refactor pagination logic in Mangopay connector components to handle cases where multiple entities share the same creation date. The code now allows processing of entities whose creation date equals the last processed date and adjusts state management to increment the page when needed. Associated tests are updated and expanded to cover these scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connector
participant MangopayAPI
Client->>Connector: Request next page (with lastCreationDate, lastPage)
Connector->>MangopayAPI: Fetch entities (with page, pageSize)
MangopayAPI-->>Connector: Return entities (some may have same creation date)
Connector->>Connector: Filter entities (creationDate >= lastCreationDate)
alt Last entity's creationDate == lastCreationDate
Connector->>Connector: Increment lastPage
else
Connector->>Connector: Reset lastPage to 1
end
Connector-->>Client: Return entities, new state (lastCreationDate, lastPage)
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/connectors/plugins/public/moneycorp/external_accounts.go (1)
95-103: 🛠️ Refactor suggestionRemove redundant filtering logic.
The new conditional check is redundant with the existing switch statement below. The
if createdAt.Before(lastCreatedAt)check will filter out all cases that the switch statement's-1, 0cases would handle, making the switch statement unreachable for those conditions.Consolidate the filtering logic by removing either the new if statement or the switch statement:
- // TODO we have the potential to skip some here, we should use strict lower - if createdAt.Before(lastCreatedAt) { - continue - } - switch createdAt.Compare(lastCreatedAt) { - case -1, 0: - continue - default: - } + if createdAt.Before(lastCreatedAt) { + continue + }Or if you want to maintain the equal-timestamp filtering for now, keep only the switch statement until the pagination logic is properly updated to handle timestamp collisions.
🧹 Nitpick comments (1)
internal/connectors/plugins/public/moneycorp/payments.go (1)
45-45: Address the pagination issue identified in the TODO comment.This TODO comment indicates the same pagination problems that were fixed in the MangoPay connector. The current implementation may miss entities when multiple items share the same
LastCreatedAttimestamp across pages, which is exactly the issue described in PMNT-111.Consider implementing a similar fix to the MangoPay approach shown in the other files in this PR. Would you like me to propose a solution that handles timestamp collisions and proper page management?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/connectors/plugins/public/mangopay/accounts.go(1 hunks)internal/connectors/plugins/public/mangopay/accounts_test.go(2 hunks)internal/connectors/plugins/public/mangopay/external_accounts.go(1 hunks)internal/connectors/plugins/public/mangopay/external_accounts_test.go(2 hunks)internal/connectors/plugins/public/mangopay/payments.go(1 hunks)internal/connectors/plugins/public/mangopay/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/users.go(1 hunks)internal/connectors/plugins/public/mangopay/users_test.go(2 hunks)internal/connectors/plugins/public/moneycorp/external_accounts.go(1 hunks)internal/connectors/plugins/public/moneycorp/payments.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/connectors/plugins/public/mangopay/users_test.go (1)
internal/models/plugin.go (1)
FetchNextOthersRequest(98-103)
internal/connectors/plugins/public/mangopay/accounts_test.go (2)
internal/models/plugin.go (1)
FetchNextAccountsRequest(62-66)internal/connectors/engine/workflow/workflow.go (1)
FromPayload(18-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (12)
internal/connectors/plugins/public/mangopay/users.go (1)
89-91: LGTM! Simplifies logic and fixes timestamp collision issue.This change correctly addresses the pagination issue described in PMNT-111. By changing from filtering
<= lastCreationDateto< lastCreationDate, entities with the same creation timestamp as the last processed item will now be included in subsequent pages, preventing entities from being skipped when timestamps collide across page boundaries.internal/connectors/plugins/public/mangopay/external_accounts.go (1)
95-97: LGTM! Consistent fix for timestamp collision pagination issue.This change mirrors the fix in
users.goand correctly addresses the pagination problem where external accounts with the same creation date could be missed across page boundaries. The simplified logic improves readability while ensuring entities with identical timestamps are properly processed.internal/connectors/plugins/public/mangopay/payments.go (2)
70-74: Important behavioral change documented well.The comment clearly explains why data trimming is skipped to prevent data loss during page transitions. This is a crucial behavioral change that could result in up to 2x the requested page size being returned, which callers should be aware of.
77-83: Correct implementation of pagination state management fix.The conditional logic properly addresses the first issue mentioned in the PR objectives:
- When the creation date remains the same, the page is incremented
- When the creation date changes, the page resets to 1
This ensures proper pagination flow when transitioning between different creation timestamps.
internal/connectors/plugins/public/mangopay/accounts.go (1)
98-100: Correct fix for handling entities with equal creation timestamps.The simplified conditional logic properly addresses the second issue mentioned in the PR objectives. By changing from
<=(switch statement) to strictly<(Before), accounts created at the same timestamp as the last processed date are now included, ensuring all entities are processed across page boundaries.internal/connectors/plugins/public/mangopay/users_test.go (2)
145-145: Index correction aligns with consistent test data setup.The change from index 38 to 39 ensures consistency with the test setup and aligns with similar changes in accounts_test.go.
177-204: Excellent test coverage for overlapping creation dates scenario.This test case effectively validates the fix for the second pagination issue:
- Sets up overlapping creation dates (users[9] and users[10])
- Starts pagination from the boundary condition
- Verifies all users with equal timestamps are processed correctly
- Confirms proper state management
The test thoroughly covers the edge case mentioned in PMNT-111.
internal/connectors/plugins/public/mangopay/accounts_test.go (2)
158-158: Index correction maintains test consistency.The change from index 38 to 39 aligns with the corresponding change in users_test.go, ensuring consistent test data setup across entity types.
190-216: Comprehensive test coverage for edge case scenario.This test case mirrors the users_test.go implementation and provides excellent coverage for the overlapping creation dates scenario:
- Simulates accounts created at the same timestamp
- Tests pagination boundary conditions
- Validates correct state transitions
- Ensures all accounts are processed despite timestamp overlap
The test effectively validates the filtering logic fix in accounts.go.
internal/connectors/plugins/public/mangopay/external_accounts_test.go (2)
146-146: Good fix for test accuracy.Changing the index from 38 to 39 correctly references the last account from the first page (with pageSize 40), making the test more logically consistent with the pagination scenario being tested.
178-205: Excellent test coverage for edge case handling.This new test case effectively validates the pagination fix mentioned in the PR objectives - specifically testing the scenario where multiple entities share the same creation timestamp across page boundaries. The test:
- Correctly simulates the edge case by setting
sampleBankAccounts[10].CreationDate = sampleBankAccounts[9].CreationDate- Properly mocks the client call to return the expected page slice
- Validates that pagination advances correctly (page increments to 2)
- Ensures the state is updated with the correct last creation date
This directly addresses the PMNT-111 issue where entities with equal timestamps could be missed during pagination.
internal/connectors/plugins/public/mangopay/payments_test.go (1)
180-215: Comprehensive test for timestamp collision scenario.This test case excellently covers the critical edge case where all entities have identical creation timestamps. The implementation:
- Simulates extreme conditions: Sets all transactions to the same creation date to test the most challenging pagination scenario
- Validates correct pagination behavior: Ensures the page advances from 1 to 2 even when timestamps are identical
- Maintains state consistency: Verifies that
LastCreationDateremains unchanged whileLastPageincrements correctly- Uses realistic mock expectations: The mocked client call matches the expected parameters for this pagination scenario
This directly addresses the first issue mentioned in the PR objectives where page number and createdAt timestamp management could cause incorrect pagination flow.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #458 +/- ##
==========================================
- Coverage 69.11% 69.09% -0.03%
==========================================
Files 601 601
Lines 30605 30593 -12
==========================================
- Hits 21153 21138 -15
- Misses 8337 8339 +2
- Partials 1115 1116 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| case -1, 0: | ||
| // creationDate <= state.LastCreationDate, nothing to do, | ||
| // we already processed this bank account. | ||
| if creationDate.Before(oldState.LastCreationDate) { |
There was a problem hiding this comment.
Is there a reason we are using Compare + switch statements instead of .Before and .Equal?
There was a problem hiding this comment.
Probably not. Many of these connectors were made by freelancers/employees who are no longer here so the style is not really fixed.
There was a problem hiding this comment.
Since before is only sending true if t1 < t2, we wanted to check the egality also if i remember correctly
| if !needMore { | ||
| payments = payments[:req.PageSize] | ||
| } | ||
| // Note that we are NOT trimming data as in other connectors to respect the pageSize; doing so would mean we could |
There was a problem hiding this comment.
Qonto behaves like that; I might have to apply it elsewhere going forward.
We can fix it to make it work with a fixed length returned, but it's a bit of added complexity that I'm happy to not do if we don't need it.
I also think it's useful to leave the comment, LMK if you disagree.
There was a problem hiding this comment.
I am for leaving comments that explain some technicalities that might not be obvious on first glance 👍
| @@ -92,6 +92,10 @@ func recipientToPSPAccounts( | |||
| return accounts, fmt.Errorf("failed to parse transaction date: %v", err) | |||
| } | |||
|
|
|||
| // TODO we have the potential to skip some here, we should use strict lower | |||
There was a problem hiding this comment.
woops I didn't mean to commit that
| case -1, 0: | ||
| // creationDate <= state.LastCreationDate, nothing to do, | ||
| // we already processed this account. | ||
| if accountCreationDate.Before(oldState.LastCreationDate) { |
There was a problem hiding this comment.
Just to be sure, Before() is only sending true if t1 < t2 and sending false when t1 = t2. This is what you want ?
There was a problem hiding this comment.
as answered on Slakc (leaving it here instead for posterity)
We need to process it when dates are equal; if not we are at risk of not processing some new records (see PMNT-111)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…tamps are returned
…pping some records before this
fguery
force-pushed
the
PMNT-111_fix-pagination
branch
from
ede402c to
f83a3f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/connectors/plugins/public/mangopay/accounts_test.go (1)
190-190: Fix typo in test description.There's a grammatical error in the test name.
- It("should fetch next accounts - when lastCreationDate form last page is equal to one of the new page's", func(ctx SpecContext) { + It("should fetch next accounts - when lastCreationDate from last page is equal to one of the new page's", func(ctx SpecContext) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/connectors/plugins/public/mangopay/accounts.go(1 hunks)internal/connectors/plugins/public/mangopay/accounts_test.go(2 hunks)internal/connectors/plugins/public/mangopay/external_accounts.go(1 hunks)internal/connectors/plugins/public/mangopay/external_accounts_test.go(2 hunks)internal/connectors/plugins/public/mangopay/payments.go(1 hunks)internal/connectors/plugins/public/mangopay/payments_test.go(1 hunks)internal/connectors/plugins/public/mangopay/users.go(1 hunks)internal/connectors/plugins/public/mangopay/users_test.go(2 hunks)test/e2e/api_payment_initiations_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/api_payment_initiations_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/connectors/plugins/public/mangopay/users.go
- internal/connectors/plugins/public/mangopay/payments.go
- internal/connectors/plugins/public/mangopay/external_accounts.go
- internal/connectors/plugins/public/mangopay/accounts.go
- internal/connectors/plugins/public/mangopay/users_test.go
- internal/connectors/plugins/public/mangopay/payments_test.go
- internal/connectors/plugins/public/mangopay/external_accounts_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (2)
internal/connectors/plugins/public/mangopay/accounts_test.go (2)
158-158: LGTM! The test setup correctly references the 40th account (index 39).The change aligns with the updated pagination logic and properly sets up the test state for the pagination scenario.
191-216: Well-designed test case for the pagination fix.This test case effectively validates the core fix mentioned in the PR objectives - ensuring that entities with creation dates equal to the last processed creation date are properly handled across pages. The test setup is comprehensive:
- Correctly simulates the scenario where two accounts have the same creation timestamp
- Properly configures the pagination state with
lastPage: 2- Uses appropriate mock expectations
- Validates both the returned accounts and the updated state
The test logic aligns perfectly with the pagination fix that allows processing entities whose creation date equals the last processed date.
This fixes 2 issues: