Fix incorrect chat opening after merging expense with self DM#79766
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@rojiphil, Pr is ready for your review. thanks |
|
@rojiphil, kindly bump. thanks. |
|
Reviewing today |
|
@marufsharifi Had a fresh look at this today but it looks like we have to fix this issue as the issue mentioned here seems to have got fixed somewhere and is not reproducible. But we will still face this issue. Can you please check this? 79766-issue_002.mp4 |
|
Also, navigation to the self dm report in RHP does not look right. What do you think? 79766-issue_001.mp4 |
@rojiphil, this is not reproducible on the latest main. |
@rojiphil, I think it redirects correctly. After the merge, it redirects to the self-dm report from search, which it should do, but the page doesn't properly show the expenses. I think that's not related to this issue and should be fixed as a follow-up. thanks. Screen.Recording.1404-11-06.at.8.59.42.PM.mov |
|
@rojiphil, kindly bump. thanks |
|
@rojiphil, quick bump, thanks. |
Yeah.. That’s solved. Thanks for the clarification. 79766-ios-hybrid-001.mp4 |
Digged in a little deeper here. And it looks like the RHP displays expense report after the merge operation whereas we are displaying the self DM report here which does not seem right. If the report is being merged from RHP such that self DM report is the destination as demonstrated below, we may want to navigate to the central pane and show the self DM report instead. 79766-query-001.mp4 |
|
@rojiphil Just to make sure I'm following, you are asking if we want to consistently redirect the user to self-DM if they merge an expense to the self-DM from anywhere, including RHP report on the reports page? |
@joekaufmanexpensify Yes. And when we merge an expense from a RHP report to the self-DM, we redirect the user to self-DM currently but this lands on RHP itself. So, I am also asking if the self-DM should instead come up in the central pane? |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Changes make sense from a product perpsective
|
@rojiphil, kindly bump. thanks. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp79766-android-hybrid-001.mp4Android: mWeb Chrome79766-mweb-chrome-001.mp4iOS: HybridApp79766-ios-hybrid-002.mp4iOS: mWeb Safari79766-mweb-safari-001.mp4MacOS: Chrome / Safari79766-web-chrome-002.mp4 |
|
@rojiphil, quick bump. thanks. |
|
@rojiphil, kindly bump. thanks. |
|
@rojiphil, quick bump. thanks. |
|
@marufsharifi Can you please merge with the latest main? I want to test this again. Thanks. |
|
@rojiphil, merged the main. thanks |
|
@rojiphil, kindly bump. thanks. |
|
@marufsharifi Thanks for your patience. Changes LGTM Test 2: Test 3: 79766-web-chrome-011.mp4 |
@rojiphil, thanks for the review, added. Please take another look. thanks. |
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @marufsharifi for the update.
@MonilBhavsar @joekaufmanexpensify Changes LGTM.
Over to you for review. Thanks.
|
thanks! |
|
🚧 @MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@marufsharifi Test 3 in this PR is failing because of a regression issue #83257 The issue is reproducible in: Web Bug7086423_1771884485618.5.mp4 |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.3.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.25-13 🚀
|
Explanation of Change
Ensure the destination report is opened instead of the workspace chat after merging an expense with a self DM expense.
Fixed Issues
$ #78842
PROPOSAL: #78842 (Comment)
Tests
Test 1:
Test 2:
1 Create an expense in Workspace Chat A.
2 Create an expense in Workspace Chat B.
3 Open Reports->Expenses
4 Select the expense in the workspace chat A created in step 1.
5 Click More > Merge.
6 Select expense created in Step 2 > Continue.
7 Select all the details from expense (second option in each section) > Next.
8 Click Merge expenses.
9 Verify that the transaction thread report of the expense report from Workspace chat B is displayed.
Test 3:
1 Create an expense in Workspace Chat A.
2 Create an expense in self DM.
3 Open Reports->Expenses
4 Select the expense in the workspace chat A created in step 1.
5 Click More > Merge.
6 Select self DM expense > Continue.
7 Select all the details from self DM expense (second option in each section) > Next.
8 Click Merge expenses.
9 Verify that the transaction thread report of the self-DM is displayed
Offline tests
Same as Tests
QA Steps
Same as Tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
Screen.Recording.1404-10-20.at.11.19.27.AM.mov
Android: mWeb Chrome
Screen.Recording.1404-10-20.at.11.21.19.AM.mov
iOS: Native
Screen.Recording.1404-10-20.at.11.24.45.AM.mov
iOS: mWeb Safari
Screen.Recording.1404-10-20.at.11.27.44.AM.mov
MacOS: Chrome / Safari
Screen.Recording.1404-10-20.at.11.08.30.AM.mov