Merge branch 'create-todos-for-merge-request-reviewers' into 'master'

Create todos for merge request reviewers

See merge request gitlab-org/gitlab!41546

app/models/todo.rb

@@ -17,9 +17,11 @@ class Todo < ApplicationRecord
   UNMERGEABLE         = 6
   DIRECTLY_ADDRESSED  = 7
   MERGE_TRAIN_REMOVED = 8 # This is an EE-only feature
+  REVIEW_REQUESTED    = 9
 
   ACTION_NAMES = {
     ASSIGNED => :assigned,
+    REVIEW_REQUESTED => :review_requested,
     MENTIONED => :mentioned,
     BUILD_FAILED => :build_failed,
     MARKED => :marked,
@@ -167,6 +169,10 @@ class Todo < ApplicationRecord
     action == ASSIGNED
   end
 
+  def review_requested?
+    action == REVIEW_REQUESTED
+  end
+
   def merge_train_removed?
     action == MERGE_TRAIN_REMOVED
   end

app/services/issuable_base_service.rb

@@ -369,8 +369,8 @@ class IssuableBaseService < BaseService
     associations
   end
 
-  def has_changes?(issuable, old_labels: [], old_assignees: [])
-    valid_attrs = [:title, :description, :assignee_ids, :milestone_id, :target_branch]
+  def has_changes?(issuable, old_labels: [], old_assignees: [], old_reviewers: [])
+    valid_attrs = [:title, :description, :assignee_ids, :reviewer_ids, :milestone_id, :target_branch]
 
     attrs_changed = valid_attrs.any? do |attr|
       issuable.previous_changes.include?(attr.to_s)
@@ -380,7 +380,9 @@ class IssuableBaseService < BaseService
 
     assignees_changed = issuable.assignees != old_assignees
 
-    attrs_changed || labels_changed || assignees_changed
+    reviewers_changed = issuable.reviewers != old_reviewers if issuable.allows_reviewers?
+
+    attrs_changed || labels_changed || assignees_changed || reviewers_changed
   end
 
   def invalidate_cache_counts(issuable, users: [])

app/services/merge_requests/update_service.rb

@@ -24,8 +24,9 @@ module MergeRequests
       old_labels = old_associations.fetch(:labels, [])
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])
       old_assignees = old_associations.fetch(:assignees, [])
+      old_reviewers = old_associations.fetch(:reviewers, [])
 
-      if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees)
+      if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees, old_reviewers: old_reviewers)
         todo_service.resolve_todos_for_target(merge_request, current_user)
       end
 
@@ -44,6 +45,8 @@ module MergeRequests
 
       handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees
 
+      handle_reviewers_change(merge_request, old_reviewers) if merge_request.reviewers != old_reviewers
+
       if merge_request.previous_changes.include?('target_branch') ||
           merge_request.previous_changes.include?('source_branch')
         merge_request.mark_as_unchecked
@@ -108,6 +111,10 @@ module MergeRequests
       todo_service.reassigned_assignable(merge_request, current_user, old_assignees)
     end
 
+    def handle_reviewers_change(merge_request, old_reviewers)
+      todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers)
+    end
+
     def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
       SystemNoteService.change_branch(
         issuable, issuable.project, current_user, branch_type,

app/services/todo_service.rb

@@ -57,6 +57,14 @@ class TodoService
     create_assignment_todo(issuable, current_user, old_assignees)
   end
 
+  # When we reassign an reviewable object (merge request) we should:
+  #
+  #  * create a pending todo for new reviewer if object is assigned
+  #
+  def reassigned_reviewable(issuable, current_user, old_reviewers = [])
+    create_reviewer_todo(issuable, current_user, old_reviewers)
+  end
+
   # When create a merge request we should:
   #
   #  * creates a pending todo for assignee if merge request is assigned
@@ -217,6 +225,7 @@ class TodoService
 
   def new_issuable(issuable, author)
     create_assignment_todo(issuable, author)
+    create_reviewer_todo(issuable, author) if issuable.allows_reviewers?
     create_mention_todos(issuable.project, issuable, author)
   end
 
@@ -250,6 +259,14 @@ class TodoService
     end
   end
 
+  def create_reviewer_todo(target, author, old_reviewers = [])
+    if target.reviewers.any?
+      reviewers = target.reviewers - old_reviewers
+      attributes = attributes_for_todo(target.project, target, author, Todo::REVIEW_REQUESTED)
+      create_todos(reviewers, attributes)
+    end
+  end
+
   def create_mention_todos(parent, target, author, note = nil, skip_users = [])
     # Create Todos for directly addressed users
     directly_addressed_users = filter_directly_addressed_users(parent, note || target, author, skip_users)

spec/services/merge_requests/create_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
-  let(:assignee) { create(:user) }
+  let(:user2) { create(:user) }
 
   describe '#execute' do
     context 'valid params' do
@@ -26,7 +26,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       before do
         project.add_maintainer(user)
-        project.add_developer(assignee)
+        project.add_developer(user2)
         allow(service).to receive(:execute_hooks)
       end
 
@@ -75,7 +75,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
               description: "well this is not done yet\n/wip",
               source_branch: 'feature',
               target_branch: 'master',
-              assignees: [assignee]
+              assignees: [user2]
             }
           end
 
