fix(storage): de-duplicate payments when updating db #286
Conversation
WalkthroughThe pull request introduces changes to three files: Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
==========================================
- Coverage 73.04% 73.04% -0.01%
==========================================
Files 537 537
Lines 27048 27065 +17
==========================================
+ Hits 19758 19769 +11
- Misses 6230 6232 +2
- Partials 1060 1064 +4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/storage/payments_test.go (1)
44-145: Comprehensive refund test data
This helper functiondefaultPaymentsRefunded()creates multiple partial refunds for the same payment ID, which is great for testing the new deduplication logic thoroughly. The approach is robust and ensures coverage of multiple refund scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/connectors/engine/activities/errors.go(0 hunks)internal/storage/payments.go(2 hunks)internal/storage/payments_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/connectors/engine/activities/errors.go
🔇 Additional comments (6)
internal/storage/payments.go (5)
69-69: Good addition for tracking refunded payments
This map-based approach to record processed refunds is a neat way to avoid double-counting duplicate payment entries in the same batch.
73-73: Consistent naming
“paymentsCapturedSeen” aligns well with the rest of the naming scheme. No concerns here.
82-89: In-place initial amount adjustments
The logic for updating theInitialAmountin place is clear and helps avoid creating multiple rows for the same payment. This should prevent duplicate inserts.
91-98: Summation of partial refunds
Accumulates partial refund amounts under the same payment ID in the slice, ensuring the final database update correctly offsets the total. This effectively solves the PostgreSQL “a second time” update error.
100-107: Capture and refund reversal handling
Reusing the same approach for captures and reversed refunds (which add amounts back) keeps the logic consistent across payment status transitions.internal/storage/payments_test.go (1)
577-592: Validating cumulative refund application
The test confirms that two consecutive refunds in the same batch yield the correct final amount (100 - 10 - 10 = 80). This checks the core objective of preventing duplicate rows and is a valuable addition to the suite.
internal/storage/payments.go
Outdated
| paymentsRefunded := make([]payment, 0) | ||
| paymentsInitatialAmountToAdjustSeen := make(map[models.PaymentID]int) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable name typo
The variable paymentsInitatialAmountToAdjustSeen seems to have a spelling error. Renaming it to something like paymentsInitialAmountToAdjustSeen would improve readability.
- paymentsInitatialAmountToAdjustSeen := make(map[models.PaymentID]int)
+ paymentsInitialAmountToAdjustSeen := make(map[models.PaymentID]int)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paymentsInitatialAmountToAdjustSeen := make(map[models.PaymentID]int) | |
| paymentsInitialAmountToAdjustSeen := make(map[models.PaymentID]int) |
Sometimes, we can have two refunds for the same payment inside the same batch, and postgres does not support it and return an error 'command cannot affect row a second time'
paul-nicolas
force-pushed
the
fix/payments-upsert
branch
from
8d382b0 to
565f8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/storage/payments_test.go (2)
44-144: Add documentation to explain the test data setup.The function creates a complex test scenario but lacks documentation explaining the purpose and structure of the test data. Consider adding a comment block explaining:
- The purpose of having multiple entries with the same payment ID
- The significance of the timing sequence
- The expected final state after processing these payments
Add this documentation at the start of the function:
+// defaultPaymentsRefunded creates test data that simulates a payment with multiple refunds. +// It generates three entries for the same payment: +// 1. Initial payment of 100 USD +// 2. First refund of 10 USD +// 3. Second refund of 10 USD +// The entries are created with different timestamps to test the handling of multiple refunds +// in the same batch. func defaultPaymentsRefunded() []models.Payment {Consider using different reference values for refunds.
While valid, using the same reference "test1" for both the original payment and refunds might not represent real-world scenarios accurately. Consider using distinct references for refunds to make the test more realistic.
- Reference: "test1", + Reference: "refund1", CreatedAt: now.Add(-59 * time.Minute).UTC().Time, Status: models.PAYMENT_STATUS_REFUNDED, }, - Reference: "test1", + Reference: "refund1",
577-591: Enhance test assertions for better coverage and clarity.While the test correctly verifies the final amount, it could benefit from additional assertions to ensure the complete state is correct.
Consider expanding the test:
actual, err := store.PaymentsGet(ctx, pID1) require.NoError(t, err) - // two refunds in the same batch, should be 100 - 10 - 10 = 80 - require.Equal(t, big.NewInt(80), actual.Amount) + // Verify complete payment state after refunds + require.Equal(t, big.NewInt(80), actual.Amount, "Final amount should be 80 (100 - 10 - 10)") + require.Equal(t, big.NewInt(100), actual.InitialAmount, "Initial amount should remain unchanged") + require.Equal(t, models.PAYMENT_STATUS_REFUNDED, actual.Status, "Payment status should be REFUNDED") + require.Len(t, actual.Adjustments, 3, "Should have 3 adjustments (1 payment + 2 refunds)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/connectors/engine/activities/errors.go(0 hunks)internal/storage/payments.go(2 hunks)internal/storage/payments_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/connectors/engine/activities/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/storage/payments.go
Sometimes, we can have two refunds for the same payment inside the same batch, and postgres does not support it and return an error 'command cannot affect row a second time'