diff options
author | Valery Sizov <valery@gitlab.com> | 2017-03-23 21:21:24 +0300 |
---|---|---|
committer | Valery Sizov <valery@gitlab.com> | 2017-03-27 20:54:08 +0300 |
commit | 7b64b205d57ba46b0a3a814aa8bab48e724d15ee (patch) | |
tree | dcf7f3655d411cc59bea1474e9253e04422fb936 | |
parent | 3f98525fc359845e0d919ba9de674723e9fb4ecd (diff) |
[Multiple issue assignees] Fixes for notification, todos, system notes[ci skip]
24 files changed, 196 insertions, 83 deletions
diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index d64e48f774b..0f847841295 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -11,10 +11,12 @@ module Emails mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end - def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id) + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id) setup_issue_mail(issue_id, recipient_id) - @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id + @previous_assignees = [] + @previous_assignees = User.where(id: previous_assignee_ids) if previous_assignee_ids.any? + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) end diff --git a/app/models/issue.rb b/app/models/issue.rb index 18b8315a6be..9109f24906d 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -140,11 +140,17 @@ class Issue < ActiveRecord::Base end def assignee_or_author?(user) - author_id == user.id || assignees.exists?(user) + author_id == user.id || assignees.exists?(user.id) end def assignee_list - assignees.pluck(:name).join(', ') + assignees.map(&:to_reference).to_sentence + end + + # TODO: This method will help us to find some silent failures. + # We should remove it before merging to master + def assignee_id + raise "assignee_id is deprecated" end # `from` argument can be a Namespace or Project. diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 58f1a14935c..13b1da1f3ce 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -193,6 +193,11 @@ class MergeRequest < ActiveRecord::Base } end + # This method is needed for compatibility with issues + def assignees + [assignee] + end + def assignee_or_author?(user) author_id == user.id || assignee_id == user.id end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 97058db9229..82531226e1e 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -1,11 +1,6 @@ class IssuableBaseService < BaseService private - def create_assignee_note(issuable) - SystemNoteService.change_assignee( - issuable, issuable.project, current_user, issuable.assignee) - end - def create_milestone_note(issuable) SystemNoteService.change_milestone( issuable, issuable.project, current_user, issuable.milestone) @@ -77,7 +72,7 @@ class IssuableBaseService < BaseService def assignee_can_read?(issuable, assignee_id) new_assignee = User.find_by_id(assignee_id) - return false unless new_assignee.present? + return false unless new_assignee ability_name = :"read_#{issuable.to_ability_name}" resource = issuable.persisted? ? issuable : project @@ -207,6 +202,7 @@ class IssuableBaseService < BaseService filter_params(issuable) old_labels = issuable.labels.to_a old_mentioned_users = issuable.mentioned_users.to_a + old_assignees = issuable.assignees.to_a label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids) params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids) @@ -222,7 +218,13 @@ class IssuableBaseService < BaseService handle_common_system_notes(issuable, old_labels: old_labels) end - handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users) + handle_changes( + issuable, + old_labels: old_labels, + old_mentioned_users: old_mentioned_users, + old_assignees: old_assignees + ) + after_update(issuable) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index ee1b40db718..588d3e5354e 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -9,11 +9,28 @@ module Issues private + def create_assignee_note(issue) + SystemNoteService.change_issue_assignees( + issue, issue.project, current_user, issue.assignees) + end + def execute_hooks(issue, action = 'open') issue_data = hook_data(issue, action) hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks issue.project.execute_hooks(issue_data, hooks_scope) issue.project.execute_services(issue_data, hooks_scope) end + + def filter_assignee(issuable) + return if params[:assignee_ids].blank? + + assignee_ids = params[:assignee_ids].split(',').map(&:strip) + + if assignee_ids == [ IssuableFinder::NONE ] + params[:assignee_ids] = "" + else + params.delete(:assignee_ids) unless assignee_ids.all?{ |assignee_id| assignee_can_read?(issuable, assignee_id)} + end + end end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index a444c78b609..fd44e10e7f9 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -12,7 +12,7 @@ module Issues spam_check(issue, current_user) end - def handle_changes(issue, old_labels: [], old_mentioned_users: []) + def handle_changes(issue, old_labels: [], old_mentioned_users: [], old_assignees: []) if has_changes?(issue, old_labels: old_labels) todo_service.mark_pending_todos_as_done(issue, current_user) end @@ -26,9 +26,9 @@ module Issues create_milestone_note(issue) end - if issue.previous_changes.include?('assignee_id') + if issue.previous_changes.include?('assignee_ids') create_assignee_note(issue) - notification_service.reassigned_issue(issue, current_user) + notification_service.reassigned_issue(issue, current_user, old_assignees) todo_service.reassigned_issue(issue, current_user) end diff --git a/app/services/merge_requests/assign_issues_service.rb b/app/services/merge_requests/assign_issues_service.rb index 066efa1acc3..93b515ef5c2 100644 --- a/app/services/merge_requests/assign_issues_service.rb +++ b/app/services/merge_requests/assign_issues_service.rb @@ -4,7 +4,7 @@ module MergeRequests @assignable_issues ||= begin if current_user == merge_request.author closes_issues.select do |issue| - !issue.is_a?(ExternalIssue) && !issue.assignee_id? && can?(current_user, :admin_issue, issue) + !issue.is_a?(ExternalIssue) && !issue.assignees.any? && can?(current_user, :admin_issue, issue) end else [] @@ -14,7 +14,7 @@ module MergeRequests def execute assignable_issues.each do |issue| - Issues::UpdateService.new(issue.project, current_user, assignee_id: current_user.id).execute(issue) + Issues::UpdateService.new(issue.project, current_user, assignee_ids: [current_user.id]).execute(issue) end { diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 5a53b973059..92c64d0dd4e 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -38,6 +38,11 @@ module MergeRequests private + def create_assignee_note(merge_request) + SystemNoteService.change_assignee( + merge_request, merge_request.project, current_user, merge_request.assignee) + end + # Returns all origin and fork merge requests from `@project` satisfying passed arguments. def merge_requests_for(source_branch, mr_states: [:opened]) MergeRequest diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 44ae23fad18..e18436740aa 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -3,11 +3,12 @@ # class NotificationRecipientService attr_reader :project - + def initialize(project) @project = project end + # TODO: refactor this: previous_assignee argument can be a user object or an array which is not really nice def build_recipients(target, current_user, action: nil, previous_assignee: nil, skip_current_user: true) custom_action = build_custom_key(action, target) @@ -23,9 +24,13 @@ class NotificationRecipientService # Re-assign is considered as a mention of the new assignee so we add the # new assignee to the list of recipients after we rejected users with # the "on mention" notification level - if [:reassign_merge_request, :reassign_issue].include?(custom_action) + case custom_action + when :reassign_merge_request recipients << previous_assignee if previous_assignee recipients << target.assignee + when :reassign_issue + recipients.concat(previous_assignee) if previous_assignee.any? + recipients.concat(target.assignees) end recipients = reject_muted_users(recipients) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index b0fb83c1951..eddd206da00 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -66,8 +66,23 @@ class NotificationService # * issue new assignee if their notification level is not Disabled # * users with custom level checked with "reassign issue" # - def reassigned_issue(issue, current_user) - reassign_resource_email(issue, issue.project, current_user, :reassigned_issue_email) + def reassigned_issue(issue, current_user, previous_assignees = []) + recipients = NotificationRecipientService.new(issue.project).build_recipients( + issue, + current_user, + action: "reassign", + previous_assignee: previous_assignees + ) + + recipients.each do |recipient| + mailer.send( + :reassigned_issue_email, + recipient.id, + issue.id, + previous_assignees.map(&:id), + current_user.id + ).deliver_later + end end # When we add labels to an issue we should send an email to: @@ -407,10 +422,10 @@ class NotificationService end def previous_record(object, attribute) - if object && attribute - if object.previous_changes.include?(attribute) - object.previous_changes[attribute].first - end + return unless object && attribute + + if object.previous_changes.include?(attribute) + object.previous_changes[attribute].first end end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index c5ae02a59a7..3fac8eff440 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -49,6 +49,42 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when the assignees of an Issue is changed or removed + # + # issue - Issue object + # project - Project owning noteable + # author - User performing the change + # assignees - User being assigned, or nil + # + # Example Note text: + # + # "removed all assignees" + # + # "assigned to @user1 additionally to @user2" + # + # "assigned to @user1, @user2 and @user3 and unassigned from @user4 and @user5" + # + # "assigned to @user1 and @user2" + # + # Returns the created Note object + def change_issue_assignees(issue, project, author, assignees) + # TODO: basic implementation, should be improved before merging the MR + body = + if issue.assignees.any? && assignees.any? + unassigned_users = issue.assignees - assignees + added_users = assignees - issue.assignees + + "assigned to #{added_users.map(&:to_reference).to_sentence} and unassigned #{unassigned_users.map(&:to_reference).to_sentence}" + elsif issue.assignees.any? + "removed all assignees" + elsif assignees.any? + "assigned to #{assignees.map(&:to_reference).to_sentence}" + end + + create_note(noteable: issue, project: project, author: author, note: body) + end + + # Called when one or more labels on a Noteable are added and/or removed # # noteable - Noteable object diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index a4844ed4cf8..a86fef926a1 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -264,13 +264,13 @@ class TodoService end def create_assignment_todo(issuable, author) - if issuable.assignee + if issuable.assignees.any? attributes = attributes_for_todo(issuable.project, issuable, author, Todo::ASSIGNED) - create_todos(issuable.assignee, attributes) + create_todos(issuable.assignees, attributes) end end - def create_mention_todos(project, target, author, note = nil) + def create_mention_todos(project, target, author, note = nil) # Create Todos for directly addressed users directly_addressed_users = filter_directly_addressed_users(project, note || target, author) attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note) diff --git a/app/views/notify/_reassigned_issuable_email.text.erb b/app/views/notify/_reassigned_issuable_email.text.erb index daf20a226dd..be217e810aa 100644 --- a/app/views/notify/_reassigned_issuable_email.text.erb +++ b/app/views/notify/_reassigned_issuable_email.text.erb @@ -1,6 +1,6 @@ -Reassigned <%= issuable.class.model_name.human.titleize %> <%= issuable.iid %> +Reassigned Issue <%= @issue.iid %> -<%= url_for([issuable.project.namespace.becomes(Namespace), issuable.project, issuable, { only_path: false }]) %> +<%= url_for([@issue.project.namespace.becomes(Namespace), @issue.project, @issue, { only_path: false }]) %> -Assignee changed <%= "from #{@previous_assignee.name}" if @previous_assignee -%> - to <%= "#{issuable.assignee_id ? issuable.assignee_name : 'Unassigned'}" %> +Assignee changed <%= "from #{@previous_assignees.map(&:name)}" if @previous_assignees.any? -%> + to <%= "#{@issue.assignees.any? ? @issue.assignees.map(&:name) : 'Unassigned'}" %> diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index d1855568215..15592711b47 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -4,6 +4,6 @@ - if @issue.description = markdown(@issue.description, pipeline: :email, author: @issue.author) -- if @issue.assignee_id.present? +- if @issue.assignees.any? %p - Assignee: #{@issue.assignee_name} + Assignee: #{@issue.assignee_list} diff --git a/app/views/notify/new_issue_email.text.erb b/app/views/notify/new_issue_email.text.erb index ca5c2f2688c..13f1ac08e94 100644 --- a/app/views/notify/new_issue_email.text.erb +++ b/app/views/notify/new_issue_email.text.erb @@ -2,6 +2,6 @@ New Issue was created. Issue <%= @issue.iid %>: <%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %> Author: <%= @issue.author_name %> -Assignee: <%= @issue.assignee_name %> +Assignee: <%= @issue.assignee_list %> <%= @issue.description %> diff --git a/app/views/notify/new_mention_in_issue_email.html.haml b/app/views/notify/new_mention_in_issue_email.html.haml index 02f21baa368..c935e9003d3 100644 --- a/app/views/notify/new_mention_in_issue_email.html.haml +++ b/app/views/notify/new_mention_in_issue_email.html.haml @@ -7,6 +7,6 @@ - if @issue.description = markdown(@issue.description, pipeline: :email, author: @issue.author) -- if @issue.assignee_id.present? +- if @issue.assignees.any? %p - Assignee: #{@issue.assignee_name} + Assignee: #{@issue.assignee_list} diff --git a/app/views/notify/new_mention_in_issue_email.text.erb b/app/views/notify/new_mention_in_issue_email.text.erb index 457e94b4800..f19ac3adfc7 100644 --- a/app/views/notify/new_mention_in_issue_email.text.erb +++ b/app/views/notify/new_mention_in_issue_email.text.erb @@ -2,6 +2,6 @@ You have been mentioned in an issue. Issue <%= @issue.iid %>: <%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %> Author: <%= @issue.author_name %> -Assignee: <%= @issue.assignee_name %> +Assignee: <%= @issue.assignee_list %> <%= @issue.description %> diff --git a/app/views/notify/reassigned_issue_email.html.haml b/app/views/notify/reassigned_issue_email.html.haml index 498ba8b8365..755e5c57868 100644 --- a/app/views/notify/reassigned_issue_email.html.haml +++ b/app/views/notify/reassigned_issue_email.html.haml @@ -1 +1,11 @@ -= render 'reassigned_issuable_email', issuable: @issue +%p + Assignee changed + - if @previous_assignees.any? + from + %strong= @previous_assignees.map(&:to_reference).to_sentence + to + - if @issue.assignees.any? + %strong= @issue.assignee_list + - else + %strong Unassigned + diff --git a/app/views/notify/reassigned_merge_request_email.text.erb b/app/views/notify/reassigned_merge_request_email.text.erb index b5b4f1ff99a..998a40fefde 100644 --- a/app/views/notify/reassigned_merge_request_email.text.erb +++ b/app/views/notify/reassigned_merge_request_email.text.erb @@ -1 +1,6 @@ -<%= render 'reassigned_issuable_email', issuable: @merge_request %> +Reassigned Merge Request <%= @merge_request.iid %> + +<%= url_for([@merge_request.project.namespace.becomes(Namespace), @merge_request.project, @merge_request, { only_path: false }]) %> + +Assignee changed <%= "from #{@previous_assignee.name}" if @previous_assignee -%> + to <%= "#{@merge_request.assignee_id ? @merge_request.assignee_name : 'Unassigned'}" %> diff --git a/db/migrate/20170320171632_create_issue_assignees_table.rb b/db/migrate/20170320171632_create_issue_assignees_table.rb index f6e6b04beb3..ef5462ab42e 100644 --- a/db/migrate/20170320171632_create_issue_assignees_table.rb +++ b/db/migrate/20170320171632_create_issue_assignees_table.rb @@ -25,8 +25,10 @@ class CreateIssueAssigneesTable < ActiveRecord::Migration def change create_table :issue_assignees do |t| - t.references :user, foreign_key: true, index: true - t.references :issue, foreign_key: true, index: true + t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false + t.references :issue, foreign_key: { on_delete: :cascade }, null: false end + + add_index :issue_assignees, [:issue_id, :user_id], unique: true end end diff --git a/db/schema.rb b/db/schema.rb index 3fdc2320998..53f0898a671 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1444,7 +1444,6 @@ ActiveRecord::Schema.define(version: 20170320173259) do t.boolean "authorized_projects_populated" t.boolean "auditor", default: false, null: false t.boolean "ghost" - t.boolean "notified_of_own_activity", default: false, null: false end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index 71a4a2c43c7..471f801c263 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -16,7 +16,7 @@ describe Dashboard::TodosController do describe 'GET #index' do context 'when using pagination' do let(:last_page) { user.todos.page.total_pages } - let!(:issues) { create_list(:issue, 2, project: project, assignee: user) } + let!(:issues) { create_list(:issue, 2, project: project, assignees: [user]) } before do issues.each { |issue| todo_service.new_issue(issue, user) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index cdb2736d925..76e059b1f33 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -4,6 +4,7 @@ describe NotificationService, services: true do include EmailHelpers let(:notification) { NotificationService.new } + let(:assignee) { create(:user) } around(:each) do |example| perform_enqueued_jobs do @@ -52,7 +53,11 @@ describe NotificationService, services: true do shared_examples 'participating by assignee notification' do it 'emails the participant' do - issuable.update_attribute(:assignee, participant) + if issuable.is_a?(Issue) + issuable.assignees << participant + else + issuable.update_attribute(:assignee, participant) + end notification_trigger @@ -103,14 +108,14 @@ describe NotificationService, services: true do describe 'Notes' do context 'issue note' do let(:project) { create(:empty_project, :private) } - let(:issue) { create(:issue, project: project, assignee: create(:user)) } - let(:mentioned_issue) { create(:issue, assignee: issue.assignee) } + let(:issue) { create(:issue, project: project, assignees: [assignee]) } + let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @outsider also') } before do build_team(note.project) project.add_master(issue.author) - project.add_master(issue.assignee) + project.add_master(assignee) project.add_master(note.author) create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy') update_custom_notification(:new_note, @u_guest_custom, project) @@ -130,7 +135,7 @@ describe NotificationService, services: true do should_email(@u_watcher) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_email(@u_custom_global) should_email(@u_mentioned) should_email(@subscriber) @@ -186,7 +191,7 @@ describe NotificationService, services: true do notification.new_note(note) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_email(@u_mentioned) should_email(@u_custom_global) should_not_email(@u_guest_custom) @@ -208,7 +213,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) } let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") } let(:guest_watcher) { create_user_with_notification(:watch, "guest-watcher-confidential") } @@ -234,8 +239,8 @@ describe NotificationService, services: true do context 'issue note mention' do let(:project) { create(:empty_project, :public) } - let(:issue) { create(:issue, project: project, assignee: create(:user)) } - let(:mentioned_issue) { create(:issue, assignee: issue.assignee) } + let(:issue) { create(:issue, project: project, assignees: [assignee]) } + let(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } before do @@ -259,7 +264,7 @@ describe NotificationService, services: true do should_email(@u_guest_watcher) should_email(note.noteable.author) - should_email(note.noteable.assignee) + should_email(note.noteable.assignees.first) should_not_email(note.author) should_email(@u_mentioned) should_not_email(@u_disabled) @@ -439,7 +444,7 @@ describe NotificationService, services: true do let(:group) { create(:group) } let(:project) { create(:empty_project, :public, namespace: group) } let(:another_project) { create(:empty_project, :public, namespace: group) } - let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' } + let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant' } before do build_team(issue.project) @@ -455,7 +460,7 @@ describe NotificationService, services: true do it do notification.new_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -470,10 +475,10 @@ describe NotificationService, services: true do end it do - create_global_setting_for(issue.assignee, :mention) + create_global_setting_for(issue.assignees.first, :mention) notification.new_issue(issue, @u_disabled) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) end it "emails subscribers of the issue's labels" do @@ -504,7 +509,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } it "emails subscribers of the issue's labels that can read the issue" do project.add_developer(member) @@ -548,9 +553,9 @@ describe NotificationService, services: true do end it 'emails new assignee' do - notification.reassigned_issue(issue, @u_disabled) + notification.reassigned_issue(issue, @u_disabled, [assignee]) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -564,9 +569,8 @@ describe NotificationService, services: true do end it 'emails previous assignee even if he has the "on mention" notif level' do - issue.update_attribute(:assignee, @u_mentioned) - issue.update_attributes(assignee: @u_watcher) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_watcher]) should_email(@u_mentioned) should_email(@u_watcher) @@ -582,11 +586,11 @@ describe NotificationService, services: true do end it 'emails new assignee even if he has the "on mention" notif level' do - issue.update_attributes(assignee: @u_mentioned) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned - should_email(issue.assignee) + expect(issue.assignees.first).to be @u_mentioned + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -600,11 +604,11 @@ describe NotificationService, services: true do end it 'emails new assignee' do - issue.update_attribute(:assignee, @u_mentioned) - notification.reassigned_issue(issue, @u_disabled) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_disabled, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned - should_email(issue.assignee) + expect(issue.assignees.first).to be @u_mentioned + should_email(issue.assignees.first) should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -618,17 +622,17 @@ describe NotificationService, services: true do end it 'does not email new assignee if they are the current user' do - issue.update_attribute(:assignee, @u_mentioned) - notification.reassigned_issue(issue, @u_mentioned) + issue.assignees = [@u_mentioned] + notification.reassigned_issue(issue, @u_mentioned, [@u_mentioned]) - expect(issue.assignee).to be @u_mentioned + expect(issue.assignees.first).to be @u_mentioned should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@u_custom_global) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -638,7 +642,7 @@ describe NotificationService, services: true do it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { issue } - let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled) } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end end @@ -668,7 +672,7 @@ describe NotificationService, services: true do it "doesn't send email to anyone but subscribers of the given labels" do notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) - should_not_email(issue.assignee) + should_not_email(issue.assignees.first) should_not_email(issue.author) should_not_email(@u_watcher) should_not_email(@u_guest_watcher) @@ -692,7 +696,7 @@ describe NotificationService, services: true do let(:member) { create(:user) } let(:guest) { create(:user) } let(:admin) { create(:admin) } - let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } + let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignees: [assignee]) } let!(:label_1) { create(:label, project: project, issues: [confidential_issue]) } let!(:label_2) { create(:label, project: project) } @@ -730,7 +734,7 @@ describe NotificationService, services: true do it 'sends email to issue assignee and issue author' do notification.close_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) @@ -761,7 +765,7 @@ describe NotificationService, services: true do it 'sends email to issue notification recipients' do notification.reopen_issue(issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) @@ -789,7 +793,7 @@ describe NotificationService, services: true do it 'sends email to issue notification recipients' do notification.issue_moved(issue, new_issue, @u_disabled) - should_email(issue.assignee) + should_email(issue.assignees.first) should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb index 922e82445d0..e2efd10abbb 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_spec.rb @@ -54,7 +54,7 @@ describe Users::DestroyService, services: true do end context "for an issue the user was assigned to" do - let!(:issue) { create(:issue, project: project, assignee: user) } + let!(:issue) { create(:issue, project: project, assignees: [user]) } before do service.execute(user) @@ -67,7 +67,7 @@ describe Users::DestroyService, services: true do it 'migrates the issue so that it is "Unassigned"' do migrated_issue = Issue.find_by_id(issue.id) - expect(migrated_issue.assignee).to be_nil + expect(migrated_issue.assignees).to be_empty end end end |