[Multiple issue assignees] Fixes for notification, todos, system notes[ci skip]

app/mailers/emails/issues.rb

@@ -11,10 +11,12 @@ module Emails
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
 
-    def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
+    def reassigned_issue_email(recipient_id, issue_id, previous_assignee_ids, updated_by_user_id)
       setup_issue_mail(issue_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(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
 

app/models/issue.rb

@@ -140,11 +140,17 @@ class Issue < ActiveRecord::Base
   end
 
   def assignee_or_author?(user)
-    author_id == user.id || assignees.exists?(user)
+    author_id == user.id || assignees.exists?(user.id)
   end
 
   def assignee_list
-    assignees.pluck(:name).join(', ')
+    assignees.map(&:to_reference).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
+    raise "assignee_id is deprecated"
   end
 
   # `from` argument can be a Namespace or Project.

app/models/merge_request.rb

@@ -193,6 +193,11 @@ class MergeRequest < ActiveRecord::Base
     }
   end
 
+  # This method is needed for compatibility with issues
+  def assignees
+    [assignee]
+  end
+
   def assignee_or_author?(user)
     author_id == user.id || assignee_id == user.id
   end

app/services/issuable_base_service.rb

 class IssuableBaseService < BaseService
   private
 
-  def create_assignee_note(issuable)
-    SystemNoteService.change_assignee(
-      issuable, issuable.project, current_user, issuable.assignee)
-  end
-
   def create_milestone_note(issuable)
     SystemNoteService.change_milestone(
       issuable, issuable.project, current_user, issuable.milestone)
@@ -77,7 +72,7 @@ class IssuableBaseService < BaseService
   def assignee_can_read?(issuable, assignee_id)
     new_assignee = User.find_by_id(assignee_id)
 
-    return false unless new_assignee.present?
+    return false unless new_assignee
 
     ability_name = :"read_#{issuable.to_ability_name}"
     resource     = issuable.persisted? ? issuable : project
@@ -207,6 +202,7 @@ class IssuableBaseService < BaseService
     filter_params(issuable)
     old_labels = issuable.labels.to_a
     old_mentioned_users = issuable.mentioned_users.to_a
+    old_assignees = issuable.assignees.to_a
 
     label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
     params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
@@ -222,7 +218,13 @@ class IssuableBaseService < BaseService
           handle_common_system_notes(issuable, old_labels: old_labels)
         end
 
-        handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users)
+        handle_changes(
+          issuable,
+          old_labels: old_labels,
+          old_mentioned_users: old_mentioned_users,
+          old_assignees: old_assignees
+        )
+
         after_update(issuable)
         issuable.create_new_cross_references!(current_user)
         execute_hooks(issuable, 'update')

app/services/issues/base_service.rb

@@ -9,11 +9,28 @@ module Issues
 
     private
 
+    def create_assignee_note(issue)
+      SystemNoteService.change_issue_assignees(
+        issue, issue.project, current_user, issue.assignees)
+    end
+
     def execute_hooks(issue, action = 'open')
       issue_data  = hook_data(issue, action)
       hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
       issue.project.execute_hooks(issue_data, hooks_scope)
       issue.project.execute_services(issue_data, hooks_scope)
     end
+
+    def filter_assignee(issuable)
+      return if params[:assignee_ids].blank?
+
+      assignee_ids = params[:assignee_ids].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
+    end
   end
 end

app/services/issues/update_service.rb

@@ -12,7 +12,7 @@ module Issues
       spam_check(issue, current_user)
     end
 
-    def handle_changes(issue, old_labels: [], old_mentioned_users: [])
+    def handle_changes(issue, old_labels: [], old_mentioned_users: [], old_assignees: [])
       if has_changes?(issue, old_labels: old_labels)
         todo_service.mark_pending_todos_as_done(issue, current_user)
       end
@@ -26,9 +26,9 @@ module Issues
         create_milestone_note(issue)
       end
 
-      if issue.previous_changes.include?('assignee_id')
+      if issue.previous_changes.include?('assignee_ids')
         create_assignee_note(issue)
-        notification_service.reassigned_issue(issue, current_user)
+        notification_service.reassigned_issue(issue, current_user, old_assignees)
         todo_service.reassigned_issue(issue, current_user)
       end
 

app/services/merge_requests/assign_issues_service.rb

@@ -4,7 +4,7 @@ module MergeRequests
       @assignable_issues ||= begin
         if current_user == merge_request.author
           closes_issues.select do |issue|
