-
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
Update optimistic data to match IOU action notification templates, send back additional data needed by Settle Up templates #17090
Update optimistic data to match IOU action notification templates, send back additional data needed by Settle Up templates #17090
Conversation
|
@Santhosh-Sellavel @dangrous 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] |
I'll test it out when read PR comes off hold |
@Santhosh-Sellavel I've updated the testing steps, good for another look! I took the HOLD off, turns out both these PRs need to be held on each other, but it's better for this one to go out first |
…ionMessagingWorkspaceIOURequests
All right thanks will test it out shortly |
Can you resolve conflicts, please @jasperhuangg |
bump @jasperhuangg |
any updates on this one? |
Seems Jasper is on OOO |
https://github.com/Expensify/Auth/pull/7785 is on prod and I'm taking this off hold cc @Julesssss @Santhosh-Sellavel @dangrous @Santhosh-Sellavel you should be able to retest this, let me know if you're still running into that error |
Cool testing this now! |
@jasperhuangg This step is confusing
User B is Currency is GBP right? Should it be like Verify the amount shown in user B's currency? |
@Santhosh-Sellavel There should be some text above the IOU action in Japanese Yen like |
Why should be in GBP? I don't get the context User A's currency is EUR. So it should be shown in EUR or JPY or whatever the currency of User B. Confused |
NormalScreen.Recording.2023-04-29.at.1.50.51.AM.movCurrency Change FlowScreen.Recording.2023-04-29.at.2.02.14.AM.mov |
For some time the report action is shown in JPY is that fine or an issue? Check the above video |
@Santhosh-Sellavel that's fine |
Sorted this out via DMs, there was a typo in the testing instructions. |
Reviewer Checklist
Screenshots/VideosIncluding Web/Desktop alone as changes don't affect UI. Note: |
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, thanks!
All yours @dangrous or @Julesssss!
🎯 @Santhosh-Sellavel, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #18166. |
@dangrous @Julesssss This PR should be ready to go, let's get it merged! |
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!
Gonna merge this since we have 3 approvals. |
Actually, I'll let @Julesssss do a final review since he encountered some issues that I couldn't reproduce. |
Yep, it's on my list for today. Catching up after UK bank holiday. |
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 retested all the test cases today and it's working as expected 👍
✋ 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/Julesssss in version: 1.3.9-12 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/270495
Tests/QA
Once the server responds, the report action may change. This will be fixed in https://github.com/Expensify/Web-Expensify/pull/36977
Request Money
Thursday lunch
.Requested $20 for Thursday lunch
.Split bill
Thursday lunch
.Requested $20 for Thursday lunch
.Settling up
Settled up $20
.Requesting Money/Settling Up in a non-US currency
Thursday lunch
.Requested $20 for Thursday lunch
.Splitting Bills/Settling Up with different initial IOU currency/output currency
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Web
web.mov
Mobile Web - Chrome
mweb_chrome.mov
Mobile Web - Safari
mweb_safari.mov
Desktop
desktop.mov
iOS
ios_native.mov
Android
android.mov