Merge branch 'ph/345365/automaticRemovalAttentionRequest' into 'master'

Automatically remove attention requested state

See merge request gitlab-org/gitlab!75221

app/assets/javascripts/sidebar/sidebar_mediator.js

@@ -79,6 +79,20 @@ export default class SidebarMediator {
           }),
         );
       } else {
+        const currentUserId = gon.current_user_id;
+
+        if (currentUserId !== user.id) {
+          const currentUserReviewerOrAssignee = isReviewer
+            ? this.store.findReviewer({ id: currentUserId })
+            : this.store.findAssignee({ id: currentUserId });
+
+          if (currentUserReviewerOrAssignee?.attention_requested) {
+            // Update current users attention_requested state
+            this.store.updateReviewer(currentUserId, 'attention_requested');
+            this.store.updateAssignee(currentUserId, 'attention_requested');
+          }
+        }
+
         toast(sprintf(__('Requested attention from @%{username}'), { username: user.username }));
       }
 

app/services/merge_requests/approval_service.rb

@@ -14,6 +14,7 @@ module MergeRequests
       create_approval_note(merge_request)
       mark_pending_todos_as_done(merge_request)
       execute_approval_hooks(merge_request, current_user)
+      remove_attention_requested(merge_request, current_user)
       merge_request_activity_counter.track_approve_mr_action(user: current_user)
 
       success

app/services/merge_requests/base_service.rb

@@ -58,6 +58,8 @@ module MergeRequests
       new_reviewers = merge_request.reviewers - old_reviewers
       merge_request_activity_counter.track_users_review_requested(users: new_reviewers)
       merge_request_activity_counter.track_reviewers_changed_action(user: current_user)
+
+      remove_attention_requested(merge_request, current_user)
     end
 
     def cleanup_environments(merge_request)
@@ -238,6 +240,18 @@ module MergeRequests
 
       Milestones::MergeRequestsCountService.new(milestone).delete_cache
     end
+
+    def remove_all_attention_requests(merge_request)
+      return unless merge_request.attention_requested_enabled?
+
+      ::MergeRequests::BulkRemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request).execute
+    end
+
+    def remove_attention_requested(merge_request, user)
+      return unless merge_request.attention_requested_enabled?
+
+      ::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: user).execute
+    end
   end
 end
 

app/services/merge_requests/bulk_remove_attention_requested_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class BulkRemoveAttentionRequestedService < MergeRequests::BaseService
+    attr_accessor :merge_request
+
+    def initialize(project:, current_user:, merge_request:)
+      super(project: project, current_user: current_user)
+
+      @merge_request = merge_request
+    end
+
+    def execute
+      return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
+
+      merge_request.merge_request_assignees.update_all(state: :reviewed)
+      merge_request.merge_request_reviewers.update_all(state: :reviewed)
+
+      success
+    end
+  end
+end

app/services/merge_requests/close_service.rb

@@ -17,6 +17,7 @@ module MergeRequests
         create_note(merge_request)
         notification_service.async.close_mr(merge_request, current_user)
         todo_service.close_merge_request(merge_request, current_user)
+        remove_all_attention_requests(merge_request)
         execute_hooks(merge_request, 'close')
         invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers)
         merge_request.update_project_counter_caches

app/services/merge_requests/handle_assignees_change_service.rb

@@ -22,6 +22,8 @@ module MergeRequests
       merge_request_activity_counter.track_assignees_changed_action(user: current_user)
 
       execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks]
+
+      remove_attention_requested(merge_request, current_user)
     end
 
     private

app/services/merge_requests/post_merge_service.rb

@@ -28,6 +28,7 @@ module MergeRequests
       notification_service.merge_mr(merge_request, current_user)
       invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers)
       merge_request.update_project_counter_caches
+      remove_all_attention_requests(merge_request)
       delete_non_latest_diffs(merge_request)
       cancel_review_app_jobs!(merge_request)
       cleanup_environments(merge_request)

app/services/merge_requests/remove_attention_requested_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class RemoveAttentionRequestedService < MergeRequests::BaseService
+    attr_accessor :merge_request, :user
+
+    def initialize(project:, current_user:, merge_request:, user:)
+      super(project: project, current_user: current_user)
+
+      @merge_request = merge_request
+      @user = user
+    end
+
+    def execute
+      return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
+
+      if reviewer || assignee
+        update_state(reviewer)
+        update_state(assignee)
+
+        success
+      else
+        error("User is not a reviewer or assignee of the merge request")
+      end
+    end
+
+    private
+
+    def assignee
+      merge_request.find_assignee(user)
+    end
+
+    def reviewer
+      merge_request.find_reviewer(user)
+    end
+
+    def update_state(reviewer_or_assignee)
+      reviewer_or_assignee&.update(state: :reviewed)
+    end
+  end
+end

