-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
4be89fc
048b204
e55b876
8abf74c
8e221db
0b0e146
1c4a897
dede976
04dcbf6
790117c
f92775b
79031b3
80759b7
90678ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||
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 | ||||||||||||||||
|
||||||||||||||||
|
@@ -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 | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably write this as guard clause:
Suggested change
But either way, |
||||||||||||||||
end | ||||||||||||||||
|
||||||||||||||||
def send_confirm_email | ||||||||||||||||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
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 |
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 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.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.
Yes, it seemed to me as well that it is not being used. I'll remove it.