✨ server: add pax for platinum to signature card upgrades #670
Conversation
🦋 Changeset detectedLatest commit: 1be9a29 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 |
Summary of ChangesHello @aguxez, 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 enhances the server's card issuance API by integrating with PAX for users who are upgrading from a Platinum card to a Signature card. The system now identifies these upgrade scenarios, retrieves necessary user information from Persona, and then registers the user with PAX for associated benefits, all while ensuring that the card issuance flow remains resilient to external service failures. Highlights
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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR adds PAX integration for platinum-to-signature card upgrades. When a user upgrades their card, the system detects the transition, retrieves persona data, derives an internal ID, and invokes PAX's addCapita function to provision travel insurance. Tests and mocks are updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CardAPI as Card API
participant DB as Database
participant Persona as Persona Service
participant PAX as PAX Service
participant Sentry
Client->>CardAPI: POST/GET card request
CardAPI->>DB: Query DELETED platinum cards
DB-->>CardAPI: Check if upgrade from platinum
alt Is Upgrade
CardAPI->>Persona: getAccount(credentialId)
Persona-->>CardAPI: Account with persona data
CardAPI->>CardAPI: Extract identity attributes & derive internalId
CardAPI->>PAX: addCapita(internalId, product: "travel insurance")
alt PAX Success
PAX-->>CardAPI: Confirmation
else PAX Error
PAX-->>CardAPI: Error
CardAPI->>Sentry: Capture error with context
end
end
CardAPI->>DB: Issue signature card
CardAPI-->>Client: Card response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces functionality to track platinum to signature card upgrades by integrating with the PAX system. The changes include adding logic to detect such upgrades, calling an external API (addCapita) with user data, and comprehensive test cases covering various scenarios, including successful integration, no-upgrade cases, and error handling for external API calls. The addition of vi.clearAllMocks() in the test suite's afterEach hook is a good practice for ensuring test isolation and reliability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 383-385: The call to addCapitaForPlatinumUpgrade(credentialId,
account) is intended as fire-and-forget but currently returns a floating
promise; prefix the call with the void operator to make the intent explicit and
satisfy linters (i.e., change the invocation inside the isUpgradeFromPlatinum
block to void addCapitaForPlatinumUpgrade(credentialId, account)). Ensure you do
not await it and keep any surrounding logic unchanged so card creation remains
non-blocking.
In `@server/test/mocks/pax.ts`:
- Around line 3-4: The imports in server/test/mocks/pax.ts are out of order;
move external-library imports before relative imports so "valibot" (InferInput)
is imported before the local "../../utils/pax" (CapitaRequest), ensuring import
order follows eslint-plugin-import conventions (external libs first, then
relative paths) and updating any surrounding import groups similarly.
| if (isUpgradeFromPlatinum) { | ||
| addCapitaForPlatinumUpgrade(credentialId, account); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using void operator to make fire-and-forget intent explicit.
The fire-and-forget pattern is appropriate here to avoid blocking card creation. However, prepending void makes the intent explicit and suppresses potential linter warnings about floating promises.
♻️ Suggested change
if (isUpgradeFromPlatinum) {
- addCapitaForPlatinumUpgrade(credentialId, account);
+ void addCapitaForPlatinumUpgrade(credentialId, account);
}📝 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.
| if (isUpgradeFromPlatinum) { | |
| addCapitaForPlatinumUpgrade(credentialId, account); | |
| } | |
| if (isUpgradeFromPlatinum) { | |
| void addCapitaForPlatinumUpgrade(credentialId, account); | |
| } |
🤖 Prompt for AI Agents
In `@server/api/card.ts` around lines 383 - 385, The call to
addCapitaForPlatinumUpgrade(credentialId, account) is intended as
fire-and-forget but currently returns a floating promise; prefix the call with
the void operator to make the intent explicit and satisfy linters (i.e., change
the invocation inside the isUpgradeFromPlatinum block to void
addCapitaForPlatinumUpgrade(credentialId, account)). Ensure you do not await it
and keep any surrounding logic unchanged so card creation remains non-blocking.
| import type { CapitaRequest } from "../../utils/pax"; | ||
| import type { InferInput } from "valibot"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import order should follow eslint-plugin-import conventions.
External library imports should come before relative path imports. As per coding guidelines, the order should be: react, external libraries, then relative paths.
♻️ Suggested fix
-import type { CapitaRequest } from "../../utils/pax";
-import type { InferInput } from "valibot";
+import type { InferInput } from "valibot";
+
+import type { CapitaRequest } from "../../utils/pax";📝 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.
| import type { CapitaRequest } from "../../utils/pax"; | |
| import type { InferInput } from "valibot"; | |
| import type { InferInput } from "valibot"; | |
| import type { CapitaRequest } from "../../utils/pax"; |
🤖 Prompt for AI Agents
In `@server/test/mocks/pax.ts` around lines 3 - 4, The imports in
server/test/mocks/pax.ts are out of order; move external-library imports before
relative imports so "valibot" (InferInput) is imported before the local
"../../utils/pax" (CapitaRequest), ensuring import order follows
eslint-plugin-import conventions (external libs first, then relative paths) and
updating any surrounding import groups similarly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
==========================================
- Coverage 63.66% 59.67% -4.00%
==========================================
Files 169 169
Lines 5882 5309 -573
Branches 1753 1502 -251
==========================================
- Hits 3745 3168 -577
- Misses 1964 1976 +12
+ Partials 173 165 -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:
|
aguxez
force-pushed
the
pax-card-upgrade
branch
from
e6c1e66 to
1be9a29
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // check if this is an upgrade from platinum to signature | ||
| const deletedPlatinumCard = await database.query.cards.findFirst({ | ||
| where: and( | ||
| eq(cards.credentialId, credentialId), | ||
| eq(cards.status, "DELETED"), | ||
| eq(cards.productId, PLATINUM_PRODUCT_ID), | ||
| ), | ||
| columns: { id: true }, | ||
| }); | ||
| const isUpgradeFromPlatinum = !!deletedPlatinumCard; |
There was a problem hiding this comment.
🔴 Pax enrollment missed for platinum users upgrading via migration path
Users whose platinum card gets deleted during the migration process will not be enrolled in pax travel insurance, even though they are effectively upgrading from platinum to signature.
Click to expand
Root Cause
The isUpgradeFromPlatinum check at lines 342-350 queries for cards with status: "DELETED" and productId: PLATINUM_PRODUCT_ID before the migration loop at lines 354-369 runs. The migration loop marks cards as DELETED if they don't exist in Panda.
Scenario
- User has an ACTIVE platinum card that no longer exists in Panda
deletedPlatinumCardquery runs and finds nothing (card is still ACTIVE)isUpgradeFromPlatinumis set tofalse- Migration loop runs, marks the platinum card as DELETED
- New signature card is created
- Pax enrollment is skipped because
isUpgradeFromPlatinumisfalse
Actual vs Expected
- Actual: User upgrading from platinum via migration doesn't get pax travel insurance
- Expected: User should be enrolled in pax since they had a platinum card that was deleted as part of upgrading to signature
Impact
Users who had platinum cards that were migrated/deleted during the upgrade process miss out on the travel insurance benefit they should receive.
Recommendation: Move the isUpgradeFromPlatinum check to after the migration loop, or track which cards get deleted during migration and check if any of them were platinum cards. For example:
let isUpgradeFromPlatinum = !!deletedPlatinumCard;
for (const card of credential.cards) {
// ... existing logic ...
if (/* card deleted */ && card.productId === PLATINUM_PRODUCT_ID) {
isUpgradeFromPlatinum = true;
}
}Note: This also requires adding productId to the cards query at line 334.
Was this helpful? React with 👍 or 👎 to provide feedback.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const deletedPlatinumCard = await database.query.cards.findFirst({ | ||
| where: and( | ||
| eq(cards.credentialId, credentialId), | ||
| eq(cards.status, "DELETED"), | ||
| eq(cards.productId, PLATINUM_PRODUCT_ID), | ||
| ), | ||
| columns: { id: true }, | ||
| }); | ||
| const isUpgradeFromPlatinum = !!deletedPlatinumCard; | ||
|
|
There was a problem hiding this comment.
Bug: A user can be enrolled in Pax multiple times by repeatedly creating and deleting a signature card, as the upgrade check doesn't track previous enrollments.
Severity: MEDIUM
Suggested Fix
To prevent duplicate enrollments, introduce a mechanism to track whether a user has already received the Pax upgrade benefit. This could be a new flag in the user's profile or a separate table that logs Pax enrollments. Before calling addCapitaForPlatinumUpgrade, check this flag to ensure the user hasn't been enrolled before.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/api/card.ts#L342-L351
Potential issue: The logic for Pax enrollment during a card upgrade can be triggered
multiple times for the same user. A user with a previously deleted platinum card who
creates a signature card gets enrolled in Pax. If they then delete this signature card
and create another one, the system will enroll them in Pax again. This happens because
the upgrade check only looks for a deleted platinum card and doesn't verify if the user
has already been enrolled. The check preventing multiple active cards (`cardCount > 0`)
doesn't account for deleted signature cards, allowing the re-creation flow. This can
lead to duplicate travel insurance policies and potential billing issues.
Did we get this right? 👍 / 👎 to inform future reviews.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.