Automatically remove attention requested state

Whenever the user interacts with a merge request, the attention
request state for that user will be removed.
If the merge request is closed or merged, the attention
request is removed from all reviewers and assignees.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/345365

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