Merge pull request #7331 from SuperTux88/fix-mentioned-in-comments-notifications

Fix mentioned in comments notifications

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}"

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}"

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

app/mailers/notification_mailers/private_message.rb

@@ -7,7 +7,6 @@ module NotificationMailers
       @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

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))
     end
 
     def mail_job

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, -> {
+  scope :for_json, -> { select("people.id, people.guid, people.diaspora_handle").includes(:profile) }
-    select("people.id, people.guid, people.diaspora_handle")
-      .distinct
-      .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?
+    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

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

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
@@ -276,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])
@@ -317,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])

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, "hi")
+        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