Implement freeze card option#81880
Conversation
melvin-bot
Bot
requested review from
JmillsExpensify and
truph01
and removed request for
a team and
JmillsExpensify
3 similar comments
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@Pujan92 will be reviewing. |
| ); | ||
| }} | ||
| /> | ||
| {isBetaEnabled(CONST.BETAS.FREEZE_CARD) && isCardHolder && !isCardFrozen(currentPhysicalCard) && ( |
There was a problem hiding this comment.
❌ PERF-2 (docs)
Reasoning: The expensive operations isBetaEnabled(CONST.BETAS.FREEZE_CARD) and isCardFrozen(currentPhysicalCard) are performed before checking the simple condition isCardHolder. If isCardHolder is false, the expensive checks are wasted.
Suggested fix: Reorder the conditions to check the simplest ones first:
{isCardHolder && isBetaEnabled(CONST.BETAS.FREEZE_CARD) && !isCardFrozen(currentPhysicalCard) && (
<MenuItem
icon={expensifyIcons.FreezeCard}
title={translate('cardPage.freezeCard')}
style={styles.mt3}
disabled={isOffline}
/>
)}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I think current condition is better since it will stop checking if beta is not enabled.
![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: dd5c63bbfb
ℹ️ 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".
|
These rows look very far apart, could you confirm the spacing and layout here? Might wanna run a test build so I can play around as well |
|
Happy to a run test build, one sec 🚀 |
MariaHCD
left a comment
MariaHCD
left a comment
There was a problem hiding this comment.
Looks good overall!
There are some type check failures:
Error: tests/unit/CardUtilsTest.ts(1885,26): error TS2352: Conversion of type '{ state: 7; nameValuePairs: { frozen: boolean; }; }' to type '{ cardID: number; state: ValueOf<{ readonly STATE_NOT_ISSUED: 2; readonly OPEN: 3; readonly NOT_ACTIVATED: 4; readonly STATE_DEACTIVATED: 5; readonly CLOSED: 6; readonly STATE_SUSPENDED: 7; }>; ... 24 more ...; nameValuePairs?: ({ ...; } & ... 2 more ... & OfflineFeedback<...>) | undefined; } & OfflineFeedback<...>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
Type '{ state: 7; nameValuePairs: { frozen: boolean; }; }' is missing the following properties from type '{ cardID: number; state: ValueOf<{ readonly STATE_NOT_ISSUED: 2; readonly OPEN: 3; readonly NOT_ACTIVATED: 4; readonly STATE_DEACTIVATED: 5; readonly CLOSED: 6; readonly STATE_SUSPENDED: 7; }>; ... 24 more ...; nameValuePairs?: ({ ...; } & ... 2 more ... & OfflineFeedback<...>) | undefined; }': cardID, bank, domainName, lastUpdated, fraud
Error: tests/unit/CardUtilsTest.ts(1903,26): error TS2352: Conversion of type '{ state: 3; nameValuePairs: { frozen: boolean; }; }' to type '{ cardID: number; state: ValueOf<{ readonly STATE_NOT_ISSUED: 2; readonly OPEN: 3; readonly NOT_ACTIVATED: 4; readonly STATE_DEACTIVATED: 5; readonly CLOSED: 6; readonly STATE_SUSPENDED: 7; }>; ... 24 more ...; nameValuePairs?: ({ ...; } & ... 2 more ... & OfflineFeedback<...>) | undefined; } & OfflineFeedback<...>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
Type '{ state: 3; nameValuePairs: { frozen: boolean; }; }' is missing the following properties from type '{ cardID: number; state: ValueOf<{ readonly STATE_NOT_ISSUED: 2; readonly OPEN: 3; readonly NOT_ACTIVATED: 4; readonly STATE_DEACTIVATED: 5; readonly CLOSED: 6; readonly STATE_SUSPENDED: 7; }>; ... 24 more ...; nameValuePairs?: ({ ...; } & ... 2 more ... & OfflineFeedback<...>) | undefined; }': cardID, bank, domainName, lastUpdated, fraud
MariaHCD
left a comment
MariaHCD
left a comment
There was a problem hiding this comment.
@ShridharGoel could you take a look at the merge conflicts?
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
PR reviewer checklist failed bcoz it considers first checklist comment of @truph01. @truph01 can you plz delete comment #81880 (comment) |
|
All yours @dangrous ! |
|
🚧 @dangrous has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.3.21-0 🚀
|
|
@ShridharGoel @MariaHCD Are expensifail accounts behind beta to have freeze card feature? |
|
@ShridharGoel, no 
|
|
@MariaHCD Do you think it will be fine to QA test this feature once it is ready for release 1 ? |
|
For now, QA is not under the Freeze Card beta as the feature is still a work in progress. Once we have Release 1 ready, then we will add QA to the beta and it will be ready for testing 👍🏼 |
|
@MariaHCD thanks. Can we check it off for now? |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.21-4 🚀
|
11 participants
Explanation of Change
Implement freeze card option.
Fixed Issues
$ #81638
PROPOSAL:
Tests
Precondition: Have an Expensify card.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari