From f4459488e5b397f768dbd0fb65f3b126f2d259d4 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Sun, 14 Aug 2016 01:02:27 +0200 Subject: [PATCH 1/2] allow other people to share with a user who ignores them otherwise we have data-inconsistency if the user stops ignoring the person. --- app/models/contact.rb | 5 +++-- spec/models/contact_spec.rb | 11 +++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index a4025ad1a..272e5cb91 100644 --- a/app/models/contact.rb +++ b/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 - diff --git a/spec/models/contact_spec.rb b/spec/models/contact_spec.rb index a1fa9e750..db7bd71b0 100644 --- a/spec/models/contact_spec.rb +++ b/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 From 0d338b6f79c0cae23b627f81c69a626fdb2b1047 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Sun, 14 Aug 2016 15:40:49 +0200 Subject: [PATCH 2/2] don't create notifications if the notification-actor is ignored Also move "shareable hidden"-logic to AlsoCommented, because it is the only one that needs it. And write some specs for mentioned and started sharing notifications. Fixes #6294 --- app/models/notification.rb | 12 ++-- app/models/notifications/also_commented.rb | 5 +- app/models/notifications/mentioned.rb | 2 +- app/models/notifications/reshared.rb | 2 +- app/models/notifications/started_sharing.rb | 2 +- spec/models/notification_spec.rb | 4 +- .../notifications/also_commented_spec.rb | 20 ++++++ spec/models/notifications/mentioned_spec.rb | 72 +++++++++++++++++++ spec/models/notifications/reshared_spec.rb | 10 +++ .../notifications/started_sharing_spec.rb | 33 +++++++++ 10 files changed, 150 insertions(+), 12 deletions(-) create mode 100644 spec/models/notifications/mentioned_spec.rb create mode 100644 spec/models/notifications/started_sharing_spec.rb diff --git a/app/models/notification.rb b/app/models/notification.rb index 11511462c..aaa56d53d 100644 --- a/app/models/notification.rb +++ b/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) - create(recipient_id: recipient_id, target: target, actors: [actor]) + def self.create_notification(recipient, target, 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) - post.is_a?(Post) && recipient.is_shareable_hidden?(post) + private_class_method def self.suppress_notification?(recipient, actor) + recipient.blocks.where(person: actor).exists? end def self.types diff --git a/app/models/notifications/also_commented.rb b/app/models/notifications/also_commented.rb index c20c88df4..a345566f3 100644 --- a/app/models/notifications/also_commented.rb +++ b/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) - .try {|notification| notification.email_the_user(comment, actor) } + next if recipient.is_shareable_hidden?(commentable) + + concatenate_or_create(recipient, commentable, actor).try(:email_the_user, comment, actor) end end end diff --git a/app/models/notifications/mentioned.rb b/app/models/notifications/mentioned.rb index 9636d574b..bede65ee4 100644 --- a/app/models/notifications/mentioned.rb +++ b/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 diff --git a/app/models/notifications/reshared.rb b/app/models/notifications/reshared.rb index 51f089e35..0b438a483 100644 --- a/app/models/notifications/reshared.rb +++ b/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 diff --git a/app/models/notifications/started_sharing.rb b/app/models/notifications/started_sharing.rb index bae3a3a1e..dfe6dfd55 100644 --- a/app/models/notifications/started_sharing.rb +++ b/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 diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index a2ecbb076..6008ac5f5 100644 --- a/spec/models/notification_spec.rb +++ b/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) diff --git a/spec/models/notifications/also_commented_spec.rb b/spec/models/notifications/also_commented_spec.rb index 74b69b6f4..72361bf03 100644 --- a/spec/models/notifications/also_commented_spec.rb +++ b/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 diff --git a/spec/models/notifications/mentioned_spec.rb b/spec/models/notifications/mentioned_spec.rb new file mode 100644 index 000000000..cb63bb9ff --- /dev/null +++ b/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 diff --git a/spec/models/notifications/reshared_spec.rb b/spec/models/notifications/reshared_spec.rb index 5c22ab350..203ed5230 100644 --- a/spec/models/notifications/reshared_spec.rb +++ b/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 diff --git a/spec/models/notifications/started_sharing_spec.rb b/spec/models/notifications/started_sharing_spec.rb new file mode 100644 index 000000000..29fb2d4e7 --- /dev/null +++ b/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