@@ -91,7 +91,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
               description: "well this is not done yet\n/wip",
               source_branch: 'feature',
               target_branch: 'master',
-              assignees: [assignee]
+              assignees: [user2]
             }
           end
 
@@ -108,17 +108,17 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             description: 'please fix',
             source_branch: 'feature',
             target_branch: 'master',
-            assignees: [assignee]
+            assignees: [user2]
           }
         end
 
-        it { expect(merge_request.assignees).to eq([assignee]) }
+        it { expect(merge_request.assignees).to eq([user2]) }
 
         it 'creates a todo for new assignee' do
           attributes = {
             project: project,
             author: user,
-            user: assignee,
+            user: user2,
             target_id: merge_request.id,
             target_type: merge_request.class.name,
             action: Todo::ASSIGNED,
@@ -129,6 +129,34 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
+      context 'when reviewer is assigned' do
+        let(:opts) do
+          {
+            title: 'Awesome merge_request',
+            description: 'please fix',
+            source_branch: 'feature',
+            target_branch: 'master',
+            reviewers: [user2]
+          }
+        end
+
+        it { expect(merge_request.reviewers).to eq([user2]) }
+
+        it 'creates a todo for new reviewer' do
+          attributes = {
+            project: project,
+            author: user,
+            user: user2,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect(Todo.where(attributes).count).to eq 1
+        end
+      end
+
       context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do
         let(:shas) { project.repository.commits(opts[:source_branch], limit: 2).map(&:id) }
         let!(:pipeline_1) { create(:ci_pipeline, project: project, ref: opts[:source_branch], project_id: project.id, sha: shas[1]) }
@@ -213,7 +241,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
             before do
               stub_feature_flags(ci_disallow_to_create_merge_request_pipelines_in_target_project: false)
-              target_project.add_developer(assignee)
+              target_project.add_developer(user2)
               target_project.add_maintainer(user)
             end
 
@@ -366,7 +394,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             assignee_ids: create(:user).id,
             milestone_id: 1,
             title: 'Title',
-            description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}"),
+            description: %(/assign @#{user2.username}\n/milestone %"#{milestone.name}"),
             source_branch: 'feature',
             target_branch: 'master'
           }
@@ -374,12 +402,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
         before do
           project.add_maintainer(user)
-          project.add_maintainer(assignee)
+          project.add_maintainer(user2)
         end
 
         it 'assigns and sets milestone to issuable from command' do
           expect(merge_request).to be_persisted
-          expect(merge_request.assignees).to eq([assignee])
+          expect(merge_request.assignees).to eq([user2])
           expect(merge_request.milestone).to eq(milestone)
         end
       end
@@ -387,7 +415,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
     context 'merge request create service' do
       context 'asssignee_id' do
-        let(:assignee) { create(:user) }
+        let(:user2) { create(:user) }
 
         before do
           project.add_maintainer(user)
@@ -410,12 +438,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
 
         it 'saves assignee when user id is valid' do
-          project.add_maintainer(assignee)
-          opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] }
+          project.add_maintainer(user2)
+          opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] }
 
           merge_request = described_class.new(project, user, opts).execute
 
-          expect(merge_request.assignees).to eq([assignee])
+          expect(merge_request.assignees).to eq([user2])
         end
 
         context 'when assignee is set' do
@@ -423,18 +451,18 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
             {
               title: 'Title',
               description: 'Description',
-              assignee_ids: [assignee.id],
+              assignee_ids: [user2.id],
               source_branch: 'feature',
               target_branch: 'master'
             }
           end
 
           it 'invalidates open merge request counter for assignees when merge request is assigned' do
-            project.add_maintainer(assignee)
+            project.add_maintainer(user2)
 
             described_class.new(project, user, opts).execute
 
