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

Add in-app notification configuration #8375

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,9 @@ def change_settings(user_data, successful="users.update.settings_updated", error
end

def set_email_preferences
@email_prefs = Hash.new(true)

@user.user_preferences.each do |pref|
@email_prefs[pref.email_type] = false
@email_prefs = @user.user_preferences.to_h do |pref|
[pref.email_type, pref.email_enabled]
end
@email_prefs.default = true
end
end
3 changes: 2 additions & 1 deletion app/mailers/report_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ def new_report(type, id, reason, role)
person = Person.find(role.person_id)
return unless person.local?
user = User.find_by_id(person.owner_id)
return if user.user_preferences.exists?(email_type: :someone_reported)
return if NotificationSettingsService.new(user).email_disabled?(:someone_reported)

I18n.with_locale(user.language) do
resource[:email] = user.email
format(resource)
Expand Down
2 changes: 1 addition & 1 deletion app/models/notifications/mentioned_in_comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def self.filter_mentions(mentions, mentionable, _recipient_user_ids)
end

def mail_job
if !recipient.user_preferences.exists?(email_type: "mentioned_in_comment")
if NotificationSettingsService.new(recipient).email_enabled?(:mentioned_in_comment)
Workers::Mail::MentionedInComment
elsif shareable.author.owner_id == recipient_id
Workers::Mail::CommentOnPost
Expand Down
23 changes: 11 additions & 12 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,20 +180,18 @@ def send_reset_password_instructions

def update_user_preferences(pref_hash)
if self.disable_mail
UserPreference::VALID_EMAIL_TYPES.each{|x| self.user_preferences.find_or_create_by(email_type: x)}
UserPreference::VALID_EMAIL_TYPES.each do |type|
user_preferences.find_or_create_by(email_type: type).update(email_enabled: false)
end
Comment on lines +183 to +185
Copy link
Member

Choose a reason for hiding this comment

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

I don't even know why this is here (looks like something very old), and it doesn't make a lot of sense to me. Maybe 10 years ago you could only disable all mails or enable all mails, and this was some sort of migration code ... but nothing in the UI reads the disable_mail flag, so setting all types to disabled silently in the backend when changing the settings in the frontend doesn't make a lot of sense. We should just set what the user did in the frontend, so I would just drop these 3 lines.

Suggested change
UserPreference::VALID_EMAIL_TYPES.each do |type|
user_preferences.find_or_create_by(email_type: type).update(email_enabled: false)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it seemed to me as well that it is not being used. I'll remove it.

self.disable_mail = false
self.save
end

pref_hash.keys.each do |key|
if pref_hash[key] == 'true'
self.user_preferences.find_or_create_by(email_type: key)
else
block = user_preferences.find_by(email_type: key)
if block
block.destroy
end
end
email_enabled = pref_hash[key] == "false"
user_preferences
.find_or_create_by(email_type: key)
.update(email_enabled: email_enabled)
cmrd-senya marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand Down Expand Up @@ -354,9 +352,10 @@ def perform_export_photos!
def mail(job, *args)
return unless job.present?
pref = job.to_s.gsub('Workers::Mail::', '').underscore
if(self.disable_mail == false && !self.user_preferences.exists?(:email_type => pref))
job.perform_async(*args)
end
email_enabled = (disable_mail == false) &&
NotificationSettingsService.new(self).email_enabled?(pref)

job.perform_async(*args) if email_enabled
Copy link
Member

Choose a reason for hiding this comment

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

I would probably write this as guard clause:

Suggested change
email_enabled = (disable_mail == false) &&
NotificationSettingsService.new(self).email_enabled?(pref)
job.perform_async(*args) if email_enabled
return if disable_mail || NotificationSettingsService.new(self).email_disabled?(pref)
job.perform_async(*args)

But either way, (disable_mail == false) doesn't make much sense, just use !disable_mail

end

def send_confirm_email
Expand Down
32 changes: 32 additions & 0 deletions app/services/notification_settings_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# frozen_string_literal: true

class NotificationSettingsService
def initialize(user)
@user = user
end

def email_disabled?(notification_type)
!email_enabled?(notification_type)
end

def email_enabled?(notification_type)
notification_enabled?(notification_type, :email_enabled?)
end

def in_app_enabled?(notification_type)
notification_enabled?(notification_type, :in_app_enabled?)
end

private

attr_reader :user

delegate :user_preferences, to: :user
Copy link
Member

Choose a reason for hiding this comment

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

user_preferences is just used once here ... so I would just use user.user_preferences that one time instead.


def notification_enabled?(notification_type, pref_method)
pref = user_preferences.find_by(email_type: notification_type)
return true if pref.nil?

pref.public_send pref_method
end
end
10 changes: 10 additions & 0 deletions db/migrate/20220721190153_add_email_enabled_to_user_preferences.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

class AddEmailEnabledToUserPreferences < ActiveRecord::Migration[6.1]
cmrd-senya marked this conversation as resolved.
Show resolved Hide resolved
def change
change_table :user_preferences, bulk: true do |t|
t.boolean :email_enabled, null: false, default: false
t.boolean :in_app_enabled, null: false, default: true
end
Comment on lines +5 to +8
Copy link
Member

Choose a reason for hiding this comment

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

I would like to suggest some general changes regarding this whole thing, as this is touched now anyways. To me the naming of this table never made sense, as it's specific a table for notifications and not about "user preferences" ... so I would suggest renaming it to something that explains what this is, for example notification_settings (as you named the service NotificationSettingsService), and then also rename the variables and everything to something regarding notifications. I think that will make the code overall more readable because I was always confused about this.

Also, after thinking about it a bit more, I don't think the _enabled suffix for the new columns is needed, I think it's self-explained anyway, but if you want to keep it, that's fine for me too.

Also, when you are at it, can you rename the existing email_type column to just type, since it's not about emails anymore. I also think that will make this more readable for the future.

end
end