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 5 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
30 changes: 30 additions & 0 deletions app/assets/stylesheets/settings.scss
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,33 @@
.account-data h6 {
color: $text-grey;
}

.preference-row {
align-items: center;
display: flex;
}

.icon-checkbox {
i {
color: $medium-gray;
cursor: pointer;
font-size: 32px;
}

[type="checkbox"] {
display: none;
}

[type="checkbox"]:checked ~ i {
color: $black;
}
}

.notification-text {
margin-left: 16px;
}

.hidden-bell {
color: transparent;
font-size: 32px;
}
11 changes: 1 addition & 10 deletions app/controllers/notifications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,7 @@ def render_as_json(unread_count, unread_count_by_type, notification_list)
end

def types
{
"also_commented" => "Notifications::AlsoCommented",
"comment_on_post" => "Notifications::CommentOnPost",
"liked" => "Notifications::Liked",
"mentioned" => "Notifications::MentionedInPost",
"mentioned_in_comment" => "Notifications::MentionedInComment",
"reshared" => "Notifications::Reshared",
"started_sharing" => "Notifications::StartedSharing",
"contacts_birthday" => "Notifications::ContactsBirthday"
}
NotificationService::NOTIFICATIONS_JSON_TYPES
end
helper_method :types
end
17 changes: 12 additions & 5 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def user_params
:post_default_public,
:exported_photos_file,
:export,
email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym)
email_preferences: UserPreference::VALID_EMAIL_TYPES.to_h {|type| [type.to_sym, %i[mail in_app]] }
Copy link
Member

Choose a reason for hiding this comment

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

As this is not about emails only anymore, renaming everything that is now email_preferences (or email_prefs) to notification_settings would make more sense to me.

)
end

Expand Down Expand Up @@ -249,10 +249,17 @@ 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, {
mail: pref.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 like to keep the naming consistent here, so as it's named email email everywhere else, I would also name this email instead of mail here too.

in_app: pref.in_app_enabled
}
]
end
@email_prefs.default = {
mail: true,
in_app: 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
7 changes: 6 additions & 1 deletion app/models/notification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ def self.create_notification(recipient, target, actor)
end

private_class_method def self.suppress_notification?(recipient, actor)
recipient.blocks.where(person: actor).exists?
return true if recipient.blocks.exists?(person: actor)
Copy link
Member

Choose a reason for hiding this comment

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

This now isn't called anymore for mails, so it now always sends mails, even if the actor is blocked/ignored. But as you extended this now to also check if in-app notifications are enabled, it can't just be called for mails. So I think the best would be to split this up again, so this can be called both for mails and for in-app notifications, and then only check for in-app notifications enabled/disabled in Notification.create_notification and Notification.concatenate_or_create?


pref_name = NotificationService::NOTIFICATIONS_REVERSE_JSON_TYPES[name]
NotificationSettingsService
.new(recipient)
.in_app_disabled?(pref_name)
end
end
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
25 changes: 13 additions & 12 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -179,21 +179,21 @@ def send_reset_password_instructions
end

def update_user_preferences(pref_hash)
# binding.pry
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]["mail"] == "false"
in_app_enabled = pref_hash[key]["in_app"] == "false"
Comment on lines +191 to +192
Copy link
Member

Choose a reason for hiding this comment

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

Can you please also remove this negation here. It should be true if it's enabled everywhere and false if it's disabled, but at the moment when submitting the form it's exactly the wrong way around and that's extremely confusing. I also already found this negation confusing in the past, but it at least kinda made sense because how the table worked in the past.

user_preferences
.find_or_create_by(email_type: key)
.update(email_enabled: email_enabled, in_app_enabled: in_app_enabled)
end
end

Expand Down Expand Up @@ -354,9 +354,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
36 changes: 36 additions & 0 deletions app/services/notification_settings_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# 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_disabled?(notfication_type)
!in_app_enabled?(notfication_type)
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