Add a user setting that toggles notifications from accounts the user follows #20545

tonymet · 2024-01-20T22:39:18Z

What type of PR is this? (check all applicable)

[✅ ] Feature

Description

As a Forem user, I want to follow accounts I like (and have their content be higher up in my feed) without receiving notifications from them, so I can focus my attention on other notifications about my own content or community interaction.

BEFORE

m.notification_setting.new_post_notifications=true

AFTER

m.notification_setting.new_post_notifications=false

Status

✅ proof-of-concept working
✅ model user setting added notification_settings.new_post_notifications
✅db migration for above setting
✅ filter notifications controller , /notifications route hides new-post notifications
✅ UI for toggling setting
✅ avoid calling notification write path when setting is disabled.
✅ fr translation
❌ clean todos & debug logs
❌test setting, controller, mentions are working

Related Tickets & Documents

Related Issues #
- Add an user setting that enables or disables notifications from accounts the user follows #20533 &
- Feature request: Turn Off Notifications for People I Follow #15328
Closes #

QA Instructions, Screenshots, Recordings

Using rails console

$ bin/rails db:migrate
$ bin/rails console
# set your user id
m= User.where(:id=>13).first
 m.notification_setting.new_post_notifications=false;
 m.notification_setting.save()

Using UI

visit settings/notifications
toggle "send notifications when people I follow post new content"
view notifications page

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:

Semantic HTML implemented?
Keyboard operability supported?
Checked with axe DevTools and addressed Critical and Serious issues?
Color contrast tested?

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.

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

Are there any post deployment tasks we need to perform?

20240120210812_add_new_post_notifications_to_users.rb

[optional] What gif best describes this PR or how it makes you feel?

github-actions · 2024-01-20T22:39:28Z

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

github-actions · 2024-01-20T22:39:32Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tonymet · 2024-01-20T22:49:55Z

@michael-tharrington FYI proof of concept seems to be working

tonymet · 2024-01-20T23:35:43Z

I have read the CLA Document and I hereby sign the CLA

tonymet · 2024-01-20T23:35:48Z

recheck

tonymet · 2024-02-04T00:35:23Z

@michael-tharrington could I ask your help finding a reviewer for initial feedback?

tonymet · 2024-02-29T21:59:23Z

@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

lightalloy

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.

lightalloy · 2024-03-26T08:34:57Z

app/controllers/notifications_controller.rb

    @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?


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.

lightalloy · 2024-03-26T08:35:14Z

app/controllers/notifications_controller.rb

@@ -19,12 +19,15 @@ def index
      num = @initial_page_size
    end

+    logger.debug "notification setting: #{@user.notification_setting.inspect}"


Don't forget to remove debug before the review.

lightalloy · 2024-03-26T08:35:56Z

app/controllers/notifications/reads_controller.rb

@@ -3,6 +3,7 @@ class ReadsController < ApplicationController
    def create
      render plain: "" && return unless current_user

+      # TODO: notification controller read


I found TODOs on this pr not very informative, so it makes sense to remove them before the review (if you use them while developing).

lightalloy · 2024-03-26T08:36:41Z

config/locales/helpers/en.yml

@@ -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


Don't forget to add to the fr locale as well.

lightalloy · 2024-03-26T08:36:52Z

db/migrate/20240120210812_add_new_post_notifications_to_users.rb

+  def change
+    add_column :users_notification_settings, :new_post_notifications, :boolean, default: true, null: false
+  end
+end


Missing newline 👀

tonymet · 2024-03-26T17:39:15Z

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.

tonymet · 2024-03-28T23:19:40Z

spec/workers/articles/publish_worker_spec.rb

@@ -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


@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.

github-actions · 2024-04-01T17:18:47Z

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!

tonymet · 2024-05-02T23:08:25Z

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.

tonymet force-pushed the new-post-notification-setting branch from 74f88c2 to 7f087d3 Compare January 21, 2024 00:44

PhilipHow requested a review from lightalloy March 25, 2024 11:31

lightalloy requested changes Mar 26, 2024

View reviewed changes

tonymet added 5 commits March 27, 2024 11:01

Add TODO for new post / published notification settings

b1f2746

add notification_setting.new_post_notifications. include db migration.

7385926

Add UI & EN translation for new_post_notifications setting

685db16

add users_where_new-post_notification

203635e

filter write side of post

3b9d69a

tonymet force-pushed the new-post-notification-setting branch from 7f087d3 to 3b9d69a Compare March 27, 2024 20:50

tonymet added 4 commits March 27, 2024 21:00

fr translation

fc8b9e5

filter article_followers

a8a12e8

debug log

f576987

WIP e2e test

e1f7552

tonymet commented Mar 28, 2024

View reviewed changes

Merge branch 'main' into new-post-notification-setting

4031da7

PhilipHow linked an issue Apr 2, 2024 that may be closed by this pull request

Add an user setting that enables or disables notifications from accounts the user follows #20533

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a user setting that toggles notifications from accounts the user follows #20545

Add a user setting that toggles notifications from accounts the user follows #20545

tonymet commented Jan 20, 2024 •

edited

github-actions bot commented Jan 20, 2024

github-actions bot commented Jan 20, 2024 •

edited

tonymet commented Jan 20, 2024

tonymet commented Jan 20, 2024

tonymet commented Jan 20, 2024

tonymet commented Feb 4, 2024

tonymet commented Feb 29, 2024

lightalloy left a comment

lightalloy Mar 26, 2024

lightalloy Mar 26, 2024

lightalloy Mar 26, 2024

lightalloy Mar 26, 2024

lightalloy Mar 26, 2024

tonymet commented Mar 26, 2024

tonymet Mar 28, 2024

github-actions bot commented Apr 1, 2024 •

edited

tonymet commented May 2, 2024

Add a user setting that toggles notifications from accounts the user follows #20545

Are you sure you want to change the base?

Add a user setting that toggles notifications from accounts the user follows #20545

Conversation

tonymet commented Jan 20, 2024 • edited

What type of PR is this? (check all applicable)

Description

BEFORE

AFTER

Status

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Using rails console

Using UI

UI accessibility checklist

Added/updated tests?

Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions bot commented Jan 20, 2024

github-actions bot commented Jan 20, 2024 • edited

tonymet commented Jan 20, 2024

tonymet commented Jan 20, 2024

tonymet commented Jan 20, 2024

tonymet commented Feb 4, 2024

tonymet commented Feb 29, 2024

lightalloy left a comment

Choose a reason for hiding this comment

lightalloy Mar 26, 2024

Choose a reason for hiding this comment

lightalloy Mar 26, 2024

Choose a reason for hiding this comment

lightalloy Mar 26, 2024

Choose a reason for hiding this comment

lightalloy Mar 26, 2024

Choose a reason for hiding this comment

lightalloy Mar 26, 2024

Choose a reason for hiding this comment

tonymet commented Mar 26, 2024

tonymet Mar 28, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 1, 2024 • edited

Uffizzi Ephemeral Environment Deploying

tonymet commented May 2, 2024

tonymet commented Jan 20, 2024 •

edited

github-actions bot commented Jan 20, 2024 •

edited

github-actions bot commented Apr 1, 2024 •

edited