(SP: 3)[Backend][Shop][Monobank] Webhook apply hardening + safer cleanup (archive test products)#305
Conversation
… in in-app routes and Stripe return_url
…ing, webhook + tests)
…, and signed status token
…elper + ops docs (Stripe unchanged)
…t dedupe + claim/lease TTL, paid terminal, mismatch→needs_review)
… payment-state + fix test cleanup
…e gate, prefer succeeded invoice, update providerModifiedAt
…d; remove test auto-migrations
…rror texts/codes, and harden DB cleanup
…lback; cleanup tests & docs
…nitpicks to follow-up PR
…f hard-delete (PSP_UNAVAILABLE cleanup)
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors Monobank webhook processing into typed, modular helpers (fetching attempts/orders, status transitions, metadata merging, atomic DB updates, restock handling) and propagates Changes
Sequence Diagram(s)sequenceDiagram
participant Receiver as Webhook Receiver
participant DB as Database
participant PaymentState as Payment State Service
participant Restock as Restock Worker
Receiver->>DB: readDbRows(invoiceId, referenceAttemptId)
activate DB
DB-->>Receiver: AttemptRow, OrderRow / null
deactivate DB
Receiver->>Receiver: computeNextProviderModifiedAt(provider_modified_at)
Receiver->>Receiver: buildMergedMetaSql(normalized)
Receiver->>Receiver: validate amount & status
alt Transition requires payment-state change
Receiver->>PaymentState: transitionPaymentStatus(orderId, to)
activate PaymentState
PaymentState-->>Receiver: {ok, applied, reason}
deactivate PaymentState
end
Receiver->>DB: persistEventOutcome(eventId, appliedResult, mergedMeta)
activate DB
DB-->>Receiver: persisted
deactivate DB
alt Applied result is paid or finalized
Receiver->>DB: atomicMarkPaidOrderAndSucceedAttempt / atomicFinalizeOrderAndAttempt
activate DB
DB-->>Receiver: success/failure
deactivate DB
opt restock needed
Receiver->>Restock: restockOrder(orderId, reason, workerId)
activate Restock
Restock-->>Receiver: restock result
deactivate Restock
end
end
Receiver-->>Receiver: buildApplyOutcome() -> response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![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: 87c1314ce0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| orderRow.payment_status === 'paid' && | ||
| (status === 'success' || status === 'processing' || status === 'created') | ||
| ) { |
There was a problem hiding this comment.
In applyWebhookToMatchedOrderAttemptEvent, this paid-state guard now short-circuits status === 'success' to applied_noop, so the success path that updates payment_attempts.provider_modified_at no longer runs for duplicate/later success webhooks. Because out-of-order filtering earlier in the same function relies on attempt.provider_modified_at, a newer success timestamp can be dropped, letting an older reversed/expired event appear fresh and be applied, which can incorrectly move the order into refund/failure handling and trigger restock.
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: 1
🧹 Nitpick comments (6)
frontend/lib/services/orders/monobank-webhook.ts (4)
41-62: Manual snake_case types mirror raw SQL results — maintenance risk.
AttemptRowandOrderRowuse snake_case to match rawdb.execute(sql\...`)results, which bypasses Drizzle's type inference. If a column is renamed or its type changes in the schema, these types silently drift. Consider using Drizzle's typed query builder (.select().from(...)) instead of raw SQL infetchAttemptForWebhookandfetchOrderForAttempt, or at least derive these types from the schema (e.g.,typeof paymentAttempts.$inferSelect`).
519-523: Redundant re-assignment ofappliedResult.At line 522,
appliedResult = 'applied_with_issue'is identical to its initial value on line 485. Same for line 606 vs 583. Theelsebranches are dead code — the value is already'applied_with_issue'.♻️ Remove redundant assignments
} else { - appliedResult = 'applied_with_issue'; + // transition blocked, appliedResult already 'applied_with_issue' }Also applies to: 604-607
789-804: Unknown status events are logged but silently nooped.Unknown/unrecognized Monobank statuses produce
applied_noopwith anUNKNOWN_STATUSerror code persisted to the event. This is reasonable as a safe default, but consider whether these should surface in operational monitoring (e.g., an alert) so that newly introduced Monobank statuses don't go unnoticed.
304-324: Consider refactoring to Drizzle query builder if the complex ORDER BY logic can be simplified.
fetchAttemptForWebhookuses raw SQL withdb.execute(sql\...`)and casts to{ rows?: AttemptRow[] }. While the cast is safe—both drivers in use (neon-http and node-postgres) reliably return a.rowsproperty—raw SQL here is justified by theORDER BY CASE` logic, which the Drizzle query builder cannot easily express. If the priority ordering can be simplified or moved to application logic, consider migrating to Drizzle's query builder for better type safety and consistency.frontend/lib/tests/shop/monobank-psp-unavailable.test.ts (1)
80-84: Minor UUID regex inconsistency withmonobank-webhook.ts.This
isUuidregex requires version nibble[1-5], whileUUID_REinmonobank-webhook.ts(line 65) uses[0-9a-f](any hex digit). Both work fine forcrypto.randomUUID()(v4), but it's worth noting the inconsistency if you ever need to share a single utility.frontend/lib/tests/shop/monobank-webhook-paid-reversal.test.ts (1)
15-22: Consider verifying theguardedPaymentStatusUpdatemock'snoteargument.The mock assertion at lines 130-137 uses
expect.objectContaining(...)but doesn't check thenotefield. The webhook code setsnote: \event:${args.eventId}:${args.status}`. Asserting onnote` would catch accidental changes to the audit trail string.
2 participants
Description
This PR hardens Monobank webhook/apply behavior and improves test cleanup safety to avoid fragile hard-deletes against production-like FK constraints. Large refactors are intentionally deferred to reduce merge risk.
Related Issue
Issue: #<issue_number>
Changes
reversed/failure/expiredevents (they must reach their handlers); keep guard only for informational/duplicate cases.inArray(...)in Monobank order/attempt queries (no behavior change intended).attemptNumberby returning/passing it from the existing attempt row / newly created attempt.Database Changes (if applicable)
How Has This Been Tested?
PowerShell:
npx vitest lib/tests/shop/monobank-*.test.tsnpx vitest lib/tests/shop/*webhook*.test.tsnpx vitest run lib/tests/shopnpm run buildScreenshots (if applicable)
N/A
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Improvements
Bug Fixes
Tests