Create a pending task when an MR is assigned to someone

6 files changed, 142 insertions, 15 deletions

diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 009d5a6867e..b5691cdf44f 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -2,7 +2,7 @@ module MergeRequests
   class CreateService < MergeRequests::BaseService
     def execute
       # @project is used to determine whether the user can set the merge request's
-      # assignee, milestone and labels. Whether they can depends on their 
+      # assignee, milestone and labels. Whether they can depends on their
       # permissions on the target project.
       source_project = @project
       @project = Project.find(params[:target_project_id]) if params[:target_project_id]
@@ -18,6 +18,7 @@ module MergeRequests
         merge_request.update_attributes(label_ids: label_params)
         event_service.open_mr(merge_request, current_user)
         notification_service.new_merge_request(merge_request, current_user)
+        task_service.new_merge_request(merge_request, current_user)
         merge_request.create_cross_references!(current_user)
         execute_hooks(merge_request)
       end
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 55322bf47bb..85b11def231 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -28,6 +28,7 @@ module MergeRequests
       if merge_request.previous_changes.include?('assignee_id')
         create_assignee_note(merge_request)
         notification_service.reassigned_merge_request(merge_request, current_user)
+        task_service.reassigned_merge_request(merge_request, current_user)
       end
 
       if merge_request.previous_changes.include?('target_branch') ||
diff --git a/app/services/task_service.rb b/app/services/task_service.rb
index 57f68c61d00..329a041061a 100644
--- a/app/services/task_service.rb
+++ b/app/services/task_service.rb
@@ -11,9 +11,7 @@ class TaskService
   #  * creates a pending task for assignee if issue is assigned
   #
   def new_issue(issue, current_user)
-    if issue.is_assigned? && issue.assignee != current_user
-      create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED)
-    end
+    new_issuable(issue, current_user)
   end
 
   # When close an issue we should:
@@ -29,9 +27,23 @@ class TaskService
   #  * creates a pending task for new assignee if issue is assigned
   #
   def reassigned_issue(issue, current_user)
-    if issue.is_assigned?
-      create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED)
-    end
+    reassigned_issuable(issue, current_user)
+  end
+
+  # When create a merge request we should:
+  #
+  #  * creates a pending task for assignee if merge request is assigned
+  #
+  def new_merge_request(merge_request, current_user)
+    new_issuable(merge_request, current_user)
+  end
+
+  # When we reassign an merge request we should:
+  #
+  #  * creates a pending task for new assignee if merge request is assigned
+  #
+  def reassigned_merge_request(merge_request, current_user)
+    reassigned_issuable(merge_request, current_user)
   end
 
   # When we mark a task as done we should:
@@ -83,4 +95,16 @@ class TaskService
   def pending_tasks_for(user, project, target)
     user.tasks.pending.where(project: project, target: target)
   end
+
+  def new_issuable(issuable, user)
+    if issuable.is_assigned? && issuable.assignee != user
+      create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED)
+    end
+  end
+
+  def reassigned_issuable(issuable, user)
+    if issuable.is_assigned?
+      create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED)
+    end
+  end
 end
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index be8f1676eeb..2e3c9f88369 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe MergeRequests::CreateService, services: true do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
+  let(:assignee) { create(:user) }
 
   describe :execute do
     context 'valid params' do
@@ -14,10 +15,12 @@ describe MergeRequests::CreateService, services: true do
           target_branch: 'master'
         }
       end
+
       let(:service) { MergeRequests::CreateService.new(project, user, opts) }
 
       before do
         project.team << [user, :master]
+        project.team << [assignee, :developer]
         allow(service).to receive(:execute_hooks)
 
         @merge_request = service.execute
@@ -25,10 +28,47 @@ describe MergeRequests::CreateService, services: true do
 
       it { expect(@merge_request).to be_valid }
       it { expect(@merge_request.title).to eq('Awesome merge_request') }
+      it { expect(@merge_request.assignee).to be_nil }
 
       it 'should execute hooks with default action' do
         expect(service).to have_received(:execute_hooks).with(@merge_request)
       end
+
+      it 'does not creates a pending task' do
+        attributes = {
+          project: project,
+          target: @merge_request
+        }
+
+        expect(Task.where(attributes).count).to be_zero
+      end
+
+      context 'when merge request is assigned to someone' do
+        let(:opts) do
+          {
+            title: 'Awesome merge_request',
+            description: 'please fix',
+            source_branch: 'feature',
+            target_branch: 'master',
+            assignee: assignee
+          }
+        end
+
+        it { expect(@merge_request.assignee).to eq assignee }
+
+        it 'creates a pending task for new assignee' do
+          attributes = {
+            project: project,
+            author: user,
+            user: assignee,
+            target: @merge_request,
+            action: Task::ASSIGNED,
+            state: :pending
+          }
+
+          expect(Task.where(attributes).count).to eq 1
+        end
+      end
     end
   end
 end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 2e9e6e0870d..88853483784 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -98,6 +98,31 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    context 'task queue' do
+      let!(:pending_task) do
+        create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2)
+      end
+
+      context 'when is reassigned' do
+        before do
+          update_merge_request({ assignee: user2 })
+        end
+
+        it 'creates a pending task for new assignee' do
+          attributes = {
+            project: project,
+            author: user,
+            user: user2,
+            target: merge_request,
+            action: Task::ASSIGNED,
+            state: :pending
+          }
+
+          expect(Task.where(attributes).count).to eq 1
+        end
+      end
+    end
+
     context 'when MergeRequest has tasks' do
       before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
 
diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb
index 2fd75d10b6c..0a072dda94b 100644
--- a/spec/services/task_service_spec.rb
+++ b/spec/services/task_service_spec.rb
@@ -1,20 +1,20 @@
 require 'spec_helper'
 
 describe TaskService, services: true do
+  let(:author) { create(:user) }
+  let(:john_doe) { create(:user) }
+  let(:project) { create(:project) }
   let(:service) { described_class.new }
 
+  before do
+    project.team << [author, :developer]
+    project.team << [john_doe, :developer]
+  end
+
   describe 'Issues' do
-    let(:author) { create(:user) }
-    let(:john_doe) { create(:user) }
-    let(:project) { create(:empty_project, :public) }
     let(:assigned_issue) { create(:issue, project: project, assignee: john_doe) }
     let(:unassigned_issue) { create(:issue, project: project, assignee: nil) }
 
-    before do
-      project.team << [author, :developer]
-      project.team << [john_doe, :developer]
-    end
-
     describe '#new_issue' do
       it 'creates a pending task if assigned' do
         service.new_issue(assigned_issue, author)
@@ -116,6 +116,42 @@ describe TaskService, services: true do
     end
   end
 
+  describe 'Merge Requests' do
+    let(:mr_assigned) { create(:merge_request, source_project: project, assignee: john_doe) }
+    let(:mr_unassigned) { create(:merge_request, source_project: project, assignee: nil) }
+
+    describe '#new_merge_request' do
+      it 'creates a pending task if assigned' do
+        service.new_merge_request(mr_assigned, author)
+
+        is_expected_to_create_pending_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED)
+      end
+
+      it 'does not create a task if unassigned' do
+        is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, author) }
+      end
+
+      it 'does not create a task if assignee is the current user' do
+        is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, john_doe) }
+      end
+    end
+
+    describe '#reassigned_merge_request' do
+      it 'creates a pending task for new assignee' do
+        mr_unassigned.update_attribute(:assignee, john_doe)
+        service.reassigned_merge_request(mr_unassigned, author)
+
+        is_expected_to_create_pending_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED)
+      end
+
+      it 'does not create a task if unassigned' do
+        mr_assigned.update_attribute(:assignee, nil)
+
+        is_expected_to_not_create_task { service.reassigned_merge_request(mr_assigned, author) }
+      end
+    end
+  end
+
   def is_expected_to_create_pending_task(attributes = {})
     attributes.reverse_merge!(
       project: project,