Commit a6218f1b authored by Nick Thomas's avatar Nick Thomas

Merge branch 'osw-multi-assignees-merge-requests' into 'master'

[Backport] Support multiple assignees for merge requests

See merge request gitlab-org/gitlab-ce!27089
parents 12e35b49 ca884980
......@@ -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(),
......
......@@ -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
......
......@@ -498,6 +498,16 @@
flex: 1;
}
.issuable-meta {
.author-link {
display: inline-block;
}
.issuable-comments {
height: 18px;
}
}
.merge-request-title {
margin-bottom: 2px;
......
......@@ -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 {
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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?
......
......@@ -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
......@@ -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',
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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,
......
# 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
......@@ -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
......
......@@ -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}"
......
......@@ -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)
......
......@@ -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
......
......@@ -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
# frozen_string_literal: true
class IssueSidebarExtrasEntity < IssuableSidebarExtrasEntity
expose :assignees, using: API::Entities::UserBasic
end
# frozen_string_literal: true
class MergeRequestAssigneeEntity < ::API::Entities::UserBasic
expose :can_merge do |assignee, options|