-
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 9 commits
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 |
---|---|---|
@@ -0,0 +1,34 @@ | ||
.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; | ||
} | ||
|
||
.email-prefs { | ||
align-items: center; | ||
display: grid; | ||
grid-template-columns: 60px 60px auto; | ||
|
||
.icon-cell { | ||
justify-self: center; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]] } | ||
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. As this is not about emails only anymore, renaming everything that is now |
||
) | ||
end | ||
|
||
|
@@ -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, | ||
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 like to keep the naming consistent here, so as it's named |
||
in_app: pref.in_app_enabled | ||
} | ||
] | ||
end | ||
@email_prefs.default = { | ||
mail: true, | ||
in_app: true | ||
} | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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. 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 |
||
|
||
pref_name = NotificationService::NOTIFICATIONS_REVERSE_JSON_TYPES[name] | ||
NotificationSettingsService | ||
.new(recipient) | ||
.in_app_disabled?(pref_name) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -180,20 +180,19 @@ 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
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 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
Suggested change
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. 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
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. Can you please also remove this negation here. It should be |
||||||||||||||||
user_preferences | ||||||||||||||||
.find_or_create_by(email_type: key) | ||||||||||||||||
.update(email_enabled: email_enabled, in_app_enabled: in_app_enabled) | ||||||||||||||||
end | ||||||||||||||||
end | ||||||||||||||||
|
||||||||||||||||
|
@@ -354,9 +353,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,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 | ||
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.
This needs some override for the dark theme in
_color_theme_override_dark.scss
for the dark theme ($white
?) and maybe also for the color when unchecked.