diff options
| author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-04-07 15:35:16 -0300 |
|---|---|---|
| committer | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-04-08 18:40:00 -0300 |
| commit | ca884980ee8e6fe1269f5abdb803519d51aa09c0 (patch) | |
| tree | 517a448ce25452f26acb5e62384778a99da2f699 | |
| parent | 225edb0d2d7737cf52ef5cd358082d08e20feaa4 (diff) | |
| download | gitlab-ce-osw-multi-assignees-merge-requests.tar.gz | |
[CE] Support multiple assignees for merge requestsosw-multi-assignees-merge-requests
Backports https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10161
(code out of ee/ folder).
126 files changed, 1181 insertions, 809 deletions
diff --git a/app/assets/javascripts/issuable_bulk_update_actions.js b/app/assets/javascripts/issuable_bulk_update_actions.js index b844e4c5e5b..ccbe591a63e 100644 --- a/app/assets/javascripts/issuable_bulk_update_actions.js +++ b/app/assets/javascripts/issuable_bulk_update_actions.js @@ -81,9 +81,6 @@ export default { const formData = { update: { state_event: this.form.find('input[name="update[state_event]"]').val(), - // For Merge Requests - assignee_id: this.form.find('input[name="update[assignee_id]"]').val(), - // For Issues assignee_ids: [this.form.find('input[name="update[assignee_ids][]"]').val()], milestone_id: this.form.find('input[name="update[milestone_id]"]').val(), issuable_ids: this.form.find('input[name="update[issuable_ids]"]').val(), diff --git a/app/assets/javascripts/sidebar/components/assignees/assignees.vue b/app/assets/javascripts/sidebar/components/assignees/assignees.vue index d1a396182b3..ce378e24289 100644 --- a/app/assets/javascripts/sidebar/components/assignees/assignees.vue +++ b/app/assets/javascripts/sidebar/components/assignees/assignees.vue @@ -74,8 +74,7 @@ export default { } if (!this.users.length) { - const emptyTooltipLabel = - this.issuableType === 'issue' ? __('Assignee(s)') : __('Assignee'); + const emptyTooltipLabel = __('Assignee(s)'); names.push(emptyTooltipLabel); } @@ -90,6 +89,27 @@ export default { return counter; }, + mergeNotAllowedTooltipMessage() { + const assigneesCount = this.users.length; + + if (this.issuableType !== 'merge_request' || assigneesCount === 0) { + return null; + } + + const cannotMergeCount = this.users.filter(u => u.can_merge === false).length; + const canMergeCount = assigneesCount - cannotMergeCount; + + if (canMergeCount === assigneesCount) { + // Everyone can merge + return null; + } else if (cannotMergeCount === assigneesCount && assigneesCount > 1) { + return 'No one can merge'; + } else if (assigneesCount === 1) { + return 'Cannot merge'; + } + + return `${canMergeCount}/${assigneesCount} can merge`; + }, }, methods: { assignSelf() { @@ -154,6 +174,15 @@ export default { </button> </div> <div class="value hide-collapsed"> + <span + v-if="mergeNotAllowedTooltipMessage" + v-tooltip + :title="mergeNotAllowedTooltipMessage" + data-placement="left" + class="float-right cannot-be-merged" + > + <i aria-hidden="true" data-hidden="true" class="fa fa-exclamation-triangle"></i> + </span> <template v-if="hasNoUsers"> <span class="assign-yourself no-value"> No assignee diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 86b58c1b1b2..709940ba6c8 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -498,6 +498,16 @@ flex: 1; } + .issuable-meta { + .author-link { + display: inline-block; + } + + .issuable-comments { + height: 18px; + } + } + .merge-request-title { margin-bottom: 2px; diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 792c618fd40..6594f20456d 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -1158,6 +1158,8 @@ pre.light-well { .cannot-be-merged:hover { color: $red-500; margin-top: 2px; + position: relative; + z-index: 2; } .private-forks-notice .private-fork-icon { diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 85aeecbf90b..065d2d3a4ec 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -192,12 +192,7 @@ module IssuableActions def bulk_update_params permitted_keys_array = permitted_keys.dup - - if resource_name == 'issue' - permitted_keys_array << { assignee_ids: [] } - else - permitted_keys_array.unshift(:assignee_id) - end + permitted_keys_array << { assignee_ids: [] } params.require(:update).permit(permitted_keys_array) end diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 6d6e0cc6c7f..91e875dca54 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -190,15 +190,15 @@ module IssuableCollections end end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def preload_for_collection + common_attributes = [:author, :assignees, :labels, :milestone] @preload_for_collection ||= case collection_type when 'Issue' - [:project, :author, :assignees, :labels, :milestone, project: :namespace] + common_attributes + [:project, project: :namespace] when 'MergeRequest' - [ - :target_project, :author, :assignee, :labels, :milestone, - source_project: :route, head_pipeline: :project, target_project: :namespace, latest_merge_request_diff: :merge_request_diff_commits - ] + common_attributes + [:target_project, source_project: :route, head_pipeline: :project, target_project: :namespace, latest_merge_request_diff: :merge_request_diff_commits] end end + # rubocop:enable Gitlab/ModuleWithInstanceVariables end diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 6045ee4e171..eb469d2d714 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -20,7 +20,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont def merge_request_params_attributes [ :allow_collaboration, - :assignee_id, :description, :force_remove_source_branch, :lock_version, @@ -35,6 +34,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont :title, :discussion_locked, label_ids: [], + assignee_ids: [], update_task: [:index, :checked, :line_number, :line_source] ] end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 64c88505a16..88ec77426d5 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -439,22 +439,6 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord - def by_assignee(items) - if filter_by_no_assignee? - items.where(assignee_id: nil) - elsif filter_by_any_assignee? - items.where('assignee_id IS NOT NULL') - elsif assignee - items.where(assignee_id: assignee.id) - elsif assignee_id? || assignee_username? # assignee not found - items.none - else - items - end - end - # rubocop: enable CodeReuse/ActiveRecord - def filter_by_no_assignee? # Assignee_id takes precedence over assignee_username [NONE, FILTER_NONE].include?(params[:assignee_id].to_s.downcase) || params[:assignee_username].to_s == NONE @@ -478,6 +462,20 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord + def by_assignee(items) + if filter_by_no_assignee? + items.unassigned + elsif filter_by_any_assignee? + items.assigned + elsif assignee + items.assigned_to(assignee) + elsif assignee_id? || assignee_username? # assignee not found + items.none + else + items + end + end + # rubocop: disable CodeReuse/ActiveRecord def by_milestone(items) if milestones? diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index cb44575d6f1..e6a82f55856 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -144,18 +144,4 @@ class IssuesFinder < IssuableFinder current_user.blank? end - - def by_assignee(items) - if filter_by_no_assignee? - items.unassigned - elsif filter_by_any_assignee? - items.assigned - elsif assignee - items.assigned_to(assignee) - elsif assignee_id? || assignee_username? # assignee not found - items.none - else - items - end - end end diff --git a/app/helpers/boards_helper.rb b/app/helpers/boards_helper.rb index be1e7016a1e..1640f4fc93f 100644 --- a/app/helpers/boards_helper.rb +++ b/app/helpers/boards_helper.rb @@ -69,7 +69,7 @@ module BoardsHelper end def board_sidebar_user_data - dropdown_options = issue_assignees_dropdown_options + dropdown_options = assignees_dropdown_options('issue') { toggle: 'dropdown', diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 8b3d270e873..f7c7f37cc38 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -17,8 +17,8 @@ module FormHelper end end - def issue_assignees_dropdown_options - { + def assignees_dropdown_options(issuable_type) + dropdown_data = { toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', title: 'Select assignee', filter: true, @@ -28,8 +28,8 @@ module FormHelper first_user: current_user&.username, null_user: true, current_user: true, - project_id: @project&.id, - field_name: 'issue[assignee_ids][]', + project_id: (@target_project || @project)&.id, + field_name: "#{issuable_type}[assignee_ids][]", default_label: 'Unassigned', 'max-select': 1, 'dropdown-header': 'Assignee', @@ -39,5 +39,36 @@ module FormHelper current_user_info: UserSerializer.new.represent(current_user) } } + + type = issuable_type.to_s + + if type == 'issue' && issue_supports_multiple_assignees? || + type == 'merge_request' && merge_request_supports_multiple_assignees? + dropdown_data = multiple_assignees_dropdown_options(dropdown_data) + end + + dropdown_data + end + + # Overwritten + def issue_supports_multiple_assignees? + false + end + + # Overwritten + def merge_request_supports_multiple_assignees? + false + end + + private + + def multiple_assignees_dropdown_options(options) + new_options = options.dup + + new_options[:title] = 'Select assignee(s)' + new_options[:data][:'dropdown-header'] = 'Assignee(s)' + new_options[:data].delete(:'max-select') + + new_options end end diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 52c49498e9b..9a12db258d5 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -15,11 +15,14 @@ module IssuablesHelper sidebar_gutter_collapsed? ? _('Expand sidebar') : _('Collapse sidebar') end - def sidebar_assignee_tooltip_label(issuable) - if issuable.assignee - issuable.assignee.name + def assignees_label(issuable, include_value: true) + label = 'Assignee'.pluralize(issuable.assignees.count) + + if include_value + sanitized_list = sanitize_name(issuable.assignee_list) + "#{label}: #{sanitized_list}" else - issuable.allows_multiple_assignees? ? _('Assignee(s)') : _('Assignee') + label end end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 9ba8f92fcbf..63148831a24 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -24,10 +24,12 @@ module Emails end # rubocop: disable CodeReuse/ActiveRecord - def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id, reason = nil) + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_ids, updated_by_user_id, reason = nil) setup_merge_request_mail(merge_request_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(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason)) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index efa1233b434..0b740809f30 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -4,6 +4,7 @@ class Notify < BaseMailer include ActionDispatch::Routing::PolymorphicRoutes include GitlabRoutingHelper include EmailsHelper + include IssuablesHelper include Emails::Issues include Emails::MergeRequests @@ -24,6 +25,7 @@ class Notify < BaseMailer helper MembersHelper helper AvatarsHelper helper GitlabRoutingHelper + helper IssuablesHelper def test_email(recipient_email, subject, body) mail(to: recipient_email, diff --git a/app/models/concerns/deprecated_assignee.rb b/app/models/concerns/deprecated_assignee.rb new file mode 100644 index 00000000000..7f12ce39c96 --- /dev/null +++ b/app/models/concerns/deprecated_assignee.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +# This module handles backward compatibility for import/export of Merge Requests after +# multiple assignees feature was introduced. Also, it handles the scenarios where +# the #26496 background migration hasn't finished yet. +# Ideally, most of this code should be removed at #59457. +module DeprecatedAssignee + extend ActiveSupport::Concern + + def assignee_ids=(ids) + nullify_deprecated_assignee + super + end + + def assignees=(users) + nullify_deprecated_assignee + super + end + + def assignee_id=(id) + self.assignee_ids = Array(id) + end + + def assignee=(user) + self.assignees = Array(user) + end + + def assignee + assignees.first + end + + def assignee_id + assignee_ids.first + end + + def assignee_ids + if Gitlab::Database.read_only? && pending_assignees_population? + return Array(deprecated_assignee_id) + end + + update_assignees_relation + super + end + + def assignees + if Gitlab::Database.read_only? && pending_assignees_population? + return User.where(id: deprecated_assignee_id) + end + + update_assignees_relation + super + end + + private + + # This will make the background migration process quicker (#26496) as it'll have less + # assignee_id rows to look through. + def nullify_deprecated_assignee + return unless persisted? && Gitlab::Database.read_only? + + update_column(:assignee_id, nil) + end + + # This code should be removed in the clean-up phase of the + # background migration (#59457). + def pending_assignees_population? + persisted? && deprecated_assignee_id && merge_request_assignees.empty? + end + + # If there's an assignee_id and no relation, it means the background + # migration at #26496 didn't reach this merge request yet. + # This code should be removed in the clean-up phase of the + # background migration (#59457). + def update_assignees_relation + if pending_assignees_population? + transaction do + merge_request_assignees.create!(user_id: deprecated_assignee_id, merge_request_id: id) + update_column(:assignee_id, nil) + end + end + end + + def deprecated_assignee_id + read_attribute(:assignee_id) + end +end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 17f94b4bd9b..3232c51bfbd 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -67,13 +67,6 @@ module Issuable allow_nil: true, prefix: true - delegate :name, - :email, - :public_email, - to: :assignee, - allow_nil: true, - prefix: true - validates :author, presence: true validates :title, presence: true, length: { maximum: 255 } validate :milestone_is_valid @@ -88,6 +81,19 @@ module Issuable scope :only_opened, -> { with_state(:opened) } scope :closed, -> { with_state(:closed) } + # rubocop:disable GitlabSecurity/SqlInjection + # The `to_ability_name` method is not an user input. + scope :assigned, -> do + where("EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)") + end + scope :unassigned, -> do + where("NOT EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)") + end + scope :assigned_to, ->(u) do + where("EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE user_id = ? AND #{to_ability_name}_id = #{to_ability_name}s.id)", u.id) + end + # rubocop:enable GitlabSecurity/SqlInjection + scope :left_joins_milestones, -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") } scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date DESC') } scope :order_milestone_due_asc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date ASC') } @@ -104,6 +110,7 @@ module Issuable participant :author participant :notes_with_associations + participant :assignees strip_attributes :title @@ -270,6 +277,10 @@ module Issuable end end + def assignee_or_author?(user) + author_id == user.id || assignees.exists?(user.id) + end + def today? Date.today == created_at.to_date end @@ -314,11 +325,7 @@ module Issuable end if old_assignees != assignees - if self.is_a?(Issue) - changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)] - else - changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs] - end + changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)] end if self.respond_to?(:total_time_spent) @@ -355,10 +362,18 @@ module Issuable def card_attributes { 'Author' => author.try(:name), - 'Assignee' => assignee.try(:name) + 'Assignee' => assignee_list } end + def assignee_list + assignees.map(&:name).to_sentence + end + + def assignee_username_list + assignees.map(&:username).to_sentence + end + def notes_with_associations # If A has_many Bs, and B has_many Cs, and you do # `A.includes(b: :c).each { |a| a.b.includes(:c) }`, sadly ActiveRecord diff --git a/app/models/issue.rb b/app/models/issue.rb index 261935fd054..5ecd2019222 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -49,10 +49,6 @@ class Issue < ApplicationRecord scope :in_projects, ->(project_ids) { where(project_id: project_ids) } - scope :assigned, -> { where('EXISTS (SELECT TRUE FROM issue_assignees WHERE issue_id = issues.id)') } - scope :unassigned, -> { where('NOT EXISTS (SELECT TRUE FROM issue_assignees WHERE issue_id = issues.id)') } - scope :assigned_to, ->(u) { where('EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = ? AND issue_id = issues.id)', u.id)} - scope :with_due_date, -> { where.not(due_date: nil) } scope :without_due_date, -> { where(due_date: nil) } scope :due_before, ->(date) { where('issues.due_date < ?', date) } @@ -75,8 +71,6 @@ class Issue < ApplicationRecord attr_spammable :title, spam_title: true attr_spammable :description, spam_description: true - participant :assignees - state_machine :state, initial: :opened do event :close do transition [:opened] => :closed @@ -155,22 +149,6 @@ class Issue < ApplicationRecord Gitlab::HookData::IssueBuilder.new(self).build end - # Returns a Hash of attributes to be used for Twitter card metadata - def card_attributes - { - 'Author' => author.try(:name), - 'Assignee' => assignee_list - } - end - - def assignee_or_author?(user) - author_id == user.id || assignees.exists?(user.id) - end - - def assignee_list - assignees.map(&:name).to_sentence - end - # `from` argument can be a Namespace or Project. def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{iid}" diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 458c57c1dc6..0a39a720766 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -16,6 +16,7 @@ class MergeRequest < ApplicationRecord include LabelEventable include ReactiveCaching include FromUnion + include DeprecatedAssignee self.reactive_cache_key = ->(model) { [model.project.id, model.iid] } self.reactive_cache_refresh_interval = 10.minutes @@ -69,8 +70,7 @@ class MergeRequest < ApplicationRecord has_many :suggestions, through: :notes has_many :merge_request_assignees - # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457 - belongs_to :assignee, class_name: "User" + has_many :assignees, class_name: "User", through: :merge_request_assignees serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize @@ -79,10 +79,6 @@ class MergeRequest < ApplicationRecord after_update :reload_diff_if_branch_changed after_save :ensure_metrics - # Required until the codebase starts using this relation for single or multiple assignees. - # TODO: Remove at gitlab-ee#2004 implementation. - after_save :refresh_merge_request_assignees, if: :assignee_id_changed? - # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests attr_accessor :allow_broken @@ -188,19 +184,14 @@ class MergeRequest < ApplicationRecord end scope :join_project, -> { joins(:target_project) } scope :references_project, -> { references(:target_project) } - scope :assigned, -> { where("assignee_id IS NOT NULL") } - scope :unassigned, -> { where("assignee_id IS NULL") } - scope :assigned_to, ->(u) { where(assignee_id: u.id)} scope :with_api_entity_associations, -> { - preload(:author, :assignee, :notes, :labels, :milestone, :timelogs, + preload(:assignees, :author, :notes, :labels, :milestone, :timelogs, latest_merge_request_diff: [:merge_request_diff_commits], metrics: [:latest_closed_by, :merged_by], target_project: [:route, { namespace: :route }], source_project: [:route, { namespace: :route }]) } - participant :assignee - after_save :keep_around_commit alias_attribute :project, :target_project @@ -337,31 +328,6 @@ class MergeRequest < ApplicationRecord Gitlab::HookData::MergeRequestBuilder.new(self).build end - # Returns a Hash of attributes to be used for Twitter card metadata - def card_attributes - { - 'Author' => author.try(:name), - 'Assignee' => assignee.try(:name) - } - end - - # These method are needed for compatibility with issues to not mess view and other code - def assignees - Array(assignee) - end - - def assignee_ids - Array(assignee_id) - end - - def assignee_ids=(ids) - write_attribute(:assignee_id, ids.last) - end - - def assignee_or_author?(user) - author_id == user.id || assignee_id == user.id - end - # `from` argument can be a Namespace or Project. def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{iid}" @@ -682,15 +648,6 @@ class MergeRequest < ApplicationRecord merge_request_diff || create_merge_request_diff end - def refresh_merge_request_assignees - transaction do - # Using it instead relation.delete_all in order to avoid adding a - # dependent: :delete_all (we already have foreign key cascade deletion). - MergeRequestAssignee.where(merge_request_id: self).delete_all - merge_request_assignees.create(user_id: assignee_id) if assignee_id - end - end - def create_merge_request_diff fetch_ref! @@ -1208,7 +1165,7 @@ class MergeRequest < ApplicationRecord variables.append(key: 'CI_MERGE_REQUEST_PROJECT_URL', value: project.web_url) variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_NAME', value: target_branch.to_s) variables.append(key: 'CI_MERGE_REQUEST_TITLE', value: title) - variables.append(key: 'CI_MERGE_REQUEST_ASSIGNEES', value: assignee.username) if assignee + variables.append(key: 'CI_MERGE_REQUEST_ASSIGNEES', value: assignee_username_list) if assignees.any? variables.append(key: 'CI_MERGE_REQUEST_MILESTONE', value: milestone.title) if milestone variables.append(key: 'CI_MERGE_REQUEST_LABELS', value: label_names.join(',')) if labels.present? variables.concat(source_project_variables) diff --git a/app/models/project.rb b/app/models/project.rb index e2869fc2ad5..1493dc95fa2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -674,6 +674,10 @@ class Project < ApplicationRecord { scope: :project, status: auto_devops&.enabled || Feature.enabled?(:force_autodevops_on_by_default, self) } end + def multiple_mr_assignees_enabled? + Feature.enabled?(:multiple_merge_request_assignees, self) + end + def daily_statistics_enabled? Feature.enabled?(:project_daily_statistics, self, default_enabled: true) end diff --git a/app/serializers/issuable_sidebar_extras_entity.rb b/app/serializers/issuable_sidebar_extras_entity.rb index d60253564e1..fb35b7522c5 100644 --- a/app/serializers/issuable_sidebar_extras_entity.rb +++ b/app/serializers/issuable_sidebar_extras_entity.rb @@ -11,4 +11,6 @@ class IssuableSidebarExtrasEntity < Grape::Entity expose :subscribed do |issuable| issuable.subscribed?(request.current_user, issuable.project) end + + expose :assignees, using: API::Entities::UserBasic end diff --git a/app/serializers/issue_sidebar_extras_entity.rb b/app/serializers/issue_sidebar_extras_entity.rb index 7b6e860140b..dee891a50b7 100644 --- a/app/serializers/issue_sidebar_extras_entity.rb +++ b/app/serializers/issue_sidebar_extras_entity.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true class IssueSidebarExtrasEntity < IssuableSidebarExtrasEntity - expose :assignees, using: API::Entities::UserBasic end diff --git a/app/serializers/merge_request_assignee_entity.rb b/app/serializers/merge_request_assignee_entity.rb new file mode 100644 index 00000000000..6849c62e759 --- /dev/null +++ b/app/serializers/merge_request_assignee_entity.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class MergeRequestAssigneeEntity < ::API::Entities::UserBasic + expose :can_merge do |assignee, options| + options[:merge_request]&.can_be_merged_by?(assignee) + end +end diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 178e72f4f0a..973e971b4c0 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/app/serializers/merge_request_basic_entity.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class MergeRequestBasicEntity < Grape::Entity - expose :assignee_id expose :merge_status expose :merge_error expose :state @@ -9,7 +8,7 @@ class MergeRequestBasicEntity < Grape::Entity expose :rebase_in_progress?, as: :rebase_in_progress expose :milestone, using: API::Entities::Milestone expose :labels, using: LabelEntity - expose :assignee, using: API::Entities::UserBasic + expose :assignees, using: API::Entities::UserBasic expose :task_status, :task_status_short expose :lock_version, :lock_version end diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb index 4cf84336aa4..6f589351670 100644 --- a/app/serializers/merge_request_serializer.rb +++ b/app/serializers/merge_request_serializer.rb @@ -8,9 +8,9 @@ class MergeRequestSerializer < BaseSerializer entity = case opts[:serializer] when 'sidebar' - MergeRequestSidebarBasicEntity + IssuableSidebarBasicEntity when 'sidebar_extras' - IssuableSidebarExtrasEntity + MergeRequestSidebarExtrasEntity when 'basic' MergeRequestBasicEntity else diff --git a/app/serializers/merge_request_sidebar_basic_entity.rb b/app/serializers/merge_request_sidebar_basic_entity.rb deleted file mode 100644 index 0ae7298a7c1..00000000000 --- a/app/serializers/merge_request_sidebar_basic_entity.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -class MergeRequestSidebarBasicEntity < IssuableSidebarBasicEntity - expose :assignee, if: lambda { |issuable| issuable.assignee } do - expose :assignee, merge: true, using: API::Entities::UserBasic - - expose :can_merge do |issuable| - issuable.can_be_merged_by?(issuable.assignee) - end - end -end diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb new file mode 100644 index 00000000000..7276509c363 --- /dev/null +++ b/app/serializers/merge_request_sidebar_extras_entity.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity + expose :assignees do |merge_request| + MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request) + end +end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 7a4ccf0d178..26132f1824a 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -34,14 +34,20 @@ class IssuableBaseService < BaseService end def filter_assignee(issuable) - return unless params[:assignee_id].present? + return if params[:assignee_ids].blank? - assignee_id = params[:assignee_id] + unless issuable.allows_multiple_assignees? + params[:assignee_ids] = params[:assignee_ids].first(1) + end + + assignee_ids = params[:assignee_ids].select { |assignee_id| assignee_can_read?(issuable, assignee_id) } - if assignee_id.to_s == IssuableFinder::NONE - params[: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_id) unless assignee_can_read?(issuable, assignee_id) + params.delete(:assignee_ids) end end @@ -352,7 +358,7 @@ class IssuableBaseService < BaseService end def has_changes?(issuable, old_labels: [], old_assignees: []) - valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] + valid_attrs = [:title, :description, :assignee_ids, :milestone_id, :target_branch] attrs_changed = valid_attrs.any? do |attr| issuable.previous_changes.include?(attr.to_s) diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index ef08adf4f92..48ed5afbc2a 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -20,7 +20,7 @@ module Issues private def create_assignee_note(issue, old_assignees) - SystemNoteService.change_issue_assignees( + SystemNoteService.change_issuable_assignees( issue, issue.project, current_user, old_assignees) end @@ -31,26 +31,6 @@ module Issues issue.project.execute_services(issue_data, hooks_scope) end - # rubocop: disable CodeReuse/ActiveRecord - def filter_assignee(issuable) - return if params[:assignee_ids].blank? - - unless issuable.allows_multiple_assignees? - params[:assignee_ids] = params[:assignee_ids].take(1) - end - - 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 - # rubocop: enable CodeReuse/ActiveRecord - def update_project_counter_caches?(issue) super || issue.confidential_changed? end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index cec5b5734c0..cb2337d29d4 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -39,7 +39,7 @@ module Issues if issue.assignees != old_assignees create_assignee_note(issue, old_assignees) notification_service.async.reassigned_issue(issue, current_user, old_assignees) - todo_service.reassigned_issue(issue, current_user, old_assignees) + todo_service.reassigned_issuable(issue, current_user, old_assignees) end if issue.previous_changes.include?('confidential') diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 8a9e5ebb014..b8334a87f6d 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -49,9 +49,9 @@ module MergeRequests MergeRequestMetricsService.new(merge_request.metrics) end - def create_assignee_note(merge_request) - SystemNoteService.change_assignee( - merge_request, merge_request.project, current_user, merge_request.assignee) + def create_assignee_note(merge_request, old_assignees) + SystemNoteService.change_issuable_assignees( + merge_request, merge_request.project, current_user, old_assignees) end def create_pipeline_for(merge_request, user) diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 8112c2a4299..faaa4d66726 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -24,13 +24,13 @@ module MergeRequests update_task_event(merge_request) || update(merge_request) end - # rubocop:disable Metrics/AbcSize def handle_changes(merge_request, options) old_associations = options.fetch(:old_associations, {}) old_labels = old_associations.fetch(:labels, []) old_mentioned_users = old_associations.fetch(:mentioned_users, []) + old_assignees = old_associations.fetch(:assignees, []) - if has_changes?(merge_request, old_labels: old_labels) + if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees) todo_service.mark_pending_todos_as_done(merge_request, current_user) end @@ -45,15 +45,10 @@ module MergeRequests merge_request.target_branch) end - if merge_request.previous_changes.include?('assignee_id') - reassigned_merge_request_args = [merge_request, current_user] - - old_assignee_id = merge_request.previous_changes['assignee_id'].first - reassigned_merge_request_args << User.find(old_assignee_id) if old_assignee_id - - create_assignee_note(merge_request) - notification_service.async.reassigned_merge_request(*reassigned_merge_request_args) - todo_service.reassigned_merge_request(merge_request, current_user) + if merge_request.assignees != old_assignees + create_assignee_note(merge_request, old_assignees) + notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees) + todo_service.reassigned_issuable(merge_request, current_user, old_assignees) end if merge_request.previous_changes.include?('target_branch') || @@ -81,7 +76,6 @@ module MergeRequests ) end end - # rubocop:enable Metrics/AbcSize def handle_task_changes(merge_request) todo_service.mark_pending_todos_as_done(merge_request, current_user) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 56f11b31110..760962346fb 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -247,15 +247,15 @@ module NotificationRecipientService attr_reader :target attr_reader :current_user attr_reader :action - attr_reader :previous_assignee + attr_reader :previous_assignees attr_reader :skip_current_user - def initialize(target, current_user, action:, custom_action: nil, previous_assignee: nil, skip_current_user: true) + def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true) @target = target @current_user = current_user @action = action @custom_action = custom_action - @previous_assignee = previous_assignee + @previous_assignees = previous_assignees @skip_current_user = skip_current_user end @@ -270,11 +270,7 @@ module NotificationRecipientService # Re-assign is considered as a mention of the new assignee case custom_action - when :reassign_merge_request - add_recipients(previous_assignee, :mention, nil) - add_recipients(target.assignee, :mention, NotificationReason::ASSIGNED) - when :reassign_issue - previous_assignees = Array(previous_assignee) + when :reassign_merge_request, :reassign_issue add_recipients(previous_assignees, :mention, nil) add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED) end @@ -287,17 +283,11 @@ module NotificationRecipientService # receive them, too. add_mentions(current_user, target: target) - # Add the assigned users, if any - assignees = case custom_action - when :new_issue - target.assignees - else - target.assignee - end - # We use the `:participating` notification level in order to match existing legacy behavior as captured # in existing specs (notification_service_spec.rb ~ line 507) - add_recipients(assignees, :participating, NotificationReason::ASSIGNED) if assignees + if target.is_a?(Issuable) + add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED) + end add_labels_subscribers end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 1a65561dd70..8d3b569498f 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -95,8 +95,8 @@ class NotificationService # When we reassign an issue we should send an email to: # - # * issue old assignee if their notification level is not Disabled - # * issue new assignee if their notification level is not Disabled + # * issue old assignees if their notification level is not Disabled + # * issue new assignees if their notification level is not Disabled # * users with custom level checked with "reassign issue" # def reassigned_issue(issue, current_user, previous_assignees = []) @@ -104,7 +104,7 @@ class NotificationService issue, current_user, action: "reassign", - previous_assignee: previous_assignees + previous_assignees: previous_assignees ) previous_assignee_ids = previous_assignees.map(&:id) @@ -140,7 +140,7 @@ class NotificationService # When create a merge request we should send an email to: # # * mr author - # * mr assignee if their notification level is not Disabled + # * mr assignees if their notification level is not Disabled # * project team members with notification level higher then Participating # * watchers of the mr's labels # * users with custom level checked with "new merge request" @@ -184,23 +184,25 @@ class NotificationService # When we reassign a merge_request we should send an email to: # - # * merge_request old assignee if their notification level is not Disabled - # * merge_request assignee if their notification level is not Disabled + # * merge_request old assignees if their notification level is not Disabled + # * merge_request new assignees if their notification level is not Disabled # * users with custom level checked with "reassign merge request" # - def reassigned_merge_request(merge_request, current_user, previous_assignee = nil) + def reassigned_merge_request(merge_request, current_user, previous_assignees = []) recipients = NotificationRecipientService.build_recipients( merge_request, current_user, action: "reassign", - previous_assignee: previous_assignee + previous_assignees: previous_assignees ) + previous_assignee_ids = previous_assignees.map(&:id) + recipients.each do |recipient| mailer.reassigned_merge_request_email( recipient.user.id, merge_request.id, - previous_assignee&.id, + previous_assignee_ids, current_user.id, recipient.reason ).deliver_later diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index acbbb0da929..a39ff76b798 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -69,7 +69,7 @@ module SystemNoteService # Called when the assignees of an Issue is changed or removed # - # issue - Issue object + # issuable - Issuable object (responds to assignees) # project - Project owning noteable # author - User performing the change # assignees - Users being assigned, or nil @@ -85,9 +85,9 @@ module SystemNoteService # "assigned to @user1 and @user2" # # Returns the created Note object - def change_issue_assignees(issue, project, author, old_assignees) - unassigned_users = old_assignees - issue.assignees - added_users = issue.assignees.to_a - old_assignees + def change_issuable_assignees(issuable, project, author, old_assignees) + unassigned_users = old_assignees - issuable.assignees + added_users = issuable.assignees.to_a - old_assignees text_parts = [] text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any? @@ -95,7 +95,7 @@ module SystemNoteService body = text_parts.join(' and ') - create_note(NoteSummary.new(issue, project, author, body, action: 'assignee')) + create_note(NoteSummary.new(issuable, project, author, body, action: 'assignee')) end # Called when the milestone of a Noteable is changed diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f357dc37fe7..0ea230a44a1 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -49,12 +49,12 @@ class TodoService todo_users.each(&:update_todos_count_cache) end - # When we reassign an issue we should: + # When we reassign an issuable we should: # - # * create a pending todo for new assignee if issue is assigned + # * create a pending todo for new assignee if issuable is assigned # - def reassigned_issue(issue, current_user, old_assignees = []) - create_assignment_todo(issue, current_user, old_assignees) + def reassigned_issuable(issuable, current_user, old_assignees = []) + create_assignment_todo(issuable, current_user, old_assignees) end # When create a merge request we should: @@ -82,14 +82,6 @@ class TodoService mark_pending_todos_as_done(merge_request, current_user) end - # When we reassign a merge request we should: - # - # * creates a pending todo for new assignee if merge request is assigned - # - def reassigned_merge_request(merge_request, current_user) - create_assignment_todo(merge_request, current_user) - end - # When merge a merge request we should: # # * mark all pending todos related to the target for the current user as done diff --git a/app/views/notify/_reassigned_issuable_email.html.haml b/app/views/notify/_reassigned_issuable_email.html.haml new file mode 100644 index 00000000000..4ab40ff2659 --- /dev/null +++ b/app/views/notify/_reassigned_issuable_email.html.haml @@ -0,0 +1,10 @@ +%p + Assignee changed + - if previous_assignees.any? + from + %strong= sanitize_name(previous_assignees.map(&:name).to_sentence) + to + - if issuable.assignees.any? + %strong= sanitize_name(issuable.assignee_list) + - else + %strong Unassigned diff --git a/app/views/notify/closed_merge_request_email.text.haml b/app/views/notify/closed_merge_request_email.text.haml index 1094d584a1c..6e84f9fb355 100644 --- a/app/views/notify/closed_merge_request_email.text.haml +++ b/app/views/notify/closed_merge_request_email.text.haml @@ -5,4 +5,4 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') Author: #{sanitize_name(@merge_request.author_name)} -Assignee: #{sanitize_name(@merge_request.assignee_name)} += assignees_label(@merge_request) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml index e81144b8fcb..08bc98ca05c 100644 --- a/app/views/notify/issue_due_email.html.haml +++ b/app/views/notify/issue_due_email.html.haml @@ -3,7 +3,7 @@ - if @issue.assignees.any? %p - Assignee: #{@issue.assignee_list} + = assignees_label(@issue) %p This issue is due on: #{@issue.due_date.to_s(:medium)} diff --git a/app/views/notify/issue_due_email.text.erb b/app/views/notify/issue_due_email.text.erb index 3c7a57a8a2e..ae50b703fe3 100644 --- a/app/views/notify/issue_due_email.text.erb +++ b/app/views/notify/issue_due_email.text.erb @@ -2,6 +2,6 @@ The following issue is due on <%= @issue.due_date %>: Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> Author: <%= @issue.author_name %> -Assignee: <%= @issue.assignee_list %> +<%= assignees_label(@issue) %> <%= @issue.description %> diff --git a/app/views/notify/merge_request_status_email.text.haml b/app/views/notify/merge_request_status_email.text.haml index b9b9e0c3ad7..e3b24bbd405 100644 --- a/app/views/notify/merge_request_status_email.text.haml +++ b/app/views/notify/merge_request_status_email.text.haml @@ -5,4 +5,4 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') Author: #{sanitize_name(@merge_request.author_name)} -Assignee: #{sanitize_name(@merge_request.assignee_name)} += assignees_label(@merge_request) diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml index 0c7bf1bb044..e9708a297d7 100644 --- a/app/views/notify/merge_request_unmergeable_email.text.haml +++ b/app/views/notify/merge_request_unmergeable_email.text.haml @@ -5,4 +5,4 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') Author: #{sanitize_name(@merge_request.author_name)} -Assignee: #{sanitize_name(@merge_request.assignee_name)} += assignees_label(@merge_request) diff --git a/app/views/notify/merged_merge_request_email.text.haml b/app/views/notify/merged_merge_request_email.text.haml index 045a43cbc84..d623e701a30 100644 --- a/app/views/notify/merged_merge_request_email.text.haml +++ b/app/views/notify/merged_merge_request_email.text.haml @@ -5,4 +5,4 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') Author: #{sanitize_name(@merge_request.author_name)} -Assignee: #{sanitize_name(@merge_request.assignee_name)} += assignees_label(@merge_request) diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index e6cdaf85c0d..8aa7939dd0b 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -4,7 +4,7 @@ - if @issue.assignees.any? %p - Assignee: #{@issue.assignee_list} + = assignees_label(@issue) - if @issue.description %div diff --git a/app/views/notify/new_issue_email.text.erb b/app/views/notify/new_issue_email.text.erb index 58a2bcbe5eb..ff258711b48 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(project_issue_url(@issue.project, @issue)) %> Author: <%= sanitize_name(@issue.author_name) %> -Assignee: <%= @issue.assignee_list %> +<%= assignees_label(@issue) %> <%= @issue.description %> 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 173091e4a80..8e95063b40f 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(project_issue_url(@issue.project, @issue)) %> Author: <%= sanitize_name(@issue.author_name) %> -Assignee: <%= sanitize_name(@issue.assignee_list) %> +<%= assignees_label(@issue) %> <%= @issue.description %> diff --git a/app/views/notify/new_mention_in_merge_request_email.text.erb b/app/views/notify/new_mention_in_merge_request_email.text.erb index 96a4f3f9eac..3c78e257a88 100644 --- a/app/views/notify/new_mention_in_merge_request_email.text.erb +++ b/app/views/notify/new_mention_in_merge_request_email.text.erb @@ -4,6 +4,6 @@ You have been mentioned in Merge Request <%= @merge_request.to_reference %> <%= merge_path_description(@merge_request, 'to') %> Author: <%= sanitize_name(@merge_request.author_name) %> -Assignee: <%= sanitize_name(@merge_request.assignee_name) %> += assignees_label(@merge_request) <%= @merge_request.description %> diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index db23447dd39..77d2e65d285 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -5,9 +5,9 @@ %p.details != merge_path_description(@merge_request, '→') -- if @merge_request.assignee_id.present? +- if @merge_request.assignees.any? %p - Assignee: #{sanitize_name(@merge_request.assignee_name)} + = assignees_label(@merge_request) = render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb index 754f4bca1cd..e6c42f1cf5f 100644 --- a/app/views/notify/new_merge_request_email.text.erb +++ b/app/views/notify/new_merge_request_email.text.erb @@ -4,7 +4,7 @@ New Merge Request <%= @merge_request.to_reference %> <%= merge_path_description(@merge_request, 'to') %> Author: <%= @merge_request.author_name %> -Assignee: <%= @merge_request.assignee_name %> +<%= assignees_label(@merge_request) %> <%= render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter %> <%= @merge_request.description %> diff --git a/app/views/notify/reassigned_issue_email.html.haml b/app/views/notify/reassigned_issue_email.html.haml index 6d25488a7e2..6b088927623 100644 --- a/app/views/notify/reassigned_issue_email.html.haml +++ b/app/views/notify/reassigned_issue_email.html.haml @@ -1,10 +1 @@ -%p - Assignee changed - - if @previous_assignees.any? - from - %strong= sanitize_name(@previous_assignees.map(&:name).to_sentence) - to - - if @issue.assignees.any? - %strong= @issue.assignee_list - - else - %strong Unassigned += render 'reassigned_issuable_email', issuable: @issue, previous_assignees: @previous_assignees diff --git a/app/views/notify/reassigned_merge_request_email.html.haml b/app/views/notify/reassigned_merge_request_email.html.haml index e4f19bc3200..0aefca6b14a 100644 --- a/app/views/notify/reassigned_merge_request_email.html.haml +++ b/app/views/notify/reassigned_merge_request_email.html.haml @@ -1,10 +1 @@ -%p - Assignee changed - - if @previous_assignee - from - %strong= sanitize_name(@previous_assignee.name) - to - - if @merge_request.assignee_id - %strong= sanitize_name(@merge_request.assignee_name) - - else - %strong Unassigned += render 'reassigned_issuable_email', issuable: @merge_request, previous_assignees: @previous_assignees diff --git a/app/views/notify/reassigned_merge_request_email.text.erb b/app/views/notify/reassigned_merge_request_email.text.erb index 96c770b5219..82ec7aa0fa4 100644 --- a/app/views/notify/reassigned_merge_request_email.text.erb +++ b/app/views/notify/reassigned_merge_request_email.text.erb @@ -2,5 +2,5 @@ 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 #{sanitize_name(@previous_assignee.name)}" if @previous_assignee -%> - to <%= "#{@merge_request.assignee_id ? sanitize_name(@merge_request.assignee_name) : 'Unassigned'}" %> +Assignee changed <%= "from #{sanitize_name(@previous_assignees.map(&:name).to_sentence)}" if @previous_assignees.any? -%> + to <%= "#{@merge_request.assignees.any? ? @merge_request.assignee_list : 'Unassigned'}" %> diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml index ce7c7091c93..377b2a6d8d9 100644 --- a/app/views/projects/issues/_issue.html.haml +++ b/app/views/projects/issues/_issue.html.haml @@ -46,7 +46,7 @@ CLOSED - if issue.assignees.any? %li - = render 'shared/issuable/assignees', project: @project, issue: issue + = render 'shared/issuable/assignees', project: @project, issuable: issue = render 'shared/issuable_meta_data', issuable: issue diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index b8e0b66e277..47c8e3d73f5 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -53,9 +53,9 @@ %li.issuable-pipeline-broken.d-none.d-sm-inline-block = link_to merge_request_path(merge_request), class: "has-tooltip", title: _('Cannot be merged automatically') do = icon('exclamation-triangle') - - if merge_request.assignee + - if merge_request.assignees.any? %li - = link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: _('Assigned to :name')) + = render 'shared/issuable/assignees', project: merge_request.project, issuable: merge_request = render_if_exists 'projects/merge_requests/approvals_count', merge_request: merge_request = render 'shared/issuable_meta_data', issuable: merge_request diff --git a/app/views/shared/boards/components/sidebar/_assignee.html.haml b/app/views/shared/boards/components/sidebar/_assignee.html.haml index 1374da9d82c..af6a519a967 100644 --- a/app/views/shared/boards/components/sidebar/_assignee.html.haml +++ b/app/views/shared/boards/components/sidebar/_assignee.html.haml @@ -19,7 +19,7 @@ ":data-name" => "assignee.name", ":data-username" => "assignee.username" } .dropdown - - dropdown_options = issue_assignees_dropdown_options + - dropdown_options = assignees_dropdown_options('issue') %button.dropdown-menu-toggle.js-user-search.js-author-search.js-multiselect.js-save-user-data.js-issue-board-sidebar{ type: 'button', ref: 'assigneeDropdown', data: board_sidebar_user_data, ":data-issuable-id" => "issue.iid" } = dropdown_options[:title] diff --git a/app/views/shared/issuable/_assignees.html.haml b/app/views/shared/issuable/_assignees.html.haml index ef3d44a9241..24734ed66cf 100644 --- a/app/views/shared/issuable/_assignees.html.haml +++ b/app/views/shared/issuable/_assignees.html.haml @@ -1,9 +1,9 @@ - max_render = 4 -- assignees_rendering_overflow = issue.assignees.size > max_render +- assignees_rendering_overflow = issuable.assignees.size > max_render - render_count = assignees_rendering_overflow ? max_render - 1 : max_render -- more_assignees_count = issue.assignees.size - render_count +- more_assignees_count = issuable.assignees.size - render_count -- issue.assignees.take(render_count).each do |assignee| # rubocop: disable CodeReuse/ActiveRecord +- issuable.assignees.take(render_count).each do |assignee| # rubocop: disable CodeReuse/ActiveRecord = link_to_member(@project, assignee, name: false, title: "Assigned to :name") - if more_assignees_count.positive? diff --git a/app/views/shared/issuable/_bulk_update_sidebar.html.haml b/app/views/shared/issuable/_bulk_update_sidebar.html.haml index 909eb738f95..a05a13814ac 100644 --- a/app/views/shared/issuable/_bulk_update_sidebar.html.haml +++ b/app/views/shared/issuable/_bulk_update_sidebar.html.haml @@ -21,10 +21,7 @@ .title Assignee .filter-item - - if type == :issues - - field_name = "update[assignee_ids][]" - - else - - field_name = "update[assignee_id]" + - field_name = "update[assignee_ids][]" = dropdown_tag("Select assignee", options: { toggle_class: "js-user-search js-update-assignee js-filter-submit js-filter-bulk-update", title: "Assign to", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable", placeholder: "Search authors", data: { first_user: (current_user.username if current_user), null_user: true, current_user: true, project_id: @project.id, field_name: field_name } }) .block diff --git a/app/views/shared/issuable/_sidebar_assignees.html.haml b/app/views/shared/issuable/_sidebar_assignees.html.haml index 1a59055f652..ab01094ed6e 100644 --- a/app/views/shared/issuable/_sidebar_assignees.html.haml +++ b/app/views/shared/issuable/_sidebar_assignees.html.haml @@ -1,42 +1,10 @@ - issuable_type = issuable_sidebar[:type] - signed_in = !!issuable_sidebar.dig(:current_user, :id) -- can_edit_issuable = issuable_sidebar.dig(:current_user, :can_edit) -- if issuable_type == "issue" - #js-vue-sidebar-assignees{ data: { field: "#{issuable_type}[assignee_ids]", signed_in: signed_in } } - .title.hide-collapsed - = _('Assignee') - = icon('spinner spin') -- else - - assignee = assignees.first - .sidebar-collapsed-icon.sidebar-collapsed-user{ data: { toggle: "tooltip", placement: "left", container: "body", boundary: 'viewport' }, title: (issuable_sidebar.dig(:assignee, :name) || _('Assignee')) } - - if issuable_sidebar[:assignee] - = link_to_member(@project, assignee, size: 24) - - else - = icon('user', 'aria-hidden': 'true') +#js-vue-sidebar-assignees{ data: { field: "#{issuable_type}[assignee_ids]", signed_in: signed_in } } .title.hide-collapsed = _('Assignee') - = icon('spinner spin', class: 'hidden block-loading', 'aria-hidden': 'true') - - if can_edit_issuable - = link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right' - - if !signed_in - %a.gutter-toggle.float-right.js-sidebar-toggle{ role: "button", href: "#", "aria-label" => _('Toggle sidebar') } - = sidebar_gutter_toggle_icon - .value.hide-collapsed - - if issuable_sidebar[:assignee] - = link_to_member(@project, assignee, size: 32, extra_class: 'bold') do - - unless issuable_sidebar[:assignee][:can_merge] - %span.float-right.cannot-be-merged{ data: { toggle: 'tooltip', placement: 'left' }, title: _('Not allowed to merge') } - = icon('exclamation-triangle', 'aria-hidden': 'true') - %span.username - @#{issuable_sidebar[:assignee][:username]} - - else - %span.assign-yourself.no-value - = _('No assignee') - - if can_edit_issuable - \- - %a.js-assign-yourself{ href: '#' } - = _('assign yourself') + = icon('spinner spin') .selectbox.hide-collapsed - if assignees.none? @@ -59,17 +27,15 @@ ability_name: issuable_type, null_user: true, display: 'static' } } - - title = _('Select assignee') - - if issuable_type == "issue" - - dropdown_options = issue_assignees_dropdown_options - - title = dropdown_options[:title] - - options[:toggle_class] += ' js-multiselect js-save-user-data' - - data = { field_name: "#{issuable_type}[assignee_ids][]" } - - data[:multi_select] = true - - data['dropdown-title'] = title - - data['dropdown-header'] = dropdown_options[:data][:'dropdown-header'] - - data['max-select'] = dropdown_options[:data][:'max-select'] if dropdown_options[:data][:'max-select'] - - options[:data].merge!(data) + - dropdown_options = assignees_dropdown_options(issuable_type) + - title = dropdown_options[:title] + - options[:toggle_class] += ' js-multiselect js-save-user-data' + - data = { field_name: "#{issuable_type}[assignee_ids][]" } + - data[:multi_select] = true + - data['dropdown-title'] = title + - data['dropdown-header'] = dropdown_options[:data][:'dropdown-header'] + - data['max-select'] = dropdown_options[:data][:'max-select'] if dropdown_options[:data][:'max-select'] + - options[:data].merge!(data) = dropdown_tag(title, options: options) diff --git a/app/views/shared/issuable/form/_merge_request_assignee.html.haml b/app/views/shared/issuable/form/_merge_request_assignee.html.haml deleted file mode 100644 index 05c03dedd91..00000000000 --- a/app/views/shared/issuable/form/_merge_request_assignee.html.haml +++ /dev/null @@ -1,31 +0,0 @@ -- merge_request = issuable -.block.assignee - .sidebar-collapsed-icon.sidebar-collapsed-user{ data: { toggle: "tooltip", placement: "left", container: "body" }, title: sidebar_assignee_tooltip_label(issuable) } - - if merge_request.assignee - = link_to_member(@project, merge_request.assignee, size: 24) - - else - = icon('user', 'aria-hidden': 'true') - .title.hide-collapsed - Assignee - = icon('spinner spin', class: 'hidden block-loading', 'aria-hidden': 'true') - - if can_edit_issuable - = link_to 'Edit', '#', class: 'js-sidebar-dropdown-toggle edit-link float-right' - .value.hide-collapsed - - if merge_request.assignee - = link_to_member(@project, merge_request.assignee, size: 32, extra_class: 'bold') do - - unless merge_request.can_be_merged_by?(merge_request.assignee) - %span.float-right.cannot-be-merged{ data: { toggle: 'tooltip', placement: 'left' }, title: 'Not allowed to merge' } - = icon('exclamation-triangle', 'aria-hidden': 'true') - %span.username - = merge_request.assignee.to_reference - - else - %span.assign-yourself.no-value - No assignee - - if can_edit_issuable - \- - %a.js-assign-yourself{ href: '#' } - assign yourself - - .selectbox.hide-collapsed - = f.hidden_field 'assignee_id', value: merge_request.assignee_id, id: 'issue_assignee_id' - = dropdown_tag('Select assignee', options: { toggle_class: 'js-user-search js-author-search', title: 'Assign to', filter: true, dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-author', placeholder: 'Search users', data: { first_user: (current_user.username if current_user), current_user: true, project_id: @project&.id, author_id: merge_request.author_id, field_name: 'merge_request[assignee_id]', issue_update: issuable_json_path(merge_request), ability_name: 'merge_request', null_user: true } }) diff --git a/app/views/shared/issuable/form/_metadata.html.haml b/app/views/shared/issuable/form/_metadata.html.haml index e370dff9526..1e03440a5dc 100644 --- a/app/views/shared/issuable/form/_metadata.html.haml +++ b/app/views/shared/issuable/form/_metadata.html.haml @@ -8,11 +8,8 @@ %hr .row %div{ class: (has_due_date ? "col-lg-6" : "col-12") } - .form-group.row.issue-assignee - - if issuable.is_a?(Issue) - = render "shared/issuable/form/metadata_issue_assignee", issuable: issuable, form: form, has_due_date: has_due_date - - else - = render "shared/issuable/form/metadata_merge_request_assignee", issuable: issuable, form: form, has_due_date: has_due_date + .form-group.row.merge-request-assignee + = render "shared/issuable/form/metadata_issuable_assignee", issuable: issuable, form: form, has_due_date: has_due_date .form-group.row.issue-milestone = form.label :milestone_id, "Milestone", class: "col-form-label #{has_due_date ? "col-md-2 col-lg-4" : "col-sm-2"}" .col-sm-10{ class: ("col-md-8" if has_due_date) } diff --git a/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml b/app/views/shared/issuable/form/_metadata_issuable_assignee.html.haml index 6d4f9ccd66f..5336159e762 100644 --- a/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml +++ b/app/views/shared/issuable/form/_metadata_issuable_assignee.html.haml @@ -1,4 +1,4 @@ -= form.label :assignee_ids, "Assignee", class: "col-form-label #{"col-md-2 col-lg-4" if has_due_date}" += form.label :assignee_id, "Assignee", class: "col-form-label #{has_due_date ? "col-lg-4" : "col-sm-2"}" .col-sm-10{ class: ("col-md-8" if has_due_date) } .issuable-form-select-holder.selectbox - issuable.assignees.each do |assignee| @@ -7,5 +7,5 @@ - if issuable.assignees.length === 0 = hidden_field_tag "#{issuable.to_ability_name}[assignee_ids][]", 0, id: nil, data: { meta: '' } - = dropdown_tag(users_dropdown_label(issuable.assignees), options: issue_assignees_dropdown_options) - = link_to 'Assign to me', '#', class: "assign-to-me-link #{'hide' if issuable.assignees.include?(current_user)}" + = dropdown_tag(users_dropdown_label(issuable.assignees), options: assignees_dropdown_options(issuable.to_ability_name)) + = link_to 'Assign to me', '#', class: "assign-to-me-link qa-assign-to-me-link #{'hide' if issuable.assignees.include?(current_user)}" diff --git a/db/fixtures/development/10_merge_requests.rb b/db/fixtures/development/10_merge_requests.rb index 1952f84ed62..43b69470d2c 100644 --- a/db/fixtures/development/10_merge_requests.rb +++ b/db/fixtures/development/10_merge_requests.rb @@ -21,7 +21,7 @@ Gitlab::Seeder.quiet do title: FFaker::Lorem.sentence(6), description: FFaker::Lorem.sentences(3).join(" "), milestone: project.milestones.sample, - assignee: project.team.users.sample, + assignees: [project.team.users.sample], label_ids: label_ids } diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 2dd3120d3fc..8e0623887a8 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -663,7 +663,11 @@ module API expose(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) } expose(:upvotes) { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) } expose(:downvotes) { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) } - expose :author, :assignee, using: Entities::UserBasic + expose :assignee, using: ::API::Entities::UserBasic do |merge_request| + merge_request.assignee + end + expose :author, :assignees, using: Entities::UserBasic + expose :source_project_id, :target_project_id expose :labels do |merge_request| # Avoids an N+1 query since labels are preloaded diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index e4b21b7d1c4..1cc0ecc6df8 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -20,6 +20,7 @@ module API def self.update_params_at_least_one_of %i[ assignee_id + assignee_ids description labels milestone_id @@ -184,6 +185,7 @@ module API params :optional_params do optional :description, type: String, desc: 'The description of the merge request' optional :assignee_id, type: Integer, desc: 'The ID of a user to assign the merge request' + optional :assignee_ids, type: Array[Integer], desc: 'The array of user IDs to assign issue' optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request' optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' @@ -231,6 +233,7 @@ module API mr_params = declared_params(include_missing: false) mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) + mr_params = convert_parameters_from_legacy_format(mr_params) merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute @@ -334,6 +337,7 @@ module API mr_params = declared_params(include_missing: false) mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present? + mr_params = convert_parameters_from_legacy_format(mr_params) merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request) diff --git a/lib/banzai/reference_parser/merge_request_parser.rb b/lib/banzai/reference_parser/merge_request_parser.rb index e8147ac591a..d7bf450465e 100644 --- a/lib/banzai/reference_parser/merge_request_parser.rb +++ b/lib/banzai/reference_parser/merge_request_parser.rb @@ -10,7 +10,7 @@ module Banzai nodes, MergeRequest.includes( :author, - :assignee, + :assignees, { # These associations are primarily used for checking permissions. # Eager loading these ensures we don't end up running dozens of diff --git a/lib/gitlab/hook_data/issuable_builder.rb b/lib/gitlab/hook_data/issuable_builder.rb index 0803df65632..b8da6731081 100644 --- a/lib/gitlab/hook_data/issuable_builder.rb +++ b/lib/gitlab/hook_data/issuable_builder.rb @@ -20,11 +20,7 @@ module Gitlab repository: issuable.project.hook_attrs.slice(:name, :url, :description, :homepage) } - if issuable.is_a?(Issue) - hook_data[:assignees] = issuable.assignees.map(&:hook_attrs) if issuable.assignees.any? - else - hook_data[:assignee] = issuable.assignee.hook_attrs if issuable.assignee - end + hook_data[:assignees] = issuable.assignees.map(&:hook_attrs) if issuable.assignees.any? hook_data end diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb index d77b1d04644..a8e993e087e 100644 --- a/lib/gitlab/hook_data/merge_request_builder.rb +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -34,7 +34,6 @@ module Gitlab end SAFE_HOOK_RELATIONS = %i[ - assignee labels total_time_spent ].freeze @@ -51,7 +50,9 @@ module Gitlab work_in_progress: merge_request.work_in_progress?, total_time_spent: merge_request.total_time_spent, human_total_time_spent: merge_request.human_total_time_spent, - human_time_estimate: merge_request.human_time_estimate + human_time_estimate: merge_request.human_time_estimate, + assignee_ids: merge_request.assignee_ids, + assignee_id: merge_request.assignee_ids.first # This key is deprecated } merge_request.attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index aa116a44254..48917ce445c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -987,9 +987,6 @@ msgstr "" msgid "Assigned Merge Requests" msgstr "" -msgid "Assigned to :name" -msgstr "" - msgid "Assigned to me" msgstr "" @@ -5474,9 +5471,6 @@ msgstr "" msgid "No activities found" msgstr "" -msgid "No assignee" -msgstr "" - msgid "No branches found" msgstr "" @@ -5564,9 +5558,6 @@ msgstr "" msgid "None" msgstr "" -msgid "Not allowed to merge" -msgstr "" - msgid "Not available" msgstr "" @@ -7277,9 +7268,6 @@ msgstr "" msgid "Select an existing Kubernetes cluster or create a new one" msgstr "" -msgid "Select assignee" -msgstr "" - msgid "Select branch/tag" msgstr "" @@ -8860,9 +8848,6 @@ msgstr "" msgid "Toggle navigation" msgstr "" -msgid "Toggle sidebar" -msgstr "" - msgid "ToggleButton|Toggle Status: OFF" msgstr "" @@ -9870,9 +9855,6 @@ msgstr "" msgid "among other things" msgstr "" -msgid "assign yourself" -msgstr "" - msgid "attach a new file" msgstr "" diff --git a/qa/qa/page/merge_request/new.rb b/qa/qa/page/merge_request/new.rb index 20d9c336367..56fbf59b9bc 100644 --- a/qa/qa/page/merge_request/new.rb +++ b/qa/qa/page/merge_request/new.rb @@ -26,7 +26,7 @@ module QA element :issuable_label end - view 'app/views/shared/issuable/form/_metadata_merge_request_assignee.html.haml' do + view 'app/views/shared/issuable/form/_metadata_issuable_assignee.html.haml' do element :assign_to_me_link end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 017162519d8..a125e470522 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -238,11 +238,11 @@ describe Projects::MergeRequestsController do assignee = create(:user) project.add_developer(assignee) - update_merge_request({ assignee_id: assignee.id }, format: :json) + update_merge_request({ assignee_ids: [assignee.id] }, format: :json) + body = JSON.parse(response.body) - expect(body['assignee'].keys) - .to match_array(%w(name username avatar_url id state web_url)) + expect(body['assignees']).to all(include(*%w(name username avatar_url id state web_url))) end end diff --git a/spec/features/dashboard/issuables_counter_spec.rb b/spec/features/dashboard/issuables_counter_spec.rb index fbc2e5cc3d3..50b71368e13 100644 --- a/spec/features/dashboard/issuables_counter_spec.rb +++ b/spec/features/dashboard/issuables_counter_spec.rb @@ -8,7 +8,7 @@ describe 'Navigation bar counter', :use_clean_rails_memory_store_caching do before do issue.assignees = [user] - merge_request.update(assignee: user) + merge_request.update(assignees: [user]) sign_in(user) end @@ -33,7 +33,7 @@ describe 'Navigation bar counter', :use_clean_rails_memory_store_caching do expect_counters('merge_requests', '1') - merge_request.update(assignee: nil) + merge_request.update(assignees: []) user.invalidate_cache_counts diff --git a/spec/features/dashboard/merge_requests_spec.rb b/spec/features/dashboard/merge_requests_spec.rb index 4965770605a..0c6713f623c 100644 --- a/spec/features/dashboard/merge_requests_spec.rb +++ b/spec/features/dashboard/merge_requests_spec.rb @@ -48,14 +48,14 @@ describe 'Dashboard Merge Requests' do let!(:assigned_merge_request) do create(:merge_request, - assignee: current_user, + assignees: [current_user], source_project: project, author: create(:user)) end let!(:assigned_merge_request_from_fork) do create(:merge_request, - source_branch: 'markdown', assignee: current_user, + source_branch: 'markdown', assignees: [current_user], target_project: public_project, source_project: forked_project, author: create(:user)) end diff --git a/spec/features/groups/merge_requests_spec.rb b/spec/features/groups/merge_requests_spec.rb index 54a8016c157..e1bc4eca619 100644 --- a/spec/features/groups/merge_requests_spec.rb +++ b/spec/features/groups/merge_requests_spec.rb @@ -38,7 +38,7 @@ describe 'Group merge requests page' do context 'when merge request assignee to user' do before do - issuable.update!(assignee: user) + issuable.update!(assignees: [user]) visit path end diff --git a/spec/features/issues/form_spec.rb b/spec/features/issues/form_spec.rb index 26c781350e5..6fa2ad8711f 100644 --- a/spec/features/issues/form_spec.rb +++ b/spec/features/issues/form_spec.rb @@ -30,8 +30,8 @@ describe 'New/edit issue', :js do # the original method, resulting in infinite recursion when called. # This is likely a bug with helper modules included into dynamically generated view classes. # To work around this, we have to hold on to and call to the original implementation manually. - original_issue_dropdown_options = FormHelper.instance_method(:issue_assignees_dropdown_options) - allow_any_instance_of(FormHelper).to receive(:issue_assignees_dropdown_options).and_wrap_original do |original, *args| + original_issue_dropdown_options = FormHelper.instance_method(:assignees_dropdown_options) + allow_any_instance_of(FormHelper).to receive(:assignees_dropdown_options).and_wrap_original do |original, *args| options = original_issue_dropdown_options.bind(original.receiver).call(*args) options[:data][:per_page] = 2 diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb index ea2bb1503bb..bcc11217389 100644 --- a/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/spec/features/merge_request/user_creates_merge_request_spec.rb @@ -68,15 +68,15 @@ describe "User creates a merge request", :js do fill_in("Title", with: title) end - click_button("Assignee") - expect(find(".js-assignee-search")["data-project-id"]).to eq(project.id.to_s) + find('.js-assignee-search').click page.within(".dropdown-menu-user") do expect(page).to have_content("Unassigned") .and have_content(user.name) .and have_content(project.users.first.name) end + find('.js-assignee-search').click click_button("Submit merge request") diff --git a/spec/features/merge_request/user_creates_mr_spec.rb b/spec/features/merge_request/user_creates_mr_spec.rb index c169a68cd1c..c9dedab048a 100644 --- a/spec/features/merge_request/user_creates_mr_spec.rb +++ b/spec/features/merge_request/user_creates_mr_spec.rb @@ -1,11 +1,18 @@ require 'rails_helper' describe 'Merge request > User creates MR' do - it_behaves_like 'a creatable merge request' + include ProjectForksHelper - context 'from a forked project' do - include ProjectForksHelper + before do + stub_licensed_features(multiple_merge_request_assignees: false) + end + context 'non-fork merge request' do + include_context 'merge request create context' + it_behaves_like 'a creatable merge request' + end + + context 'from a forked project' do let(:canonical_project) { create(:project, :public, :repository) } let(:source_project) do @@ -15,6 +22,7 @@ describe 'Merge request > User creates MR' do end context 'to canonical project' do + include_context 'merge request create context' it_behaves_like 'a creatable merge request' end @@ -25,6 +33,7 @@ describe 'Merge request > User creates MR' do namespace: user.namespace) end + include_context 'merge request create context' it_behaves_like 'a creatable merge request' end end diff --git a/spec/features/merge_request/user_edits_mr_spec.rb b/spec/features/merge_request/user_edits_mr_spec.rb index 3152707136c..25979513ead 100644 --- a/spec/features/merge_request/user_edits_mr_spec.rb +++ b/spec/features/merge_request/user_edits_mr_spec.rb @@ -1,13 +1,21 @@ -require 'rails_helper' +require 'spec_helper' describe 'Merge request > User edits MR' do include ProjectForksHelper - it_behaves_like 'an editable merge request' + before do + stub_licensed_features(multiple_merge_request_assignees: false) + end + + context 'non-fork merge request' do + include_context 'merge request edit context' + it_behaves_like 'an editable merge request' + end context 'for a forked project' do - it_behaves_like 'an editable merge request' do - let(:source_project) { fork_project(target_project, nil, repository: true) } - end + let(:source_project) { fork_project(target_project, nil, repository: true) } + + include_context 'merge request edit context' + it_behaves_like 'an editable merge request' end end diff --git a/spec/features/merge_requests/user_filters_by_assignees_spec.rb b/spec/features/merge_requests/user_filters_by_assignees_spec.rb index d6c770c93f1..0cbf1bcae30 100644 --- a/spec/features/merge_requests/user_filters_by_assignees_spec.rb +++ b/spec/features/merge_requests/user_filters_by_assignees_spec.rb @@ -7,7 +7,7 @@ describe 'Merge Requests > User filters by assignees', :js do let(:user) { project.creator } before do - create(:merge_request, assignee: user, title: 'Bugfix1', source_project: project, target_project: project, source_branch: 'bugfix1') + create(:merge_request, assignees: [user], title: 'Bugfix1', source_project: project, target_project: project, source_branch: 'bugfix1') create(:merge_request, title: 'Bugfix2', source_project: project, target_project: project, source_branch: 'bugfix2') sign_in(user) diff --git a/spec/features/merge_requests/user_filters_by_multiple_criteria_spec.rb b/spec/features/merge_requests/user_filters_by_multiple_criteria_spec.rb index 1615899a047..4627931f26a 100644 --- a/spec/features/merge_requests/user_filters_by_multiple_criteria_spec.rb +++ b/spec/features/merge_requests/user_filters_by_multiple_criteria_spec.rb @@ -10,7 +10,7 @@ describe 'Merge requests > User filters by multiple criteria', :js do before do sign_in(user) - mr = create(:merge_request, title: 'Bugfix2', author: user, assignee: user, source_project: project, target_project: project, milestone: milestone) + mr = create(:merge_request, title: 'Bugfix2', author: user, assignees: [user], source_project: project, target_project: project, milestone: milestone) mr.labels << wontfix visit project_merge_requests_path(project) diff --git a/spec/features/merge_requests/user_lists_merge_requests_spec.rb b/spec/features/merge_requests/user_lists_merge_requests_spec.rb index c691011b9ca..bd91fae1453 100644 --- a/spec/features/merge_requests/user_lists_merge_requests_spec.rb +++ b/spec/features/merge_requests/user_lists_merge_requests_spec.rb @@ -12,7 +12,7 @@ describe 'Merge requests > User lists merge requests' do title: 'fix', source_project: project, source_branch: 'fix', - assignee: user, + assignees: [user], milestone: create(:milestone, project: project, due_date: '2013-12-11'), created_at: 1.minute.ago, updated_at: 1.minute.ago) @@ -20,7 +20,7 @@ describe 'Merge requests > User lists merge requests' do title: 'markdown', source_project: project, source_branch: 'markdown', - assignee: user, + assignees: [user], milestone: create(:milestone, project: project, due_date: '2013-12-12'), created_at: 2.minutes.ago, updated_at: 2.minutes.ago) diff --git a/spec/features/merge_requests/user_mass_updates_spec.rb b/spec/features/merge_requests/user_mass_updates_spec.rb index e535c7e5811..c2dd105324d 100644 --- a/spec/features/merge_requests/user_mass_updates_spec.rb +++ b/spec/features/merge_requests/user_mass_updates_spec.rb @@ -54,8 +54,7 @@ describe 'Merge requests > User mass updates', :js do describe 'remove assignee' do before do - merge_request.assignee = user - merge_request.save + merge_request.assignees = [user] visit project_merge_requests_path(project) end diff --git a/spec/features/search/user_uses_header_search_field_spec.rb b/spec/features/search/user_uses_header_search_field_spec.rb index 444de26733f..1cc47cd6bd1 100644 --- a/spec/features/search/user_uses_header_search_field_spec.rb +++ b/spec/features/search/user_uses_header_search_field_spec.rb @@ -36,7 +36,7 @@ describe 'User uses header search field' do end context 'when clicking merge requests' do - let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignee: user) } + let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignees: [user]) } it 'shows assigned merge requests' do find('.search-input-container .dropdown-menu').click_link('Merge requests assigned to me') @@ -100,7 +100,7 @@ describe 'User uses header search field' do end context 'when clicking merge requests' do - let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignee: user) } + let!(:merge_request) { create(:merge_request, source_project: project, author: user, assignees: [user]) } it 'shows assigned merge requests' do find('.dropdown-menu').click_link('Merge requests assigned to me') diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index fe53fabe54c..6e6dc334354 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -13,60 +13,32 @@ describe IssuesFinder do expect(issues).to contain_exactly(issue1, issue2, issue3, issue4) end - context 'filtering by assignee ID' do - let(:params) { { assignee_id: user.id } } + context 'assignee filtering' do + let(:issuables) { issues } - it 'returns issues assigned to that user' do - expect(issues).to contain_exactly(issue1, issue2) - end - end - - context 'filtering by assignee usernames' do - set(:user3) { create(:user) } - let(:params) { { assignee_username: [user2.username, user3.username] } } - - before do - project2.add_developer(user3) - - issue3.assignees = [user2, user3] - end - - it 'returns issues assigned to those users' do - expect(issues).to contain_exactly(issue3) - end - end - - context 'filtering by no assignee' do - let(:params) { { assignee_id: 'None' } } - - it 'returns issues not assigned to any assignee' do - expect(issues).to contain_exactly(issue4) - end - - it 'returns issues not assigned to any assignee' do - params[:assignee_id] = 0 - - expect(issues).to contain_exactly(issue4) + it_behaves_like 'assignee ID filter' do + let(:params) { { assignee_id: user.id } } + let(:expected_issuables) { [issue1, issue2] } end - it 'returns issues not assigned to any assignee' do - params[:assignee_id] = 'none' + it_behaves_like 'assignee username filter' do + before do + project2.add_developer(user3) + issue3.assignees = [user2, user3] + end - expect(issues).to contain_exactly(issue4) + set(:user3) { create(:user) } + let(:params) { { assignee_username: [user2.username, user3.username] } } + let(:expected_issuables) { [issue3] } end - end - context 'filtering by any assignee' do - let(:params) { { assignee_id: 'Any' } } - - it 'returns issues assigned to any assignee' do - expect(issues).to contain_exactly(issue1, issue2, issue3) + it_behaves_like 'no assignee filter' do + set(:user3) { create(:user) } + let(:expected_issuables) { [issue4] } end - it 'returns issues assigned to any assignee' do - params[:assignee_id] = 'any' - - expect(issues).to contain_exactly(issue1, issue2, issue3) + it_behaves_like 'any assignee filter' do + let(:expected_issuables) { [issue1, issue2, issue3] } end end diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index f508b9bdb6f..ef002125635 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -136,21 +136,50 @@ describe MergeRequestsFinder do end end - context 'filtering by group milestone' do - let(:group_milestone) { create(:milestone, group: group) } + context 'assignee filtering' do + let(:issuables) { described_class.new(user, params).execute } - before do - project2.update(namespace: group) - merge_request2.update(milestone: group_milestone) - merge_request3.update(milestone: group_milestone) + it_behaves_like 'assignee ID filter' do + let(:params) { { assignee_id: user.id } } + let(:expected_issuables) { [merge_request1, merge_request2] } end - it 'returns merge requests assigned to that group milestone' do - params = { milestone_title: group_milestone.title } + it_behaves_like 'assignee username filter' do + before do + project2.add_developer(user3) + merge_request3.assignees = [user2, user3] + end - merge_requests = described_class.new(user, params).execute + set(:user3) { create(:user) } + let(:params) { { assignee_username: [user2.username, user3.username] } } + let(:expected_issuables) { [merge_request3] } + end + + it_behaves_like 'no assignee filter' do + set(:user3) { create(:user) } + let(:expected_issuables) { [merge_request4, merge_request5] } + end + + it_behaves_like 'any assignee filter' do + let(:expected_issuables) { [merge_request1, merge_request2, merge_request3] } + end + + context 'filtering by group milestone' do + let(:group_milestone) { create(:milestone, group: group) } + + before do + project2.update(namespace: group) + merge_request2.update(milestone: group_milestone) + merge_request3.update(milestone: group_milestone) + end + + it 'returns merge requests assigned to that group milestone' do + params = { milestone_title: group_milestone.title } - expect(merge_requests).to contain_exactly(merge_request2, merge_request3) + merge_requests = described_class.new(user, params).execute + + expect(merge_requests).to contain_exactly(merge_request2, merge_request3) + end end end diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index 3006b482d41..88a600398b1 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -6,14 +6,14 @@ "source_branch_exists": { "type": "boolean" }, "merge_error": { "type": ["string", "null"] }, "rebase_in_progress": { "type": "boolean" }, - "assignee_id": { "type": ["integer", "null"] }, "allow_collaboration": { "type": "boolean"}, "allow_maintainer_to_push": { "type": "boolean"}, - "assignee": { - "oneOf": [ - { "type": "null" }, - { "$ref": "user.json" } - ] + "assignees": { + "type": ["array"], + "items": { + "type": "object", + "$ref": "../public_api/v4/user/basic.json" + } }, "milestone": { "type": [ "object", "null" ] diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_request.json b/spec/fixtures/api/schemas/public_api/v4/merge_request.json index 918f2c4b47d..a423bf70b69 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_request.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_request.json @@ -64,6 +64,11 @@ }, "additionalProperties": false }, + "assignees": { + "items": { + "$ref": "./merge_request.json" + } + }, "source_project_id": { "type": "integer" }, "target_project_id": { "type": "integer" }, "labels": { diff --git a/spec/javascripts/sidebar/assignees_spec.js b/spec/javascripts/sidebar/assignees_spec.js index 57b16b12cb0..47fee5d2b21 100644 --- a/spec/javascripts/sidebar/assignees_spec.js +++ b/spec/javascripts/sidebar/assignees_spec.js @@ -132,9 +132,94 @@ describe('Assignee component', () => { -1, ); }); + + it('has correct "cannot merge" tooltip when user cannot merge', () => { + const user = Object.assign({}, UsersMock.user, { can_merge: false }); + + component = new AssigneeComponent({ + propsData: { + rootPath: 'http://localhost:3000/', + users: [user], + editable: true, + issuableType: 'merge_request', + }, + }).$mount(); + + expect(component.mergeNotAllowedTooltipMessage).toEqual('Cannot merge'); + }); }); describe('Two or more assignees/users', () => { + it('has correct "cannot merge" tooltip when one user can merge', () => { + const users = UsersMockHelper.createNumberRandomUsers(3); + users[0].can_merge = true; + users[1].can_merge = false; + users[2].can_merge = false; + + component = new AssigneeComponent({ + propsData: { + rootPath: 'http://localhost:3000/', + users, + editable: true, + issuableType: 'merge_request', + }, + }).$mount(); + + expect(component.mergeNotAllowedTooltipMessage).toEqual('1/3 can merge'); + }); + + it('has correct "cannot merge" tooltip when no user can merge', () => { + const users = UsersMockHelper.createNumberRandomUsers(2); + users[0].can_merge = false; + users[1].can_merge = false; + + component = new AssigneeComponent({ + propsData: { + rootPath: 'http://localhost:3000/', + users, + editable: true, + issuableType: 'merge_request', + }, + }).$mount(); + + expect(component.mergeNotAllowedTooltipMessage).toEqual('No one can merge'); + }); + + it('has correct "cannot merge" tooltip when more than one user can merge', () => { + const users = UsersMockHelper.createNumberRandomUsers(3); + users[0].can_merge = false; + users[1].can_merge = true; + users[2].can_merge = true; + + component = new AssigneeComponent({ + propsData: { + rootPath: 'http://localhost:3000/', + users, + editable: true, + issuableType: 'merge_request', + }, + }).$mount(); + + expect(component.mergeNotAllowedTooltipMessage).toEqual('2/3 can merge'); + }); + + it('has no "cannot merge" tooltip when every user can merge', () => { + const users = UsersMockHelper.createNumberRandomUsers(2); + users[0].can_merge = true; + users[1].can_merge = true; + + component = new AssigneeComponent({ + propsData: { + rootPath: 'http://localhost:3000/', + users, + editable: true, + issuableType: 'merge_request', + }, + }).$mount(); + + expect(component.mergeNotAllowedTooltipMessage).toEqual(null); + }); + it('displays two assignee icons when collapsed', () => { const users = UsersMockHelper.createNumberRandomUsers(2); component = new AssigneeComponent({ diff --git a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb index 26529c4759d..569d5dcc757 100644 --- a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb @@ -97,13 +97,13 @@ describe Gitlab::HookData::IssuableBuilder do end context 'merge_request is assigned' do - let(:merge_request) { create(:merge_request, assignee: user) } + let(:merge_request) { create(:merge_request, assignees: [user]) } let(:data) { described_class.new(merge_request).build(user: user) } it 'returns correct hook data' do expect(data[:object_attributes]['assignee_id']).to eq(user.id) - expect(data[:assignee]).to eq(user.hook_attrs) - expect(data).not_to have_key(:assignees) + expect(data[:assignees].first).to eq(user.hook_attrs) + expect(data).not_to have_key(:assignee) end end end diff --git a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb index 9ce697adbba..39f80f92fa6 100644 --- a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb @@ -10,6 +10,7 @@ describe Gitlab::HookData::MergeRequestBuilder do it 'includes safe attribute' do %w[ assignee_id + assignee_ids author_id created_at description diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index e418516569a..ed557ffd4e3 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -102,6 +102,7 @@ merge_requests: - merge_request_pipelines - merge_request_assignees - suggestions +- assignees merge_request_diff: - merge_request - merge_request_diff_commits @@ -336,6 +337,9 @@ push_event_payload: issue_assignees: - issue - assignee +merge_request_assignees: +- merge_request +- assignee lfs_file_locks: - user project_badges: diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index d0ed588f05f..06995604a24 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -621,3 +621,7 @@ Suggestion: - outdated - lines_above - lines_below +MergeRequestAssignee: +- id +- user_id +- merge_request_id diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 6ec86163233..916f3876a8e 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/spec/lib/gitlab/issuable_metadata_spec.rb @@ -19,7 +19,7 @@ describe Gitlab::IssuableMetadata do let!(:closed_issue) { create(:issue, state: :closed, author: user, project: project) } let!(:downvote) { create(:award_emoji, :downvote, awardable: closed_issue) } let!(:upvote) { create(:award_emoji, :upvote, awardable: issue) } - let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") } + let!(:merge_request) { create(:merge_request, :simple, author: user, assignees: [user], source_project: project, target_project: project, title: "Test") } let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) } it 'aggregates stats on issues' do @@ -39,7 +39,7 @@ describe Gitlab::IssuableMetadata do end context 'merge requests' do - let!(:merge_request) { create(:merge_request, :simple, author: user, assignee: user, source_project: project, target_project: project, title: "Test") } + let!(:merge_request) { create(:merge_request, :simple, author: user, assignees: [user], source_project: project, target_project: project, title: "Test") } let!(:merge_request_closed) { create(:merge_request, state: "closed", source_project: project, target_project: project, title: "Closed Test") } let!(:downvote) { create(:award_emoji, :downvote, awardable: merge_request) } let!(:upvote) { create(:award_emoji, :upvote, awardable: merge_request) } diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 5fa1369c00a..fee1d701e3a 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -19,7 +19,7 @@ describe Notify do create(:merge_request, source_project: project, target_project: project, author: current_user, - assignee: assignee, + assignees: [assignee], description: 'Awesome description') end @@ -275,7 +275,7 @@ describe Notify do context 'for merge requests' do describe 'that are new' do - subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id) } + subject { described_class.new_merge_request_email(merge_request.assignee_ids.first, merge_request.id) } it_behaves_like 'an assignee email' it_behaves_like 'an email starting a new thread with reply-by-email enabled' do @@ -300,7 +300,7 @@ describe Notify do end context 'when sent with a reason' do - subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id, NotificationReason::ASSIGNED) } + subject { described_class.new_merge_request_email(merge_request.assignee_ids.first, merge_request.id, NotificationReason::ASSIGNED) } it_behaves_like 'appearance header and footer enabled' it_behaves_like 'appearance header and footer not enabled' @@ -324,7 +324,7 @@ describe Notify do describe 'that are reassigned' do let(:previous_assignee) { create(:user, name: 'Previous Assignee') } - subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) } + subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, [previous_assignee.id], current_user.id) } it_behaves_like 'a multiple recipients email' it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do @@ -351,7 +351,7 @@ describe Notify do end context 'when sent with a reason' do - subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::ASSIGNED) } + subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, [previous_assignee.id], current_user.id, NotificationReason::ASSIGNED) } it_behaves_like 'appearance header and footer enabled' it_behaves_like 'appearance header and footer not enabled' @@ -364,11 +364,11 @@ describe Notify do text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::ASSIGNED) is_expected.to have_body_text(text) - new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, NotificationReason::MENTIONED) + new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, [previous_assignee.id], current_user.id, NotificationReason::MENTIONED) text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(NotificationReason::MENTIONED) expect(new_subject).to have_body_text(text) - new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id, nil) + new_subject = described_class.reassigned_merge_request_email(recipient.id, merge_request.id, [previous_assignee.id], current_user.id, nil) text = EmailsHelper.instance_method(:notification_reason_text).bind(self).call(nil) expect(new_subject).to have_body_text(text) end @@ -376,7 +376,7 @@ describe Notify do end describe 'that are new with a description' do - subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id) } + subject { described_class.new_merge_request_email(merge_request.assignee_ids.first, merge_request.id) } it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like "an unsubscribeable thread" @@ -476,7 +476,7 @@ describe Notify do source_project: project, target_project: project, author: current_user, - assignee: assignee, + assignees: [assignee], description: 'Awesome description') end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 83b0f172f03..f3e78630c1b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -684,12 +684,12 @@ describe Ci::Pipeline, :mailer do source_branch: 'feature', target_project: project, target_branch: 'master', - assignee: assignee, + assignees: assignees, milestone: milestone, labels: labels) end - let(:assignee) { create(:user) } + let(:assignees) { create_list(:user, 2) } let(:milestone) { create(:milestone, project: project) } let(:labels) { create_list(:label, 2) } @@ -710,7 +710,7 @@ describe Ci::Pipeline, :mailer do 'CI_MERGE_REQUEST_SOURCE_BRANCH_NAME' => merge_request.source_branch.to_s, 'CI_MERGE_REQUEST_SOURCE_BRANCH_SHA' => pipeline.source_sha.to_s, 'CI_MERGE_REQUEST_TITLE' => merge_request.title, - 'CI_MERGE_REQUEST_ASSIGNEES' => assignee.username, + 'CI_MERGE_REQUEST_ASSIGNEES' => merge_request.assignee_username_list, 'CI_MERGE_REQUEST_MILESTONE' => milestone.title, 'CI_MERGE_REQUEST_LABELS' => labels.map(&:title).join(',')) end @@ -730,7 +730,7 @@ describe Ci::Pipeline, :mailer do end context 'without assignee' do - let(:assignee) { nil } + let(:assignees) { [] } it 'does not expose assignee variable' do expect(subject.to_hash.keys).not_to include('CI_MERGE_REQUEST_ASSIGNEES') diff --git a/spec/models/concerns/deprecated_assignee_spec.rb b/spec/models/concerns/deprecated_assignee_spec.rb new file mode 100644 index 00000000000..e394de0aa34 --- /dev/null +++ b/spec/models/concerns/deprecated_assignee_spec.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DeprecatedAssignee do + let(:user) { create(:user) } + + describe '#assignee_id=' do + it 'creates the merge_request_assignees relation' do + merge_request = create(:merge_request, assignee_id: user.id) + + merge_request.reload + + expect(merge_request.merge_request_assignees.count).to eq(1) + end + + it 'nullifies the assignee_id column' do + merge_request = create(:merge_request, assignee_id: user.id) + + merge_request.reload + + expect(merge_request.read_attribute(:assignee_id)).to be_nil + end + + context 'when relation already exists' do + it 'overwrites existing assignees' do + other_user = create(:user) + merge_request = create(:merge_request, assignee_id: nil) + merge_request.merge_request_assignees.create!(user_id: user.id) + merge_request.merge_request_assignees.create!(user_id: other_user.id) + + expect { merge_request.update!(assignee_id: other_user.id) } + .to change { merge_request.reload.merge_request_assignees.count } + .from(2).to(1) + end + end + end + + describe '#assignee=' do + it 'creates the merge_request_assignees relation' do + merge_request = create(:merge_request, assignee: user) + + merge_request.reload + + expect(merge_request.merge_request_assignees.count).to eq(1) + end + + it 'nullifies the assignee_id column' do + merge_request = create(:merge_request, assignee: user) + + merge_request.reload + + expect(merge_request.read_attribute(:assignee_id)).to be_nil + end + + context 'when relation already exists' do + it 'overwrites existing assignees' do + other_user = create(:user) + merge_request = create(:merge_request, assignee: nil) + merge_request.merge_request_assignees.create!(user_id: user.id) + merge_request.merge_request_assignees.create!(user_id: other_user.id) + + expect { merge_request.update!(assignee: other_user) } + .to change { merge_request.reload.merge_request_assignees.count } + .from(2).to(1) + end + end + end + + describe '#assignee_id' do + it 'returns the first assignee ID' do + other_user = create(:user) + merge_request = create(:merge_request, assignees: [user, other_user]) + + merge_request.reload + + expect(merge_request.assignee_id).to eq(merge_request.assignee_ids.first) + end + end + + describe '#assignees' do + context 'when assignee_id exists and there is no relation' do + it 'creates the relation' do + merge_request = create(:merge_request, assignee_id: nil) + merge_request.update_column(:assignee_id, user.id) + + expect { merge_request.assignees }.to change { merge_request.merge_request_assignees.count }.from(0).to(1) + end + + it 'nullifies the assignee_id' do + merge_request = create(:merge_request, assignee_id: nil) + merge_request.update_column(:assignee_id, user.id) + + expect { merge_request.assignees } + .to change { merge_request.read_attribute(:assignee_id) } + .from(user.id).to(nil) + end + end + + context 'when DB is read-only' do + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'returns a users relation' do + merge_request = create(:merge_request, assignee_id: user.id) + + expect(merge_request.assignees).to be_a(ActiveRecord::Relation) + expect(merge_request.assignees).to eq([user]) + end + + it 'returns an empty relation if no assignee_id is set' do + merge_request = create(:merge_request, assignee_id: nil) + + expect(merge_request.assignees).to be_a(ActiveRecord::Relation) + expect(merge_request.assignees).to eq([]) + end + end + end + + describe '#assignee_ids' do + context 'when assignee_id exists and there is no relation' do + it 'creates the relation' do + merge_request = create(:merge_request, assignee_id: nil) + merge_request.update_column(:assignee_id, user.id) + + expect { merge_request.assignee_ids }.to change { merge_request.merge_request_assignees.count }.from(0).to(1) + end + + it 'nullifies the assignee_id' do + merge_request = create(:merge_request, assignee_id: nil) + merge_request.update_column(:assignee_id, user.id) + + expect { merge_request.assignee_ids } + .to change { merge_request.read_attribute(:assignee_id) } + .from(user.id).to(nil) + end + end + + context 'when DB is read-only' do + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'returns a list of user IDs' do + merge_request = create(:merge_request, assignee_id: user.id) + + expect(merge_request.assignee_ids).to be_a(Array) + expect(merge_request.assignee_ids).to eq([user.id]) + end + + it 'returns an empty relation if no assignee_id is set' do + merge_request = create(:merge_request, assignee_id: nil) + + expect(merge_request.assignee_ids).to be_a(Array) + expect(merge_request.assignee_ids).to eq([]) + end + end + end +end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 27ed298ae08..64f02978d79 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -502,8 +502,8 @@ describe Issuable do let(:user2) { create(:user) } before do - merge_request.update(assignee: user) - merge_request.update(assignee: user2) + merge_request.update(assignees: [user]) + merge_request.update(assignees: [user, user2]) expect(Gitlab::HookData::IssuableBuilder) .to receive(:new).with(merge_request).and_return(builder) end @@ -512,8 +512,7 @@ describe Issuable do expect(builder).to receive(:build).with( user: user, changes: hash_including( - 'assignee_id' => [user.id, user2.id], - 'assignee' => [user.hook_attrs, user2.hook_attrs] + 'assignees' => [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]] )) merge_request.to_hook_data(user, old_associations: { assignees: [user] }) diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index d192fe70506..e91b5c4c86f 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -263,7 +263,7 @@ describe Event do context 'merge request diff note event' do let(:project) { create(:project, :public) } - let(:merge_request) { create(:merge_request, source_project: project, author: author, assignee: assignee) } + let(:merge_request) { create(:merge_request, source_project: project, author: author, assignees: [assignee]) } let(:note_on_merge_request) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project) } let(:target) { note_on_merge_request } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6f34ef9c1bc..f61857ea5ff 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -13,7 +13,7 @@ describe MergeRequest do it { is_expected.to belong_to(:target_project).class_name('Project') } it { is_expected.to belong_to(:source_project).class_name('Project') } it { is_expected.to belong_to(:merge_user).class_name("User") } - it { is_expected.to belong_to(:assignee) } + it { is_expected.to have_many(:assignees).through(:merge_request_assignees) } it { is_expected.to have_many(:merge_request_diffs) } context 'for forks' do @@ -181,31 +181,6 @@ describe MergeRequest do expect(MergeRequest::Metrics.count).to eq(1) end end - - describe '#refresh_merge_request_assignees' do - set(:user) { create(:user) } - - it 'creates merge request assignees relation upon MR creation' do - merge_request = create(:merge_request, assignee: nil) - - expect(merge_request.merge_request_assignees).to be_empty - - expect { merge_request.update!(assignee: user) } - .to change { merge_request.reload.merge_request_assignees.count } - .from(0).to(1) - end - - it 'updates merge request assignees relation upon MR assignee change' do - another_user = create(:user) - merge_request = create(:merge_request, assignee: user) - - expect { merge_request.update!(assignee: another_user) } - .to change { merge_request.reload.merge_request_assignees.first.assignee } - .from(user).to(another_user) - - expect(merge_request.merge_request_assignees.count).to eq(1) - end - end end describe 'respond to' do @@ -337,34 +312,18 @@ describe MergeRequest do describe '#card_attributes' do it 'includes the author name' do allow(subject).to receive(:author).and_return(double(name: 'Robert')) - allow(subject).to receive(:assignee).and_return(nil) + allow(subject).to receive(:assignees).and_return([]) expect(subject.card_attributes) - .to eq({ 'Author' => 'Robert', 'Assignee' => nil }) + .to eq({ 'Author' => 'Robert', 'Assignee' => "" }) end - it 'includes the assignee name' do + it 'includes the assignees name' do allow(subject).to receive(:author).and_return(double(name: 'Robert')) - allow(subject).to receive(:assignee).and_return(double(name: 'Douwe')) + allow(subject).to receive(:assignees).and_return([double(name: 'Douwe'), double(name: 'Robert')]) expect(subject.card_attributes) - .to eq({ 'Author' => 'Robert', 'Assignee' => 'Douwe' }) - end - end - - describe '#assignee_ids' do - it 'returns an array of the assigned user id' do - subject.assignee_id = 123 - - expect(subject.assignee_ids).to eq([123]) - end - end - - describe '#assignee_ids=' do - it 'sets assignee_id to the last id in the array' do - subject.assignee_ids = [123, 456] - - expect(subject.assignee_id).to eq(456) + .to eq({ 'Author' => 'Robert', 'Assignee' => 'Douwe and Robert' }) end end @@ -372,7 +331,7 @@ describe MergeRequest do let(:user) { create(:user) } it 'returns true for a user that is assigned to a merge request' do - subject.assignee = user + subject.assignees = [user] expect(subject.assignee_or_author?(user)).to eq(true) end @@ -1949,15 +1908,14 @@ describe MergeRequest do it 'updates when assignees change' do user1 = create(:user) user2 = create(:user) - mr = create(:merge_request, assignee: user1) + mr = create(:merge_request, assignees: [user1]) mr.project.add_developer(user1) mr.project.add_developer(user2) expect(user1.assigned_open_merge_requests_count).to eq(1) expect(user2.assigned_open_merge_requests_count).to eq(0) - mr.assignee = user2 - mr.save + mr.assignees = [user2] expect(user1.assigned_open_merge_requests_count).to eq(0) expect(user2.assigned_open_merge_requests_count).to eq(1) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a45a2737b13..d1338e34bb8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2816,9 +2816,9 @@ describe User do project = create(:project, :public) archived_project = create(:project, :public, :archived) - create(:merge_request, source_project: project, author: user, assignee: user) - create(:merge_request, :closed, source_project: project, author: user, assignee: user) - create(:merge_request, source_project: archived_project, author: user, assignee: user) + create(:merge_request, source_project: project, author: user, assignees: [user]) + create(:merge_request, :closed, source_project: project, author: user, assignees: [user]) + create(:merge_request, source_project: archived_project, author: user, assignees: [user]) expect(user.assigned_open_merge_requests_count(force: true)).to eq 1 end diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb index 0ac23505de7..065b16c6221 100644 --- a/spec/requests/api/events_spec.rb +++ b/spec/requests/api/events_spec.rb @@ -270,8 +270,8 @@ describe API::Events do end context 'when exists some events' do - let(:merge_request1) { create(:merge_request, :closed, author: user, assignee: user, source_project: private_project, title: 'Test') } - let(:merge_request2) { create(:merge_request, :closed, author: user, assignee: user, source_project: private_project, title: 'Test') } + let(:merge_request1) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } + let(:merge_request2) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } before do create_event(merge_request1) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 7ffa365c651..45818edbf68 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -5,14 +5,15 @@ describe API::MergeRequests do let(:base_time) { Time.now } set(:user) { create(:user) } + set(:user2) { create(:user) } set(:admin) { create(:user, :admin) } let(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) } let(:milestone) { create(:milestone, title: '1.0.0', project: project) } let(:milestone1) { create(:milestone, title: '0.9', project: project) } - let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) } - let!(:merge_request_closed) { create(:merge_request, state: "closed", milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Closed test", created_at: base_time + 1.second) } - let!(:merge_request_merged) { create(:merge_request, state: "merged", author: user, assignee: user, source_project: project, target_project: project, title: "Merged test", created_at: base_time + 2.seconds, merge_commit_sha: '9999999999999999999999999999999999999999') } - let!(:merge_request_locked) { create(:merge_request, state: "locked", milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Locked test", created_at: base_time + 1.second) } + let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Test", created_at: base_time) } + let!(:merge_request_closed) { create(:merge_request, state: "closed", milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Closed test", created_at: base_time + 1.second) } + let!(:merge_request_merged) { create(:merge_request, state: "merged", author: user, assignees: [user], source_project: project, target_project: project, title: "Merged test", created_at: base_time + 2.seconds, merge_commit_sha: '9999999999999999999999999999999999999999') } + let!(:merge_request_locked) { create(:merge_request, state: "locked", milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Locked test", created_at: base_time + 1.second) } let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") } let!(:note2) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "another comment on a MR") } let(:label) { create(:label, title: 'label', color: '#FFAABB', project: project) } @@ -20,6 +21,9 @@ describe API::MergeRequests do before do project.add_reporter(user) + project.add_reporter(user2) + + stub_licensed_features(multiple_merge_request_assignees: false) end shared_context 'with labels' do @@ -45,9 +49,9 @@ describe API::MergeRequests do get api(endpoint_path, user) end - create(:merge_request, state: 'closed', milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: 'Test', created_at: base_time) + create(:merge_request, state: 'closed', milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: 'Test', created_at: base_time) - merge_request = create(:merge_request, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: 'Test', created_at: base_time) + merge_request = create(:merge_request, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: 'Test', created_at: base_time) merge_request.metrics.update!(merged_by: user, latest_closed_by: user, @@ -333,7 +337,7 @@ describe API::MergeRequests do state: 'closed', milestone: milestone1, author: user, - assignee: user, + assignees: [user], source_project: project, target_project: project, title: "Test", @@ -451,7 +455,7 @@ describe API::MergeRequests do context 'when authenticated' do let!(:project2) { create(:project, :public, namespace: user.namespace) } - let!(:merge_request2) { create(:merge_request, :simple, author: user, assignee: user, source_project: project2, target_project: project2) } + let!(:merge_request2) { create(:merge_request, :simple, author: user, assignees: [user], source_project: project2, target_project: project2) } let(:user2) { create(:user) } it 'returns an array of all merge requests except unauthorized ones' do @@ -494,7 +498,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests created by current user if no scope is given' do - merge_request3 = create(:merge_request, :simple, author: user2, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user2, assignees: [user], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user2) @@ -502,7 +506,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests authored by the given user' do - merge_request3 = create(:merge_request, :simple, author: user2, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user2, assignees: [user], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user), params: { author_id: user2.id, scope: :all } @@ -510,7 +514,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests assigned to the given user' do - merge_request3 = create(:merge_request, :simple, author: user, assignee: user2, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user, assignees: [user2], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user), params: { assignee_id: user2.id, scope: :all } @@ -535,7 +539,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests assigned to me' do - merge_request3 = create(:merge_request, :simple, author: user, assignee: user2, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user, assignees: [user2], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user2), params: { scope: 'assigned_to_me' } @@ -543,7 +547,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests assigned to me (kebab-case)' do - merge_request3 = create(:merge_request, :simple, author: user, assignee: user2, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user, assignees: [user2], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user2), params: { scope: 'assigned-to-me' } @@ -551,7 +555,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests created by me' do - merge_request3 = create(:merge_request, :simple, author: user2, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user2, assignees: [user], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user2), params: { scope: 'created_by_me' } @@ -559,7 +563,7 @@ describe API::MergeRequests do end it 'returns an array of merge requests created by me (kebab-case)' do - merge_request3 = create(:merge_request, :simple, author: user2, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user2, assignees: [user], source_project: project2, target_project: project2, source_branch: 'other-branch') get api('/merge_requests', user2), params: { scope: 'created-by-me' } @@ -567,7 +571,7 @@ describe API::MergeRequests do end it 'returns merge requests reacted by the authenticated user by the given emoji' do - merge_request3 = create(:merge_request, :simple, author: user, assignee: user, source_project: project2, target_project: project2, source_branch: 'other-branch') + merge_request3 = create(:merge_request, :simple, author: user, assignees: [user], source_project: project2, target_project: project2, source_branch: 'other-branch') award_emoji = create(:award_emoji, awardable: merge_request3, user: user2, name: 'star') get api('/merge_requests', user2), params: { my_reaction_emoji: award_emoji.name, scope: 'all' } @@ -700,7 +704,7 @@ describe API::MergeRequests do get api("/projects/#{project.id}/merge_requests", user) end.count - create(:merge_request, author: user, assignee: user, source_project: project, target_project: project, created_at: base_time) + create(:merge_request, author: user, assignees: [user], source_project: project, target_project: project, created_at: base_time) expect do get api("/projects/#{project.id}/merge_requests", user) @@ -730,7 +734,7 @@ describe API::MergeRequests do describe "GET /projects/:id/merge_requests/:merge_request_iid" do it 'matches json schema' do - merge_request = create(:merge_request, :with_test_reports, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) + merge_request = create(:merge_request, :with_test_reports, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, title: "Test", created_at: base_time) get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) expect(response).to have_gitlab_http_status(200) @@ -851,7 +855,7 @@ describe API::MergeRequests do end context 'Work in Progress' do - let!(:merge_request_wip) { create(:merge_request, author: user, assignee: user, source_project: project, target_project: project, title: "WIP: Test", created_at: base_time + 1.second) } + let!(:merge_request_wip) { create(:merge_request, author: user, assignees: [user], source_project: project, target_project: project, title: "WIP: Test", created_at: base_time + 1.second) } it "returns merge request" do get api("/projects/#{project.id}/merge_requests/#{merge_request_wip.iid}", user) @@ -867,7 +871,7 @@ describe API::MergeRequests do merge_request_overflow = create(:merge_request, :simple, author: user, - assignee: user, + assignees: [user], source_project: project, source_branch: 'expand-collapse-files', target_project: project, @@ -1005,6 +1009,71 @@ describe API::MergeRequests do end describe 'POST /projects/:id/merge_requests' do + context 'support for deprecated assignee_id' do + let(:params) do + { + title: 'Test merge request', + source_branch: 'feature_conflict', + target_branch: 'master', + author_id: user.id, + assignee_id: user2.id + } + end + + it 'creates a new merge request' do + post api("/projects/#{project.id}/merge_requests", user), params: params + + expect(response).to have_gitlab_http_status(201) + expect(json_response['title']).to eq('Test merge request') + expect(json_response['assignee']['name']).to eq(user2.name) + expect(json_response['assignees'].first['name']).to eq(user2.name) + end + + it 'creates a new merge request when assignee_id is empty' do + params[:assignee_id] = '' + + post api("/projects/#{project.id}/merge_requests", user), params: params + + expect(response).to have_gitlab_http_status(201) + expect(json_response['title']).to eq('Test merge request') + expect(json_response['assignee']).to be_nil + end + + it 'filters assignee_id of unauthorized user' do + private_project = create(:project, :private, :repository) + another_user = create(:user) + private_project.add_maintainer(user) + params[:assignee_id] = another_user.id + + post api("/projects/#{private_project.id}/merge_requests", user), params: params + + expect(response).to have_gitlab_http_status(201) + expect(json_response['assignee']).to be_nil + end + end + + context 'single assignee restrictions' do + let(:params) do + { + title: 'Test merge request', + source_branch: 'feature_conflict', + target_branch: 'master', + author_id: user.id, + assignee_ids: [user.id, user2.id] + } + end + + it 'creates a new project merge request with no more than one assignee' do + post api("/projects/#{project.id}/merge_requests", user), params: params + + expect(response).to have_gitlab_http_status(201) + expect(json_response['title']).to eq('Test merge request') + expect(json_response['assignees'].count).to eq(1) + expect(json_response['assignees'].first['name']).to eq(user.name) + expect(json_response.dig('assignee', 'name')).to eq(user.name) + end + end + context 'between branches projects' do context 'different labels' do let(:params) do @@ -1574,6 +1643,19 @@ describe API::MergeRequests do expect(json_response['force_remove_source_branch']).to be_truthy end + it 'filters assignee_id of unauthorized user' do + private_project = create(:project, :private, :repository) + mr = create(:merge_request, source_project: private_project, target_project: private_project) + another_user = create(:user) + private_project.add_maintainer(user) + params = { assignee_id: another_user.id } + + put api("/projects/#{private_project.id}/merge_requests/#{mr.iid}", user), params: params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['assignee']).to be_nil + end + context 'when updating labels' do it 'allows special label names' do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), @@ -1728,7 +1810,7 @@ describe API::MergeRequests do issue = create(:issue, project: jira_project) description = "Closes #{ext_issue.to_reference(jira_project)}\ncloses #{issue.to_reference}" merge_request = create(:merge_request, - :simple, author: user, assignee: user, source_project: jira_project, description: description) + :simple, author: user, assignees: [user], source_project: jira_project, description: description) get api("/projects/#{jira_project.id}/merge_requests/#{merge_request.iid}/closes_issues", user) diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index ca366cdf1df..363b7266940 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -76,14 +76,14 @@ describe Issuable::BulkUpdateService do end describe 'updating merge request assignee' do - let(:merge_request) { create(:merge_request, target_project: project, source_project: project, assignee: user) } + let(:merge_request) { create(:merge_request, target_project: project, source_project: project, assignees: [user]) } context 'when the new assignee ID is a valid user' do it 'succeeds' do new_assignee = create(:user) project.add_developer(new_assignee) - result = bulk_update(merge_request, assignee_id: new_assignee.id) + result = bulk_update(merge_request, assignee_ids: [user.id, new_assignee.id]) expect(result[:success]).to be_truthy expect(result[:count]).to eq(1) @@ -93,22 +93,22 @@ describe Issuable::BulkUpdateService do assignee = create(:user) project.add_developer(assignee) - expect { bulk_update(merge_request, assignee_id: assignee.id) } - .to change { merge_request.reload.assignee }.from(user).to(assignee) + expect { bulk_update(merge_request, assignee_ids: [assignee.id]) } + .to change { merge_request.reload.assignee_ids }.from([user.id]).to([assignee.id]) end end context "when the new assignee ID is #{IssuableFinder::NONE}" do it 'unassigns the issues' do - expect { bulk_update(merge_request, assignee_id: IssuableFinder::NONE) } - .to change { merge_request.reload.assignee }.to(nil) + expect { bulk_update(merge_request, assignee_ids: [IssuableFinder::NONE]) } + .to change { merge_request.reload.assignee_ids }.to([]) end end context 'when the new assignee ID is not present' do it 'does not unassign' do - expect { bulk_update(merge_request, assignee_id: nil) } - .not_to change { merge_request.reload.assignee } + expect { bulk_update(merge_request, assignee_ids: []) } + .not_to change { merge_request.reload.assignee_ids } end end end diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb index 8ccbba7fa58..15d1bb73ca3 100644 --- a/spec/services/issuable/destroy_service_spec.rb +++ b/spec/services/issuable/destroy_service_spec.rb @@ -34,7 +34,7 @@ describe Issuable::DestroyService do end context 'when issuable is a merge request' do - let!(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: user, assignee: user) } + let!(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: user, assignees: [user]) } it 'destroys the merge request' do expect { service.execute(merge_request) }.to change { project.merge_requests.count }.by(-1) diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index d37ca13ebd2..91bf4dccd77 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -43,9 +43,9 @@ describe Members::DestroyService do shared_examples 'a service destroying a member with access' do it_behaves_like 'a service destroying a member' - it 'invalidates cached counts for todos and assigned issues and merge requests', :aggregate_failures do + it 'invalidates cached counts for assigned issues and merge requests', :aggregate_failures do create(:issue, project: group_project, assignees: [member_user]) - create(:merge_request, source_project: group_project, assignee: member_user) + create(:merge_request, source_project: group_project, assignees: [member_user]) create(:todo, :pending, project: group_project, user: member_user) create(:todo, :done, project: group_project, user: member_user) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 433ffbd97f0..706bcea8199 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -4,7 +4,7 @@ describe MergeRequests::CloseService do let(:user) { create(:user) } let(:user2) { create(:user) } let(:guest) { create(:user) } - let(:merge_request) { create(:merge_request, assignee: user2, author: create(:user)) } + let(:merge_request) { create(:merge_request, assignees: [user2], author: create(:user)) } let(:project) { merge_request.project } let!(:todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 393299cce00..20bf1cbb8b6 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -118,7 +118,7 @@ describe MergeRequests::CreateFromIssueService do result = service.execute - expect(result[:merge_request].assignee).to eq(user) + expect(result[:merge_request].assignees).to eq([user]) end context 'when ref branch is set' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index dc5d1cf2f04..30271e04c8e 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -32,7 +32,7 @@ describe MergeRequests::CreateService do expect(merge_request).to be_valid expect(merge_request.work_in_progress?).to be(false) expect(merge_request.title).to eq('Awesome merge_request') - expect(merge_request.assignee).to be_nil + expect(merge_request.assignees).to be_empty expect(merge_request.merge_params['force_remove_source_branch']).to eq('1') end @@ -73,7 +73,7 @@ describe MergeRequests::CreateService do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignee: assignee + assignees: [assignee] } end @@ -89,7 +89,7 @@ describe MergeRequests::CreateService do description: "well this is not done yet\n/wip", source_branch: 'feature', target_branch: 'master', - assignee: assignee + assignees: [assignee] } end @@ -106,11 +106,11 @@ describe MergeRequests::CreateService do description: 'please fix', source_branch: 'feature', target_branch: 'master', - assignee: assignee + assignees: [assignee] } end - it { expect(merge_request.assignee).to eq assignee } + it { expect(merge_request.assignees).to eq([assignee]) } it 'creates a todo for new assignee' do attributes = { @@ -301,7 +301,7 @@ describe MergeRequests::CreateService do let(:opts) do { - assignee_id: create(:user).id, + assignee_ids: create(:user).id, milestone_id: 1, title: 'Title', description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"), @@ -317,7 +317,7 @@ describe MergeRequests::CreateService do it 'assigns and sets milestone to issuable from command' do expect(merge_request).to be_persisted - expect(merge_request.assignee).to eq(assignee) + expect(merge_request.assignees).to eq([assignee]) expect(merge_request.milestone).to eq(milestone) end end @@ -332,28 +332,28 @@ describe MergeRequests::CreateService do end it 'removes assignee_id when user id is invalid' do - opts = { title: 'Title', description: 'Description', assignee_id: -1 } + opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } merge_request = described_class.new(project, user, opts).execute - expect(merge_request.assignee_id).to be_nil + expect(merge_request.assignee_ids).to be_empty end it 'removes assignee_id when user id is 0' do - opts = { title: 'Title', description: 'Description', assignee_id: 0 } + opts = { title: 'Title', description: 'Description', assignee_ids: [0] } merge_request = described_class.new(project, user, opts).execute - expect(merge_request.assignee_id).to be_nil + expect(merge_request.assignee_ids).to be_empty end it 'saves assignee when user id is valid' do project.add_maintainer(assignee) - opts = { title: 'Title', description: 'Description', assignee_id: assignee.id } + opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } merge_request = described_class.new(project, user, opts).execute - expect(merge_request.assignee).to eq(assignee) + expect(merge_request.assignees).to eq([assignee]) end context 'when assignee is set' do @@ -361,7 +361,7 @@ describe MergeRequests::CreateService do { title: 'Title', description: 'Description', - assignee_id: assignee.id, + assignee_ids: [assignee.id], source_branch: 'feature', target_branch: 'master' } @@ -387,7 +387,7 @@ describe MergeRequests::CreateService do levels.each do |level| it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do project.update(visibility_level: level) - opts = { title: 'Title', description: 'Description', assignee_id: assignee.id } + opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } merge_request = described_class.new(project, user, opts).execute diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 1430e12a07e..a87d8b8752c 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -7,7 +7,7 @@ describe MergeRequests::FfMergeService do create(:merge_request, source_branch: 'flatten-dir', target_branch: 'improve/awesome', - assignee: user2, + assignees: [user2], author: create(:user)) end let(:project) { merge_request.project } diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 887ec17171e..b0b3273e3dc 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe MergeRequests::MergeService do set(:user) { create(:user) } set(:user2) { create(:user) } - let(:merge_request) { create(:merge_request, :simple, author: user2, assignee: user2) } + let(:merge_request) { create(:merge_request, :simple, author: user2, assignees: [user2]) } let(:project) { merge_request.project } before do @@ -111,7 +111,7 @@ describe MergeRequests::MergeService do end context 'closes related todos' do - let(:merge_request) { create(:merge_request, assignee: user, author: user) } + let(:merge_request) { create(:merge_request, assignees: [user], author: user) } let(:project) { merge_request.project } let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } let!(:todo) do diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index a3b48abae26..24d09c1fd00 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -149,7 +149,7 @@ describe MergeRequests::MergeToRefService do end context 'does not close related todos' do - let(:merge_request) { create(:merge_request, assignee: user, author: user) } + let(:merge_request) { create(:merge_request, assignees: [user], author: user) } let(:project) { merge_request.project } let!(:todo) do create(:todo, :assigned, diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 5ad6f5528f9..2cebefee5d6 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe MergeRequests::PostMergeService do let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, assignee: user) } + let(:merge_request) { create(:merge_request, assignees: [user]) } let(:project) { merge_request.project } before do diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 21e71509ed6..8b6db1ce33e 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -4,7 +4,7 @@ describe MergeRequests::ReopenService do let(:user) { create(:user) } let(:user2) { create(:user) } let(:guest) { create(:user) } - let(:merge_request) { create(:merge_request, :closed, assignee: user2, author: create(:user)) } + let(:merge_request) { create(:merge_request, :closed, assignees: [user2], author: create(:user)) } let(:project) { merge_request.project } before do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 8e367db031c..0525899ebfa 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -13,7 +13,7 @@ describe MergeRequests::UpdateService, :mailer do let(:merge_request) do create(:merge_request, :simple, title: 'Old title', description: "FYI #{user2.to_reference}", - assignee_id: user3.id, + assignee_ids: [user3.id], source_project: project, author: create(:user)) end @@ -48,7 +48,7 @@ describe MergeRequests::UpdateService, :mailer do { title: 'New title', description: 'Also please fix', - assignee_id: user2.id, + assignee_ids: [user.id], state_event: 'close', label_ids: [label.id], target_branch: 'target', @@ -71,7 +71,7 @@ describe MergeRequests::UpdateService, :mailer do it 'matches base expectations' do expect(@merge_request).to be_valid expect(@merge_request.title).to eq('New title') - expect(@merge_request.assignee).to eq(user2) + expect(@merge_request.assignees).to match_array([user]) expect(@merge_request).to be_closed expect(@merge_request.labels.count).to eq(1) expect(@merge_request.labels.first.title).to eq(label.name) @@ -106,7 +106,7 @@ describe MergeRequests::UpdateService, :mailer do note = find_note('assigned to') expect(note).not_to be_nil - expect(note.note).to include "assigned to #{user2.to_reference}" + expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}" end it 'creates a resource label event' do @@ -293,7 +293,7 @@ describe MergeRequests::UpdateService, :mailer do context 'when is reassigned' do before do - update_merge_request({ assignee: user2 }) + update_merge_request({ assignee_ids: [user2.id] }) end it 'marks previous assignee pending todos as done' do @@ -387,7 +387,7 @@ describe MergeRequests::UpdateService, :mailer do context 'when the assignee changes' do it 'updates open merge request counter for assignees when merge request is reassigned' do - update_merge_request(assignee_id: user2.id) + update_merge_request(assignee_ids: [user2.id]) expect(user3.assigned_open_merge_requests_count).to eq 0 expect(user2.assigned_open_merge_requests_count).to eq 1 @@ -541,36 +541,36 @@ describe MergeRequests::UpdateService, :mailer do end end - context 'updating asssignee_id' do + context 'updating asssignee_ids' do it 'does not update assignee when assignee_id is invalid' do - merge_request.update(assignee_id: user.id) + merge_request.update(assignee_ids: [user.id]) - update_merge_request(assignee_id: -1) + update_merge_request(assignee_ids: [-1]) - expect(merge_request.reload.assignee).to eq(user) + expect(merge_request.reload.assignees).to eq([user]) end it 'unassigns assignee when user id is 0' do - merge_request.update(assignee_id: user.id) + merge_request.update(assignee_ids: [user.id]) - update_merge_request(assignee_id: 0) + update_merge_request(assignee_ids: [0]) - expect(merge_request.assignee_id).to be_nil + expect(merge_request.assignee_ids).to be_empty end it 'saves assignee when user id is valid' do - update_merge_request(assignee_id: user.id) + update_merge_request(assignee_ids: [user.id]) - expect(merge_request.assignee_id).to eq(user.id) + expect(merge_request.assignee_ids).to eq([user.id]) end it 'does not update assignee_id when user cannot read issue' do - non_member = create(:user) - original_assignee = merge_request.assignee + non_member = create(:user) + original_assignees = merge_request.assignees - update_merge_request(assignee_id: non_member.id) + update_merge_request(assignee_ids: [non_member.id]) - expect(merge_request.assignee_id).to eq(original_assignee.id) + expect(merge_request.reload.assignees).to eq(original_assignees) end context "when issuable feature is private" do @@ -583,7 +583,7 @@ describe MergeRequests::UpdateService, :mailer do feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level" project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE) - expect { update_merge_request(assignee_id: assignee) }.not_to change { merge_request.assignee } + expect { update_merge_request(assignee_ids: [assignee]) }.not_to change { merge_request.reload.assignees } end end end @@ -619,7 +619,7 @@ describe MergeRequests::UpdateService, :mailer do end it 'is allowed by a user that can push to the source and can update the merge request' do - merge_request.update!(assignee: user) + merge_request.update!(assignees: [user]) source_project.add_developer(user) update_merge_request(allow_collaboration: true, title: 'Updated title') diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 9ba4a11104a..14c73852e65 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -125,11 +125,7 @@ describe NotificationService, :mailer do shared_examples 'participating by assignee notification' do it 'emails the participant' do - if issuable.is_a?(Issue) - issuable.assignees << participant - else - issuable.update_attribute(:assignee, participant) - end + issuable.assignees << participant notification_trigger @@ -620,13 +616,13 @@ describe NotificationService, :mailer do context "merge request diff note" do let(:project) { create(:project, :repository) } let(:user) { create(:user) } - let(:merge_request) { create(:merge_request, source_project: project, assignee: user, author: create(:user)) } + let(:merge_request) { create(:merge_request, source_project: project, assignees: [user], author: create(:user)) } let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } before do build_team(note.project) project.add_maintainer(merge_request.author) - project.add_maintainer(merge_request.assignee) + merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } end describe '#new_note' do @@ -637,7 +633,7 @@ describe NotificationService, :mailer do notification.new_note(note) expect(SentNotification.last(3).map(&:recipient).map(&:id)) - .to contain_exactly(merge_request.assignee.id, merge_request.author.id, @u_watcher.id) + .to contain_exactly(*merge_request.assignees.pluck(:id), merge_request.author.id, @u_watcher.id) expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end end @@ -1223,11 +1219,12 @@ describe NotificationService, :mailer do let(:group) { create(:group) } let(:project) { create(:project, :public, :repository, namespace: group) } let(:another_project) { create(:project, :public, namespace: group) } - let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' } + let(:assignee) { create(:user) } + let(:merge_request) { create :merge_request, source_project: project, assignees: [assignee], description: 'cc @participant' } before do project.add_maintainer(merge_request.author) - project.add_maintainer(merge_request.assignee) + merge_request.assignees.each { |assignee| project.add_maintainer(assignee) } build_team(merge_request.target_project) add_users_with_subscription(merge_request.target_project, merge_request) update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) @@ -1239,7 +1236,7 @@ describe NotificationService, :mailer do it do notification.new_merge_request(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_watcher) should_email(@watcher_and_subscriber) should_email(@u_participant_mentioned) @@ -1254,9 +1251,11 @@ describe NotificationService, :mailer do it 'adds "assigned" reason for assignee, if any' do notification.new_merge_request(merge_request, @u_disabled) - email = find_email_for(merge_request.assignee) + merge_request.assignees.each do |assignee| + email = find_email_for(assignee) - expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end end it "emails any mentioned users with the mention level" do @@ -1347,9 +1346,9 @@ describe NotificationService, :mailer do end it do - notification.reassigned_merge_request(merge_request, current_user, merge_request.author) + notification.reassigned_merge_request(merge_request, current_user, [assignee]) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(merge_request.author) should_email(@u_watcher) should_email(@u_participant_mentioned) @@ -1365,17 +1364,19 @@ describe NotificationService, :mailer do end it 'adds "assigned" reason for new assignee' do - notification.reassigned_merge_request(merge_request, current_user, merge_request.author) + notification.reassigned_merge_request(merge_request, current_user, [assignee]) - email = find_email_for(merge_request.assignee) + merge_request.assignees.each do |assignee| + email = find_email_for(assignee) - expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::ASSIGNED) + end end it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { merge_request } - let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, merge_request.author) } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } end end @@ -1388,7 +1389,7 @@ describe NotificationService, :mailer do it do notification.push_to_merge_request(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_guest_custom) should_email(@u_custom_global) should_email(@u_participant_mentioned) @@ -1430,7 +1431,7 @@ describe NotificationService, :mailer do should_email(subscriber_1_to_group_label_2) should_email(subscriber_2_to_group_label_2) should_email(subscriber_to_label_2) - should_not_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_not_email(assignee) } should_not_email(merge_request.author) should_not_email(@u_watcher) should_not_email(@u_participant_mentioned) @@ -1499,7 +1500,7 @@ describe NotificationService, :mailer do it do notification.close_mr(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -1529,7 +1530,7 @@ describe NotificationService, :mailer do it do notification.merge_mr(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_watcher) should_email(@u_guest_watcher) should_email(@u_guest_custom) @@ -1581,7 +1582,7 @@ describe NotificationService, :mailer do it do notification.reopen_mr(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_watcher) should_email(@u_participant_mentioned) should_email(@subscriber) @@ -1606,7 +1607,7 @@ describe NotificationService, :mailer do it do notification.resolve_all_discussions(merge_request, @u_disabled) - should_email(merge_request.assignee) + merge_request.assignees.each { |assignee| should_email(assignee) } should_email(@u_watcher) should_email(@u_participant_mentioned) should_email(@subscriber) @@ -1850,8 +1851,8 @@ describe NotificationService, :mailer do let(:guest) { create(:user) } let(:developer) { create(:user) } let(:assignee) { create(:user) } - let(:merge_request) { create(:merge_request, source_project: private_project, assignee: assignee) } - let(:merge_request1) { create(:merge_request, source_project: private_project, assignee: assignee, description: "cc @#{guest.username}") } + let(:merge_request) { create(:merge_request, source_project: private_project, assignees: [assignee]) } + let(:merge_request1) { create(:merge_request, source_project: private_project, assignees: [assignee], description: "cc @#{guest.username}") } let(:note) { create(:note, noteable: merge_request, project: private_project) } before do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index c7e5cca324f..c450f89c3cb 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -16,7 +16,9 @@ describe QuickActions::InterpretService do let(:service) { described_class.new(project, developer) } before do - stub_licensed_features(multiple_issue_assignees: false) + stub_licensed_features(multiple_issue_assignees: false, + multiple_merge_request_assignees: false) + project.add_developer(developer) end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 13d7d795703..51c5a803dbd 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -166,8 +166,8 @@ describe SystemNoteService do end end - describe '.change_issue_assignees' do - subject { described_class.change_issue_assignees(noteable, project, author, [assignee]) } + describe '.change_issuable_assignees' do + subject { described_class.change_issuable_assignees(noteable, project, author, [assignee]) } let(:assignee) { create(:user) } let(:assignee1) { create(:user) } @@ -180,7 +180,7 @@ describe SystemNoteService do def build_note(old_assignees, new_assignees) issue.assignees = new_assignees - described_class.change_issue_assignees(issue, project, author, old_assignees).note + described_class.change_issuable_assignees(issue, project, author, old_assignees).note end it_behaves_like 'a note with overridable created_at' diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 8631f3f9a33..89411b2e908 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -272,28 +272,6 @@ describe TodoService do end end - describe '#reassigned_issue' do - it 'creates a pending todo for new assignee' do - unassigned_issue.assignees << john_doe - service.reassigned_issue(unassigned_issue, author) - - should_create_todo(user: john_doe, target: unassigned_issue, action: Todo::ASSIGNED) - end - - it 'does not create a todo if unassigned' do - issue.assignees.destroy_all # rubocop: disable DestroyAll - - should_not_create_any_todo { service.reassigned_issue(issue, author) } - end - - it 'creates a todo if new assignee is the current user' do - unassigned_issue.assignees << john_doe - service.reassigned_issue(unassigned_issue, john_doe) - - should_create_todo(user: john_doe, target: unassigned_issue, author: john_doe, action: Todo::ASSIGNED) - end - end - describe '#mark_pending_todos_as_done' do it 'marks related pending todos to the target for the user as done' do first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) @@ -504,10 +482,60 @@ describe TodoService do end end + describe '#reassigned_issuable' do + shared_examples 'reassigned issuable' do + it 'creates a pending todo for new assignee' do + issuable_unassigned.assignees = [john_doe] + service.reassigned_issuable(issuable_unassigned, author) + + should_create_todo(user: john_doe, target: issuable_unassigned, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + issuable_assigned.assignees = [] + + should_not_create_any_todo { service.reassigned_issuable(issuable_assigned, author) } + end + + it 'creates a todo if new assignee is the current user' do + issuable_assigned.assignees = [john_doe] + service.reassigned_issuable(issuable_assigned, john_doe) + + should_create_todo(user: john_doe, target: issuable_assigned, author: john_doe, action: Todo::ASSIGNED) + end + + it 'does not create a todo for guests' do + service.reassigned_issuable(issuable_assigned, author) + should_not_create_todo(user: guest, target: issuable_assigned, action: Todo::MENTIONED) + end + + it 'does not create a directly addressed todo for guests' do + service.reassigned_issuable(addressed_issuable_assigned, author) + should_not_create_todo(user: guest, target: addressed_issuable_assigned, action: Todo::DIRECTLY_ADDRESSED) + end + end + + context 'issuable is a merge request' do + it_behaves_like 'reassigned issuable' do + let(:issuable_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_issuable_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:issuable_unassigned) { create(:merge_request, source_project: project, author: author, assignees: []) } + end + end + + context 'issuable is an issue' do + it_behaves_like 'reassigned issuable' do + let(:issuable_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_issuable_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:issuable_unassigned) { create(:issue, project: project, author: author, assignees: []) } + end + end + end + describe 'Merge Requests' do - let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } - let(:addressed_mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } - let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } + let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } + let(:addressed_mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } + let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignees: []) } describe '#new_merge_request' do it 'creates a pending todo if assigned' do @@ -659,38 +687,6 @@ describe TodoService do end end - describe '#reassigned_merge_request' do - it 'creates a pending todo for new assignee' do - mr_unassigned.update_attribute(:assignee, john_doe) - service.reassigned_merge_request(mr_unassigned, author) - - should_create_todo(user: john_doe, target: mr_unassigned, action: Todo::ASSIGNED) - end - - it 'does not create a todo if unassigned' do - mr_assigned.update_attribute(:assignee, nil) - - should_not_create_any_todo { service.reassigned_merge_request(mr_assigned, author) } - end - - it 'creates a todo if new assignee is the current user' do - mr_assigned.update_attribute(:assignee, john_doe) - service.reassigned_merge_request(mr_assigned, john_doe) - - should_create_todo(user: john_doe, target: mr_assigned, author: john_doe, action: Todo::ASSIGNED) - end - - it 'does not create a todo for guests' do - service.reassigned_merge_request(mr_assigned, author) - should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED) - end - - it 'does not create a directly addressed todo for guests' do - service.reassigned_merge_request(addressed_mr_assigned, author) - should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) - end - end - describe '#merge_merge_request' do it 'marks related pending todos to the target for the user as done' do first_todo = create(:todo, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 83f1495a1c6..450e76d5f58 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -78,7 +78,7 @@ describe Users::DestroyService do end context "for an merge request the user was assigned to" do - let!(:merge_request) { create(:merge_request, source_project: project, assignee: user) } + let!(:merge_request) { create(:merge_request, source_project: project, assignees: [user]) } before do service.execute(user) @@ -91,7 +91,7 @@ describe Users::DestroyService do it 'migrates the merge request so that it is "Unassigned"' do migrated_merge_request = MergeRequest.find_by_id(merge_request.id) - expect(migrated_merge_request.assignee).to be_nil + expect(migrated_merge_request.assignees).to be_empty end end end diff --git a/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb b/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb index 4df80b4168a..ab6687f1d07 100644 --- a/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb +++ b/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb @@ -46,9 +46,9 @@ RSpec.shared_context 'MergeRequestsFinder multiple projects with merge requests allow_gitaly_n_plus_1 { create(:project, group: subgroup) } end - let!(:merge_request1) { create(:merge_request, author: user, source_project: project2, target_project: project1, target_branch: 'merged-target') } - let!(:merge_request2) { create(:merge_request, :conflict, author: user, source_project: project2, target_project: project1, state: 'closed') } - let!(:merge_request3) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project2, state: 'locked', title: 'thing WIP thing') } + let!(:merge_request1) { create(:merge_request, assignees: [user], author: user, source_project: project2, target_project: project1, target_branch: 'merged-target') } + let!(:merge_request2) { create(:merge_request, :conflict, assignees: [user], author: user, source_project: project2, target_project: project1, state: 'closed') } + let!(:merge_request3) { create(:merge_request, :simple, author: user, assignees: [user2], source_project: project2, target_project: project2, state: 'locked', title: 'thing WIP thing') } let!(:merge_request4) { create(:merge_request, :simple, author: user, source_project: project3, target_project: project3, title: 'WIP thing') } let!(:merge_request5) { create(:merge_request, :simple, author: user, source_project: project4, target_project: project4, title: '[WIP]') } diff --git a/spec/support/shared_contexts/merge_request_create.rb b/spec/support/shared_contexts/merge_request_create.rb new file mode 100644 index 00000000000..529f481c2b6 --- /dev/null +++ b/spec/support/shared_contexts/merge_request_create.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +shared_context 'merge request create context' do + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:target_project) { create(:project, :public, :repository) } + let(:source_project) { target_project } + let!(:milestone) { create(:milestone, project: target_project) } + let!(:label) { create(:label, project: target_project) } + let!(:label2) { create(:label, project: target_project) } + + before do + source_project.add_maintainer(user) + target_project.add_maintainer(user) + target_project.add_maintainer(user2) + + sign_in(user) + visit project_new_merge_request_path(target_project, + merge_request: { + source_project_id: source_project.id, + target_project_id: target_project.id, + source_branch: 'fix', + target_branch: 'master' + }) + end +end diff --git a/spec/support/shared_contexts/merge_request_edit.rb b/spec/support/shared_contexts/merge_request_edit.rb new file mode 100644 index 00000000000..c84510ff47d --- /dev/null +++ b/spec/support/shared_contexts/merge_request_edit.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +shared_context 'merge request edit context' do + let(:user) { create(:user) } + let(:user2) { create(:user) } + let!(:milestone) { create(:milestone, project: target_project) } + let!(:label) { create(:label, project: target_project) } + let!(:label2) { create(:label, project: target_project) } + let(:target_project) { create(:project, :public, :repository) } + let(:source_project) { target_project } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fix', + target_branch: 'master') + end + + before do + source_project.add_maintainer(user) + target_project.add_maintainer(user) + target_project.add_maintainer(user2) + + sign_in(user) + visit edit_project_merge_request_path(target_project, merge_request) + end +end diff --git a/spec/support/shared_examples/features/creatable_merge_request_shared_examples.rb b/spec/support/shared_examples/features/creatable_merge_request_shared_examples.rb index 7038a366144..ec1b1754cf0 100644 --- a/spec/support/shared_examples/features/creatable_merge_request_shared_examples.rb +++ b/spec/support/shared_examples/features/creatable_merge_request_shared_examples.rb @@ -1,42 +1,17 @@ RSpec.shared_examples 'a creatable merge request' do include WaitForRequests - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:target_project) { create(:project, :public, :repository) } - let(:source_project) { target_project } - let!(:milestone) { create(:milestone, project: target_project) } - let!(:label) { create(:label, project: target_project) } - let!(:label2) { create(:label, project: target_project) } - - before do - source_project.add_maintainer(user) - target_project.add_maintainer(user) - target_project.add_maintainer(user2) - - sign_in(user) - visit project_new_merge_request_path( - target_project, - merge_request: { - source_project_id: source_project.id, - target_project_id: target_project.id, - source_branch: 'fix', - target_branch: 'master' - }) - end - it 'creates new merge request', :js do - click_button 'Assignee' + find('.js-assignee-search').click page.within '.dropdown-menu-user' do click_link user2.name end - expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user2.id.to_s) + expect(find('input[name="merge_request[assignee_ids][]"]', visible: false).value).to match(user2.id.to_s) page.within '.js-assignee-search' do expect(page).to have_content user2.name end - click_link 'Assign to me' - expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s) + expect(find('input[name="merge_request[assignee_ids][]"]', visible: false).value).to match(user.id.to_s) page.within '.js-assignee-search' do expect(page).to have_content user.name end diff --git a/spec/support/shared_examples/features/editable_merge_request_shared_examples.rb b/spec/support/shared_examples/features/editable_merge_request_shared_examples.rb index eef0327c9a6..a6121fcc50a 100644 --- a/spec/support/shared_examples/features/editable_merge_request_shared_examples.rb +++ b/spec/support/shared_examples/features/editable_merge_request_shared_examples.rb @@ -1,34 +1,10 @@ RSpec.shared_examples 'an editable merge request' do - let(:user) { create(:user) } - let(:user2) { create(:user) } - let!(:milestone) { create(:milestone, project: target_project) } - let!(:label) { create(:label, project: target_project) } - let!(:label2) { create(:label, project: target_project) } - let(:target_project) { create(:project, :public, :repository) } - let(:source_project) { target_project } - let(:merge_request) do - create(:merge_request, - source_project: source_project, - target_project: target_project, - source_branch: 'fix', - target_branch: 'master') - end - - before do - source_project.add_maintainer(user) - target_project.add_maintainer(user) - target_project.add_maintainer(user2) - - sign_in(user) - visit edit_project_merge_request_path(target_project, merge_request) - end - it 'updates merge request', :js do - click_button 'Assignee' + find('.js-assignee-search').click page.within '.dropdown-menu-user' do click_link user.name end - expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s) + expect(find('input[name="merge_request[assignee_ids][]"]', visible: false).value).to match(user.id.to_s) page.within '.js-assignee-search' do expect(page).to have_content user.name end diff --git a/spec/support/shared_examples/features/multiple_assignees_mr_shared_examples.rb b/spec/support/shared_examples/features/multiple_assignees_mr_shared_examples.rb new file mode 100644 index 00000000000..bab7963f06f --- /dev/null +++ b/spec/support/shared_examples/features/multiple_assignees_mr_shared_examples.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +shared_examples 'multiple assignees merge request' do |action, save_button_title| + it "#{action} a MR with multiple assignees", :js do + find('.js-assignee-search').click + page.within '.dropdown-menu-user' do + click_link user.name + click_link user2.name + end + + # Extra click needed in order to toggle the dropdown + find('.js-assignee-search').click + + expect(all('input[name="merge_request[assignee_ids][]"]', visible: false).map(&:value)) + .to match_array([user.id.to_s, user2.id.to_s]) + + page.within '.js-assignee-search' do + expect(page).to have_content "#{user2.name} + 1 more" + end + + click_button save_button_title + + page.within '.issuable-sidebar' do + page.within '.assignee' do + expect(page).to have_content '2 Assignees' + + click_link 'Edit' + + expect(page).to have_content user.name + expect(page).to have_content user2.name + end + end + + page.within '.dropdown-menu-user' do + click_link user.name + end + + page.within '.issuable-sidebar' do + page.within '.assignee' do + # Closing dropdown to persist + click_link 'Edit' + + expect(page).to have_content user2.name + end + end + end +end diff --git a/spec/support/shared_examples/finders/assignees_filter_spec.rb b/spec/support/shared_examples/finders/assignees_filter_spec.rb new file mode 100644 index 00000000000..782a2d97746 --- /dev/null +++ b/spec/support/shared_examples/finders/assignees_filter_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +shared_examples 'assignee ID filter' do + it 'returns issuables assigned to that user' do + expect(issuables).to contain_exactly(*expected_issuables) + end +end + +shared_examples 'assignee username filter' do + it 'returns issuables assigned to those users' do + expect(issuables).to contain_exactly(*expected_issuables) + end +end + +shared_examples 'no assignee filter' do + let(:params) { { assignee_id: 'None' } } + + it 'returns issuables not assigned to any assignee' do + expect(issuables).to contain_exactly(*expected_issuables) + end + + it 'returns issuables not assigned to any assignee' do + params[:assignee_id] = 0 + + expect(issuables).to contain_exactly(*expected_issuables) + end + + it 'returns issuables not assigned to any assignee' do + params[:assignee_id] = 'none' + + expect(issuables).to contain_exactly(*expected_issuables) + end +end + +shared_examples 'any assignee filter' do + context '' do + let(:params) { { assignee_id: 'Any' } } + + it 'returns issuables assigned to any assignee' do + expect(issuables).to contain_exactly(*expected_issuables) + end + + it 'returns issuables assigned to any assignee' do + params[:assignee_id] = 'any' + + expect(issuables).to contain_exactly(*expected_issuables) + end + end +end diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb index c13eab30054..529afa03f9c 100644 --- a/spec/views/projects/merge_requests/edit.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/edit.html.haml_spec.rb @@ -17,7 +17,7 @@ describe 'projects/merge_requests/edit.html.haml' do source_project: forked_project, target_project: project, author: user, - assignee: user, + assignees: [user], milestone: milestone) end @@ -40,7 +40,7 @@ describe 'projects/merge_requests/edit.html.haml' do expect(rendered).to have_field('merge_request[title]') expect(rendered).to have_field('merge_request[description]') - expect(rendered).to have_selector('#merge_request_assignee_id', visible: false) + expect(rendered).to have_selector('input[name="merge_request[label_ids][]"]', visible: false) expect(rendered).to have_selector('#merge_request_milestone_id', visible: false) expect(rendered).not_to have_selector('#merge_request_target_branch', visible: false) end @@ -52,7 +52,7 @@ describe 'projects/merge_requests/edit.html.haml' do expect(rendered).to have_field('merge_request[title]') expect(rendered).to have_field('merge_request[description]') - expect(rendered).to have_selector('#merge_request_assignee_id', visible: false) + expect(rendered).to have_selector('input[name="merge_request[label_ids][]"]', visible: false) expect(rendered).to have_selector('#merge_request_milestone_id', visible: false) expect(rendered).to have_selector('#merge_request_target_branch', visible: false) end diff --git a/spec/views/projects/merge_requests/show.html.haml_spec.rb b/spec/views/projects/merge_requests/show.html.haml_spec.rb index d9bda1a3414..23cb319a202 100644 --- a/spec/views/projects/merge_requests/show.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/show.html.haml_spec.rb @@ -53,19 +53,6 @@ describe 'projects/merge_requests/show.html.haml' do expect(rendered).not_to have_css('.cannot-be-merged') end end - - context 'when assignee is not allowed to merge' do - it 'shows a warning icon' do - reporter = create(:user) - project.add_reporter(reporter) - closed_merge_request.update(assignee_id: reporter.id) - assign(:issuable_sidebar, serialize_issuable_sidebar(user, project, closed_merge_request)) - - render - - expect(rendered).to have_css('.cannot-be-merged') - end - end end context 'when the merge request is closed' do |