-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix - Add BA thread is missing Add button, incomplete thread header and "Hidden" user #38884
Fix - Add BA thread is missing Add button, incomplete thread header and "Hidden" user #38884
Conversation
bump @akinwale |
Thanks for the bump. I'll review this later today or tomorrow at the latest. |
Reviewing today. |
@FitseTLT I am unable to complete the business bank account process as I'm stuck on step 5 and I am unable to use the Pay with Expensify button. How did you get past this? |
You need to make |
Yes, I already did this and completed the steps up to the last step. Then I'm asked to chat to finish setup. |
Here is how I finished the flow @akinwale 2024-04-01.13-59-25.mp4 |
Ah, I see. That's the wallet flow which is different from the VBA flow. I'll get this set up and then I should be able to finish the review. Thanks. |
bump @akinwale |
@FitseTLT I'm still stuck trying to test settling a payment as the workspace admin. Even after adding a bank account, I get stuck in a loop as shown in this video. Any ideas? bba-loop.mp4 |
@akinwale What we need to test is totally different to what you are showing me. while the employee bank account is not set then when you approve any iou report the |
Ah, I see. I was working under the assumption that the workspace admin should have a bank account which would be used to settle the employee IOU (based on the listed test preconditions). Thanks for the clarification. I will have another look in a couple of hours. |
Nope U only add bank account on the collect policy (reimbursement -> Direct). YOur workspace should have USD default currency and then on OD U can follow this instruction. Now, when u approve an employee's report who hasn't set their bank account that |
Reviewing today. |
Ok, finally got it to work. Will finish reviewing shortly. The Add bank account message shows up after the request is approved, not after payment, which is where I got mostly confused as I kept trying to "Pay with Expensify" which is where I got stuck. I also did not have the Reimbursement setting set correctly on OD. @FitseTLT Please update the test steps as follows: Precondition: Also need to add this to the preconditions.
Test step: |
Reviewer Checklist
Screenshots/VideosAndroid: Native38884-android-native.mp4Android: mWeb Chrome38884-android-chrome.mp4iOS: Native38884-ios-native.mp4iOS: mWeb Safari38884-ios-safari.mp4MacOS: Chrome / Safari38884-web.mp4MacOS: Desktop38884-desktop.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@FitseTLT Please fix the test steps and then I can approve.
Wooh. Very very sorry @akinwale I have really played a substantial role in your confusion, it really surprised me when I saw it was like that. I have updated according to your request but only this part
, I think, would add more confusion because it is already included in this step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, there just an unnecessary line break to remove and then I'll approve and merge this.
Let's merge this @chiragsalian |
✋ 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/chiragsalian in version: 1.4.63-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
return ''; | ||
} | ||
if (ReportActionsUtils.isApprovedOrSubmittedReportAction(reportAction)) { | ||
return ReportActionsUtils.getReportActionMessageText(reportAction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details
Fixed Issues
$ #38126
PROPOSAL: #38126 (comment)
Tests
Precondition:
Hidden
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
nat.mp4
Android: mWeb Chrome
a.w.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
i.w.mp4
MacOS: Chrome / Safari
w.mp4
MacOS: Desktop
d.mp4