Skip to content
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

Remove deleted messages from the unread message count #6248

Merged
merged 9 commits into from
Nov 20, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Nov 5, 2021

Details

Fixed Issues

$ #5237

Tests | QA Steps

  1. Open the App on the web with account A.
  2. Send a few messages. Count the number.
  3. Delete some of the recently sent messages. Count the remaining number.
  4. Log out, log in with account B for the same Chat.
  5. Count should be correct for unread messages.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

output_file.mp4

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat marked this pull request as ready for review November 6, 2021 12:03
@parasharrajat parasharrajat requested a review from a team as a code owner November 6, 2021 12:03
@botify botify requested a review from puneetlath November 6, 2021 12:03
@MelvinBot MelvinBot removed the request for review from a team November 6, 2021 12:03
we are getting deleted messages upto a sequence number but that does not include the message at that sequence number
@parasharrajat
Copy link
Member Author

Awaiting review....

@puneetlath
Copy link
Contributor

@parasharrajat a couple interesting things I noticed while testing this:

  1. When you mark a chat as unread, the unread count is immediately updated in the tab bar. However, when you delete a comment, the unread count in the tab bar in the current tab isn't updated. It is correct if you open in a new tab, but it doesn't update the existing tab. Why is that?
  2. When you open the app in the new tab, the tab bar shows the proper unread count, but the "new" line that indicates the point at which the messages are unread from isn't shown. Do you want to raise a new issue for this.

@parasharrajat
Copy link
Member Author

When you mark a chat as unread, the unread count is immediately updated in the tab bar. However, when you delete a comment, the unread count in the tab bar in the current tab isn't updated. It is correct if you open in a new tab, but it doesn't update the existing tab. Why is that?

I think because we manually set the count. And don't do that when a message is deleted. Do you want me to look into that as well in this PR?

When you open the app in the new tab, the tab bar shows the proper unread count, but the "new" line that indicates the point at which the messages are unread from isn't shown. Do you want to raise a new issue for this?

I noticed this too. The new line indicator is broken again and it is not predictable. Sometimes shows and somtimes not. I was about to raise this as well after review.

@puneetlath
Copy link
Contributor

puneetlath commented Nov 18, 2021

Do you want me to look into that as well in this PR?

Yes, I think we should do that as well. We can increase the payment by $250 to account for the increase in scope. cc @mallenexpensify

I was about to raise this as well after review.

Great!

@puneetlath
Copy link
Contributor

The code generally looks good to me. I'll give it another review after that update. Thanks!

@parasharrajat
Copy link
Member Author

Sure, Let me fix that.

@parasharrajat
Copy link
Member Author

@puneetlath I have fixed it. Please review.

puneetlath
puneetlath previously approved these changes Nov 19, 2021
src/libs/actions/ReportActions.js Outdated Show resolved Hide resolved
src/libs/actions/ReportActions.js Outdated Show resolved Hide resolved
@puneetlath
Copy link
Contributor

@parasharrajat I left you two minor comments, otherwise it looks good to me. Let me know what you think and then I'm happy to merge.

@parasharrajat
Copy link
Member Author

@puneetlath Updated those. Thanks for pointing it out.

@puneetlath puneetlath merged commit 12d3694 into Expensify:main Nov 20, 2021
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @puneetlath in version: 1.1.15-18 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants