From 63c1cdce42f140146c2dfac1e49832a33b84b2cf Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Mon, 23 Jun 2014 11:54:58 +0200 Subject: [PATCH 1/4] Show @all in autocomplete list. --- app/controllers/projects_controller.rb | 2 +- lib/gitlab/markdown.rb | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 2dcc19bed07..8f2253bfb7c 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -130,7 +130,7 @@ class ProjectsController < ApplicationController [] end team_members = sorted(@project.team.members) - participants = team_members + participating + participants = [{ username: "all", name: "Group Members" }] + team_members + participating @suggestions = { emojis: Emoji.names.map { |e| { name: e, path: view_context.image_url("emoji/#{e}.png") } }, issues: @project.issues.select([:iid, :title, :description]), diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index c04be788f07..759bedee80b 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -169,7 +169,12 @@ module Gitlab end def reference_user(identifier, project = @project) - if user = User.find_by(username: identifier) + if identifier == "all" + options = html_options.merge( + class: "gfm gfm-team_member #{html_options[:class]}" + ) + link_to("@all", project_url(project), options) + elsif user = User.find_by(username: identifier) options = html_options.merge( class: "gfm gfm-team_member #{html_options[:class]}" ) From 185da5f976db725c072d3b738b264a401b73cba6 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Mon, 23 Jun 2014 12:28:57 +0200 Subject: [PATCH 2/4] Make sure that at mentioning all properly notifies. --- app/controllers/projects_controller.rb | 2 +- app/models/concerns/mentionable.rb | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8f2253bfb7c..66fc037cead 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -130,7 +130,7 @@ class ProjectsController < ApplicationController [] end team_members = sorted(@project.team.members) - participants = [{ username: "all", name: "Group Members" }] + team_members + participating + participants = [{ username: "all", name: "Project and Group Members" }] + team_members + participating @suggestions = { emojis: Emoji.names.map { |e| { name: e, path: view_context.image_url("emoji/#{e}.png") } }, issues: @project.issues.select([:iid, :title, :description]), diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index a46a593c6e3..80d3677526c 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -49,12 +49,16 @@ module Mentionable matches = mentionable_text.scan(/@[a-zA-Z][a-zA-Z0-9_\-\.]*/) matches.each do |match| identifier = match.delete "@" - if has_project - id = project.team.members.find_by(username: identifier).try(:id) + if identifier == "all" + users += project.team.members.flatten else - id = User.where(username: identifier).pluck(:id).first + if has_project + id = project.team.members.find_by(username: identifier).try(:id) + else + id = User.where(username: identifier).pluck(:id).first + end + users << User.find(id) unless id.blank? end - users << User.find(id) unless id.blank? end users.uniq end From 8f0051e2c2250728cc660bd9770ca6f3a70b084a Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Mon, 23 Jun 2014 14:02:09 +0200 Subject: [PATCH 3/4] Test @all mentioning notifications. --- spec/services/notification_service_spec.rb | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 57a4240f7a2..f82a14d482c 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -95,6 +95,49 @@ describe NotificationService do end end + context 'issue note mention' do + let(:issue) { create(:issue, assignee: create(:user)) } + let(:mentioned_issue) { create(:issue, assignee: issue.assignee) } + let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } + + before do + build_team(note.project) + end + + describe :new_note do + it do + # Notify all team members + note.project.team.members.each do |member| + # User with disabled notification should not be notified + next if member.id == @u_disabled.id + should_email(member.id) + end + should_email(note.noteable.author_id) + should_email(note.noteable.assignee_id) + + should_not_email(note.author_id) + should_not_email(@u_disabled.id) + should_not_email(@u_not_mentioned.id) + notification.new_note(note) + end + + it 'filters out "mentioned in" notes' do + mentioned_note = Note.create_cross_reference_note(mentioned_issue, issue, issue.author, issue.project) + + Notify.should_not_receive(:note_issue_email) + notification.new_note(mentioned_note) + end + end + + def should_email(user_id) + Notify.should_receive(:note_issue_email).with(user_id, note.id) + end + + def should_not_email(user_id) + Notify.should_not_receive(:note_issue_email).with(user_id, note.id) + end + end + context 'commit note' do let(:note) { create(:note_on_commit) } @@ -312,6 +355,7 @@ describe NotificationService do @u_disabled = create(:user, notification_level: Notification::N_DISABLED) @u_mentioned = create(:user, username: 'mention', notification_level: Notification::N_PARTICIPATING) @u_committer = create(:user, username: 'committer') + @u_not_mentioned = create(:user, username: 'regular', notification_level: Notification::N_PARTICIPATING) project.team << [@u_watcher, :master] project.team << [@u_participating, :master] From 4c575b72d542783e889cf11961f33ead7e23cc59 Mon Sep 17 00:00:00 2001 From: Marin Jankovski Date: Mon, 23 Jun 2014 15:44:49 +0200 Subject: [PATCH 4/4] Move checking of recepients to a service. --- app/controllers/projects_controller.rb | 31 +------------ app/models/concerns/mentionable.rb | 2 +- app/services/projects/participants_service.rb | 43 +++++++++++++++++++ lib/gitlab/markdown.rb | 8 ++-- 4 files changed, 49 insertions(+), 35 deletions(-) create mode 100644 app/services/projects/participants_service.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 66fc037cead..0d15b458b70 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -124,18 +124,12 @@ class ProjectsController < ApplicationController def autocomplete_sources note_type = params['type'] note_id = params['type_id'] - participating = if note_type && note_id - participants_in(note_type, note_id) - else - [] - end - team_members = sorted(@project.team.members) - participants = [{ username: "all", name: "Project and Group Members" }] + team_members + participating + participants = ::Projects::ParticipantsService.new(@project).execute(note_type, note_id) @suggestions = { emojis: Emoji.names.map { |e| { name: e, path: view_context.image_url("emoji/#{e}.png") } }, issues: @project.issues.select([:iid, :title, :description]), mergerequests: @project.merge_requests.select([:iid, :title, :description]), - members: participants.uniq + members: participants } respond_to do |format| @@ -191,25 +185,4 @@ class ProjectsController < ApplicationController def user_layout current_user ? "projects" : "public_projects" end - - def participants_in(type, id) - users = case type - when "Issue" - issue = @project.issues.find_by_iid(id) - issue ? issue.participants : [] - when "MergeRequest" - merge_request = @project.merge_requests.find_by_iid(id) - merge_request ? merge_request.participants : [] - when "Commit" - author_ids = Note.for_commit_id(id).pluck(:author_id).uniq - User.where(id: author_ids) - else - [] - end - sorted(users) - end - - def sorted(users) - users.uniq.to_a.compact.sort_by(&:username).map { |user| { username: user.username, name: user.name } } - end end diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 80d3677526c..81414959f3b 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -55,7 +55,7 @@ module Mentionable if has_project id = project.team.members.find_by(username: identifier).try(:id) else - id = User.where(username: identifier).pluck(:id).first + id = User.find_by(username: identifier).try(:id) end users << User.find(id) unless id.blank? end diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb new file mode 100644 index 00000000000..c4d2c0963b7 --- /dev/null +++ b/app/services/projects/participants_service.rb @@ -0,0 +1,43 @@ +module Projects + class ParticipantsService < BaseService + def initialize(project) + @project = project + end + + def execute(note_type, note_id) + participating = if note_type && note_id + participants_in(note_type, note_id) + else + [] + end + team_members = sorted(@project.team.members) + participants = all_members + team_members + participating + participants.uniq + end + + def participants_in(type, id) + users = case type + when "Issue" + issue = @project.issues.find_by_iid(id) + issue ? issue.participants : [] + when "MergeRequest" + merge_request = @project.merge_requests.find_by_iid(id) + merge_request ? merge_request.participants : [] + when "Commit" + author_ids = Note.for_commit_id(id).pluck(:author_id).uniq + User.where(id: author_ids) + else + [] + end + sorted(users) + end + + def sorted(users) + users.uniq.to_a.compact.sort_by(&:username).map { |user| { username: user.username, name: user.name } } + end + + def all_members + [{ username: "all", name: "Project and Group Members" }] + end + end +end diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 759bedee80b..b248d8f9436 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -169,15 +169,13 @@ module Gitlab end def reference_user(identifier, project = @project) - if identifier == "all" - options = html_options.merge( + options = html_options.merge( class: "gfm gfm-team_member #{html_options[:class]}" ) + + if identifier == "all" link_to("@all", project_url(project), options) elsif user = User.find_by(username: identifier) - options = html_options.merge( - class: "gfm gfm-team_member #{html_options[:class]}" - ) link_to("@#{identifier}", user_url(identifier), options) end end