fix: archived reports are unable to mark as read #45240
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
|
@mkhutornyi 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] |
6 tasks
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeScreen.Recording.2024-07-22.at.11.49.34.AM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2024-07-22.at.11.52.25.AM.movMacOS: Desktop |
mkhutornyi
suggested changes
Jul 11, 2024
|
@mkhutornyi This maybe a regression. In my investigation, this only happens because of the Based on this message we went on to create the PR because the backend changes might take some time. For unable to mark as unread, I was only able to reproduce it on some archived read chats. This only happens for chats which have some sort of transactions. When we click on Mark as unread the below code is executed: const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction) ?? '-1';
Report.markCommentAsUnread(originalReportID, reportAction?.created);So this helper function
In the case of where we are unable to mark as unread, we get the
We don't see any update in the LHN because the Videoissue.mp4For other archived chats we don't see this issue, in below video we get the Videoother.mp4Now that we know what's the issue, we want to figure out why is that happening. From git lens, I stumbled upon this PR which introduced the usage of Videofix-1.mp4We can fix this by explicitly checking, if the Videofix-2.mp4We can also explore another solution, which is to check whether the report associated with the result.isUnread = isOriginalReportUnread || ReportUtils.isUnread(report) && !!report.lastActorAccountID;In summary, I don't think these two issues have the same root cause and must be handled separately. |
|
"Mark as read" in LHN context menu works perfect without this PR. Why can't we apply same approach when we open report? |
I am not sure you're following through the findings shared in the slack thread and here. Again, the reason is that |
|
@hurali97 you may think I am asking stupid question but your solution doesn't work for me at all. Please see this video: Screen.Recording.2024-07-18.at.12.27.53.AM.mov |
@mkhutornyi Thanks for showing the video, it's easier and clearer to see the issue now. What we fixed in this PR was the case where the archived report is already unread and in that case, this PR works fine. Now that you're trying to manually mark it as unread and then trying to navigate to that report to mark it as read, it's a different case. I will see why this is not working, thanks 🙂 |
|
@mkhutornyi I tried this and it happened only 1 archived chat. The reason is that we have
Now, I think we can adjust the condition to check if it's an archived room, we will mark it as read, so the below change is what I suggest and it works as well: diff --git a/src/pages/home/report/ReportActionsList.tsx b/src/pages/home/report/ReportActionsList.tsx
index 74629eb0df..6ce1ab4d0f 100644
--- a/src/pages/home/report/ReportActionsList.tsx
+++ b/src/pages/home/report/ReportActionsList.tsx
@@ -547,9 +547,7 @@ function ReportActionsList({
* So the condition should be `<=` to mark the report as read. We will only use this condition when
* the report is archived to not introduce any regression for other reports.
*/
- const isTimeLesserOrEqual = ReportUtils.isArchivedRoom(report)
- ? newMessageTimeReference && newMessageTimeReference <= reportAction.created
- : newMessageTimeReference && newMessageTimeReference < reportAction.created;
+ const isTimeLesserOrEqual = ReportUtils.isArchivedRoom(report) || (newMessageTimeReference && newMessageTimeReference < reportAction.created);
return (
isTimeLesserOrEqual &&
(ReportActionsUtils.isReportPreviewAction(reportAction) ? reportAction.childLastActorAccountID : reportAction.actorAccountID) !== Report.getCurrentUserAccountID()
video.mp4 |
|
Thanks. That solution looks promising. Still need to test and verify on my side. P.S. It would be good to provide clear repro step from the beginning because
|
Great. I mentioned the following steps in the PR description, which you can also take in order to validate whether an unread archived chat is being marked as read:
I have applied the changes. Please let me know if you're able to mark an unread archived chat as read. |
|
@mkhutornyi Any updates on the review? 🙂 |
|
@hurali97 still there's case: Screen.Recording.2024-07-22.at.12.49.34.PM.movAfter debugging, I found that report object flickers when open report. So lastMessageText is sometimes undefined, sometimes "closed this report" |
|
@mkhutornyi Oh my bad. I have adjusted the condition now, we only check if the report is archived and if yes, then we mark it as read, no need for other conditions. Please check this again and let me know, fingers crossed 🙂 |
mkhutornyi
reviewed
Jul 23, 2024
Comment on lines
+547
to
+550
| * If the report is archived, we will mark the report as read. | ||
| */ | ||
| const isArchivedReport = ReportUtils.isArchivedRoom(report); | ||
| const isUnread = isArchivedReport || (newMessageTimeReference && newMessageTimeReference < reportAction.created); |
There was a problem hiding this comment.
Archived reports are always unread? are you sure this won't cause regression after we fix "Mark as unread" bug some day?
There was a problem hiding this comment.
@mkhutornyi No this won't cause any regression. The reason is that this code will only be executed when we click on the LHN item. So if we fix the "Mark as unread" bug in future, it would mean that we will be able to right click on the LHN item and mark that as unread, which is irrespective of the changes here.
Also, since "Mark as unread" currently works for me, so I tested this PR and it works fine 👍
video.mp4
mkhutornyi
approved these changes
Jul 24, 2024
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
84 tasks
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
This was referenced Jul 30, 2024
50 tasks
Closed
6 tasks
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀
|
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
This fixes the issue where the user is unable to mark an archived chat as read by switching to it. Issue is that we check whether the
newMessageTimeReferenceis lesser than thereportAction.createdbut in the case of archived reports, we havenewMessageTimeReferenceequal toreportAction.created. So the fix is to update the condition to account for equal to and also future proof of any regression by using this new condition if the report is archived.Fixed Issues
$ #45228
PROPOSAL: https://expensify.slack.com/archives/C05LX9D6E07/p1720520521629939?thread_ts=1720290639.217409&cid=C05LX9D6E07
Tests
Steps:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
android.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov