Merge pull request #6984 from SuperTux88/6294-ignore-notifications

Don't create notifications from ignored people

Changelog.md

@@ -197,6 +197,7 @@ The command will report queues that still have jobs and launch sidekiq process f
 * Make screenreaders read alerts [#6973](https://github.com/diaspora/diaspora/pull/6973)
 * Display message when there are no posts in a stream [#6974](https://github.com/diaspora/diaspora/pull/6974)
 * Add bootstrap-markdown editor to the publisher [#6551](https://github.com/diaspora/diaspora/pull/6551)
+* Don't create notifications for ignored users [#6984](https://github.com/diaspora/diaspora/pull/6984)
 
 # 0.5.10.2
 

app/models/contact.rb

@@ -118,7 +118,8 @@ class Contact < ActiveRecord::Base
   end
 
   def not_blocked_user
-    errors.add(:base, "Cannot connect to an ignored user") if user && user.blocks.where(person_id: person_id).exists?
+    if receiving && user && user.blocks.where(person_id: person_id).exists?
+      errors.add(:base, "Cannot connect to an ignored user")
+    end
   end
 end
-

app/models/notification.rb

@@ -29,7 +29,7 @@ class Notification < ActiveRecord::Base
   end
 
   def self.concatenate_or_create(recipient, target, actor)
-    return nil if suppress_notification?(recipient, target)
+    return nil if suppress_notification?(recipient, actor)
 
     find_or_initialize_by(recipient: recipient, target: target, unread: true).tap do |notification|
       notification.actors |= [actor]
@@ -42,12 +42,14 @@ class Notification < ActiveRecord::Base
     end
   end
 
-  def self.create_notification(recipient_id, target, actor)
+  def self.create_notification(recipient, target, actor)
-    create(recipient_id: recipient_id, target: target, actors: [actor])
+    return nil if suppress_notification?(recipient, actor)
+
+    create(recipient: recipient, target: target, actors: [actor])
   end
 
-  private_class_method def self.suppress_notification?(recipient, post)
+  private_class_method def self.suppress_notification?(recipient, actor)
-    post.is_a?(Post) && recipient.is_shareable_hidden?(post)
+    recipient.blocks.where(person: actor).exists?
   end
 
   def self.types

app/models/notifications/also_commented.rb

@@ -18,8 +18,9 @@ module Notifications
       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)
+        next if recipient.is_shareable_hidden?(commentable)
-          .try {|notification| notification.email_the_user(comment, actor) }
+
+        concatenate_or_create(recipient, commentable, actor).try(:email_the_user, comment, actor)
       end
     end
   end

app/models/notifications/mentioned.rb

@@ -24,7 +24,7 @@ module Notifications
 
         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)
+        create_notification(recipient.owner, mention, actor).try(:email_the_user, mention, actor)
       end
     end
   end

app/models/notifications/reshared.rb

@@ -16,7 +16,7 @@ module Notifications
       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)
+      concatenate_or_create(reshare.root.author.owner, reshare.root, actor).try(:email_the_user, reshare, actor)
     end
   end
 end

app/models/notifications/started_sharing.rb

@@ -10,7 +10,7 @@ module Notifications
 
     def self.notify(contact, _recipient_user_ids)
       sender = contact.person
-      create_notification(contact.user_id, sender, sender).email_the_user(sender, sender)
+      create_notification(contact.user, sender, sender).try(:email_the_user, sender, sender)
     end
 
     def contact

spec/models/contact_spec.rb

@@ -62,14 +62,21 @@ describe Contact, type: :model do
     end
 
     describe "#not_blocked_user" do
-      it "add error if potential contact is blocked by user" do
+      it "adds an error when start sharing with a blocked person" do
         alice.blocks.create(person: eve.person)
-        bad_contact = alice.contacts.create(person: eve.person)
+        bad_contact = alice.contacts.create(person: eve.person, receiving: true)
 
         expect(bad_contact).not_to be_valid
         expect(bad_contact.errors.full_messages.count).to eq(1)
         expect(bad_contact.errors.full_messages.first).to eq("Cannot connect to an ignored user")
       end
+
+      it "is valid when a blocked person starts sharing with the user" do
+        alice.blocks.create(person: eve.person)
+        bad_contact = alice.contacts.create(person: eve.person, receiving: false, sharing: true)
+
+        expect(bad_contact).to be_valid
+      end
     end
   end
 

spec/models/notification_spec.rb

@@ -70,14 +70,14 @@ describe Notification, :type => :model do
     end
 
     it "appends the actors to the already existing notification" do
-      notification = Notification.create_notification(alice.id, @sm, @person)
+      notification = Notification.create_notification(alice, @sm, @person)
       expect {
         Notification.concatenate_or_create(alice, @sm, eve.person)
       }.to change(notification.actors, :count).by(1)
     end
 
     it "doesn't append the actor to an existing notification if it is already there" do
-      notification = Notification.create_notification(alice.id, @sm, @person)
+      notification = Notification.create_notification(alice, @sm, @person)
       expect {
         Notification.concatenate_or_create(alice, @sm, @person)
       }.not_to change(notification.actors, :count)

spec/models/notifications/also_commented_spec.rb

@@ -43,5 +43,25 @@ describe Notifications::AlsoCommented, type: :model do
 
       Notifications::AlsoCommented.notify(comment, [])
     end
