[Multiple issue assignees] Addressing review comments

app/finders/issues_finder.rb

@@ -47,7 +47,7 @@ class IssuesFinder < IssuableFinder
       issues.confidential IS NOT TRUE
       OR (issues.confidential = TRUE
         AND (issues.author_id = :user_id
-          OR EXISTS (SELECT true FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
+          OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
           OR issues.project_id IN(:project_ids)))',
       user_id: user.id,
       project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id))

app/models/issue.rb

@@ -36,9 +36,9 @@ class Issue < ActiveRecord::Base
 
   scope :open_for, ->(user) { opened.assigned_to(user) }
   scope :in_projects, ->(project_ids) { where(project_id: project_ids) }
-  scope :assigned, -> { where('EXISTS (SELECT * FROM issue_assignees WHERE issue_id = issues.id)') }
-  scope :unassigned, -> { where('NOT EXISTS (SELECT * 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 :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 :without_due_date, -> { where(due_date: nil) }
   scope :due_before, ->(date) { where('issues.due_date < ?', date) }

app/models/user.rb

@@ -106,7 +106,7 @@ 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 :issue_assignees, dependent: :destroy
+  has_many :issue_assignees
   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"
 

app/services/issues/base_service.rb

@@ -24,7 +24,7 @@ module Issues
     def filter_assignee(issuable)
       return if params[:assignee_ids].blank?
 
-      assignee_ids = params[:assignee_ids].select{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
+      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] = []

app/services/issues/export_csv_service.rb

@@ -21,7 +21,7 @@ module Issues
 
     def csv_builder
       @csv_builder ||=
-        CsvBuilder.new(@issues.includes(:author), header_to_value_hash)
+        CsvBuilder.new(@issues.includes(:author, :assignees), header_to_value_hash)
     end
 
     private
@@ -35,8 +35,8 @@ module Issues
        'Description' => 'description',
        'Author' => 'author_name',
        'Author Username' => -> (issue) { issue.author&.username },
-       'Assignee' => -> (issue) { issue.assignees.pluck(:name).join(', ') },
-       'Assignee Username' => -> (issue) { issue.assignees.pluck(:username).join(', ') },
+       'Assignee' => -> (issue) { issue.assignees.map(&:name).join(', ') },
+       'Assignee Username' => -> (issue) { issue.assignees.map(&:username).join(', ') },
        'Confidential' => -> (issue) { issue.confidential? ? 'Yes' : 'No' },
        'Due Date' => -> (issue) { issue.due_date&.to_s(:csv) },
        'Created At (UTC)' => -> (issue) { issue.created_at&.to_s(:csv) },

app/services/merge_requests/assign_issues_service.rb

@@ -4,7 +4,7 @@ module MergeRequests
       @assignable_issues ||= begin
         if current_user == merge_request.author
           closes_issues.select do |issue|
-            !issue.is_a?(ExternalIssue) && !issue.assignees.any? && can?(current_user, :admin_issue, issue)
+            !issue.is_a?(ExternalIssue) && !issue.assignees.present? && can?(current_user, :admin_issue, issue)
           end
         else
           []

app/services/notification_recipient_service.rb

@@ -29,7 +29,7 @@ class NotificationRecipientService
       recipients << target.assignee
     when :reassign_issue
       previous_assignees = Array(previous_assignee)
-      recipients.concat(previous_assignees) if previous_assignees.any?
+      recipients.concat(previous_assignees)
       recipients.concat(target.assignees)
     end
 

app/services/slash_commands/interpret_service.rb

@@ -86,18 +86,18 @@ module SlashCommands
       current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :assign do |assignee_param|
-      users = extract_references(assignee_param, :user)
+      user_ids = extract_references(assignee_param, :user).map(&:id)
 
-      if users.empty?
-        users = User.find_by(username: assignee_param.split(' ').map(&:strip))
+      if user_ids.empty?
+        user_ids = User.where(username: assignee_param.split(' ').map(&:strip)).pluck(:id)
       end
 
-      next if users.empty?
+      next if user_ids.empty?
 
       if issuable.is_a?(Issue)
-        @updates[:assignee_ids] = users.map(&:id)
+        @updates[:assignee_ids] = user_ids
       else
-        @updates[:assignee_id] = users.last.id
+        @updates[:assignee_id] = user_ids.last
       end
     end
 

app/views/notify/_reassigned_issuable_email.text.erb

-Reassigned Issue <%= @issue.iid %>
-
-<%= url_for([@issue.project.namespace.becomes(Namespace), @issue.project, @issue, { only_path: false }]) %>
-
-Assignee changed <%= "from #{@previous_assignees.map(&:name).to_sentence}" if @previous_assignees.any? -%>
- to <%= "#{@issue.assignees.any? ? @issue.assignee_list : 'Unassigned'}" %>

app/views/notify/new_mention_in_issue_email.html.haml

@@ -7,6 +7,6 @@
 - if @issue.description
   = markdown(@issue.description, pipeline: :email, author: @issue.author)
 
-- if @issue.assignees.any?
+- if @issue.assignees.present?
   %p
     Assignee: #{@issue.assignee_list}

app/views/notify/reassigned_issue_email.html.haml

@@ -8,4 +8,3 @@
     %strong= @issue.assignee_list
   - else
     %strong Unassigned
-

app/views/notify/reassigned_issue_email.text.erb

-<%= render 'reassigned_issuable_email', issuable: @issue %>
+Reassigned Issue <%= @issue.iid %>
+
+<%= url_for([@issue.project.namespace.becomes(Namespace), @issue.project, @issue, { only_path: false }]) %>
+
+Assignee changed <%= "from #{@previous_assignees.map(&:name).to_sentence}" if @previous_assignees.any? -%>
+ to <%= "#{@issue.assignees.any? ? @issue.assignee_list : 'Unassigned'}" %>

app/views/notify/reassigned_merge_request_email.html.haml

-= render 'reassigned_issuable_email', issuable: @merge_request
+Reassigned Merge Request #{ @merge_request.iid }
+
+= url_for([@merge_request.project.namespace.becomes(Namespace), @merge_request.project, @merge_request, { only_path: false }])
+
+Assignee changed
+- if @previous_assignee
+  from #{@previous_assignee.name}
+to
+= @merge_request.assignee_id ? @merge_request.assignee_name : 'Unassigned'

db/migrate/20170320171632_create_issue_assignees_table.rb

@@ -35,10 +35,6 @@ class CreateIssueAssigneesTable < ActiveRecord::Migration
   end
 
   def down
-    if index_exists?(:issue_assignees, name: INDEX_NAME)
-      remove_index :issue_assignees, name: INDEX_NAME
-    end
-
     drop_table :issue_assignees
   end
 end

db/migrate/20170320173259_migrate_assignees.rb

@@ -21,14 +21,24 @@ class MigrateAssignees < ActiveRecord::Migration
   #
   # To disable transactions uncomment the following line and remove these
   # comments:
-  # disable_ddl_transaction!
+  disable_ddl_transaction!
 
   def up
-    execute <<-EOF
-      UPDATE issues
-      SET assignee_id = NULL
-      WHERE assignee_id NOT IN(SELECT id FROM users);
+    # Optimisation: this accounts for most of the invalid assignee IDs on GitLab.com
+    update_column_in_batches(:issues, :assignee_id, nil) do |table, query|
+      query.where(table[:assignee_id].eq(0))
+    end
+
+    users = Arel::Table.new(:users)
 
+    update_column_in_batches(:issues, :assignee_id, nil) do |table, query|
+      query.where(table[:assignee_id].not_eq(nil)\
+        .and(
+          users.project("true").where(users[:id].eq(table[:assignee_id])).exists.not
+        ))
+    end
+
+    execute <<-EOF
       INSERT INTO issue_assignees(issue_id, user_id)
       SELECT id, assignee_id FROM issues WHERE assignee_id IS NOT NULL
     EOF

doc/user/project/integrations/webhooks.md

@@ -232,7 +232,7 @@ X-Gitlab-Event: Issue Hook
   "object_attributes": {
     "id": 301,
     "title": "New API: create/update/delete file",
-    "assignee_id": 51,
+    "assignee_ids": [51],
     "author_id": 51,
     "project_id": 14,
     "created_at": "2013-12-03T17:15:43Z",
@@ -246,11 +246,11 @@ X-Gitlab-Event: Issue Hook
     "url": "http://example.com/diaspora/issues/23",
     "action": "open"
   },
-  "assignee": {
+  "assignees": [{
     "name": "User1",
     "username": "user1",
     "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
-  },
+  }],
   "labels": [{
     "id": 206,
     "title": "API",
@@ -544,7 +544,7 @@ X-Gitlab-Event: Note Hook
   "issue": {
     "id": 92,
     "title": "test",
-    "assignee_id": null,
+    "assignee_ids": [],
     "author_id": 1,
     "project_id": 5,
     "created_at": "2015-04-12 14:53:17 UTC",

spec/features/atom/issues_spec.rb

@@ -22,7 +22,7 @@ describe 'Issues Feed', feature: true  do
           to have_content('application/atom+xml')
         expect(body).to have_selector('title', text: "#{project.name} issues")
         expect(body).to have_selector('author email', text: issue.author_public_email)
-        expect(body).to have_selector('assignee email', text: issue.author_public_email)
+        expect(body).to have_selector('assignees email', text: issue.author_public_email)
         expect(body).to have_selector('entry summary', text: issue.title)
       end
     end
@@ -36,7 +36,7 @@ describe 'Issues Feed', feature: true  do
           to have_content('application/atom+xml')
         expect(body).to have_selector('title', text: "#{project.name} issues")
         expect(body).to have_selector('author email', text: issue.author_public_email)
-        expect(body).to have_selector('assignee email', text: issue.author_public_email)
+        expect(body).to have_selector('assignees email', text: issue.author_public_email)
         expect(body).to have_selector('entry summary', text: issue.title)
       end
     end

spec/features/issues/csv_spec.rb

@@ -80,6 +80,6 @@ describe 'Issues csv', feature: true do
                 author: user,
                 milestone: milestone,
                 labels: [feature_label, idea_label])
-    expect{ request_csv }.not_to exceed_query_limit(control_count + 23)
+    expect{ request_csv }.not_to exceed_query_limit(control_count + 5)
   end
 end

spec/services/merge_requests/assign_issues_service_spec.rb

@@ -15,18 +15,18 @@ describe MergeRequests::AssignIssuesService, services: true do
     expect(service.assignable_issues.map(&:id)).to include(issue.id)
   end
 
-  it 'ignores issues already assigned to any user' do
-    issue.assignees = [create(:user)]
-
-    expect(service.assignable_issues).to be_empty
-  end
-
   it 'ignores issues the user cannot update assignee on' do
     project.team.truncate
 
     expect(service.assignable_issues).to be_empty
   end
 
+   it 'ignores issues already assigned to any user' do
+     issue.assignees = [create(:user)]
+
+     expect(service.assignable_issues).to be_empty
+   end
+
   it 'ignores all issues unless current_user is merge_request.author' do
     merge_request.update!(author: create(:user))
 

spec/services/notification_service_spec.rb

@@ -460,7 +460,7 @@ describe NotificationService, services: true do
       it do
         notification.new_issue(issue, @u_disabled)
 
-        should_email(issue.assignees.first)
+        should_email(assignee)
         should_email(@u_watcher)
         should_email(@u_guest_watcher)
         should_email(@u_guest_custom)