From cef6389e4bf135a1cfef1a6b556c7a2352e6e1bf Mon Sep 17 00:00:00 2001 From: Jon Lemmon Date: Tue, 27 Nov 2012 23:07:33 +1300 Subject: [PATCH] Do not notify non-group members when at-mentioned. This commit makes it so that at-mentions do not notify users who are not members of the group. I've also prevented mentioned users from receiving "new comment" notifications, since they are already receiving at-mention notifications for the comment. However, the code is a bit sloppy. It will need to be refactored with Rob's upcoming event model refactor. --- app/models/comment.rb | 5 +++- app/models/discussion.rb | 6 ----- app/models/event.rb | 13 +++++++++-- features/mention_user.feature | 14 ++++++++++- features/step_definitions/group_steps.rb | 20 +++++++++------- .../step_definitions/mention_user_steps.rb | 23 +++++++++++++++---- spec/models/discussion_spec.rb | 4 ++-- 7 files changed, 60 insertions(+), 25 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index f5b3037d91e..73170e2b49e 100644 --- a/app/models/comment.rb +++ b/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 diff --git a/app/models/discussion.rb b/app/models/discussion.rb index d65ee171fe5..33ee624730f 100644 --- a/app/models/discussion.rb +++ b/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 diff --git a/app/models/event.rb b/app/models/event.rb index fed545ef466..23761ed54a0 100644 --- a/app/models/event.rb +++ b/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 diff --git a/features/mention_user.feature b/features/mention_user.feature index 96bcdad00e8..5c2a30f5a3e 100644 --- a/features/mention_user.feature +++ b/features/mention_user.feature @@ -6,13 +6,25 @@ Background: Given a group "demo-group" with "furry@example.com" as admin And I am logged in as "furry@example.com" -Scenario: Mention user in comment +Scenario: Mention user when writing a comment Given "harry@example.com" 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 "harry@example.com" 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 "harry@example.com" 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 "harry@example.com" is a member of "demo-group" And I am viewing a discussion titled "hello" in "demo-group" diff --git a/features/step_definitions/group_steps.rb b/features/step_definitions/group_steps.rb index 45ab24029d8..838388348f7 100644 --- a/features/step_definitions/group_steps.rb +++ b/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 diff --git a/features/step_definitions/mention_user_steps.rb b/features/step_definitions/mention_user_steps.rb index 0c03aa04e61..87433263e1b 100644 --- a/features/step_definitions/mention_user_steps.rb +++ b/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 diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index faacf0737f3..acd9c5463ab 100644 --- a/spec/models/discussion_spec.rb +++ b/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