+
+    it "does not notify if the commentable is hidden" do
+      bob.participate!(sm)
+      bob.add_hidden_shareable(sm.class.base_class.to_s, sm.id.to_s)
+
+      expect(Notifications::AlsoCommented).not_to receive(:concatenate_or_create)
+
+      Notifications::AlsoCommented.notify(comment, [])
+    end
+
+    it "does not notify if the author of the comment is ignored" do
+      bob.participate!(sm)
+      bob.blocks.create(person: comment.author)
+
+      expect_any_instance_of(Notifications::AlsoCommented).not_to receive(:email_the_user)
+
+      Notifications::AlsoCommented.notify(comment, [])
+
+      expect(Notifications::AlsoCommented.where(target: sm)).not_to exist
+    end
   end
 end

spec/models/notifications/mentioned_spec.rb

@@ -0,0 +1,72 @@
+require "spec_helper"
+
+describe Notifications::Mentioned, type: :model do
+  let(:sm) {
+    FactoryGirl.create(:status_message, author: alice.person, text: "hi @{bob; #{bob.diaspora_handle}}", public: true)
+  }
+  let(:mentioned_notification) { Notifications::Mentioned.new(recipient: bob) }
+
+  describe ".notify" do
+    it "calls create_notification with mention" do
+      expect(Notifications::Mentioned).to receive(:create_notification).with(
+        bob, sm.mentions.first, sm.author
+      ).and_return(mentioned_notification)
+
+      Notifications::Mentioned.notify(sm, [])
+    end
+
+    it "sends an email to the mentioned person" do
+      allow(Notifications::Mentioned).to receive(:create_notification).and_return(mentioned_notification)
+      expect(bob).to receive(:mail).with(Workers::Mail::Mentioned, bob.id, sm.author.id, sm.mentions.first.id)
+
+      Notifications::Mentioned.notify(sm, [])
+    end
+
+    it "does nothing if the mentioned person is not local" do
+      sm = FactoryGirl.create(
+        :status_message,
+        author: alice.person,
+        text:   "hi @{raphael; #{remote_raphael.diaspora_handle}}",
+        public: true
+      )
+      expect(Notifications::Mentioned).not_to receive(:create_notification)
+
+      Notifications::Mentioned.notify(sm, [])
+    end
+
+    it "does not notify if the author of the post is ignored" do
+      bob.blocks.create(person: sm.author)
+
+      expect_any_instance_of(Notifications::Mentioned).not_to receive(:email_the_user)
+
+      Notifications::Mentioned.notify(sm, [])
+
+      expect(Notifications::Mentioned.where(target: sm.mentions.first)).not_to exist
+    end
+
+    context "with private post" do
+      let(:private_sm) {
+        FactoryGirl.create(
+          :status_message,
+          author: remote_raphael,
+          text:   "hi @{bob; #{bob.diaspora_handle}}",
+          public: false
+        )
+      }
+
+      it "calls create_notification if the mentioned person is a recipient of the post" do
+        expect(Notifications::Mentioned).to receive(:create_notification).with(
+          bob, private_sm.mentions.first, private_sm.author
+        ).and_return(mentioned_notification)
+
+        Notifications::Mentioned.notify(private_sm, [bob.id])
+      end
+
+      it "does not call create_notification if the mentioned person is not a recipient of the post" do
+        expect(Notifications::Mentioned).not_to receive(:create_notification)
+
+        Notifications::Mentioned.notify(private_sm, [alice.id])
+      end
+    end
+  end
+end

spec/models/notifications/reshared_spec.rb

@@ -38,5 +38,15 @@ describe Notifications::Reshared, type: :model do
 
       Notifications::Reshared.notify(reshare, [])
     end
+
+    it "does not notify if the author of the reshare is ignored" do
+      alice.blocks.create(person: reshare.author)
+
+      expect_any_instance_of(Notifications::Reshared).not_to receive(:email_the_user)
+
+      Notifications::Reshared.notify(reshare, [])
+
+      expect(Notifications::Reshared.where(target: sm)).not_to exist
+    end
   end
 end

spec/models/notifications/started_sharing_spec.rb

@@ -0,0 +1,33 @@
+require "spec_helper"
+
+describe Notifications::StartedSharing, type: :model do
+  let(:contact) { alice.contact_for(bob.person) }
+  let(:started_sharing_notification) { Notifications::StartedSharing.new(recipient: alice) }
+
+  describe ".notify" do
+    it "calls create_notification with sender" do
+      expect(Notifications::StartedSharing).to receive(:create_notification).with(
+        alice, bob.person, bob.person
+      ).and_return(started_sharing_notification)
+
+      Notifications::StartedSharing.notify(contact, [])
+    end
+
+    it "sends an email to the contacted user" do
+      allow(Notifications::StartedSharing).to receive(:create_notification).and_return(started_sharing_notification)
+      expect(alice).to receive(:mail).with(Workers::Mail::StartedSharing, alice.id, bob.person.id, bob.person.id)
+
+      Notifications::StartedSharing.notify(contact, [])
+    end
+
+    it "does not notify if the sender of the contact is ignored" do
+      alice.blocks.create(person: contact.person)
+
+      expect_any_instance_of(Notifications::StartedSharing).not_to receive(:email_the_user)
+
+      Notifications::StartedSharing.notify(contact, [])
+
+      expect(Notifications::StartedSharing.where(target: bob.person)).not_to exist
+    end
+  end
+end