-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Mute notifications for your own article #1594
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
Mute notifications for your own article #1594
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.
Left two comments that I was unsure about.
def send_new_comment_notifications(notifiable) | ||
user_ids = notifiable.ancestors.map(&:user_id).to_set | ||
user_ids.add(notifiable.commentable.user.id) if user_ids.empty? | ||
user_ids = notifiable.ancestors.select(:receive_notifications, :user_id).select(&:receive_notifications).pluck(:user_id).to_set |
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.
This might be an overkill "faster" query.
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.
Looks good overall. Good clean implementation. There were some changes related to HTTP stuff that need to happen before it should be accepted.
Have to fix some tests first for the email notification part. |
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.
Okay. I think this is solid enough.
Ideally the endpoint is idempotent, meaning if there are multiple requests (like a user clicks twice) that it returns the same result.
But it's not the end of the world and could be patched in the future.
What type of PR is this? (check all applicable)
Description
This allows authors to mute notifications from their articles.
Resolves #234, partially.
WIP:Wasn't sure how to write a test for muting email notifications.
Screenshots
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?