-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add createdReportActionID to sendMoney and requestMoney #13502
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
Conversation
|
@eVoloshchak @srikarparsi 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] |
|
Note: the HOLD is for merging only. This is ready for review internally so I'm removing the C+ assignment. |
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.
@youssef-lr What about split bill is that covered in different PRs?
It is I see in slack
|
Hey @youssef-lr, I'm unassigning myself from this pr as I'm taking a break to finish up classes this semester. |
|
Since Youssef is ooo, I will look into this tomorrow morning hopefully. |
flodnv
left a comment
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.
Who's going to do the reviewer checklist? 😅
thienlnam
left a comment
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.
Changes look good, going to run through the checklist
Reviewer Checklist
Screenshots/VideosAndroidAndroid emulator borked, but changes here are pretty safe cross platform |
|
Off hold, merging 🎉 |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to production by @AndrewGable in version: 1.2.55-0 🚀
|







Details
Building the created report action and passing its id to the request money and sen money flows when we create chats as well!
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/247419
Tests
Must be tested with Auth, Web
Offline tests
Send Money
reportActions_<chatReportID>reportActionIDscript/sql.sh>select * from reportActions where reportActionID=<the reportActionID you noted down>;Request Money
The same steps as above, except in the 'Request money' flow.
QA Steps - Regression tests
Refer to this PR for tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
request.money.mov
send.money.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Not needed as the changes are not platform specific.
Android