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
8 changes: 0 additions & 8 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 Down
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 NotificationSettingsService.new(recipient).email_enabled?(: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
8 changes: 0 additions & 8 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -350,14 +350,6 @@ def perform_export_photos!
end

######### Mailer #######################
def mail(job, *args)
return unless job.present?
pref = job.to_s.gsub('Workers::Mail::', '').underscore
email_enabled = (disable_mail == false) &&
NotificationSettingsService.new(self).email_enabled?(pref)

job.perform_async(*args) if email_enabled
end

def send_confirm_email
return if unconfirmed_email.blank?
Expand Down
30 changes: 23 additions & 7 deletions app/services/notification_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

class NotificationService
NOTIFICATION_TYPES = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename that NOTIFICATION_SERVICES now?

Comment => [Notifications::MentionedInComment, Notifications::CommentOnPost, Notifications::AlsoCommented],
Like => [Notifications::Liked],
StatusMessage => [Notifications::MentionedInPost],
Conversation => [Notifications::PrivateMessage],
Message => [Notifications::PrivateMessage],
Reshare => [Notifications::Reshared],
Contact => [Notifications::StartedSharing]
Comment => [
Notifications::MentionedInCommentService,
Notifications::CommentOnPostService,
Notifications::AlsoCommentedService
],
Like => [Notifications::LikedService],
StatusMessage => [Notifications::MentionedInPostService],
Conversation => [Notifications::PrivateMessageService],
Message => [Notifications::PrivateMessageService],
Reshare => [Notifications::ResharedService],
Contact => [Notifications::StartedSharingService]
}.freeze

NOTIFICATIONS_JSON_TYPES = {
Expand Down Expand Up @@ -51,12 +55,24 @@ def update_status_by_guid(guid, is_read_status)
true
end

def mail(job, *args)
return if job.blank?

pref = job.to_s.gsub("Workers::Mail::", "").underscore
email_enabled = (user.disable_mail == false) &&
NotificationSettingsService.new(user).email_enabled?(pref)

job.perform_async(*args) if email_enabled
Comment on lines +62 to +65
Copy link
Member

Choose a reason for hiding this comment

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

I still would write this as guard clause and the == false doesn't make a lot of sense (see old comment)

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

end

def notify(object, recipient_user_ids)
notification_types(object).each {|type| type.notify(object, recipient_user_ids) }
end

private

attr_reader :user

def notification_types(object)
NOTIFICATION_TYPES.fetch(object.class, [])
end
Expand Down
25 changes: 25 additions & 0 deletions app/services/notifications/also_commented_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module Notifications
class AlsoCommentedService
def self.notify(comment, _)

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for notify is too high. [<4, 20, 2> 20.49/20]

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)
Copy link
Member

Choose a reason for hiding this comment

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

The mention_notification_exists?(comment, recipient.person) still relies on the notification object existing. So if somebody has disabled the in-app notifications for mentions (but enabled mails for both mentions and comments), they would receive two mails for the same comment, as this would trigger a second notification, as the mention notification object wasn't created.

I don't know what's the best solution for this. The mentions notification already handles mails being disabled for mentions, and then sends a normal commented mail. But when we would for example check here, if there is a mention in the comment, then we wouldn't send duplicate mails (but still one mail, as that's already handled in the mentions notification service ... but we wouldn't send an in-app notification if that's disabled for mentions (unless we also move that logic to the mentions notification service as well?) But then we have even more logic in the mentions notification service, that's kinda duplicated? 🤷‍♂️ Maybe you have an idea how to solve that the best?


Notifications::AlsoCommented
.concatenate_or_create(recipient, commentable, actor)

NotificationService.new(recipient).mail(
Workers::Mail::AlsoCommented,
recipient.id,
actor.id,
comment.id
)
Comment on lines +16 to +21
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit duplicate that the recipient is already a parameter for the service, and then the recipient.id is added again as parameter for the mail method. I think we can remove the recipient.id parameter here for all notification services, and then set it for all in the NotificationService#mail:

job.perform_async(user.id, *args)

end
end
end
end
24 changes: 24 additions & 0 deletions app/services/notifications/comment_on_post_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

module Notifications
class CommentOnPostService
def self.notify(comment, _)
actor = comment.author
commentable_author = comment.commentable.author

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

recipient = commentable_author.owner
Notifications::CommentOnPost
.concatenate_or_create(recipient, comment.commentable, actor)

NotificationService.new(recipient).mail(
Workers::Mail::CommentOnPost,
recipient.id,
actor.id,
comment.id
)
end
end
end
19 changes: 19 additions & 0 deletions app/services/notifications/contacts_birthday_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

module Notifications
class ContactsBirthdayService
def self.notify(contact, _=nil)
recipient = contact.user
actor = contact.person
Notifications::ContactsBirthday
.create_notification(recipient, actor, actor)

NotificationService.new(recipient).mail(
Workers::Mail::ContactsBirthday,
recipient.id,
actor.id,
actor.id
)
end
end
end