Revert "Merge pull request #83638 from samranahm/82853/allow-to-pay-w…#85279
Revert "Merge pull request #83638 from samranahm/82853/allow-to-pay-w…#85279luacmartins wants to merge 1 commit intomainfrom
Conversation
melvin-bot
bot
requested review from
Gonals,
JmillsExpensify and
ikevin127
and removed request for
a team
|
@ikevin127 @Gonals One of you needs to 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] |
|
on it 👍 |
![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: 7664f392bf
ℹ️ 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".
| confirmPayment?.(paymentType, pendingPaymentAdditionalDataRef.current); | ||
| pendingPaymentAdditionalDataRef.current = undefined; | ||
| }} | ||
| onSuccessfulKYC={(paymentType) => confirmPayment?.(paymentType)} |
There was a problem hiding this comment.
onSuccessfulKYC now forwards only paymentType, but handleBulkPayItemSelected() sends Expensify/VBBA choices through triggerKYCFlow() and no longer calls confirmPayment directly. That drops submenu additionalData (such as payAsBusiness and selected account info), so bulk invoice payments can silently fall back to defaults in onBulkPaySelected and execute with the wrong payment configuration.
Useful? React with 👍 / 👎.
| additionalData: { | ||
| payAsBusiness, | ||
| bankAccountID: formattedPaymentMethod.methodID, | ||
| methodID: formattedPaymentMethod.methodID, |
There was a problem hiding this comment.
Invoice submenu items now store the selected account as additionalData.methodID, but the bulk-pay submit path still reads paymentAdditionalData.bankAccountID in useSearchBulkActions. In flows that do not enter KYC (for example when latestBankItems is empty), the chosen account ID is ignored and the code falls back to the last policy bank account, so the user’s explicit account choice is not honored.
Useful? React with 👍 / 👎.
| if (originalMessage?.type === CONST.IOU.REPORT_ACTION_TYPE.PAY) { | ||
| const reportPolicy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`]; | ||
| const last4Digits = getBankAccountLastFourDigits(originalMessage?.bankAccountID, bankAccountList, reportPolicy); | ||
| const last4Digits = reportPolicy?.achAccount?.accountNumber?.slice(-4) ?? ''; |
There was a problem hiding this comment.
This now always derives last4Digits from the policy’s current ACH account, instead of the account ID recorded on the payment action. For VBBA/Expensify pay actions, if the policy bank account has changed (or the payment used a different account), the displayed/copied “paid with account ****” message becomes inaccurate and no longer reflects the account actually used.
Useful? React with 👍 / 👎.
|
hmm there are several checks failing. Maybe it's better to let the original author/C+ work on the fix since I'm not very familiar with these flows. |
|
@ChavdaSachin there are several checks failing. Let's wait for the PR author to continue their fix. |
|
That is due to a bad conflict resolution. |
| setIsHoldMenuVisible(false); | ||
| isSelectionModePaymentRef.current = false; | ||
| }} | ||
| onClose={() => setIsHoldMenuVisible(false)} |
There was a problem hiding this comment.
| onClose={() => setIsHoldMenuVisible(false)} | |
| onClose={() => { | |
| setIsHoldMenuVisible(false); | |
| isSelectionModePaymentRef.current = false; | |
| }} |
| @@ -408,8 +407,6 @@ function BaseReportActionContextMenu({ | |||
| currentUserAccountID: currentUserPersonalDetails?.accountID, | |||
| currentUserPersonalDetails, | |||
| encryptedAuthToken, | |||
There was a problem hiding this comment.
| encryptedAuthToken, | |
| encryptedAuthToken, | |
| iouTransaction, | |
| @@ -289,8 +288,6 @@ type ContextMenuActionPayload = { | |||
| showDelegateNoAccessModal?: () => void; | |||
| currentUserPersonalDetails: ReturnType<typeof useCurrentUserPersonalDetails>; | |||
| encryptedAuthToken: string; | |||
There was a problem hiding this comment.
| encryptedAuthToken: string; | |
| encryptedAuthToken: string; | |
| iouTransaction: OnyxEntry<Transaction>; |
|
Accept the suggestions and that should vanish |
|
Ran tests locally, all tests passed. |
|
Closing this one, we merged and CP'd the hotfix |
|
We merged a fix. We can close this PR. |
3 participants
Explanation of Change
This reverts commit d0cc2f3, reversing changes made to 3fec5bc.
Fixed Issues
$ #85180
Tests
Precondition :
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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