app/services/merge_requests/toggle_attention_requested_service.rb

@@ -21,6 +21,10 @@ module MergeRequests
         if reviewer&.attention_requested? || assignee&.attention_requested?
           create_attention_request_note
           notity_user
+
+          if current_user.id != user.id
+            remove_attention_requested(merge_request, current_user)
+          end
         else
           create_remove_attention_request_note
         end

spec/services/merge_requests/approval_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe MergeRequests::ApprovalService do
   describe '#execute' do
     let(:user)          { create(:user) }
-    let(:merge_request) { create(:merge_request) }
+    let(:merge_request) { create(:merge_request, reviewers: [user]) }
     let(:project)       { merge_request.project }
     let!(:todo)         { create(:todo, user: user, project: project, target: merge_request) }
 
@@ -59,6 +59,14 @@ RSpec.describe MergeRequests::ApprovalService do
         service.execute(merge_request)
       end
 
+      it 'removes attention requested state' do
+        expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new)
+          .with(project: project, current_user: user, merge_request: merge_request, user: user)
+          .and_call_original
+
+        service.execute(merge_request)
+      end
+
       context 'with remaining approvals' do
         it 'fires an approval webhook' do
           expect(service).to receive(:execute_hooks).with(merge_request, 'approved')

spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::BulkRemoveAttentionRequestedService do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+  let(:assignee_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) }
+  let(:reviewer) { merge_request.find_reviewer(user) }
+  let(:assignee) { merge_request.find_assignee(assignee_user) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) }
+  let(:result) { service.execute }
+
+  before do
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  describe '#execute' do
+    context 'invalid permissions' do
+      let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    context 'updates reviewers and assignees' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        service.execute
+        reviewer.reload
+        assignee.reload
+
+        expect(reviewer.state).to eq 'reviewed'
+        expect(assignee.state).to eq 'reviewed'
+      end
+    end
+  end
+end

spec/services/merge_requests/close_service_spec.rb

@@ -54,6 +54,10 @@ RSpec.describe MergeRequests::CloseService do
         expect(todo.reload).to be_done
       end
 
+      it 'removes attention requested state' do
+        expect(merge_request.find_assignee(user2).attention_requested?).to eq(false)
+      end
+
       context 'when auto merge is enabled' do
         let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
 

spec/services/merge_requests/handle_assignees_change_service_spec.rb

@@ -87,6 +87,14 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do
       expect(todo).to be_pending
     end
 
+    it 'removes attention requested state' do
+      expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new)
+        .with(project: project, current_user: user, merge_request: merge_request, user: user)
+        .and_call_original
+
+      execute
+    end
+
     it 'tracks users assigned event' do
       expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
         .to receive(:track_users_assigned_to_mr).once.with(users: [assignee])

spec/services/merge_requests/remove_attention_requested_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::RemoveAttentionRequestedService do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+  let(:assignee_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) }
+  let(:reviewer) { merge_request.find_reviewer(user) }
+  let(:assignee) { merge_request.find_assignee(assignee_user) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) }
+  let(:result) { service.execute }
+
+  before do
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  describe '#execute' do
+    context 'invalid permissions' do
+      let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request, user: user) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    context 'reviewer does not exist' do
+      let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    context 'reviewer exists' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        service.execute
+        reviewer.reload
+
+        expect(reviewer.state).to eq 'reviewed'
+      end
+    end
+
+    context 'assignee exists' do
+      let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: assignee_user) }
+
+      before do
+        assignee.update!(state: :reviewed)
+      end
+
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates assignees state' do
+        service.execute
+        assignee.reload
+
+        expect(assignee.state).to eq 'reviewed'
+      end
+    end
+
+    context 'assignee is the same as reviewer' do
+      let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) }
+      let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) }
+      let(:assignee) { merge_request.find_assignee(user) }
+
+      it 'updates reviewers and assignees state' do
+        service.execute
+        reviewer.reload
+        assignee.reload
+
+        expect(reviewer.state).to eq 'reviewed'
+        expect(assignee.state).to eq 'reviewed'
+      end
+    end
+  end
+end

spec/services/merge_requests/toggle_attention_requested_service_spec.rb

@@ -70,6 +70,14 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do
 
         service.execute
       end
+
+      it 'removes attention requested state' do
+        expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new)
+          .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user)
+          .and_call_original
+
+        service.execute
+      end
     end
 
     context 'assignee exists' do
@@ -101,6 +109,14 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do
 
         service.execute
       end
+
+      it 'removes attention requested state' do
+        expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new)
+          .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user)
+          .and_call_original
+
+        service.execute
+      end
     end
 
     context 'assignee is the same as reviewer' do