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

Stop duplicate comment notification emission #4977

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Conversation

timolegros
Copy link
Collaborator

@timolegros timolegros commented Sep 6, 2023

Link to Issue

Closes: #4345

Description of Changes

  • Added the parent comment author's addresses as excluded from the root thread notification.
  • This does not remove duplicate notifications that have been emitted in the past but ensures that users don't see duplicate notifications in the future.
  • This also does not stop notifications from having near-duplicate data as the issue describes. To normalize that data would require reworking how we fetch and emit comment notifications which is an excessive amount of work when there is a trivial solution. In other words, the denormalized data should be acceptable here given the trivial solution.

"How We Fixed It"

Parent comment authors are excluded from the notification sent to all users subscribed to a thread.

Test Plan

  • Sign-in on account 1.
  • Sign-in on account 2 on a separate browser.
  • Create a thread from account 2 in any community.
  • Subscribe to that thread from account 1.
  • Create a comment on that thread with account 1.
  • Reply to the new comment from account 2.
  • Ensure there is only 1 notification in the notification drop-down of both accounts 1 and 2.

Deployment Plan

Other Considerations

@timolegros timolegros marked this pull request as ready for review September 6, 2023 20:28
@jnaviask jnaviask merged commit d326389 into master Sep 6, 2023
5 of 6 checks passed
@jnaviask jnaviask deleted the tim/rm-dup-comment branch September 6, 2023 20:38
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.

Duplicate comment notifications
2 participants