-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add a user setting that toggles notifications from accounts the user follows #20545
base: main
Are you sure you want to change the base?
Conversation
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@michael-tharrington FYI proof of concept seems to be working |
I have read the CLA Document and I hereby sign the CLA |
recheck |
74f88c2
to
7f087d3
Compare
@michael-tharrington could I ask your help finding a reviewer for initial feedback? |
@lightalloy @maestromac could I ask your help finding a reviewer. there seems to be some moderate demand for this functionality and this PR is about 85% complete. just need feedback |
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.
Hey, @tonymet .
Thanks for your pr. There are a few things in the pr that need to be changed/improved:
The main change needed is where to put logic to avoid getting notifications when the setting is set to false (I added more details in one of the comments).
It's important to add specs:
- most important: make sure that the notification is not created when the new notification setting is set to false (and is sent when it's set to true)
- nice to have: request test to updating the notification settings
I have also left several smaller comments.
@notifications = if (params[:org_id].present? || params[:filter] == "org") && allowed_user? | ||
organization_notifications | ||
elsif params[:org_id].blank? && params[:filter].present? | ||
filtered_notifications | ||
else | ||
elsif @user.notification_setting.new_post_notifications? |
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.
Instead of filtering notifications before displaying them, we should avoid creating notifications in the first place.
To do this we need to adjust finding article followers in Notifications::NotifiableAction::Send
: exclude users with the corresponding notification setting set to false
.
@@ -19,12 +19,15 @@ def index | |||
num = @initial_page_size | |||
end | |||
|
|||
logger.debug "notification setting: #{@user.notification_setting.inspect}" |
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.
Don't forget to remove debug
before the review.
@@ -3,6 +3,7 @@ class ReadsController < ApplicationController | |||
def create | |||
render plain: "" && return unless current_user | |||
|
|||
# TODO: notification controller read |
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 found TODO
s on this pr not very informative, so it makes sense to remove them before the review (if you use them while developing).
@@ -63,6 +63,7 @@ en: | |||
mobile_comment_notifications: Send me a push notification when someone replies to me in a comment thread | |||
welcome_notifications: Send me occasional tips on how to enhance my %{community} experience | |||
reaction_notifications: Send notifications when someone reacts to my content, such as comments or posts | |||
new_post_notifications: Send notifications when people I follow post new content. When this is off, you can still see their content in your newsfeed |
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.
Don't forget to add to the fr
locale as well.
def change | ||
add_column :users_notification_settings, :new_post_notifications, :boolean, default: true, null: false | ||
end | ||
end |
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.
Missing newline 👀
thanks for the code pointers i'll take a look . good advice to address the write side. Could you point me to a good test case or api that could be used to cover the write side? I'd like to do an on-off test of the feature and confirm the notif is not written. |
7f087d3
to
3b9d69a
Compare
@@ -89,6 +89,19 @@ | |||
end.to change(Notification, :count).by(2) | |||
end | |||
|
|||
it "does not create a notification for each users with new-post-notification setting = false" do |
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.
@lightalloy can I ask your advice on a workable approach for this test? testing for :upsert_all
or other Notifications::
methods doesn't seem to work.
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/forem/forem/pull/20545 ⚙️ Updating now by workflow run 8513874376. What is Uffizzi? Learn more! |
anyone volunteer for a little support time to help me figure out the unit test and resolve the review? sometime over the next couple weeks. it would be good to put this one to use. |
What type of PR is this? (check all applicable)
Description
BEFORE
m.notification_setting.new_post_notifications=true
AFTER
m.notification_setting.new_post_notifications=false
Status
notification_settings.new_post_notifications
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Using rails console
Using UI
Visit visit http://localhost:3000/notifications and notice the new-post notifs are gone.
UI accessibility checklist
If your PR includes UI changes, please utilize this checklist:
Critical
andSerious
issues?For more info, check out the
Forem Accessibility Docs.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?