Merge branch 'multiple_assignees_review' into 'multiple-assignees-fe-sidebar'

# Conflicts:
#   app/models/issue.rb

app/controllers/projects/issues_controller.rb

@@ -269,8 +269,8 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def issue_params
     params.require(:issue).permit(
-      :title, :assignee_id, :position, :description, :confidential, :weight,
-      :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
+      :title, :position, :description, :confidential, :weight,
+      :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [], assignee_ids: [],
     )
   end
 end

app/finders/issuable_finder.rb

@@ -231,17 +231,6 @@ class IssuableFinder
     klass.all
   end
 
-  def by_scope(items)
-    case params[:scope]
-    when 'created-by-me', 'authored'
-      items.where(author_id: current_user.id)
-    when 'assigned-to-me'
-      items.where(assignee_id: current_user.id)
-    else
-      items
-    end
-  end
-
   def by_state(items)
     case params[:state].to_s
     when 'closed'

app/finders/issues_finder.rb

@@ -38,6 +38,17 @@ class IssuesFinder < IssuableFinder
     items
   end
 
+  def by_scope(items)
+    case params[:scope]
+    when 'created-by-me', 'authored'
+      items.where(author_id: current_user.id)
+    when 'assigned-to-me'
+      items.where("issue_assignees.user_id = ?", current_user.id)
+    else
+      items
+    end
+  end
+
   def self.not_restricted_by_confidentiality(user)
     issues = Issue.with_assignees
 

app/finders/merge_requests_finder.rb

@@ -23,6 +23,17 @@ class MergeRequestsFinder < IssuableFinder
 
   private
 
+  def by_scope(items)
+    case params[:scope]
+    when 'created-by-me', 'authored'
+      items.where(author_id: current_user.id)
+    when 'assigned-to-me'
+      items.where(assignee_id: current_user.id)
+    else
+      items
+    end
+  end
+
   def item_project_ids(items)
     items&.reorder(nil)&.select(:target_project_id)
   end

app/models/concerns/issuable.rb

@@ -63,11 +63,8 @@ module Issuable
     validates :title, presence: true, length: { maximum: 255 }
 
     scope :authored, ->(user) { where(author_id: user) }
-    scope :assigned_to, ->(u) { where(assignee_id: u.id)}
     scope :recent, -> { reorder(id: :desc) }
     scope :order_position_asc, -> { reorder(position: :asc) }
-    scope :assigned, -> { where("assignee_id IS NOT NULL") }
-    scope :unassigned, -> { where("assignee_id IS NULL") }
     scope :of_projects, ->(ids) { where(project_id: ids) }
     scope :of_milestones, ->(ids) { where(milestone_id: ids) }
     scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) }
@@ -99,16 +96,8 @@ module Issuable
 
     acts_as_paranoid
 
-    after_save :update_assignee_cache_counts, if: :assignee_id_changed?
     after_save :record_metrics
 
-    def update_assignee_cache_counts
-      # make sure we flush the cache for both the old *and* new assignees(if they exist)
-      previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
-      previous_assignee&.update_cache_counts
-      assignee&.update_cache_counts
-    end
-
     # We want to use optimistic lock for cases when only title or description are involved
     # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
     def locking_enabled?
@@ -245,10 +234,6 @@ module Issuable
     today? && created_at == updated_at
   end
 
-  def is_being_reassigned?
-    assignee_id_changed?
-  end
-
   def open?
     opened? || reopened?
   end

app/models/issue.rb

@@ -29,14 +29,18 @@ class Issue < ActiveRecord::Base
 
   has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues', dependent: :delete_all
 
-  has_and_belongs_to_many :assignees, class_name: "User", join_table: :issue_assignees
+  has_many :issue_assignees
+  has_many :assignees, class_name: "User", through: :issue_assignees
 
   validates :project, presence: true
 
   scope :cared, ->(user) { with_assignees.where("issue_assignees.user_id IN(?)", user.id) }
   scope :open_for, ->(user) { opened.assigned_to(user) }
   scope :in_projects, ->(project_ids) { where(project_id: project_ids) }
-  scope :with_assignees, -> { joins('LEFT JOIN issue_assignees ON issue_id = issues.id') }
+  scope :with_assignees, -> { joins("LEFT JOIN issue_assignees ON issue_id = issues.id") }
+  scope :assigned, -> { with_assignees.where('issue_assignees.user_id IS NOT NULL') }
+  scope :unassigned, -> { with_assignees.where('issue_assignees.user_id IS NULL') }
+  scope :assigned_to, ->(u) { with_assignees.where('issue_assignees.user_id = ?', u.id)}
 
   scope :without_due_date, -> { where(due_date: nil) }
   scope :due_before, ->(date) { where('issues.due_date < ?', date) }
@@ -49,7 +53,7 @@ class Issue < ActiveRecord::Base
 
   scope :created_after, -> (datetime) { where("created_at >= ?", datetime) }
 
-  scope :include_associations, -> { includes(:assignee, :labels, project: :namespace) }
+  scope :include_associations, -> { includes(:labels, project: :namespace) }
 
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true
@@ -132,6 +136,14 @@ class Issue < ActiveRecord::Base
               "id DESC")
   end
 
+  def update_assignee_cache_counts
+    return true # TODO implement it properly
+    # make sure we flush the cache for both the old *and* new assignees(if they exist)
+    previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
+    previous_assignee&.update_cache_counts
+    assignee&.update_cache_counts
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
@@ -148,13 +160,6 @@ class Issue < ActiveRecord::Base
     assignees.map(&:name).to_sentence
   end
 
-  # TODO: This method will help us to find some silent failures.
-  # We should remove it before merging to master
-  def assignee_id
-    return ''
-    raise "assignee_id is deprecated"
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
@@ -302,4 +307,4 @@ class Issue < ActiveRecord::Base
   def publicly_visible?
     project.public? && !confidential?
   end
-end
+end
\ No newline at end of file

app/models/issue_assignee.rb

+class IssueAssignee < ActiveRecord::Base
+	belongs_to :issue
+	belongs_to :assignee, class_name: "User", foreign_key: :user_id
+end
\ No newline at end of file

app/models/merge_request.rb

@@ -121,11 +121,15 @@ class MergeRequest < ActiveRecord::Base
   scope :from_source_branches, ->(branches) { where(source_branch: branches) }
   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)}
 
   participant :approvers_left
   participant :assignee
 
   after_save :keep_around_commit
+  after_save :update_assignee_cache_counts, if: :assignee_id_changed?
 
   def self.reference_prefix
     '!'
@@ -185,6 +189,17 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def is_being_reassigned?
+    assignee_id_changed?
+  end
+
+  def update_assignee_cache_counts
+    # make sure we flush the cache for both the old *and* new assignees(if they exist)
+    previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
+    previous_assignee&.update_cache_counts
+    assignee&.update_cache_counts
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
@@ -193,9 +208,9 @@ class MergeRequest < ActiveRecord::Base
     }
   end
 
-  # This method is needed for compatibility with issues
+  # This method is needed for compatibility with issues to not mess view and other code
   def assignees
-    [assignee]
+    assignee ? [assignee] : []
   end
 
   def assignee_or_author?(user)

app/models/milestone.rb

@@ -23,7 +23,6 @@ class Milestone < ActiveRecord::Base
   has_many :issues
   has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues
   has_many :merge_requests
-  has_many :participants, -> { distinct.reorder('users.name') }, through: :issues, source: :assignee
   has_many :events, as: :target, dependent: :destroy
 
   scope :active, -> { with_state(:active) }
@@ -109,6 +108,10 @@ class Milestone < ActiveRecord::Base
     end
   end
 
+  def participants
+    User.joins(assigned_issues: :milestone).where("milestones.id = ?", id)
+  end
+
   ##
   # Returns the String necessary to reference this Milestone in Markdown
   #

app/models/user.rb

@@ -88,9 +88,7 @@ class User < ActiveRecord::Base
   has_many :merge_requests,           dependent: :destroy, foreign_key: :author_id
   has_many :events,                   dependent: :destroy, foreign_key: :author_id
   has_many :subscriptions,            dependent: :destroy
-  has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id,   class_name: "Event"
-  has_many :assigned_issues,          dependent: :destroy, foreign_key: :assignee_id, class_name: "Issue"
-  has_many :assigned_merge_requests,  dependent: :destroy, foreign_key: :assignee_id, class_name: "MergeRequest"
+  has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id, class_name: "Event"
   has_many :oauth_applications,       class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy
   has_many :approvals,                dependent: :destroy
   has_many :approvers,                dependent: :destroy
@@ -108,7 +106,8 @@ class User < ActiveRecord::Base
   has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ProtectedBranch::PushAccessLevel
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id
 
-  has_many :assigned_issues,          dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
+  has_many :issue_assignees, dependent: :destroy
+  has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
 
   # Issues that a user owns are expected to be moved to the "ghost" user before

app/services/issues/base_service.rb

@@ -22,15 +22,9 @@ module Issues
     end
 
     def filter_assignee(issuable)
-      return if params[:assignee_ids].blank?
+      return if params[:assignee_ids].to_a.empty?
 
-      assignee_ids = params[:assignee_ids].split(',').map(&:strip)
-
-      if assignee_ids == [ IssuableFinder::NONE ]
-        params[:assignee_ids] = ""
-      else
-        params.delete(:assignee_ids) unless assignee_ids.all?{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
-      end
+      params[:assignee_ids] = params[:assignee_ids].select{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
     end
   end
 end

app/services/issues/update_service.rb

@@ -22,7 +22,7 @@ module Issues
       end
 
       if issue.previous_changes.include?('title') ||
-          issue.previous_changes.include?('description')
+        issue.previous_changes.include?('description')
         todo_service.update_issue(issue, current_user)
       end
 
@@ -30,7 +30,7 @@ module Issues
         create_milestone_note(issue)
       end
 
-      if issue.previous_changes.include?('assignee_ids')
+      if issue.assignees != old_assignees
         create_assignee_note(issue)
         notification_service.reassigned_issue(issue, current_user, old_assignees)
         todo_service.reassigned_issue(issue, current_user)

app/views/shared/milestones/_issuable.html.haml

 -# @project is present when viewing Project's milestone
 - project = @project || issuable.project
-- assignee = issuable.assignee
+- assignees = issuable.assignees
 - issuable_type = issuable.class.table_name
 - base_url_args = [project.namespace.becomes(Namespace), project, issuable_type]
 - can_update = can?(current_user, :"update_#{issuable.to_ability_name}", issuable)
@@ -23,7 +23,7 @@
         - render_colored_label(label)
 
     %span.assignee-icon
-      - if assignee
-        = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }),
+      - assignees.each do |assignee|
+        = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: assignee.id, state: 'all' }),
                   class: 'has-tooltip', title: "Assigned to #{assignee.name}", data: { container: 'body' } do
-          - image_tag(avatar_icon(issuable.assignee, 16), class: "avatar s16", alt: '')
+          - image_tag(avatar_icon(assignee, 16), class: "avatar s16", alt: '')

spec/controllers/projects/issues_controller_spec.rb

@@ -161,11 +161,11 @@ describe Projects::IssuesController do
           namespace_id: project.namespace.to_param,
           project_id: project,
           id: issue.iid,
-          issue: { assignee_id: assignee.id },
+          issue: { assignee_ids: [assignee.id] },
           format: :json
         body = JSON.parse(response.body)
 
-        expect(body['assignee'].keys)
+        expect(body['assignees'].first.keys)
           .to match_array(%w(name username avatar_url))
       end
     end

spec/services/issues/update_service_spec.rb

@@ -13,7 +13,7 @@ describe Issues::UpdateService, services: true do
 
   let(:issue) do
     create(:issue, title: 'Old title',
-                   assignee_id: user3.id,
+                   assignees: [user3],
                    project: project)
   end
 
@@ -39,7 +39,7 @@ describe Issues::UpdateService, services: true do
         {
           title: 'New title',
           description: 'Also please fix',
-          assignee_id: user2.id,
+          assignee_ids: [user2.id, user3.id],
           state_event: 'close',
           label_ids: [label.id],
           due_date: Date.tomorrow
@@ -52,15 +52,15 @@ describe Issues::UpdateService, services: true do
         expect(issue).to be_valid
         expect(issue.title).to eq 'New title'
         expect(issue.description).to eq 'Also please fix'
-        expect(issue.assignee).to eq user2
+        expect(issue.assignees).to match_array([user2, user3])
         expect(issue).to be_closed
         expect(issue.labels).to match_array [label]
         expect(issue.due_date).to eq Date.tomorrow
       end
 
       it 'sorts issues as specified by parameters' do
-        issue1 = create(:issue, project: project, assignee_id: user3.id)
-        issue2 = create(:issue, project: project, assignee_id: user3.id)
+        issue1 = create(:issue, project: project, assignees: [user3])
+        issue2 = create(:issue, project: project, assignees: [user3])
 
         [issue, issue1, issue2].each do |issue|
           issue.move_to_end
@@ -86,7 +86,7 @@ describe Issues::UpdateService, services: true do
           expect(issue).to be_valid
           expect(issue.title).to eq 'New title'
           expect(issue.description).to eq 'Also please fix'
-          expect(issue.assignee).to eq user3
+          expect(issue.assignees).to match_array [user3]
           expect(issue.labels).to be_empty
           expect(issue.milestone).to be_nil
           expect(issue.due_date).to be_nil
@@ -136,7 +136,7 @@ describe Issues::UpdateService, services: true do
         {
           title: 'New title',
           description: 'Also please fix',
-          assignee_id: user2.id,
+          assignee_ids: [user2],
           state_event: 'close',
           label_ids: [label.id],
           confidential: true
@@ -163,11 +163,11 @@ describe Issues::UpdateService, services: true do
         project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
         update_issue(confidential: true)
         non_member        = create(:user)
-        original_assignee = issue.assignee
+        original_assignees = issue.assignees
 
-        update_issue(assignee_id: non_member.id)
+        update_issue(assignee_ids: non_member.id.to_s)
 
-        expect(issue.reload.assignee_id).to eq(original_assignee.id)
+        expect(issue.reload.assignees).to eq(original_assignees)
       end
     end
 
@@ -399,6 +399,41 @@ describe Issues::UpdateService, services: true do
       end
     end
 
+    context 'updating asssignee_id' do
+      it 'does not update assignee when assignee_id is invalid' do
+        update_issue(assignee_ids: [-1])
+
+        expect(issue.reload.assignees).to eq([user3])
+      end
+
+      it 'unassigns assignee when user id is 0' do
+        update_issue(assignee_ids: [0])
+
+        expect(issue.reload.assignees).to be_empty
+      end
+
+      it 'does not update assignee_id when user cannot read issue' do
+        update_issue(assignee_ids: [create(:user).id])
+
+        expect(issue.reload.assignees).to eq([user3])
+      end
+
+      context "when issuable feature is private" do
+        levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
+
+        levels.each do |level|
+          it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
+            assignee = create(:user)
+            project.update(visibility_level: level)
+            feature_visibility_attr = :"#{issue.model_name.plural}_access_level"
+            project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
+
+            expect{ update_issue(assignee_ids: [assignee.id]) }.not_to change{ issue.assignees }
+          end
+        end
+      end
+    end
+
     context 'updating mentions' do
       let(:mentionable) { issue }
       include_examples 'updating mentions', Issues::UpdateService

spec/services/merge_requests/update_service_spec.rb

@@ -466,6 +466,54 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    context 'updating asssignee_id' do
+      it 'does not update assignee when assignee_id is invalid' do
+        merge_request.update(assignee_id: user.id)
+
+        update_merge_request(assignee_id: -1)
+
+        expect(merge_request.reload.assignee).to eq(user)
+      end
+
+      it 'unassigns assignee when user id is 0' do
+        merge_request.update(assignee_id: user.id)
+
+        update_merge_request(assignee_id: 0)
+
+        expect(merge_request.assignee_id).to be_nil
+      end
+
+      it 'saves assignee when user id is valid' do
+        update_merge_request(assignee_id: user.id)
+
+        expect(merge_request.assignee_id).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
+
+        update_merge_request(assignee_id: non_member.id)
+
+        expect(merge_request.assignee_id).to eq(original_assignee.id)
+      end
+
+      context "when issuable feature is private" do
+        levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
+
+        levels.each do |level|
+          it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
+            assignee = create(:user)
+            project.update(visibility_level: level)
+            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 }
+          end
+        end
+      end
+    end
+
     include_examples 'issuable update service' do
       let(:open_issuable) { merge_request }
       let(:closed_issuable) { create(:closed_merge_request, source_project: project) }

spec/support/services/issuable_update_service_shared_examples.rb

@@ -18,52 +18,4 @@ shared_examples 'issuable update service' do
       end
     end
   end
-
-  context 'asssignee_id' do
-    it 'does not update assignee when assignee_id is invalid' do
-      open_issuable.update(assignee_id: user.id)
-
-      update_issuable(assignee_id: -1)
-
-      expect(open_issuable.reload.assignee).to eq(user)
-    end
-
-    it 'unassigns assignee when user id is 0' do
-      open_issuable.update(assignee_id: user.id)
-
-      update_issuable(assignee_id: 0)
-
-      expect(open_issuable.assignee_id).to be_nil
-    end
-
-    it 'saves assignee when user id is valid' do
-      update_issuable(assignee_id: user.id)
-
-      expect(open_issuable.assignee_id).to eq(user.id)
-    end
-
-    it 'does not update assignee_id when user cannot read issue' do
-      non_member        = create(:user)
-      original_assignee = open_issuable.assignee
-
-      update_issuable(assignee_id: non_member.id)
-
-      expect(open_issuable.assignee_id).to eq(original_assignee.id)
-    end
-
-    context "when issuable feature is private" do
-      levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
-
-      levels.each do |level|
-        it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
-          assignee = create(:user)
-          project.update(visibility_level: level)
-          feature_visibility_attr = :"#{open_issuable.model_name.plural}_access_level"
-          project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
-
-          expect{ update_issuable(assignee_id: assignee) }.not_to change{ open_issuable.assignee }
-        end
-      end
-    end
-  end
 end