Merge pull request loomio#396 from enspiral/bug/non-member-mentions

Do not notify non-group members when at-mentioned.

app/models/comment.rb

@@ -103,7 +103,10 @@ def parse_mentions
     usernames = extract_mentioned_screen_names(self.body)
     usernames.each do |name|
       user = User.find_by_username(name)
-      users.push(user) if user
+      # Only users that belong to this discussion's group
+      if user && user.group_ids.include?(discussion.group_id)
+        users.push(user)
+      end
     end
     users
   end

app/models/discussion.rb

@@ -50,12 +50,6 @@ def add_comment(user, comment)
       comment.save
       if comment.valid?
         Event.new_comment!(comment)
-        mentions = comment.parse_mentions
-        if mentions.present?
-          mentions.each do |mentioned_user|
-            Event.user_mentioned!(comment, mentioned_user)
-          end
-        end
       end
       comment
     end

app/models/event.rb

@@ -1,6 +1,7 @@
 class Event < ActiveRecord::Base
   KINDS = %w[new_discussion new_comment new_motion new_vote motion_blocked
-             motion_closing_soon motion_closed membership_requested user_added_to_group comment_liked user_mentioned]
+             motion_closing_soon motion_closed membership_requested
+             user_added_to_group comment_liked user_mentioned]
 
   has_many :notifications, :dependent => :destroy
   belongs_to :eventable, :polymorphic => true
@@ -23,8 +24,16 @@ def self.new_discussion!(discussion)
 
   def self.new_comment!(comment)
     event = create!(:kind => "new_comment", :eventable => comment)
+    mentions = comment.parse_mentions
+    # Fire off mentions
+    if mentions.present?
+      mentions.each do |mentioned_user|
+        Event.user_mentioned!(comment, mentioned_user)
+      end
+    end
     comment.discussion_participants.each do |user|
-      unless user == comment.user
+      # Do not notify yourself or mentioned users (they have already been notified)
+      unless (user == comment.user) || mentions.include?(user)
         event.notifications.create! :user => user
       end
     end

features/mention_user.feature

@@ -6,13 +6,25 @@ Background:
   Given a group "demo-group" with "[email protected]" as admin
   And I am logged in as "[email protected]"
 
-Scenario: Mention user in comment
+Scenario: Mention user when writing a comment
   Given "[email protected]" is a member of "demo-group"
   And I am viewing a discussion titled "hello" in "demo-group"
   When I am adding a comment and type in "@h"
   And I click on "@harry" in the menu that pops up
   Then I should see "@harry" added to the "new-comment" field
 
+Scenario: Submit a comment mentioning a group member
+  Given "[email protected]" is a member of "demo-group"
+  And I am viewing a discussion titled "hello" in "demo-group"
+  When I submit a comment mentioning "@harry"
+  Then the user should be notified that they were mentioned
+
+Scenario: Submit a comment mentioning a group non-member
+  Given "[email protected]" is a member of "a different group"
+  And I am viewing a discussion titled "hello" in "demo-group"
+  When I submit a comment mentioning "@harry"
+  Then the user should not be notified that they were mentioned
+
 Scenario: View comment with mentions
   Given "[email protected]" is a member of "demo-group"
   And I am viewing a discussion titled "hello" in "demo-group"

features/step_definitions/group_steps.rb

@@ -14,20 +14,24 @@
   click_link("subgroup-new")
 end
 
-Given /^"(.*?)" is a(?: non-admin)?(?: member)? of(?: group)? "(.*?)"$/ do |email, group|
+Given /^"(.*?)" is a(?: non-admin)?(?: member)? of(?: group)? "(.*?)"$/ do |email, group_name|
   @user = User.find_by_email(email)
-  if !@user 
+  if !@user
     @user = FactoryGirl.create(:user, :name => email.split("@").first, :email => email)
-  end 
-  Group.find_by_name(group).add_member!(@user)
+  end
+  group = Group.find_by_name(group_name)
+  group ||= FactoryGirl.create(:group, :name => group_name)
+  group.add_member!(@user)
 end
 
-Given /^"(.*?)" is a[n]? admin(?: member)? of(?: group)? "(.*?)"$/ do |email, group|
+Given /^"(.*?)" is an admin of(?: group)? "(.*?)"$/ do |email, group_name|
   user = User.find_by_email(email)
-  if !user 
+  if !user
     user = FactoryGirl.create(:user, :email => email)
-  end 
-  Group.find_by_name(group).add_admin!(user)
+  end
+  group = Group.find_by_name(group_name)
+  group ||= FactoryGirl.create(:group, :name => group_name)
+  group.add_admin!(user)
 end
 
 When /^I fill details for the subgroup$/ do

features/step_definitions/mention_user_steps.rb

@@ -10,16 +10,29 @@
   end
 end
 
+When /^a comment exists mentioning "(.*?)"$/ do |text|
+  @discussion.add_comment @user, "Hey #{text}"
+end
+
+When /^I submit a comment mentioning "(.*?)"$/ do |mention|
+  fill_in 'new-comment', with: mention
+  click_button "post-new-comment"
+end
+
 Then /^I should see "(.*?)" added to the "(.*?)" field$/ do |text, field|
   input = find_field(field)
   input.value.should =~ /#{text}/
 end
 
-When /^a comment exists mentioning "(.*?)"$/ do |text|
-  @discussion.add_comment @user, "Hey #{text}"
-end
-
-Then /^I should see a link to "(.*?)"'s user$/ do |user|
+Then /^I should see a link to "(.*?)"\'s user$/ do |user|
   visit(current_path)
   page.should have_link("@#{user}")
 end
+
+Then /^the user should be notified that they were mentioned$/ do
+  Event.where(:kind => "user_mentioned").count.should == 1
+end
+
+Then /^the user should not be notified that they were mentioned$/ do
+  Event.where(:kind => "user_mentioned").count.should == 0
+end

spec/models/discussion_spec.rb

@@ -28,7 +28,7 @@
       @user = create(:user)
       @discussion.group.add_member! @user
     end
-    
+
     it "fires new_comment event if comment was created successfully" do
       Event.should_receive(:new_comment!)
       @discussion.add_comment(@user, "this is a test comment")
@@ -69,7 +69,7 @@
       @discussion.should have(@version_count + 1).versions
     end
   end
-  
+
   describe "#never_read_by(user)" do
     before do
       @discussion = create :discussion