From 26b281035512758715ed9381b083bf003befbd5e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 30 Mar 2016 18:41:21 -0300 Subject: [PATCH] Mentions on confidential issues doesn't create todos for non-members --- CHANGELOG | 3 ++ app/services/todo_service.rb | 24 ++++++++-- spec/services/todo_service_spec.rb | 77 ++++++++++++++++++++++-------- 3 files changed, 79 insertions(+), 25 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 25eeb24b499..139d8de8ac6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -14,6 +14,9 @@ v 8.7.0 (unreleased) - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.) - Gracefully handle notes on deleted commits in merge requests (Stan Hu) +v 8.6.3 (unreleased) + - Mentions on confidential issues doesn't create todos for non-members + v 8.6.2 - Fix dropdown alignment. !3298 - Fix issuable sidebar overlaps on tablet. !3299 diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f2662922e90..cfd69064548 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -170,14 +170,30 @@ class TodoService end def filter_mentioned_users(project, target, author) - mentioned_users = target.mentioned_users.select do |user| - user.can?(:read_project, project) - end - + mentioned_users = target.mentioned_users + mentioned_users = reject_users_without_access(mentioned_users, project, target) mentioned_users.delete(author) mentioned_users.uniq end + def reject_users_without_access(users, project, target) + if target.is_a?(Note) && target.for_issue? + target = target.noteable + end + + if target.is_a?(Issue) + select_users(users, :read_issue, target) + else + select_users(users, :read_project, project) + end + end + + def select_users(users, ability, subject) + users.select do |user| + user.can?(ability.to_sym, subject) + end + end + def pending_todos(user, criteria = {}) valid_keys = [:project_id, :target_id, :target_type, :commit_id] user.todos.pending.where(criteria.slice(*valid_keys)) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index b4728807b8b..82b7fbfa816 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -2,22 +2,25 @@ require 'spec_helper' describe TodoService, services: true do let(:author) { create(:user) } - let(:john_doe) { create(:user, username: 'john_doe') } - let(:michael) { create(:user, username: 'michael') } - let(:stranger) { create(:user, username: 'stranger') } + let(:assignee) { create(:user) } + let(:non_member) { create(:user) } + let(:member) { create(:user) } + let(:admin) { create(:admin) } + let(:john_doe) { create(:user) } let(:project) { create(:project) } - let(:mentions) { [author.to_reference, john_doe.to_reference, michael.to_reference, stranger.to_reference].join(' ') } + let(:mentions) { [author, assignee, john_doe, member, non_member, admin].map(&:to_reference).join(' ') } let(:service) { described_class.new } before do project.team << [author, :developer] + project.team << [member, :developer] project.team << [john_doe, :developer] - project.team << [michael, :developer] end describe 'Issues' do let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: mentions) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee, description: mentions) } describe '#new_issue' do it 'creates a todo if assigned' do @@ -37,10 +40,20 @@ describe TodoService, services: true do it 'creates a todo for each valid mentioned user' do service.new_issue(issue, author) - should_create_todo(user: michael, target: issue, action: Todo::MENTIONED) + should_create_todo(user: member, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) - should_not_create_todo(user: stranger, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED) + end + + it 'does not create todo for non project members when issue is confidential' do + service.new_issue(confidential_issue, john_doe) + + should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::ASSIGNED) + should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end end @@ -48,16 +61,26 @@ describe TodoService, services: true do it 'creates a todo for each valid mentioned user' do service.update_issue(issue, author) - should_create_todo(user: michael, target: issue, action: Todo::MENTIONED) + should_create_todo(user: member, target: issue, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) - should_not_create_todo(user: stranger, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED) end it 'does not create a todo if user was already mentioned' do - create(:todo, :mentioned, user: michael, project: project, target: issue, author: author) + create(:todo, :mentioned, user: member, project: project, target: issue, author: author) - expect { service.update_issue(issue, author) }.not_to change(michael.todos, :count) + expect { service.update_issue(issue, author) }.not_to change(member.todos, :count) + end + + it 'does not create todo for non project members when issue is confidential' do + service.update_issue(confidential_issue, john_doe) + + should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end end @@ -109,8 +132,10 @@ describe TodoService, services: true do describe '#new_note' do let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } let(:note_on_commit) { create(:note_on_commit, project: project, author: john_doe, note: mentions) } + let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project, note: mentions) } let(:note_on_project_snippet) { create(:note_on_project_snippet, project: project, author: john_doe, note: mentions) } let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } let(:system_note) { create(:system_note, project: project, noteable: issue) } @@ -142,19 +167,29 @@ describe TodoService, services: true do it 'creates a todo for each valid mentioned user' do service.new_note(note, john_doe) - should_create_todo(user: michael, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_create_todo(user: member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_create_todo(user: author, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_not_create_todo(user: john_doe, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) - should_not_create_todo(user: stranger, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_not_create_todo(user: non_member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + end + + it 'does not create todo for non project members when leaving a note on a confidential issue' do + service.new_note(note_on_confidential_issue, john_doe) + + should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) end it 'creates a todo for each valid mentioned user when leaving a note on commit' do service.new_note(note_on_commit, john_doe) - should_create_todo(user: michael, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) + should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) should_not_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) - should_not_create_todo(user: stranger, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) + should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) end it 'does not create todo when leaving a note on snippet' do @@ -185,10 +220,10 @@ describe TodoService, services: true do it 'creates a todo for each valid mentioned user' do service.new_merge_request(mr_assigned, author) - should_create_todo(user: michael, target: mr_assigned, action: Todo::MENTIONED) + should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) - should_not_create_todo(user: stranger, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED) end end @@ -196,16 +231,16 @@ describe TodoService, services: true do it 'creates a todo for each valid mentioned user' do service.update_merge_request(mr_assigned, author) - should_create_todo(user: michael, target: mr_assigned, action: Todo::MENTIONED) + should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) - should_not_create_todo(user: stranger, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED) end it 'does not create a todo if user was already mentioned' do - create(:todo, :mentioned, user: michael, project: project, target: mr_assigned, author: author) + create(:todo, :mentioned, user: member, project: project, target: mr_assigned, author: author) - expect { service.update_merge_request(mr_assigned, author) }.not_to change(michael.todos, :count) + expect { service.update_merge_request(mr_assigned, author) }.not_to change(member.todos, :count) end end