🐛 server: require identity-validation completion for ramp tracking#812
🐛 server: require identity-validation completion for ramp tracking#812
Conversation
🦋 Changeset detectedLatest commit: 3c1be87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRequire identity-validation completion on USER_ONBOARDING_UPDATE before triggering RampAccount tracking; export UserOnboardingTasks schema; update tests to cover positive and negative onboarding scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerHook as Manteca Hook
participant Ramp as RampAccount
participant Segment
Client->>ServerHook: USER_ONBOARDING_UPDATE event (user, updatedTasks)
ServerHook->>ServerHook: verify user.status == "ACTIVE" && updatedTasks contains IDENTITY_VALIDATION && user.onboarding.IDENTITY_VALIDATION.status == "COMPLETED"
alt conditions met
ServerHook->>Ramp: RampAccount.track(provider: "manteca", source: null, user)
ServerHook->>Segment: segment.track(...)
else conditions not met
ServerHook-->>Client: no tracking
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello @mainqueg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the server's webhook processing for user onboarding updates, specifically addressing an issue where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #812 +/- ##
==========================================
- Coverage 70.52% 69.82% -0.71%
==========================================
Files 212 212
Lines 8391 7990 -401
Branches 2770 2603 -167
==========================================
- Hits 5918 5579 -339
+ Misses 2227 2157 -70
- Partials 246 254 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/hooks/manteca.test.ts (1)
451-480: 🧹 Nitpick | 🔵 TrivialMissing assertion for
sendPushNotificationin the positive case.The handler calls
sendPushNotificationwhenever the tracking condition fires, but the test only asserts onsegment.track. A regression that removes or changes the push notification call would go undetected.🧪 Suggested addition
+import * as onesignal from "../../utils/onesignal"; ... it("tracks RampAccount when user is active and identity validation is completed", async () => { vi.spyOn(segment, "track").mockReturnValue(); + vi.spyOn(onesignal, "sendPushNotification").mockResolvedValue(undefined); ... expect(segment.track).toHaveBeenCalledWith({...}); + expect(onesignal.sendPushNotification).toHaveBeenCalledWith({ + userId: account, + headings: { en: "Fiat onramp activated" }, + contents: { en: "Your fiat onramp account has been activated" }, + }); });
cruzdanilo
force-pushed
the
onboarding
branch
from
6aa0506 to
2c8911f
Compare
mainqueg
force-pushed
the
onboarding
branch
2 times, most recently
from
fc8a58f to
3ff94c8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/test/hooks/manteca.test.ts (1)
451-480: 🧹 Nitpick | 🔵 TrivialConsider asserting
sendPushNotificationwas called in the positive-path test.The positive path calls
sendPushNotification(lines 233–237 inserver/hooks/manteca.ts), but the test only asserts onsegment.track. Sinceonesignalis already mocked at module level, adding a spy onsendPushNotificationwould lock in the full contract of the activation event.🔍 Proposed addition
import * as segment from "../../utils/segment"; +import * as onesignal from "../../utils/onesignal"; it("tracks RampAccount when user is active and identity validation is completed", async () => { vi.spyOn(segment, "track").mockReturnValue(); + vi.spyOn(onesignal, "sendPushNotification").mockResolvedValue(undefined); // ... rest of test ... + expect(onesignal.sendPushNotification).toHaveBeenCalledWith( + expect.objectContaining({ headings: { en: "Fiat onramp activated" } }), + ); });
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.changeset/ready-groups-beam.mdserver/hooks/manteca.tsserver/test/hooks/manteca.test.tsserver/utils/ramps/manteca.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/test/hooks/manteca.test.ts (2)
451-480: 🧹 Nitpick | 🔵 TrivialLGTM — consider asserting
sendPushNotificationwas also called in the positive path.
track()andsendPushNotification()both live inside the sameifblock in the hook. The test verifiestrackwas called, butsendPushNotification(mocked via../mocks/onesignal) is unasserted. Adding it would provide complete coverage of the successful activation flow.
509-561: 🧹 Nitpick | 🔵 TrivialConsider adding a test for
IDENTITY_VALIDATIONentirely absent from theonboardingobject.The current negative tests cover
IN_PROGRESSstatus and non-ACTIVE user status, but not the case whereuser.onboarding.IDENTITY_VALIDATIONisundefined(the key is absent). SinceOnboardingTaskInfoisoptional(), this is a valid real-world payload shape. The?.status === "COMPLETED"guard handles it correctly, but an explicit test would pin that behaviour.✅ Suggested additional test
+ it("does not track when identity validation task is absent from onboarding", async () => { + vi.spyOn(segment, "track").mockReturnValue(); + const payload = { + event: "USER_ONBOARDING_UPDATE", + data: { + updatedTasks: ["IDENTITY_VALIDATION"], + user: { + email: "test@example.com", + id: "user123", + numberId: "456", + externalId: userExternalId, + exchange: "ARGENTINA", + status: "ACTIVE", + onboarding: {}, + }, + }, + }; + const response = await appClient.index.$post({ + header: { "md-webhook-signature": createSignature(payload) }, + json: payload as never, + }); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toStrictEqual({ code: "ok" }); + expect(segment.track).not.toHaveBeenCalled(); + });server/hooks/manteca.ts (1)
223-238:⚠️ Potential issue | 🟠 MajorAdd guard to verify all conditions before tracking or query Manteca for current state.
Manteca's
USER_ONBOARDING_UPDATEwebhook is not guaranteed to be atomic—the public integration guidance treats it as eventually consistent and recommends querying Manteca for the current onboarding state rather than assuming a single event contains all related updates. If the user status transition and identity validation completion arrive in separate events (e.g., IDENTITY_VALIDATION → COMPLETED first, then user → ACTIVE in a follow-up), theRampAccounttracking will silently never fire because the triple-AND guard will fail on the second event.Either query Manteca's current user state when the webhook arrives to re-verify all three conditions, or implement idempotent tracking that handles split events correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.changeset/ready-groups-beam.mdserver/hooks/manteca.tsserver/test/hooks/manteca.test.tsserver/utils/ramps/manteca.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5b7021622
ℹ️ 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".
| externalId: string(), | ||
| exchange: string(), | ||
| status: picklist(UserStatus), | ||
| onboarding: UserOnboardingTasks, |
There was a problem hiding this comment.
Requiring user.onboarding at validation time causes partial USER_ONBOARDING_UPDATE webhook payloads (that include updatedTasks and user metadata but no full onboarding object) to fail early as bad manteca, so the handler never reaches the activation tracking/push logic for those events. Fresh evidence in this commit is that onboarding tests were rewritten to add user.onboarding everywhere, indicating this shape became mandatory only due to the new validator constraint.
Useful? React with 👍 / 👎.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(response.status).toBe(200); | ||
| await expect(response.json()).resolves.toStrictEqual({ code: "ok" }); | ||
| expect(segment.track).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Missing edge-case test: the current tests all populate onboarding.IDENTITY_VALIDATION, but there's no test for when this key is absent from the onboarding object (which is valid per the OnboardingTaskInfo = optional(...) schema). In that case payload.data.user.onboarding.IDENTITY_VALIDATION?.status === "COMPLETED" evaluates to false, so no tracking fires — which is correct. Adding a test explicitly documents this contract and prevents regressions if the optional chaining is ever refactored:
| }); | |
| it("does not track when identity validation task is absent from onboarding", async () => { | |
| vi.spyOn(segment, "track").mockReturnValue(); | |
| const payload = { | |
| event: "USER_ONBOARDING_UPDATE", | |
| data: { | |
| updatedTasks: ["IDENTITY_VALIDATION"], | |
| user: { | |
| email: "test@example.com", | |
| id: "user123", | |
| numberId: "456", | |
| externalId: userExternalId, | |
| exchange: "ARGENTINA", | |
| status: "ACTIVE", | |
| onboarding: {}, // IDENTITY_VALIDATION absent | |
| }, | |
| }, | |
| }; | |
| const response = await appClient.index.$post({ | |
| header: { "md-webhook-signature": createSignature(payload) }, | |
| json: payload as never, | |
| }); | |
| expect(response.status).toBe(200); | |
| await expect(response.json()).resolves.toStrictEqual({ code: "ok" }); | |
| expect(segment.track).not.toHaveBeenCalled(); | |
| }); |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Greptile Summary
This PR correctly fixes the premature ramp-account tracking bug by adding three guard conditions to the
USER_ONBOARDING_UPDATEhandler:user.status === "ACTIVE"updatedTasksincludes"IDENTITY_VALIDATION"onboarding.IDENTITY_VALIDATION.status === "COMPLETED"The changes are well-tested. Four new test cases cover every tracking scenario:
The test at line 547 explicitly validates that tracking is suppressed when IDENTITY_VALIDATION is absent from the onboarding object, confirming the optional chaining works correctly.
UserOnboardingTasksis now exported fromserver/utils/ramps/manteca.tsto allow direct schema reuse in the hook layer.Confidence Score: 5/5
Last reviewed commit: 3c1be87