-            !issue.is_a?(ExternalIssue) && !issue.assignee_id? && can?(current_user, :admin_issue, issue)
+            !issue.is_a?(ExternalIssue) && !issue.assignees.any? && can?(current_user, :admin_issue, issue)
           end
         else
           []
@@ -14,7 +14,7 @@ module MergeRequests
 
     def execute
       assignable_issues.each do |issue|
-        Issues::UpdateService.new(issue.project, current_user, assignee_id: current_user.id).execute(issue)
+        Issues::UpdateService.new(issue.project, current_user, assignee_ids: [current_user.id]).execute(issue)
       end
 
       {

app/services/merge_requests/base_service.rb

@@ -38,6 +38,11 @@ module MergeRequests
 
     private
 
+    def create_assignee_note(merge_request)
+      SystemNoteService.change_assignee(
+        merge_request, merge_request.project, current_user, merge_request.assignee)
+    end
+
     # Returns all origin and fork merge requests from `@project` satisfying passed arguments.
     def merge_requests_for(source_branch, mr_states: [:opened])
       MergeRequest

app/services/notification_recipient_service.rb

@@ -3,11 +3,12 @@
 #
 class NotificationRecipientService
   attr_reader :project
-  
+
   def initialize(project)
     @project = project
   end
 
+  # TODO: refactor this: previous_assignee argument can be a user object or an array which is not really nice
   def build_recipients(target, current_user, action: nil, previous_assignee: nil, skip_current_user: true)
     custom_action = build_custom_key(action, target)
 
@@ -23,9 +24,13 @@ class NotificationRecipientService
     # Re-assign is considered as a mention of the new assignee so we add the
     # new assignee to the list of recipients after we rejected users with
     # the "on mention" notification level
-    if [:reassign_merge_request, :reassign_issue].include?(custom_action)
+    case custom_action
+    when :reassign_merge_request
       recipients << previous_assignee if previous_assignee
       recipients << target.assignee
+    when :reassign_issue
+      recipients.concat(previous_assignee) if previous_assignee.any?
+      recipients.concat(target.assignees)
     end
 
     recipients = reject_muted_users(recipients)

app/services/notification_service.rb

@@ -66,8 +66,23 @@ class NotificationService
   #  * issue new assignee if their notification level is not Disabled
   #  * users with custom level checked with "reassign issue"
   #
-  def reassigned_issue(issue, current_user)
-    reassign_resource_email(issue, issue.project, current_user, :reassigned_issue_email)
+  def reassigned_issue(issue, current_user, previous_assignees = [])
+    recipients = NotificationRecipientService.new(issue.project).build_recipients(
+      issue,
+      current_user,
+      action: "reassign",
+      previous_assignee: previous_assignees
+    )
+
+    recipients.each do |recipient|
+      mailer.send(
+        :reassigned_issue_email,
+        recipient.id,
+        issue.id,
+        previous_assignees.map(&:id),
+        current_user.id
+      ).deliver_later
+    end
   end
 
   # When we add labels to an issue we should send an email to:
@@ -407,10 +422,10 @@ class NotificationService
   end
 
   def previous_record(object, attribute)
-    if object && attribute
-      if object.previous_changes.include?(attribute)
-        object.previous_changes[attribute].first
-      end
+    return unless object && attribute
+
+    if object.previous_changes.include?(attribute)
+      object.previous_changes[attribute].first
     end
   end
 end

app/services/system_note_service.rb

@@ -49,6 +49,42 @@ module SystemNoteService
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
+  # Called when the assignees of an Issue is changed or removed
+  #
+  # issue - Issue object
+  # project  - Project owning noteable
+  # author   - User performing the change
+  # assignees - User being assigned, or nil
+  #
+  # Example Note text:
+  #
+  #   "removed all assignees"
+  #
+  #   "assigned to @user1 additionally to @user2"
+  #
+  #   "assigned to @user1, @user2 and @user3 and unassigned from @user4 and @user5"
+  #
+  #   "assigned to @user1 and @user2"
+  #
+  # Returns the created Note object
+  def change_issue_assignees(issue, project, author, assignees)
+    # TODO: basic implementation, should be improved before merging the MR
+    body =
+      if issue.assignees.any? && assignees.any?
+        unassigned_users = issue.assignees - assignees
+        added_users = assignees - issue.assignees
+
+        "assigned to #{added_users.map(&:to_reference).to_sentence} and unassigned #{unassigned_users.map(&:to_reference).to_sentence}"
+      elsif issue.assignees.any?
+        "removed all assignees"
+      elsif assignees.any?
+        "assigned to #{assignees.map(&:to_reference).to_sentence}"
+      end
+
+    create_note(noteable: issue, project: project, author: author, note: body)
+  end
+
+
   # Called when one or more labels on a Noteable are added and/or removed
   #
   # noteable       - Noteable object

app/services/todo_service.rb

@@ -264,13 +264,13 @@ class TodoService
   end
 
   def create_assignment_todo(issuable, author)
-    if issuable.assignee
+    if issuable.assignees.any?
       attributes = attributes_for_todo(issuable.project, issuable, author, Todo::ASSIGNED)
-      create_todos(issuable.assignee, attributes)
+      create_todos(issuable.assignees, attributes)
     end
   end
 
-  def create_mention_todos(project, target, author, note = nil)
+    def create_mention_todos(project, target, author, note = nil)
     # Create Todos for directly addressed users
     directly_addressed_users = filter_directly_addressed_users(project, note || target, author)
     attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note)

app/views/notify/_reassigned_issuable_email.text.erb

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

app/views/notify/new_issue_email.html.haml

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

app/views/notify/new_issue_email.text.erb

@@ -2,6 +2,6 @@ New Issue was created.
 
 Issue <%= @issue.iid %>: <%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %>
 Author:    <%= @issue.author_name %>
-Assignee:  <%= @issue.assignee_name %>
+Assignee:  <%= @issue.assignee_list %>
 
 <%= @issue.description %>

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.assignee_id.present?
+- if @issue.assignees.any?
   %p
-    Assignee: #{@issue.assignee_name}
+    Assignee: #{@issue.assignee_list}

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(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %>
 Author:    <%= @issue.author_name %>
-Assignee:  <%= @issue.assignee_name %>
+Assignee:  <%= @issue.assignee_list %>
 
 <%= @issue.description %>

app/views/notify/reassigned_issue_email.html.haml

-= render 'reassigned_issuable_email', issuable: @issue
+%p
+  Assignee changed
+  - if @previous_assignees.any?
+    from
+    %strong= @previous_assignees.map(&:to_reference).to_sentence
+  to
+  - if @issue.assignees.any?
+    %strong= @issue.assignee_list
+  - else
+    %strong Unassigned
+

app/views/notify/reassigned_merge_request_email.text.erb

-<%= 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 <%= "from #{@previous_assignee.name}" if @previous_assignee -%>
+ to <%= "#{@merge_request.assignee_id ? @merge_request.assignee_name : 'Unassigned'}" %>

db/migrate/20170320171632_create_issue_assignees_table.rb

@@ -25,8 +25,10 @@ class CreateIssueAssigneesTable < ActiveRecord::Migration
 
   def change
     create_table :issue_assignees do |t|
-      t.references :user, foreign_key: true, index: true
-      t.references :issue, foreign_key: true, index: true
+      t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false
+      t.references :issue, foreign_key: { on_delete: :cascade }, null: false
     end
+
+    add_index :issue_assignees, [:issue_id, :user_id], unique: true
   end
 end

db/schema.rb

@@ -1444,7 +1444,6 @@ ActiveRecord::Schema.define(version: 20170320173259) do
     t.boolean "authorized_projects_populated"
     t.boolean "auditor", default: false, null: false
     t.boolean "ghost"
-    t.boolean "notified_of_own_activity", default: false, null: false
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

spec/controllers/dashboard/todos_controller_spec.rb

@@ -16,7 +16,7 @@ describe Dashboard::TodosController do
   describe 'GET #index' do
     context 'when using pagination' do
       let(:last_page) { user.todos.page.total_pages }
-      let!(:issues) { create_list(:issue, 2, project: project, assignee: user) }
+      let!(:issues) { create_list(:issue, 2, project: project, assignees: [user]) }
 
       before do
         issues.each { |issue| todo_service.new_issue(issue, user) }

spec/services/users/destroy_spec.rb

@@ -54,7 +54,7 @@ describe Users::DestroyService, services: true do
       end
 
       context "for an issue the user was assigned to" do
-        let!(:issue) { create(:issue, project: project, assignee: user) }
+        let!(:issue) { create(:issue, project: project, assignees: [user]) }
 
         before do
           service.execute(user)
@@ -67,7 +67,7 @@ describe Users::DestroyService, services: true do
         it 'migrates the issue so that it is "Unassigned"' do
           migrated_issue = Issue.find_by_id(issue.id)
 
-          expect(migrated_issue.assignee).to be_nil
+          expect(migrated_issue.assignees).to be_empty
         end
       end
     end