-            expect(assignee.assigned_open_merge_requests_count).to eq 1
+            expect(user2.assigned_open_merge_requests_count).to eq 1
           end
         end
 
@@ -449,7 +477,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           levels.each do |level|
             it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
               project.update!(visibility_level: level)
-              opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] }
+              opts = { title: 'Title', description: 'Description', assignee_ids: [user2.id] }
 
               merge_request = described_class.new(project, user, opts).execute
 
@@ -475,7 +503,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       before do
         project.add_maintainer(user)
-        project.add_developer(assignee)
+        project.add_developer(user2)
       end
 
       it 'creates a `MergeRequestsClosingIssues` record for each issue' do
@@ -503,7 +531,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       context 'when user can not access source project' do
         before do
-          target_project.add_developer(assignee)
+          target_project.add_developer(user2)
           target_project.add_maintainer(user)
         end
 
@@ -515,7 +543,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
       context 'when user can not access target project' do
         before do
-          target_project.add_developer(assignee)
+          target_project.add_developer(user2)
           target_project.add_maintainer(user)
         end
 
@@ -567,7 +595,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
       end
 
       before do
-        project.add_developer(assignee)
+        project.add_developer(user2)
         project.add_maintainer(user)
       end
 

spec/services/merge_requests/update_service_spec.rb

@@ -52,6 +52,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           title: 'New title',
           description: 'Also please fix',
           assignee_ids: [user.id],
+          reviewer_ids: [user.id],
           state_event: 'close',
           label_ids: [label.id],
           target_branch: 'target',
@@ -75,6 +76,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         expect(@merge_request).to be_valid
         expect(@merge_request.title).to eq('New title')
         expect(@merge_request.assignees).to match_array([user])
+        expect(@merge_request.reviewers).to match_array([user])
         expect(@merge_request).to be_closed
         expect(@merge_request.labels.count).to eq(1)
         expect(@merge_request.labels.first.title).to eq(label.name)
@@ -402,6 +404,30 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         end
       end
 
+      context 'when reviewers gets changed' do
+        before do
+          update_merge_request({ reviewer_ids: [user2.id] })
+        end
+
+        it 'marks pending todo as done' do
+          expect(pending_todo.reload).to be_done
+        end
+
+        it 'creates a pending todo for new review request' do
+          attributes = {
+            project: project,
+            author: user,
+            user: user2,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect(Todo.where(attributes).count).to eq 1
+        end
+      end
+
       context 'when the milestone is removed' do
         let!(:non_subscriber) { create(:user) }
 

spec/services/todo_service_spec.rb

@@ -65,6 +65,40 @@ RSpec.describe TodoService do
     end
   end
 
+  shared_examples 'reassigned reviewable target' do
+    context 'with no existing reviewers' do
+      let(:assigned_reviewers) { [] }
+
+      it 'creates a pending todo for new reviewer' do
+        target.reviewers = [john_doe]
+        service.send(described_method, target, author)
+
+        should_create_todo(user: john_doe, target: target, action: Todo::REVIEW_REQUESTED)
+      end
+    end
+
+    context 'with an existing reviewer' do
+      let(:assigned_reviewers) { [john_doe] }
+
+      it 'does not create a todo if unassigned' do
+        target.reviewers = []
+
+        should_not_create_any_todo { service.send(described_method, target, author) }
+      end
+
+      it 'creates a todo if new reviewer is the current user' do
+        target.reviewers = [john_doe]
+        service.send(described_method, target, john_doe)
+
+        should_create_todo(user: john_doe, target: target, author: john_doe, action: Todo::REVIEW_REQUESTED)
+      end
+
+      it 'does not create a todo if already assigned' do
+        should_not_create_any_todo { service.send(described_method, target, author, [john_doe]) }
+      end
+    end
+  end
+
   describe 'Issues' do
     let(:issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
     let(:addressed_issue) { create(:issue, project: project, assignees: [john_doe], author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
@@ -605,6 +639,17 @@ RSpec.describe TodoService do
     end
   end
 
+  describe '#reassigned_reviewable' do
+    let(:described_method) { :reassigned_reviewable }
+
+    context 'reviewable is a merge request' do
+      it_behaves_like 'reassigned reviewable target' do
+        let(:assigned_reviewers) { [] }
+        let(:target) { create(:merge_request, source_project: project, author: author, reviewers: assigned_reviewers) }
+      end
+    end
+  end
+
   describe 'Merge Requests' do
     let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
     let(:addressed_mr_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }