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 all 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
1 change: 1 addition & 0 deletions app/assets/stylesheets/_application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

// profile and settings pages
@import 'settings';
@import 'user_preferences';
@import 'cropperjs/dist/cropper';

// new SPV
Expand Down
4 changes: 3 additions & 1 deletion app/assets/stylesheets/mobile/mobile.scss
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
@import 'markdown-editor';
@import 'mobile/markdown_editor';

@import 'user_preferences';

@import 'typography';

a {
Expand Down Expand Up @@ -856,7 +858,7 @@ select#aspect_ids_ {
word-wrap: break-word;
}

#email_prefs {
.email-prefs {
.checkbox{
margin: 15px 0;
}
Expand Down
34 changes: 34 additions & 0 deletions app/assets/stylesheets/user_preferences.scss
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;
Copy link
Member

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.

}
}

.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;
}
}
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
15 changes: 6 additions & 9 deletions app/models/notification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,10 @@ def self.for(recipient, opts={})
where(opts.merge!(recipient_id: recipient.id)).order("updated_at DESC")
end

def email_the_user(target, actor)
recipient.mail(mail_job, recipient_id, actor.id, target.id)
end

def set_read_state( read_state )
update_column(:unread, !read_state)
end

def mail_job
raise NotImplementedError.new("Subclass this.")
end

def linked_object
target
end
Expand All @@ -53,6 +45,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
16 changes: 0 additions & 16 deletions app/models/notifications/also_commented.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,8 @@ module Notifications
class AlsoCommented < Notification
include Notifications::Commented

def mail_job
Workers::Mail::AlsoCommented
end

def popup_translation_key
"notifications.also_commented"
end

def self.notify(comment, _recipient_user_ids)
actor = comment.author
commentable = comment.commentable
recipient_ids = commentable.participants.local.where.not(id: [commentable.author_id, actor.id]).pluck(:owner_id)

User.where(id: recipient_ids).find_each do |recipient|
next if recipient.is_shareable_hidden?(commentable) || mention_notification_exists?(comment, recipient.person)

concatenate_or_create(recipient, commentable, actor).try(:email_the_user, comment, actor)
end
end
end
end
14 changes: 0 additions & 14 deletions app/models/notifications/comment_on_post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,8 @@ module Notifications
class CommentOnPost < Notification
include Notifications::Commented

def mail_job
Workers::Mail::CommentOnPost
end

def popup_translation_key
"notifications.comment_on_post"
end

def self.notify(comment, _recipient_user_ids)
actor = comment.author
commentable_author = comment.commentable.author

return unless commentable_author.local? && actor != commentable_author
return if mention_notification_exists?(comment, commentable_author)

concatenate_or_create(commentable_author.owner, comment.commentable, actor).email_the_user(comment, actor)
end
end
end
10 changes: 0 additions & 10 deletions app/models/notifications/contacts_birthday.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,8 @@

module Notifications
class ContactsBirthday < Notification
def mail_job
Workers::Mail::ContactsBirthday
end

def popup_translation_key
"notifications.contacts_birthday"
end

def self.notify(contact, _recipient_user_ids)
recipient = contact.user
actor = contact.person
create_notification(recipient, actor, actor).try(:email_the_user, actor, actor)
end
end
end
13 changes: 0 additions & 13 deletions app/models/notifications/liked.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,12 @@

module Notifications
class Liked < Notification
def mail_job
Workers::Mail::Liked
end

def popup_translation_key
"notifications.liked"
end

def deleted_translation_key
"notifications.liked_post_deleted"
end

def self.notify(like, _recipient_user_ids)
actor = like.author
target_author = like.target.author

return unless like.target_type == "Post" && target_author.local? && actor != target_author

concatenate_or_create(target_author.owner, like.target, actor).email_the_user(like, actor)
end
end
end
18 changes: 0 additions & 18 deletions app/models/notifications/mentioned.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,5 @@ module Mentioned
def linked_object
target.mentions_container
end

module ClassMethods
def notify(mentionable, recipient_user_ids)
actor = mentionable.author
relevant_mentions = filter_mentions(
mentionable.mentions.local.where.not(person: actor),
mentionable,
recipient_user_ids
)

relevant_mentions.each do |mention|
recipient = mention.person.owner
unless exists?(recipient: recipient, target: mention)
create_notification(recipient, mention, actor).try(:email_the_user, mention, actor)
end
end
end
end
end
end
20 changes: 0 additions & 20 deletions app/models/notifications/mentioned_in_comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,5 @@ def popup_translation_key
def deleted_translation_key
"notifications.mentioned_in_comment_deleted"
end

def self.filter_mentions(mentions, mentionable, _recipient_user_ids)
mentions.includes(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent))
end

def mail_job
if !recipient.user_preferences.exists?(email_type: "mentioned_in_comment")
Workers::Mail::MentionedInComment
elsif shareable.author.owner_id == recipient_id
Workers::Mail::CommentOnPost
elsif shareable.participants.local.where(owner_id: recipient_id)
Workers::Mail::AlsoCommented
end
end

private

def shareable
linked_object.parent
end
end
end
9 changes: 0 additions & 9 deletions app/models/notifications/mentioned_in_post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,12 @@ module Notifications
class MentionedInPost < Notification
include Notifications::Mentioned

def mail_job
Workers::Mail::Mentioned
end

def popup_translation_key
"notifications.mentioned"
end

def deleted_translation_key
"notifications.mentioned_deleted"
end

def self.filter_mentions(mentions, mentionable, recipient_user_ids)
return mentions if mentionable.public
mentions.where(person: Person.where(owner_id: recipient_user_ids).ids)
end
end
end
21 changes: 0 additions & 21 deletions app/models/notifications/private_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,8 @@

module Notifications
class PrivateMessage < Notification
def mail_job
Workers::Mail::PrivateMessage
end

def popup_translation_key
"notifications.private_message"
end

def self.notify(object, _recipient_user_ids)
case object
when Conversation
object.messages.each {|message| notify_message(message) }
when Message
notify_message(object)
end
end

private_class_method def self.notify_message(message)
recipient_ids = message.conversation.participants.local.where.not(id: message.author_id).pluck(:owner_id)
User.where(id: recipient_ids).find_each do |recipient|
message.increase_unread(recipient)
new(recipient: recipient).email_the_user(message, message.author)
end
end
end
end
11 changes: 0 additions & 11 deletions app/models/notifications/reshared.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,12 @@

module Notifications
class Reshared < Notification
def mail_job
Workers::Mail::Reshared
end

def popup_translation_key
"notifications.reshared"
end

def deleted_translation_key
"notifications.reshared_post_deleted"
end

def self.notify(reshare, _recipient_user_ids)
return unless reshare.root.present? && reshare.root.author.local?

actor = reshare.author
concatenate_or_create(reshare.root.author.owner, reshare.root, actor).try(:email_the_user, reshare, actor)
end
end
end
9 changes: 0 additions & 9 deletions app/models/notifications/started_sharing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,10 @@

module Notifications
class StartedSharing < Notification
def mail_job
Workers::Mail::StartedSharing
end

def popup_translation_key
"notifications.started_sharing"
end

def self.notify(contact, _recipient_user_ids)
sender = contact.person
create_notification(contact.user, sender, sender).try(:email_the_user, sender, sender)
end

def contact
recipient.contact_for(target)
end
Expand Down