feat: standardize pay button for expenses and IOUs#66790
feat: standardize pay button for expenses and IOUs#66790grgia merged 27 commits intoExpensify:mainfrom
Conversation
|
@hungvu193 will review this |
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Still under reviewing 👀 |
src/libs/IOUUtils.ts
Outdated
| let lastUsedPaymentMethods: OnyxEntry<LastPaymentMethod>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.NVP_LAST_PAYMENT_METHOD, | ||
| callback: (value) => (lastUsedPaymentMethods = value), | ||
| }); | ||
|
|
There was a problem hiding this comment.
I believe we avoid using Onyx.connect from now on and will remove it completely soon.
FYI: https://expensify.slack.com/archives/C05LX9D6E07/p1748457444774569
I also recently reviewed a PR related to this kind of issue (the function is not PURE function, so when Onyx.connect update the data, the component doesn't re-render).
There was a problem hiding this comment.
Lets test it thoroughly this required a lot of change :/
There was a problem hiding this comment.
Yes we stopped using Onyx.connect. We should avoid adding a new call to it in this PR
Previously reported bugs supposed to be fixed here:
|
|
Reported another bug here |
|
Thanks @joekaufmanexpensify I'll check it today |
| name: policyID, | ||
| }, | ||
| lastUsed: { | ||
| name: policyID, | ||
| }, |
There was a problem hiding this comment.
I don't think this is correct. Do we need this update at all?
There was a problem hiding this comment.
We can pay with Policy, as I remember. that's why we saved it here
There was a problem hiding this comment.
@getusha, shouldn't the name key be a type, not the policyID? ie. CONST.IOU.PAYMENT_TYPE
src/libs/actions/BankAccounts.ts
Outdated
| ], | ||
| }; | ||
|
|
||
| if (personalPolicy?.id && !lastUsedPaymentMethod) { |
There was a problem hiding this comment.
We're not making BE updates for the Bank account creation or policy creation.
I opened a BE issue here https://github.com/Expensify/Expensify/issues/528452
This is the list of affected commands
- ConnectBankAccountWithPlaid
- AddPersonalBankAccount
- RestartBankAccountSetUp
- DeletePaymentBankAccount
- DeleteWorkspace
- MoveIOUReportToPolicyAndInviteSubmitter
- MoveIOUReportToPolicy
There was a problem hiding this comment.
Quick question: lastUsedPaymentMethod is object with several options. For example, if lastUsedPaymentMethod = {expense: CONST.IOU.PAYMENT_TYPE.EXPENSIFY}, don’t we want to update iou in this case?
There was a problem hiding this comment.
So shouldn't it look like this?
if (personalPolicy?.id && !lastUsedPaymentMethod?.iou) {
|
Found one more bug where deleting a workspace is broken. Reported here. |
|
I think this PR introduced the broken test check on main. |
|
@Julesssss PR to fix it is up |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.1.89-1 🚀
|
|
Note follow up: #68027 |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.89-21 🚀
|
| savePreferredPaymentMethod(policyIDKey, option.value); | ||
| }} | ||
| options={paymentButtonOptions} | ||
| onOptionSelected={(option) => handlePaymentSelection(undefined, option.value, triggerKYCFlow)} |
There was a problem hiding this comment.
This change caused
We should only call handlePaymentSelection if paymentButtonOptions.length === 1
| return { | ||
| text: title ?? '', | ||
| description: description ?? '', | ||
| icon: typeof icon === 'number' ? Bank : icon, |
There was a problem hiding this comment.
Hi @getusha, why do we need this change? Is SVG icon broken on native?
There was a problem hiding this comment.
Yes strangely icon has a number value on android Native. Tried to investigate but i wasn't able to get far with the RCA.
| <View style={[styles.alignItemsCenter, styles.flexColumn, styles.flexShrink1]}> | ||
| {primaryText} | ||
| <Text style={[isLoading && styles.opacity0, styles.pointerEventsNone, styles.fontWeightNormal, styles.textDoubleDecker]}>{secondLineText}</Text> | ||
| <Text |
There was a problem hiding this comment.
We neglected to allow truncation here, so we fixed it in #70837
| }; | ||
|
|
||
| const approveButtonOption = { | ||
| text: translate('iou.approve', {formattedAmount}), |
There was a problem hiding this comment.
Hi @getusha @hungvu193 , do you know if there are any steps to follow to access this button on the UI? We’re working on a PR and need to remove the amount from the approve buttons when necessary. If we can see where this button appears in the app, it’ll be easier to determine whether we want to remove it or not.
| buttonOptions.push({ | ||
| text: latestBankItem.at(0)?.text ?? '', | ||
| icon: latestBankItem.at(0)?.icon, | ||
| value: CONST.PAYMENT_METHODS.BUSINESS_BANK_ACCOUNT, | ||
| description: latestBankItem.at(0)?.description, |
There was a problem hiding this comment.
{BZ Checklist #71298 } We don't pass the iconStyles and iconSize here, so Bank account icon has the wrong style in the settlement button
| if (!policyExpenseChatReportID) { | ||
| const {policyExpenseChatReportID: newPolicyExpenseChatReportID} = moveIOUReportToPolicyAndInviteSubmitter(iouReport.reportID, policy.id, formatPhoneNumber) ?? {}; | ||
| savePreferredPaymentMethod(iouReport.policyID, policy.id, CONST.LAST_PAYMENT_METHOD.IOU, lastPaymentMethod?.[policy.id]); | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(newPolicyExpenseChatReportID)); |
There was a problem hiding this comment.
{BZ CHECKLIST} there was a regression, the navigation occurs too soon—before the optimistic report from moveIOUReportToPolicyAndInviteSubmitter is fully merged into Onyx, this was fixed using setNavigationActionToMicrotaskQueue
Explanation of Change
Standardize pay button for IOUs and Expenses
Fixed Issues
$ #61744 #55874 #55875
PROPOSAL:
Tests
Paying IOU for the first time - preview
Pay %amount%buttonPay with personal account, Pay with business account, Mark as paidPay with personal accountPay %amount%buttonPay with business accountis presentPay with business accountPay via %workspace name%optionPaid IOU before - preview
Paying expense report for the first time - preview
Steps
Pay with business accountandMark as paid.Paying expense report for the first time - held expense
Steps
Paid expense report before - preview
Steps
Test 65118:
Test 65118:
Test 65125
Paybutton.Test 65120
Precondition:
User B has paid an expense from User A at least once.
Paybutton is disabled.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))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
Paying IOU for the first time - preview
Paid IOU before - preview
Paying expense report for the first time - preview
Paying expense report for the first time - held expense
Screen.Recording.2025-04-22.at.6.11.01.in.the.evening.mov
Paid expense report before - preview