Adapt Split Bill to workspace chats #24058
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youssef-lr
changed the title
melvin-bot
bot
requested review from
thesahindia
and removed request for
a team
|
@thesahindia 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] |
youssef-lr
added
the
Internal
melvin-bot
bot
requested review from
rushatgabhane
and removed request for
a team
|
@rushatgabhane 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] |
|
Sorry folks I'm trying to get an internal engineer assigned but it won't work even though I have the |
youssef-lr
force-pushed
the
youssef_splitbill_workspace
branch
from
0c63b50 to
11cfc57
Compare
|
Hey @youssef-lr not sure if you are aware of this behaviour: After I create the "Split Bill" if I click on the first report action created the web app Crashes. Everything starts working again after I click on "refresh" though: Screen.Recording.2023-08-10.at.15.31.03.mov |
|
@PauloGasparSv should be fixed now! |
youssef-lr
commented
Aug 10, 2023
|
Hey @youssef-lr tested this on Web and is looking great!!!! Do you mind taking a look at the following before I test other platforms? When I open the "bill report" (the following image) I'm redirected to the correct report but there is a warning on the console. 
Do you mind checking why that is happening, I tested creating a Split Bill on a 1:1 chat in Staging and I don't think we have that warning there |
|
Hmm I managed to produce it once but never consistently, still on it. |
|
@PauloGasparSv I think that got fixed in Web-E main, after merging main on Web-E I can no longer see the error. I'll push an update to the Web-E PR. |
Nice, re-testing this with the update Web-e branch! |
|
Looks like everything is working fine!
I see things a little different here but I think it's still correct, since I logged in as the teacher I only see 2 participants: The teacher and the Policy Owner (I do not see the Principal/manager there). Despite that, the manager id and email used in the Split Bill report is the Pricinpal's/Manager's so I think this is ok. Screenshots |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-17.at.13.29.58.movScreen.Recording.2023-08-17.at.13.46.24.movMobile Web - ChromeScreen.Recording.2023-08-17.at.17.05.57.movMobile Web - SafariScreen.Recording.2023-08-17.at.17.03.14.movDesktopScreen.Recording.2023-08-17.at.16.50.04.moviOSScreen.Recording.2023-08-17.at.16.55.48.movAndroidScreen.Recording.2023-08-17.at.18.47.06.mov |
|
@youssef-lr can you please complete the Author's checklist? Will wait on that to finish testing on all platforms : ) |
|
On it! I just merged Web-E, we'll still need to wait for it to deploy before merging this. |
youssef-lr
changed the title
| @@ -132,8 +132,13 @@ function IOUPreview(props) { | |||
| const sessionAccountID = lodashGet(props.session, 'accountID', null); | |||
| const managerID = props.iouReport.managerID || ''; | |||
| const ownerAccountID = props.iouReport.ownerAccountID || ''; | |||
| const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(props.chatReport); | |||
There was a problem hiding this comment.
I think we should add this chatReport to the propTypes and defaultProps
| @@ -1852,6 +1868,7 @@ function getIOUReportActionMessage(type, total, comment, currency, paymentType = | |||
| * @param {Boolean} [isSettlingUp] - Whether we are settling up an IOU. | |||
| * @param {Boolean} [isSendMoneyFlow] - Whether this is send money flow | |||
| * @param {Object} [receipt] | |||
| * @param {Boolean} [isOwnPolicyExpenseChat] - Whether this is an expense report create from the current user's policy expense chat | |||
There was a problem hiding this comment.
Suggested change
| * @param {Boolean} [isOwnPolicyExpenseChat] - Whether this is an expense report create from the current user's policy expense chat | |
| * @param {Boolean} [isOwnPolicyExpenseChat] - Whether this is an expense report created from the current user's policy expense chat |
Nitpicking here!
|
@youssef-lr tests are looking good but can you check one thing? In the following evidence (at ~1:14) I opened the split bill report and it quickly showed a negative value. Do you mind taking a look if it happens for you too? Screen.Recording.2023-08-17.at.13.29.58.movAlso left 2 very small review comments : ) |
|
I couldn't reproduce this after creating another Split Bill in the same Policy/Workspace! But I was able to reproduce it again after creating a new Control Policy* (re-doing all test steps with the same accounts): Screen.Recording.2023-08-17.at.13.46.24.mov |
|
Checking now @PauloGasparSv! |
|
Sorry @PauloGasparSv I'm trying to fix an issue in my dev env https://expensify.slack.com/archives/C05F0U613LY/p1692292276076729, it's preventing me from testing properly |
|
Only missing android now (build is super slow here), testing good on all platforms! I'll check Trying to run using Android Studio (also following the recent "[ACTION REQUIRED] Manually update your NewDot XCode scheme" email for that) Still waiting on this: I'm having this issue now (CloucFlare certificate related I think), trying to fix it! |
|
Thank you so much @PauloGasparSv! |
|
Still waiting for my latest build attempt after following this! Will merge this asap : )
|
PauloGasparSv
approved these changes
Aug 17, 2023
There was a problem hiding this comment.
LGTM and tests well on all platforms.
Android took a huge amount time to Build and was really slow, prob related to Cloudflare. Sorry for the delay anyway.
I left a couple of comments but I think they don't need to be addressed right now, going to merge this already!
|
✋ 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/PauloGasparSv in version: 1.3.56-0 🚀
|
|
@youssef-lr This is ready to QA |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/303750
Tests
Must be tested with Auth and Web-E PRs.
[email protected]and[email protected]submitsToof[email protected]to[email protected], meaning the principal will be the manager of the teacher.managerIDof the report belongs to the[email protected].Offline tests
QA Steps
Regression QA
Test that the normal flow of split bill in group DMs / from Global create with a single participant work properly.
For the rest of the QA, please ping me.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android