-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix(lhn): update for deleted messages #58127
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
fix(lhn): update for deleted messages #58127
Conversation
c24bd96
to
672320c
Compare
After deleting an expense, the GBR is not removed so looking into that and also need to check failing tests |
I was away today, so could not focus on this. I will pick it up as a priority tomorrow and share the updates here. |
@brunovjk while testing I found that the GBR stays for user A even after the expenses are deleted by user B. It is happening from here due to check for hasOutstandingChildRequest in report which is true even after expense is deleted. Similarly, if user B sends a message mentioning user A then deletes it. It stays as unread for user A due to logic in isUnreadWithMention For both of these, I think we should update backend because the indicators can be for older report actions that are not yet loaded in front-end. |
Great! I will review it first thing tomorrow and let you know. Thank you :D |
Thank you @jaydamani, I think you RCA make sense, I asked @thienlnam to check for us. Thank you. |
Minor bump @thienlnam |
Sorry, was OOO for a couple days and catching up on things - I'll try to take a look tommorow |
@thienlnam minor bump for updates |
To confirm, when an expense is deleted we want the parent report to have hasOutstandingChildRequest set to false? Similarly for a mention - if a mention is deleted we should have isUnreadWithMention set to false for the report the comment was in? |
Yes but if there are any other outstanding child request then it should not changed
Yes and also update cc: @brunovjk |
This comment was marked as off-topic.
This comment was marked as off-topic.
Okay sounds good, I've started looking into it - and have made some progress. Will aim to get a PR into review next week. Have a few other priorities going on |
Okay, looks like someone else got to the mention issue so that should be fixed. Still working on the deleted transaction update |
@jaydamani Could you merge main here? I want to do some testing. |
@brunovjk Merged with main, let me know if any updates are required |
Looking into this and the expense report looks incorrect for the receiver/user A. It shows only the total but it should be a single expense report |
Let me check, I'm going back to the office now. |
Sorry for the delay @jaydamani. I believe so, is this the issue you are referring to: Screen.Recording.2025-04-21.at.18.31.15.movI don't know if it is within the scope of our original issue, what do you think? But anyway I will check if there is not already a reported issue. |
@brunovjk I meant the report total shows 2$ but there are no expenses or any other detail but I also can't replicate that anymore so I think it is resolved and yeah it was probably out of scope for this PR. I think the only thing left here is the backend PR |
I think I misunderstood then. But anyway I'll ask, in slack, for them to confirm the "Expected result" of these situations. Thanks for the re-test, let's wait for Jack with the changes in the backend then. |
@thienlnam any update for us here? Thanks. |
Haven't had a chance to look into this yet unfortunately |
@thienlnam any updates here 👀 |
Still no update yet, but I've gotten through some of the things last week and so should have more time this week to take a look |
Screen.Recording.2025-05-09.at.6.39.47.PM.movFound the issue, and sending a PR into review! |
Alright, BE fix is live now |
Great! @jaydamani could you please merge main, I just want to do one last check and we can go. Thanks. |
…te-lhn-on-delete--57694
@brunovjk merged main |
Just an heads up, |
@jaydamani did you test it again? It seems that the initial issue is back here: Screen.Recording.2025-05-16.at.12.26.23.movI just gave it a quick test, I can investigate it further later. |
@brunovjk Sorry I couldn't test earlier. Record_select-area_20250518165142.mp4 |
Strange, now that I tested it everything works fine @jaydamani. Sorry, I must have done something wrong last time |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUploading 58957_android_native.mov… Android: mWeb ChromeUploading 58957_android_web.mov… iOS: HybridAppUploading 58957_ios_native.mov… iOS: mWeb SafariUploading 58957_ios_web.mov… MacOS: Chrome / SafariUploading 58957_web_chrome.mov… MacOS: DesktopUploading 58957_web_desktop.mov… |
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. Notes:
- We just have to pay attention to this comment
- The videos in the "Screenshots/Videos" section of my checklist were not uploaded, something is happening with my internet, I will come back tomorrow and try to upload again.
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.
Nice!
@thienlnam looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Failing due to
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
When last message of a report is deleted use getReportLastMessage instead of report.lastMessageText
Fixed Issues
$ #57694
PROPOSAL: #57694 (comment)
Tests
User A logged in on the main testing device, User B - on secondary device
Offline tests
NA
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android.mp4
Android: mWeb Chrome
mweb-a.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-mweb.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov