fix: 81235 Tighten check for last payment method when using bulk options - #81296
Conversation
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4092fbff5
ℹ️ 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".
| const {paymentType, selectedPolicy, shouldSelectPaymentMethod} = getActivePaymentType(item.key, activeAdminPolicies, latestBankItems, policy?.id); | ||
| // Policy id is also a last payment method so we shouldn't early return here for that case. | ||
| if (!isValidBulkPayOption(item) && !selectedPolicy) { | ||
| const hasPolicyLastPaymentMethod = selectedPolicy && !!lastPaymentMethods?.[selectedPolicy.id]; | ||
| // Early return if item is not a valid payment method and user has no saved payment method for this policy | ||
| if (!isValidBulkPayOption(item) && !hasPolicyLastPaymentMethod) { |
There was a problem hiding this comment.
Allow pay-via-workspace on first-time bulk pay
The new guard requires lastPaymentMethods[selectedPolicy.id] to exist before allowing a policy-id option through. However the bulk-pay submenu for first-time payments includes policy-id entries (from useBulkPayOptions), and the whole point of this path is that there may be no saved last payment method yet. In that scenario (first bulk payment via a workspace), this early return prevents any action when the user selects “Pay via workspace,” so nothing happens. Consider allowing policy-id options even when there is no lastPaymentMethods entry yet, or gate this check on whether the option actually represents a saved last payment method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This comment might actually be true so looking into it
There was a problem hiding this comment.
Indeed this one case wasn't working. Updated accordingly
|
@dominictb Please review this PR |
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.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-03.at.20.48.27.mov |
|
Requested CP to staging for the PR |
…e-opens fix: 81235 Tighten check for last payment method when using bulk options (cherry picked from commit 80d94cb) (cherry-picked to staging by Julesssss)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.3.11-35 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.3.11-36 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
Explanation of Change
Tightened check for last payment method in bulk pay. Previously we were only checking whether selectedPolicy exists but it needs to be narrowed down to whether selectPolicy exists and was used as lastPaymentMethod (Pay via workspace) option
Fixed Issues
$ #81235
PROPOSAL:
Tests
Same as QA steps
Offline tests
QA Steps
Precondition:
Account is unverified.
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
81235.mov