diff options
author | Valery Sizov <valery@gitlab.com> | 2017-05-04 15:11:15 +0300 |
---|---|---|
committer | Valery Sizov <valery@gitlab.com> | 2017-05-04 17:11:53 +0300 |
commit | 387c4b2c21a44360386a9b8ce6849e7f1b8a3de9 (patch) | |
tree | 446b8338efe8ad22ca03b00b2dc72b22c4174e02 /app/services | |
parent | 68c12e15cc236548918f91393ebef3c06c124814 (diff) |
Backport of multiple_assignees_feature [ci skip]
Diffstat (limited to 'app/services')
-rw-r--r-- | app/services/issuable/bulk_update_service.rb | 6 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 23 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 19 | ||||
-rw-r--r-- | app/services/issues/update_service.rb | 14 | ||||
-rw-r--r-- | app/services/merge_requests/assign_issues_service.rb | 4 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 5 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 5 | ||||
-rw-r--r-- | app/services/notification_recipient_service.rb | 7 | ||||
-rw-r--r-- | app/services/notification_service.rb | 27 | ||||
-rw-r--r-- | app/services/slash_commands/interpret_service.rb | 23 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 38 | ||||
-rw-r--r-- | app/services/todo_service.rb | 4 |
12 files changed, 143 insertions, 32 deletions
diff --git a/app/services/issuable/bulk_update_service.rb b/app/services/issuable/bulk_update_service.rb index 60891cbb255..40ff9b8b867 100644 --- a/app/services/issuable/bulk_update_service.rb +++ b/app/services/issuable/bulk_update_service.rb @@ -7,10 +7,14 @@ module Issuable ids = params.delete(:issuable_ids).split(",") items = model_class.where(id: ids) - %i(state_event milestone_id assignee_id add_label_ids remove_label_ids subscription_event).each do |key| + %i(state_event milestone_id assignee_id assignee_ids add_label_ids remove_label_ids subscription_event).each do |key| params.delete(key) unless params[key].present? end + if params[:assignee_ids] == [IssuableFinder::NONE.to_s] + params[:assignee_ids] = [] + end + items.each do |issuable| next unless can?(current_user, :"update_#{type}", issuable) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index b071a398481..6c2777a8d2d 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) @@ -53,6 +48,7 @@ class IssuableBaseService < BaseService params.delete(:add_label_ids) params.delete(:remove_label_ids) params.delete(:label_ids) + params.delete(:assignee_ids) params.delete(:assignee_id) params.delete(:due_date) end @@ -77,7 +73,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 +203,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 +219,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') @@ -272,7 +275,7 @@ class IssuableBaseService < BaseService end end - def has_changes?(issuable, old_labels: []) + def has_changes?(issuable, old_labels: [], old_assignees: []) valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] attrs_changed = valid_attrs.any? do |attr| @@ -281,7 +284,9 @@ class IssuableBaseService < BaseService labels_changed = issuable.labels != old_labels - attrs_changed || labels_changed + assignees_changed = issuable.assignees != old_assignees + + attrs_changed || labels_changed || assignees_changed end def handle_common_system_notes(issuable, old_labels: []) diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index ee1b40db718..eedbfa724ff 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -9,11 +9,30 @@ module Issues private + def create_assignee_note(issue, old_assignees) + SystemNoteService.change_issue_assignees( + issue, issue.project, current_user, old_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].select { |assignee_id| assignee_can_read?(issuable, assignee_id) } + + if params[:assignee_ids].map(&:to_s) == [IssuableFinder::NONE] + params[:assignee_ids] = [] + elsif assignee_ids.any? + params[:assignee_ids] = assignee_ids + else + params.delete(:assignee_ids) + end + end end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index b7fe5cb168b..cd9f9a4a16e 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -12,8 +12,12 @@ module Issues spam_check(issue, current_user) end - def handle_changes(issue, old_labels: [], old_mentioned_users: []) - if has_changes?(issue, old_labels: old_labels) + def handle_changes(issue, options) + old_labels = options[:old_labels] || [] + old_mentioned_users = options[:old_mentioned_users] || [] + old_assignees = options[:old_assignees] || [] + + if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees) todo_service.mark_pending_todos_as_done(issue, current_user) end @@ -26,9 +30,9 @@ module Issues create_milestone_note(issue) end - if issue.previous_changes.include?('assignee_id') - create_assignee_note(issue) - notification_service.reassigned_issue(issue, current_user) + if issue.assignees != old_assignees + create_assignee_note(issue, old_assignees) + 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..8c6c4841020 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.present? && 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 582d5c47b66..3542a41ac83 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, :reopened]) MergeRequest diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index ab7fcf3b6e2..5c843a258fb 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -21,7 +21,10 @@ module MergeRequests update(merge_request) end - def handle_changes(merge_request, old_labels: [], old_mentioned_users: []) + def handle_changes(merge_request, options) + old_labels = options[:old_labels] || [] + old_mentioned_users = options[:old_mentioned_users] || [] + if has_changes?(merge_request, old_labels: old_labels) todo_service.mark_pending_todos_as_done(merge_request, current_user) end diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 8bb995158de..988bd0a7cdb 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -19,9 +19,14 @@ 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 + previous_assignees = Array(previous_assignee) + recipients.concat(previous_assignees) + 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 6b186263bd1..fe9f5ae2b33 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: @@ -367,10 +382,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/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb index 6aeebc26685..dc2ee695384 100644 --- a/app/services/slash_commands/interpret_service.rb +++ b/app/services/slash_commands/interpret_service.rb @@ -88,20 +88,33 @@ module SlashCommands current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :assign do |assignee_param| - user = extract_references(assignee_param, :user).first - user ||= User.find_by(username: assignee_param) + user_ids = extract_references(assignee_param, :user).map(&:id) - @updates[:assignee_id] = user.id if user + if user_ids.empty? + user_ids = User.where(username: assignee_param.split(' ').map(&:strip)).pluck(:id) + end + + next if user_ids.empty? + + if issuable.is_a?(Issue) + @updates[:assignee_ids] = user_ids + else + @updates[:assignee_id] = user_ids.last + end end desc 'Remove assignee' condition do issuable.persisted? && - issuable.assignee_id? && + issuable.assignees.any? && current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :unassign do - @updates[:assignee_id] = nil + if issuable.is_a?(Issue) + @updates[:assignee_ids] = [] + else + @updates[:assignee_id] = nil + end end desc 'Set milestone' diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index c9e25c7aaa2..898c69b3f8c 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -49,6 +49,44 @@ module SystemNoteService create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee')) 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 - Users 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, old_assignees) + body = + if issue.assignees.any? && old_assignees.any? + unassigned_users = old_assignees - issue.assignees + added_users = issue.assignees.to_a - old_assignees + + text_parts = [] + text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any? + text_parts << "unassigned #{unassigned_users.map(&:to_reference).to_sentence}" if unassigned_users.any? + + text_parts.join(' and ') + elsif old_assignees.any? + "removed all assignees" + elsif issue.assignees.any? + "assigned to #{issue.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 8ae61694b50..322c6286365 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -251,9 +251,9 @@ 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 |