-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Handle isWaitingOnBankAccount for IOU requests #20821
Conversation
a8ae87e
to
1a418ba
Compare
acd1d97
to
bef9a22
Compare
3c66ef4
to
c341b44
Compare
@nkuoch is this PR ready for review? |
@luacmartins yes for a first pass of code review and main case test. Meanwhile, I'll add more tests. Just don't forget to pull the auth and web PRs if you want to test it. |
@luacmartins all yours I cannot merge this but I agree we should merge this and do any polish later. |
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.
I tested the flow again today and I'm seeing two bugs that I think are blockers:
BUG 1: Expense report disappears from LHN and ReportScreen
- As the admin, pay the money request
- Notice that the report temporarily disappears from the LHN and ReportScreen and we're navigated to the Concierge chat
BUG 2: Settlement button is still visible after paying and crashes the App
- After the steps above, navigate back to the Expense report once it reappears in the LHN
- Notice that the settlement button is still visible (it shouldn't)
- Tap the button
- Notice that the App crashes
bug.mov
BUG 3: Settlement is not triggered when employee adds bank account
- After the payment was initiated by the admin, as the employee, add your bank account
- Notice that the
added a bank account...
message doesn't show up and the payment is not automatically triggered.
bug2.mov
Let me know if I'm missing something.
Thanks for addressing those @nkuoch. Retesting this now. |
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.
I started a group DM so we can get this merged faster, but I'm still seeing some issues when the employee adds a bank account:
web.mov
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 and tests well.
Since @mountiny had already reviewed this PR but then committed to the branch and C+ won't be able to reliably test this flow given the restricted access to bank accounts, I'm gonna go ahead and merge this PR. We'll address the smaller issues in follow up PRs.
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ 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/luacmartins in version: 1.3.46-0 🚀
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.47-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.46-2 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.47-6 🚀
|
onButtonPress={() => { | ||
Navigation.navigate(ROUTES.SETTINGS_PAYMENTS); | ||
}} | ||
onButtonPress={this.exitFlow} |
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.
👋 Coming from #38558, this has caused a small issue where the same page was added twice to the navigation stack
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/270705
$ #22763
Tests
Open browser 1:
Create user admin@thedomain20821.com
Create new workspace and invite employee2@thedomain20821.com
Create workspace OPEN vbba using test credentials
Open browser 2:
Sign into employee2@thedomain20821.com
In the workspace chat, request money
Chat view:
On browser 1 (admin@thedomain20821.com), you should see the request
Chat view:
Request view:
Click on Pay with Expensify.
You should see "started settling up, payment is held until ... adds a bank account":
Request view:
On browser 2 (employee2@thedomain20821.com)
You should see a green dot and a button to add your bank account.
Click on the button and add a personal bank account. You should see the payment as settled:
Request view:
On browser 1 (admin@thedomain20821.com), make sure you see the request as Paid
Chat view:
Request view:
Offline tests
N/A
QA Steps
Same as Tests
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
The screenshots and videos can be seen in the comments of the PR. Also the reviewer included all the platforms they could run at the time. Checking off the checkbox.
Web
https://user-images.githubusercontent.com/36083550/253777608-39e9fdfe-0bdf-4671-94ac-7f8b6a5d26fa.png
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android