-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make sure that we reduce the unread message count when we delete an unread message #6746
Conversation
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.
Tests well and lgtm minus one linting change
src/libs/actions/Report.js
Outdated
if (!message.text) { | ||
setLocalLastRead(reportID, lastReadSequenceNumbers[reportID]); | ||
} |
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.
is this what the linter is looking for?
if (!message.text) { | |
setLocalLastRead(reportID, lastReadSequenceNumbers[reportID]); | |
} | |
if (message.text) { | |
return; | |
} | |
setLocalLastRead(reportID, lastReadSequenceNumbers[reportID]); |
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.
I can't see the whole code on GH mobile app 🤭. But this function should be called on live delete pusher event.
I think it is but just to be sure.
src/libs/actions/Report.js
Outdated
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge); | ||
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge).then(() => { | ||
// If the message is deleted, update the last read in case the deleted message is being counted in the unreadActionCount | ||
if (!message.text) { |
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.
You should check html instead of text just to be consistent across app.
Updated! |
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 👍 will leave this for Jules to final approve/merge
✋ 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 @Julesssss in version: 1.1.20-3 🚀
|
🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀
|
@Julesssss please review
cc @parasharrajat
Details
This will call
setLocalLastRead
when we see that the message being updated is getting deleted. Also a small change to make a variable name a bit clearer.Fixed Issues
$ #6472
Tests/QA
Video:
https://user-images.githubusercontent.com/4741899/145924224-69ac2239-bc67-495c-84db-5b2e8539d62f.mp4
1
Video:
https://user-images.githubusercontent.com/4741899/145924341-b8f5ea80-1e36-40e5-a832-f1360098e334.mp4
1
Video:
https://user-images.githubusercontent.com/4741899/145924454-2dc772b3-0dcd-4e90-b759-b1eb6c7dc48e.mp4