diff --git a/app/models/notification.rb b/app/models/notification.rb index 91d697a47..2cdae679a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -3,15 +3,15 @@ # the COPYRIGHT file. # class Notification < ActiveRecord::Base - belongs_to :recipient, :class_name => 'User' - has_many :notification_actors, :dependent => :destroy - has_many :actors, :class_name => 'Person', :through => :notification_actors, :source => :person - belongs_to :target, :polymorphic => true + belongs_to :recipient, class_name: "User" + has_many :notification_actors, dependent: :destroy + has_many :actors, class_name: "Person", through: :notification_actors, source: :person + belongs_to :target, polymorphic: true attr_accessor :note_html def self.for(recipient, opts={}) - self.where(opts.merge!(:recipient_id => recipient.id)).order('updated_at desc') + where(opts.merge!(recipient_id: recipient.id)).order("updated_at DESC") end def self.notify(recipient, target, actor) @@ -33,11 +33,11 @@ class Notification < ActiveRecord::Base end def as_json(opts={}) - super(opts.merge(:methods => :note_html)) + super(opts.merge(methods: :note_html)) end def email_the_user(target, actor) - self.recipient.mail(self.mail_job, self.recipient_id, actor.id, target.id) + recipient.mail(mail_job, recipient_id, actor.id, target.id) end def set_read_state( read_state ) @@ -45,14 +45,13 @@ class Notification < ActiveRecord::Base end def mail_job - raise NotImplementedError.new('Subclass this.') + raise NotImplementedError.new("Subclass this.") end - def effective_target - self.popup_translation_key == "notifications.mentioned" ? self.target.post : self.target + def linked_object + target end -private def self.concatenate_or_create(recipient, target, actor, notification_type) return nil if suppress_notification?(recipient, target) @@ -76,7 +75,6 @@ private end end - def self.make_notification(recipient, target, actor, notification_type) return nil if suppress_notification?(recipient, target) n = notification_type.new(:target => target, @@ -87,9 +85,28 @@ private n end + def self.concatenate_or_create(recipient, target, actor) + return nil if suppress_notification?(recipient, target) + + find_or_initialize_by(recipient: recipient, target: target, unread: true).tap do |notification| + notification.actors |= [actor] + # Explicitly touch the notification to update updated_at whenever new actor is inserted in notification. + if notification.new_record? || notification.changed? + notification.save! + else + notification.touch + end + end + end + + def self.create_notification(recipient_id, target, actor) + create(recipient_id: recipient_id, target: target, actors: [actor]) + end + def self.suppress_notification?(recipient, post) post.is_a?(Post) && recipient.is_shareable_hidden?(post) end + private_class_method :suppress_notification? def self.types { diff --git a/app/models/notifications/also_commented.rb b/app/models/notifications/also_commented.rb index 7b698cd36..c20c88df4 100644 --- a/app/models/notifications/also_commented.rb +++ b/app/models/notifications/also_commented.rb @@ -1,17 +1,26 @@ -class Notifications::AlsoCommented < Notification - def mail_job - Workers::Mail::AlsoCommented - end - - def popup_translation_key - 'notifications.also_commented' - end +module Notifications + class AlsoCommented < Notification + def mail_job + Workers::Mail::AlsoCommented + end - def deleted_translation_key - 'notifications.also_commented_deleted' - end + def popup_translation_key + "notifications.also_commented" + end - def linked_object - Post.where(:id => self.target_id).first + def deleted_translation_key + "notifications.also_commented_deleted" + 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| + concatenate_or_create(recipient, commentable, actor) + .try {|notification| notification.email_the_user(comment, actor) } + end + end end end diff --git a/app/models/notifications/comment_on_post.rb b/app/models/notifications/comment_on_post.rb index 631317880..df23b558e 100644 --- a/app/models/notifications/comment_on_post.rb +++ b/app/models/notifications/comment_on_post.rb @@ -1,17 +1,24 @@ -class Notifications::CommentOnPost < Notification - def mail_job - Workers::Mail::CommentOnPost - end +module Notifications + class CommentOnPost < Notification + def mail_job + Workers::Mail::CommentOnPost + end - def popup_translation_key - 'notifications.comment_on_post' - end + def popup_translation_key + "notifications.comment_on_post" + end - def deleted_translation_key - 'notifications.also_commented_deleted' - end + def deleted_translation_key + "notifications.also_commented_deleted" + end - def linked_object - Post.where(:id => self.target_id).first + def self.notify(comment, _recipient_user_ids) + actor = comment.author + commentable_author = comment.commentable.author + + return unless commentable_author.local? && actor != commentable_author + + concatenate_or_create(commentable_author.owner, comment.commentable, actor).email_the_user(comment, actor) + end end end diff --git a/app/models/notifications/liked.rb b/app/models/notifications/liked.rb index 05607b100..00b33113a 100644 --- a/app/models/notifications/liked.rb +++ b/app/models/notifications/liked.rb @@ -1,19 +1,24 @@ -class Notifications::Liked < Notification - def mail_job - Workers::Mail::Liked - end - - def popup_translation_key - 'notifications.liked' - end +module Notifications + class Liked < Notification + def mail_job + Workers::Mail::Liked + end - def deleted_translation_key - 'notifications.liked_post_deleted' - end - - def linked_object - post = self.target - post = post.target if post.is_a? Like - post + 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 diff --git a/app/models/notifications/mentioned.rb b/app/models/notifications/mentioned.rb index 16482ace7..9636d574b 100644 --- a/app/models/notifications/mentioned.rb +++ b/app/models/notifications/mentioned.rb @@ -1,17 +1,31 @@ -class Notifications::Mentioned < Notification - def mail_job - Workers::Mail::Mentioned - end - - def popup_translation_key - 'notifications.mentioned' - end +module Notifications + class Mentioned < Notification + def mail_job + Workers::Mail::Mentioned + end - def deleted_translation_key - 'notifications.mentioned_deleted' - end + def popup_translation_key + "notifications.mentioned" + end - def linked_object - Mention.find(self.target_id).post + def deleted_translation_key + "notifications.mentioned_deleted" + end + + def linked_object + target.post + end + + def self.notify(mentionable, recipient_user_ids) + actor = mentionable.author + + mentionable.mentions.select {|mention| mention.person.local? }.each do |mention| + recipient = mention.person + + next if recipient == actor || !(mentionable.public || recipient_user_ids.include?(recipient.owner_id)) + + create_notification(recipient.owner_id, mention, actor).email_the_user(mention, actor) + end + end end end diff --git a/app/models/notifications/private_message.rb b/app/models/notifications/private_message.rb index 4ef273041..b97c40183 100644 --- a/app/models/notifications/private_message.rb +++ b/app/models/notifications/private_message.rb @@ -1,15 +1,30 @@ -class Notifications::PrivateMessage < Notification - def mail_job - Workers::Mail::PrivateMessage - end - def popup_translation_key - 'notifications.private_message' - end - def self.make_notification(recipient, target, actor, notification_type) - n = notification_type.new(:target => target, - :recipient_id => recipient.id) - target.increase_unread(recipient) - n.actors << actor - n +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 do |message| + recipient_ids = recipient_user_ids - [message.author.owner_id] + User.where(id: recipient_ids).find_each {|recipient| notify_message(message, recipient) } + end + when Message + recipients = object.conversation.participants.select(&:local?) - [object.author] + recipients.each {|recipient| notify_message(object, recipient.owner) } + end + end + + def self.notify_message(message, recipient) + message.increase_unread(recipient) + new(recipient: recipient).email_the_user(message, message.author) + end + private_class_method :notify_message end end diff --git a/app/models/notifications/request_accepted.rb b/app/models/notifications/request_accepted.rb deleted file mode 100644 index 3651a32f0..000000000 --- a/app/models/notifications/request_accepted.rb +++ /dev/null @@ -1,8 +0,0 @@ -class Notifications::RequestAccepted < Notification - def mail_job - Workers::Mail::RequestAcceptance - end - def popup_translation_key - 'notifications.request_accepted' - end -end diff --git a/app/models/notifications/reshared.rb b/app/models/notifications/reshared.rb index fb9559d6f..51f089e35 100644 --- a/app/models/notifications/reshared.rb +++ b/app/models/notifications/reshared.rb @@ -1,17 +1,22 @@ -class Notifications::Reshared < Notification - def mail_job - Workers::Mail::Reshared - end +module Notifications + class Reshared < Notification + def mail_job + Workers::Mail::Reshared + end - def popup_translation_key - 'notifications.reshared' - end + def popup_translation_key + "notifications.reshared" + end - def deleted_translation_key - 'notifications.reshared_post_deleted' - end + def deleted_translation_key + "notifications.reshared_post_deleted" + end - def linked_object - self.target + 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).email_the_user(reshare, actor) + end end end diff --git a/app/models/notifications/started_sharing.rb b/app/models/notifications/started_sharing.rb index ff4975be5..c80ad88a9 100644 --- a/app/models/notifications/started_sharing.rb +++ b/app/models/notifications/started_sharing.rb @@ -1,20 +1,16 @@ -class Notifications::StartedSharing < Notification - def mail_job - Workers::Mail::StartedSharing +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_id, sender, sender).email_the_user(sender, sender) + end end - - def popup_translation_key - 'notifications.started_sharing' - end - - def email_the_user(target, actor) - super(target.sender, actor) - end - - private - - def self.make_notification(recipient, target, actor, notification_type) - super(recipient, target.sender, actor, notification_type) - end - end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb new file mode 100644 index 000000000..d356a0211 --- /dev/null +++ b/app/services/notification_service.rb @@ -0,0 +1,21 @@ +class NotificationService + NOTIFICATION_TYPES = { + Comment => [Notifications::CommentOnPost, Notifications::AlsoCommented], + Like => [Notifications::Liked], + StatusMessage => [Notifications::Mentioned], + Conversation => [Notifications::PrivateMessage], + Message => [Notifications::PrivateMessage], + Reshare => [Notifications::Reshared], + Contact => [Notifications::StartedSharing] + }.freeze + + def notify(object, recipient_user_ids) + notification_types(object).each {|type| type.notify(object, recipient_user_ids) } + end + + private + + def notification_types(object) + NOTIFICATION_TYPES.fetch(object.class, []) + end +end diff --git a/app/views/notifications/_notification.haml b/app/views/notifications/_notification.haml index b8dfcab52..7a76decd9 100644 --- a/app/views/notifications/_notification.haml +++ b/app/views/notifications/_notification.haml @@ -1,9 +1,9 @@ .media.stream_element{:data=>{:guid => note.id, :type => (Notification.types.key(note.type) || '') }, :class => (note.unread ? 'unread' : 'read')} .unread-toggle.pull-right %i.entypo-eye{title: (note.unread ? t("notifications.index.mark_read") : t("notifications.index.mark_unread"))} - - if note.type == "Notifications::StartedSharing" && contact = current_user.contact_for(note.effective_target) + - if note.type == "Notifications::StartedSharing" && contact = current_user.contact_for(note.target) .pull-right - = aspect_membership_dropdown(contact, note.effective_target, "right") + = aspect_membership_dropdown(contact, note.target, "right") .media-object.pull-left = person_image_link note.actors.first, :size => :thumb_small, :class => 'hovercardable' diff --git a/app/workers/receive_local.rb b/app/workers/receive_local.rb index 409d33d4f..cb04884c2 100644 --- a/app/workers/receive_local.rb +++ b/app/workers/receive_local.rb @@ -5,7 +5,7 @@ module Workers def perform(object_class_string, object_id, recipient_user_ids) object = object_class_string.constantize.find(object_id) # TODO: create visibilities - # TODO: send notifications + NotificationService.new.notify(object, recipient_user_ids) rescue ActiveRecord::RecordNotFound # Already deleted before the job could run end end diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb index 24fb115d6..c46d2cf38 100644 --- a/spec/integration/federation/receive_federation_messages_spec.rb +++ b/spec/integration/federation/receive_federation_messages_spec.rb @@ -92,14 +92,13 @@ describe "Receive federation messages feature" do expect(new_contact).not_to be_nil expect(new_contact.sharing).to eq(true) - # TODO: handle notifications - # expect( - # Notifications::StartedSharing.exists?( - # recipient_id: alice.id, - # target_type: "Person", - # target_id: sender.person.id - # ) - # ).to be_truthy + expect( + Notifications::StartedSharing.exists?( + recipient_id: alice.id, + target_type: "Person", + target_id: sender.person.id + ) + ).to be_truthy end context "with sharing" do diff --git a/spec/integration/federation/shared_receive_stream_items.rb b/spec/integration/federation/shared_receive_stream_items.rb index 1ab087bc6..77ab41342 100644 --- a/spec/integration/federation/shared_receive_stream_items.rb +++ b/spec/integration/federation/shared_receive_stream_items.rb @@ -26,8 +26,6 @@ shared_examples_for "messages which are indifferent about sharing fact" do describe "notifications are sent where required" do it "for comment on local post" do - skip("TODO: handle notifications") # TODO - entity = create_relayable_entity(:comment_entity, local_parent, remote_user_on_pod_b.diaspora_handle) post_message(generate_xml(entity, sender, recipient), recipient) @@ -41,8 +39,6 @@ shared_examples_for "messages which are indifferent about sharing fact" do end it "for like on local post" do - skip("TODO: handle notifications") # TODO - entity = create_relayable_entity(:like_entity, local_parent, remote_user_on_pod_b.diaspora_handle) post_message(generate_xml(entity, sender, recipient), recipient) @@ -134,8 +130,6 @@ shared_examples_for "messages which can't be send without sharing" do # this one shouldn't depend on the sharing fact. this must be fixed describe "notifications are sent where required" do it "for comment on remote post where we participate" do - skip("TODO: handle notifications") # TODO - alice.participate!(remote_parent) author_id = remote_user_on_pod_c.diaspora_handle entity = create_relayable_entity(:comment_entity, remote_parent, author_id) diff --git a/spec/models/notifications/also_commented_spec.rb b/spec/models/notifications/also_commented_spec.rb new file mode 100644 index 000000000..74b69b6f4 --- /dev/null +++ b/spec/models/notifications/also_commented_spec.rb @@ -0,0 +1,47 @@ +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +require "spec_helper" + +describe Notifications::AlsoCommented, type: :model do + let(:sm) { FactoryGirl.build(:status_message, author: alice.person, public: true) } + let(:comment) { FactoryGirl.create(:comment, commentable: sm) } + let(:notification) { Notifications::AlsoCommented.new(recipient: bob) } + + describe ".notify" do + it "does not notify the commentable author" do + expect(Notifications::AlsoCommented).not_to receive(:concatenate_or_create) + + Notifications::AlsoCommented.notify(comment, []) + end + + it "notifies a local participant" do + bob.participate!(sm) + + expect(Notifications::AlsoCommented).to receive(:concatenate_or_create).with( + bob, sm, comment.author + ).and_return(notification) + expect(bob).to receive(:mail).with(Workers::Mail::AlsoCommented, bob.id, comment.author.id, comment.id) + + Notifications::AlsoCommented.notify(comment, []) + end + + it "does not notify the a remote participant" do + FactoryGirl.create(:participation, target: sm) + + expect(Notifications::AlsoCommented).not_to receive(:concatenate_or_create) + + Notifications::AlsoCommented.notify(comment, []) + end + + it "does not notify the author of the comment" do + bob.participate!(sm) + comment = FactoryGirl.create(:comment, commentable: sm, author: bob.person) + + expect(Notifications::AlsoCommented).not_to receive(:concatenate_or_create) + + Notifications::AlsoCommented.notify(comment, []) + end + end +end