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 1 commit
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
12 changes: 0 additions & 12 deletions app/models/notifications/also_commented.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,5 @@ def mail_job
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
10 changes: 0 additions & 10 deletions app/models/notifications/comment_on_post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,5 @@ def mail_job
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
6 changes: 0 additions & 6 deletions app/models/notifications/contacts_birthday.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,5 @@ def mail_job
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
9 changes: 0 additions & 9 deletions app/models/notifications/liked.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,5 @@ def popup_translation_key
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
4 changes: 0 additions & 4 deletions app/models/notifications/mentioned_in_comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@ 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
Expand Down
5 changes: 0 additions & 5 deletions app/models/notifications/mentioned_in_post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,5 @@ def popup_translation_key
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
17 changes: 0 additions & 17 deletions app/models/notifications/private_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,5 @@ def mail_job
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
7 changes: 0 additions & 7 deletions app/models/notifications/reshared.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,5 @@ def popup_translation_key
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
5 changes: 0 additions & 5 deletions app/models/notifications/started_sharing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ 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
18 changes: 11 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
19 changes: 19 additions & 0 deletions app/services/notifications/also_comment_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

module Notifications
class AlsoCommentedService
def self.notify(comment, _)
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)

Notifications::AlsoCommented
.concatenate_or_create(recipient, commentable, actor)
.try(:email_the_user, comment, actor)
end
end
end
end
17 changes: 17 additions & 0 deletions app/services/notifications/comment_on_post_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# 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)

Notifications::CommentOnPost
.concatenate_or_create(commentable_author.owner, comment.commentable, actor)
.email_the_user(comment, actor)
end
end
end
11 changes: 11 additions & 0 deletions app/services/notifications/contacts_birthday_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module Notifications
class ContactsBirthdayService
def self.notify(contact, _=nil)
recipient = contact.user
actor = contact.person
create_notification(recipient, actor, actor).try(:email_the_user, actor, actor)
end
end
end
16 changes: 16 additions & 0 deletions app/services/notifications/liked_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

module Notifications
class LikedService
def self.notify(like, _)
actor = like.author
target_author = like.target.author

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

Notifications::Liked
.concatenate_or_create(target_author.owner, like.target, actor)
.email_the_user(like, actor)
end
end
end
31 changes: 31 additions & 0 deletions app/services/notifications/mentioned_in_comment_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true

module Notifications
class MentionedInCommentService
def self.model
Notifications::MentionedInComment
end

def self.notify(mentionable, recipient_user_ids)

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. [<6, 27, 6> 28.3/20]

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. [<6, 28, 6> 29.26/20]
  • Metrics/MethodLength: Method has too many lines. [25/20]

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
next if model.exists?(recipient: recipient, target: mention)

model
.create_notification(recipient, mention, actor)
.try(:email_the_user, mention, actor)
end
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
end
end
33 changes: 33 additions & 0 deletions app/services/notifications/mentioned_in_post_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

module Notifications
class MentionedInPostService
def self.model
Notifications::MentionedInPost
end

def self.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
next if model.exists?(recipient: recipient, target: mention)
Copy link
Member

Choose a reason for hiding this comment

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

This relies on the notification entity existing, which isn't the case if in-app notifications are disabled. This was a workaround for when posts were received twice, no duplicate notifications were created (#7721). I don't remember all the details and reasons, but Diaspora::Federation::Receive.status_message still processes a received post even, if it already exists, and because of that it also calls ReceiveLocal again.

I don't know/remember why a post should be received again if it already exists, maybe that was a workaround for when a post was created incomplete because of the scenario described here where some data for posts was missing, and receiving them again properly through the federation, would add this missing data. But if that gets fixed so it fetches the posts using federation code, it doesn't need to be received a second time, as it was already complete the first time. That would then also solve the problem of duplicate notifications, without needing the check for them here (as I don't know how otherwise this should be solved for not sending duplicate mails).

Copy link
Member Author

@cmrd-senya cmrd-senya Sep 4, 2022

Choose a reason for hiding this comment

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

@SuperTux88 is it enough to do the proper fetching using federation code? It seems quite easy.

Commit: ecb3923

Didn't look at the tests yet.

Copy link
Member Author

@cmrd-senya cmrd-senya Sep 4, 2022

Choose a reason for hiding this comment

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

(Probably worth adding exception handling too) Updated

Copy link
Member

Choose a reason for hiding this comment

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

is it enough to do the proper fetching using federation code? It seems quite easy.

Yes, that's theoretically all that's needed, but the type should be the lowercase, snake_case version of the entity name (doc).

But you can even improve that a little bit more by just using :post as type (as it could also be a reshare) and you can remove the check_type in validate, so that way we can even fetch reshares (you could also use the type from the json, but then you would need to convert that to the snake_case version, so just always use :post works and is easier) :)

But yes, basically that, it's fairly easy 👍

Copy link
Member

Choose a reason for hiding this comment

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

And the exception should be DiasporaFederation::Federation::Fetcher::NotFetchable.


model
.create_notification(recipient, mention, actor)
.try(:email_the_user, mention, actor)
end
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
24 changes: 24 additions & 0 deletions app/services/notifications/private_message_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

module Notifications
class PrivateMessageService
def self.notify(object, _)
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)
Notifications::PrivateMessage
.new(recipient: recipient)
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed, this was only a workaround because the mail needed the object, but it's never persisted.

.email_the_user(message, message.author)
end
end
end
end
14 changes: 14 additions & 0 deletions app/services/notifications/reshared_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# frozen_string_literal: true

module Notifications
class ResharedService
def self.notify(reshare, _)
return unless reshare.root.present? && reshare.root.author.local?

actor = reshare.author
Notifications::Reshared
.concatenate_or_create(reshare.root.author.owner, reshare.root, actor)
.try(:email_the_user, reshare, actor)
end
end
end
12 changes: 12 additions & 0 deletions app/services/notifications/started_sharing_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true

module Notifications
class StartedSharingService
def self.notify(contact, _)
sender = contact.person
Notifications::StartedSharing
.create_notification(contact.user, sender, sender)
.try(:email_the_user, sender, sender)
end
end
end
2 changes: 1 addition & 1 deletion app/workers/check_birthday.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def perform
.where("EXTRACT(DAY FROM birthday) = ?", Time.zone.today.day)
profiles.each do |profile|
profile.person.contacts.where(sharing: true, receiving: true).each do |contact|
Notifications::ContactsBirthday.notify(contact, [])
Notifications::ContactsBirthdayService.notify(contact)
end
end
end
Expand Down