From 97c351c7b4e312b6eae647e86a7fe8f84f1ad267 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Thu, 16 Feb 2017 01:45:13 +0100 Subject: [PATCH 1/4] Add In-Reply-To and References header to mentioned in comment mail --- app/mailers/notification_mailers/mentioned_in_comment.rb | 1 + spec/mailers/notifier_spec.rb | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/app/mailers/notification_mailers/mentioned_in_comment.rb b/app/mailers/notification_mailers/mentioned_in_comment.rb index abb346ea8..0e9a51bba 100644 --- a/app/mailers/notification_mailers/mentioned_in_comment.rb +++ b/app/mailers/notification_mailers/mentioned_in_comment.rb @@ -5,6 +5,7 @@ module NotificationMailers def set_headers(target_id) # rubocop:disable Style/AccessorMethodName @comment = Mention.find_by_id(target_id).mentions_container + @headers[:in_reply_to] = @headers[:references] = "<#{@comment.parent.guid}@#{AppConfig.pod_uri.host}>" @headers[:subject] = I18n.t("notifier.mentioned.subject", name: @sender.name) end end diff --git a/spec/mailers/notifier_spec.rb b/spec/mailers/notifier_spec.rb index 2ded0d80f..eb871f7cd 100644 --- a/spec/mailers/notifier_spec.rb +++ b/spec/mailers/notifier_spec.rb @@ -120,6 +120,11 @@ describe Notifier, type: :mailer do expect(mail.subject).to include(comment.author.name) end + it "IN-REPLY-TO and REFERENCES: references the commented post" do + expect(mail.in_reply_to).to eq("#{comment.parent.guid}@#{AppConfig.pod_uri.host}") + expect(mail.references).to eq("#{comment.parent.guid}@#{AppConfig.pod_uri.host}") + end + it "has the comment link in the body" do expect(mail.body.encoded).to include(post_url(comment.parent, anchor: comment.guid)) end From eac8c7572ca63813d7397ef95e3bc0fc7c3fee7b Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Thu, 16 Feb 2017 01:58:19 +0100 Subject: [PATCH 2/4] Remove unused From headers It is already added in the default_headers --- app/mailers/notification_mailers/also_commented.rb | 1 - app/mailers/notification_mailers/comment_on_post.rb | 1 - app/mailers/notification_mailers/private_message.rb | 3 +-- spec/mailers/notifier_spec.rb | 6 ++++-- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/mailers/notification_mailers/also_commented.rb b/app/mailers/notification_mailers/also_commented.rb index aa063cfa8..c6c4da8de 100644 --- a/app/mailers/notification_mailers/also_commented.rb +++ b/app/mailers/notification_mailers/also_commented.rb @@ -7,7 +7,6 @@ module NotificationMailers @comment = Comment.find_by_id(comment_id) if mail? - @headers[:from] = "\"#{@comment.author_name} (diaspora*)\" <#{AppConfig.mail.sender_address}>" @headers[:in_reply_to] = @headers[:references] = "<#{@comment.parent.guid}@#{AppConfig.pod_uri.host}>" if @comment.public? @headers[:subject] = "Re: #{@comment.comment_email_subject}" diff --git a/app/mailers/notification_mailers/comment_on_post.rb b/app/mailers/notification_mailers/comment_on_post.rb index 4782f52b2..c9957e86f 100644 --- a/app/mailers/notification_mailers/comment_on_post.rb +++ b/app/mailers/notification_mailers/comment_on_post.rb @@ -5,7 +5,6 @@ module NotificationMailers def set_headers(comment_id) @comment = Comment.find(comment_id) - @headers[:from] = "\"#{@comment.author_name} (diaspora*)\" <#{AppConfig.mail.sender_address}>" @headers[:in_reply_to] = @headers[:references] = "<#{@comment.parent.guid}@#{AppConfig.pod_uri.host}>" if @comment.public? @headers[:subject] = "Re: #{@comment.comment_email_subject}" diff --git a/app/mailers/notification_mailers/private_message.rb b/app/mailers/notification_mailers/private_message.rb index 9cbe71dc3..60a5013e8 100644 --- a/app/mailers/notification_mailers/private_message.rb +++ b/app/mailers/notification_mailers/private_message.rb @@ -3,11 +3,10 @@ module NotificationMailers attr_accessor :message, :conversation, :participants def set_headers(message_id) - @message = Message.find_by_id(message_id) + @message = Message.find_by_id(message_id) @conversation = @message.conversation @participants = @conversation.participants - @headers[:from] = "\"#{@message.author_name} (diaspora*)\" <#{AppConfig.mail.sender_address}>" @headers[:subject] = I18n.t("notifier.private_message.subject") @headers[:in_reply_to] = @headers[:references] = "<#{@conversation.guid}@#{AppConfig.pod_uri.host}>" end diff --git a/spec/mailers/notifier_spec.rb b/spec/mailers/notifier_spec.rb index eb871f7cd..3adaca132 100644 --- a/spec/mailers/notifier_spec.rb +++ b/spec/mailers/notifier_spec.rb @@ -281,7 +281,9 @@ describe Notifier, type: :mailer do let(:comment) { eve.comment!(commented_post, "Totally is") } describe ".comment_on_post" do - let(:comment_mail) { Notifier.send_notification("comment_on_post", bob.id, person.id, comment.id).deliver_now } + let(:comment_mail) { + Notifier.send_notification("comment_on_post", bob.id, eve.person.id, comment.id).deliver_now + } it "TO: goes to the right person" do expect(comment_mail.to).to eq([bob.email]) @@ -322,7 +324,7 @@ describe Notifier, type: :mailer do end describe ".also_commented" do - let(:comment_mail) { Notifier.send_notification("also_commented", bob.id, person.id, comment.id) } + let(:comment_mail) { Notifier.send_notification("also_commented", bob.id, eve.person.id, comment.id) } it "TO: goes to the right person" do expect(comment_mail.to).to eq([bob.email]) From 3d95642aca30c3dddc665c6d2379e7545227aaa4 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Thu, 16 Feb 2017 03:02:38 +0100 Subject: [PATCH 3/4] Fix receiving multiple mentioned-in-comment notifications When somebody commented multiple times and is mentioned in another comment afterwards, the person received multiple notifications. --- app/models/notifications/mentioned_in_comment.rb | 2 +- spec/integration/mentioning_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/notifications/mentioned_in_comment.rb b/app/models/notifications/mentioned_in_comment.rb index 5e5466b68..5b57f395b 100644 --- a/app/models/notifications/mentioned_in_comment.rb +++ b/app/models/notifications/mentioned_in_comment.rb @@ -11,7 +11,7 @@ module Notifications end def self.filter_mentions(mentions, mentionable, _recipient_user_ids) - mentions.joins(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent)) + mentions.includes(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent)).distinct end def mail_job diff --git a/spec/integration/mentioning_spec.rb b/spec/integration/mentioning_spec.rb index 2e6b4292d..367758bf8 100644 --- a/spec/integration/mentioning_spec.rb +++ b/spec/integration/mentioning_spec.rb @@ -376,6 +376,18 @@ describe "mentioning", type: :request do end end end + + it "only creates one notification for the mentioned person, when mentioned person commented twice before" do + parent = FactoryGirl.create(:status_message_in_aspect, author: author.person) + mentioned_user = FactoryGirl.create(:user_with_aspect, friends: [author]) + mentioned_user.comment!(parent, "test comment 1") + mentioned_user.comment!(parent, "test comment 2") + comment = inlined_jobs do + author.comment!(parent, text_mentioning(mentioned_user)) + end + + expect(notifications_about_mentioning(mentioned_user, comment).count).to eq(1) + end end end end From 509f429cc8b5bd530545dd3dc189f54105d4d409 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Sun, 5 Mar 2017 17:41:12 +0100 Subject: [PATCH 4/4] Move 'distinct' to 'allowed_to_be_mentioned_in_a_comment_to' --- .../notifications/mentioned_in_comment.rb | 2 +- app/models/person.rb | 19 ++++++++---------- spec/models/person_spec.rb | 20 +++++++++---------- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/app/models/notifications/mentioned_in_comment.rb b/app/models/notifications/mentioned_in_comment.rb index 5b57f395b..35ef9b61a 100644 --- a/app/models/notifications/mentioned_in_comment.rb +++ b/app/models/notifications/mentioned_in_comment.rb @@ -11,7 +11,7 @@ module Notifications 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)).distinct + mentions.includes(:person).merge(Person.allowed_to_be_mentioned_in_a_comment_to(mentionable.parent)) end def mail_job diff --git a/app/models/person.rb b/app/models/person.rb index e9eefc6ec..3da18333b 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -61,11 +61,7 @@ class Person < ActiveRecord::Base } scope :remote, -> { where('people.owner_id IS NULL') } scope :local, -> { where('people.owner_id IS NOT NULL') } - scope :for_json, -> { - select("people.id, people.guid, people.diaspora_handle") - .distinct - .includes(:profile) - } + scope :for_json, -> { select("people.id, people.guid, people.diaspora_handle").includes(:profile) } # @note user is passed in here defensively scope :all_from_aspects, ->(aspect_ids, user) { @@ -131,12 +127,13 @@ class Person < ActiveRecord::Base # @param [Post] the post for which we query mentionable in comments people # @return [Person::ActiveRecord_Relation] scope :allowed_to_be_mentioned_in_a_comment_to, ->(post) { - if post.public? - all - else - left_join_visible_post_interactions_on_authorship(post.id) - .where("comments.id IS NOT NULL OR likes.id IS NOT NULL OR people.id = #{post.author_id}") - end + allowed = if post.public? + all + else + left_join_visible_post_interactions_on_authorship(post.id) + .where("comments.id IS NOT NULL OR likes.id IS NOT NULL OR people.id = #{post.author_id}") + end + allowed.distinct } # This scope adds sorting of people in the order, appropriate for suggesting to a user (current user) who diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 4e65c6ccc..9c54e98ee 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -24,16 +24,6 @@ describe Person, :type => :model do Person.for_json.first.serialized_public_key }.to raise_error ActiveModel::MissingAttributeError end - - it 'selects distinct people' do - aspect = bob.aspects.create(:name => 'hilarious people') - aspect.contacts << bob.contact_for(eve.person) - person_ids = Person.for_json.joins(:contacts => :aspect_memberships). - where(:contacts => {:user_id => bob.id}, - :aspect_memberships => {:aspect_id => bob.aspect_ids}).map{|p| p.id} - - expect(person_ids.uniq).to eq(person_ids) - end end describe '.local' do @@ -131,9 +121,17 @@ describe Person, :type => :model do ).to match_array([alice, bob, eve, kate].map(&:person_id)) end + it "selects distinct people" do + alice.comment!(status_bob, "hyi") + alice.comment!(status_bob, "how are you?") + expect( + Person.allowed_to_be_mentioned_in_a_comment_to(status_bob).ids + ).to match_array([alice, bob].map(&:person_id)) + end + it "returns all for public posts" do status_bob.update(public: true) # set parent public - expect(Person.allowed_to_be_mentioned_in_a_comment_to(status_bob)).to eq(Person.all) + expect(Person.allowed_to_be_mentioned_in_a_comment_to(status_bob).ids).to match_array(Person.ids) end end