Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-21876 Fix toast not showing up on marking channel as unread again #4737

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

sudheerDev
Copy link
Contributor

Summary

  • Use change in lastViewedAt on client to determine if channel is marked as unread again.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21876

Known issue: Toast wont show up if the same message is marked as unread. This case is hard to solve as it is. We could consider keeping a counter for keeping tracking for this i.e something of the sorts 2times marked as unread etc but otherwise this is hard to do with redux state.

  * Use lastViewedAt on client to determine if channel is marked
    as unread again
@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 24, 2020
@sudheerDev sudheerDev added this to the v5.20.0 milestone Jan 24, 2020
@sudheerDev
Copy link
Contributor Author

@esethna @andrewbrown00

There is one case this PR still does not solve. i.e marking the same message as unread. IMO we should leave it as a known issue as solving it means tracking some unnecessary changes we most likely ever use.

@andrewbrown00
Copy link

@sudheerDev what do you think the mana would be to fix this issue?

@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Jan 24, 2020
Copy link
Contributor

@deanwhillier deanwhillier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested locally on Chrome for Mac and works over and over again for me!

Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the comments, maintaining that part of the feature is going to be difficult in the future, so having those is going to be really nice :)

@Willyfrog Willyfrog removed the 2: Dev Review Requires review by a core commiter label Jan 27, 2020
@jgilliam17 jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jan 27, 2020
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, looks good to merge.

  • Toast is present when marking different posts in the same channel as unread.
  • Marking the same post as unread more then once will not create a toast as noted in the description as a known issue.

@jgilliam17 jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jan 29, 2020
@mattermod
Copy link
Contributor

Test server destroyed

@sudheerDev
Copy link
Contributor Author

/update-branch

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Jan 29, 2020

@sudheerDev what do you think the mana would be to fix this issue?

Talked with ES on this. We are going to leave this as known issue for now. It is not hard < 8 Mana but the code will always be hacky and it will be specifically for this corner case.

@sudheerDev sudheerDev merged commit 90c165b into mattermost:master Jan 29, 2020
@sudheerDev sudheerDev deleted the MM-21876 branch January 29, 2020 14:57
@sudheerDev sudheerDev removed their assignment Jan 29, 2020
@mattermod mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Jan 29, 2020
@mattermod mattermod removed the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Jan 29, 2020
sudheerDev added a commit that referenced this pull request Jan 29, 2020
* MM-21876 Fix toast not showing up on marking channel as unread again

  * Use lastViewedAt on client to determine if channel is marked
    as unread again

* Update tests

* Lint fix

Co-authored-by: Sudheer <[email protected]>
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
7 participants