-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add a created report action for bill splits #13465
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
|
@danieldoglas Please 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 know this is on hold but I could still use an early review @danieldoglas. Also @chiragsalian @youssef-lr @flodnv if you can take a look. |
danieldoglas
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.
LGTM
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
|
@danieldoglas this is ready for you to test please 🙇 Make sure you're on the latest main of Web-E and Auth (just merged some PRs there) |
|
Will test it now, building auth locally. |
|
Test A, step 2 |
src/libs/actions/IOU.js
Outdated
| amount: splitAmount, | ||
| iouReportID: oneOnOneIOUReport.reportID, | ||
| chatReportID: oneOnOneChatReport.reportID, | ||
| createdReportActionID: oneOnOneCreatedReportActionData[0].reportActionID, |
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.
If, on line 368, we fill this property with {}, this will error out
src/libs/actions/IOU.js
Outdated
| return { | ||
| groupData: { | ||
| chatReportID: groupChatReport.reportID, | ||
| createdReportActionID: groupCreatedReportActionData[0].reportActionID, |
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.
This line too
|
@danieldoglas my bad, sorry 🤦 Updated and actually tested this time |
|
@flodnv So, for the test cases that were described in the PR, it worked as expected. But there's a new warning in the console: There's also a new warning in console. Also, when trying to do a split with someone that I already had a chat with (not an IOU split), it fails like this: Looks like is the call from here that fails. This doesn't make a lot of sense since I don't have any IOU with that account, only a created chat. Line 348 in 1cc14f7
|
I don't know if this was a new warning, but it is fixed now. Edit: Okay now it's fixed. The important takeaway is that it's never safe to assume that any props provided by Edit 2: Proposed a new lint rule to enforce this going forward: https://expensify.slack.com/archives/C01GTK53T8Q/p1673641691365339 |
|
Pushed a fix to the duplicate IOU actions even though the bug wasn't introduced by this PR. Not sure how this has never come up in QA before... Edit: Must have been caused by a recent change somewhere because I can't reproduce it on staging |
This comment was marked as off-topic.
This comment was marked as off-topic.
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-01-13.at.1.53.43.PM.movMobile Web - ChromeScreen.Recording.2023-01-13.at.1.58.52.PM.movMobile Web - SafariDesktopScreen.Recording.2023-01-13.at.2.08.33.PM.moviOSScreen.Recording.2023-01-13.at.2.00.17.PM.movAndroid |
|
I'm still consistently running into the bug you mentioned here #13465 (comment) From the global create menu, split a bill with 1 person you DO NOT have an existing chat with Screen.Recording.2023-01-13.at.2.02.59.PM.movHappens on desktop as well |
|
Good testing @thienlnam, it was still happening when you split a bill with only one person. Created https://github.com/Expensify/Web-Expensify/pull/36080 to fix this. |
|
@danieldoglas @thienlnam This is off hold! |
|
For transparency, there is an existing and unrelated problem with the offline Split Bill flow which was not introduced by this PR, and which I reported here |
|
@danieldoglas can you please confirm that you are:
? |
|
I'm using production API, which has Web-E and Auth on latest version with the PRs that this depended on deployed. So this shouldn't be happening |
|
I think it's something in the app, since one of those is appearing on top of the avatar with the The request is |
danieldoglas
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.
LGTM
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to staging by @danieldoglas in version: 1.2.56-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀
|






cc @chiragsalian @youssef-lr @flodnv
Details
Pass the
createdReportActionIDto the backend when splitting a bill.Note: There are some existing bugs on staging when executing these testing steps. I have found that sometimes the IOU previews and messages do not load in the proper order. Regarding the offline test I also found these bugs. I will report them in #expensify-bugs on Slack and work on fixes in a separate PR.
longer grayed out
Edit: I reported the bug in Slack here.
Related Issues
$ https://github.com/Expensify/Expensify/issues/247417
PROPOSAL: N/A
Tests
Pull changes from Auth https://github.com/Expensify/Auth/pull/7329 and Web-E https://github.com/Expensify/Web-Expensify/pull/35799, if they haven't been merged yet.
A
+, selectSplit billSplit $X with <user1> and <user2>+ > Split billand send another split bill requestB
+ > Split billand go through the prompts selecting only ONE participant that you have never split a bill with before.Offline tests
+, selectSplit billSplit $X with <user1> and <user2>(this should be greyed out)+ > Split billand send another split bill requestQA Steps
Same as the tests above but on staging.
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
Online
web-online.mov
Offline
web-offline.mp4
Mobile Web - Chrome
Online
android-Chrome-online.mov
Offline
android-Chrome-offline.mov
Mobile Web - Safari
Online
iOS-Safari-online.mp4
Offline
iOS-Safari-offline.mp4
Desktop
Online
desktop-online.mov
Offline
https://user-images.githubusercontent.com/26260477/207471919-e515f27d-fbfd-46f2-ad35-ae68b9424835.mp4
iOS
Online
iOS-online.mp4
Offline
iOS-offline.mov
Android
Online
android-online.mov
Offline
android-offline.mov