Merge branch 'ph/fixAttentionRequestedRaceConditions' into 'master'

Fixes a bug with attention requested state

See merge request gitlab-org/gitlab!75136

app/controllers/projects/merge_requests_controller.rb

@@ -282,7 +282,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
         if merge_request.errors.present?
           render json: @merge_request.errors, status: :bad_request
         else
-          render json: serializer.represent(@merge_request, serializer: 'basic')
+          render json: serializer.represent(@merge_request, serializer: params[:serializer] || 'basic')
         end
       end
     end

app/models/concerns/merge_request_reviewer_state.rb

@@ -15,11 +15,5 @@ module MergeRequestReviewerState
       inclusion: { in: self.states.keys }
 
     after_initialize :set_state, unless: :persisted?
-
-    def set_state
-      if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml)
-        self.state = :attention_requested
-      end
-    end
   end
 end

app/models/merge_request_assignee.rb

@@ -10,6 +10,12 @@ class MergeRequestAssignee < ApplicationRecord
 
   scope :in_projects, ->(project_ids) { joins(:merge_request).where(merge_requests: { target_project_id: project_ids }) }
 
+  def set_state
+    if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml)
+      self.state = MergeRequestReviewer.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested
+    end
+  end
+
   def cache_key
     [model_name.cache_key, id, state, assignee.cache_key]
   end

app/models/merge_request_reviewer.rb

@@ -6,6 +6,12 @@ class MergeRequestReviewer < ApplicationRecord
   belongs_to :merge_request
   belongs_to :reviewer, class_name: 'User', foreign_key: :user_id, inverse_of: :merge_request_reviewers
 
+  def set_state
+    if Feature.enabled?(:mr_attention_requests, self.merge_request&.project, default_enabled: :yaml)
+      self.state = MergeRequestAssignee.find_by(user_id: self.user_id, merge_request_id: self.merge_request_id)&.state || :attention_requested
+    end
+  end
+
   def cache_key
     [model_name.cache_key, id, state, reviewer.cache_key]
   end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -367,7 +367,8 @@ RSpec.describe Projects::MergeRequestsController do
         namespace_id: project.namespace,
         project_id: project,
         id: merge_request.iid,
-        merge_request: mr_params
+        merge_request: mr_params,
+        serializer: 'basic'
       }.merge(additional_params)
 
       put :update, params: params

spec/models/merge_request_assignee_spec.rb

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestAssignee do
+  let(:assignee) { create(:user) }
   let(:merge_request) { create(:merge_request) }
 
-  subject { merge_request.merge_request_assignees.build(assignee: create(:user)) }
+  subject { merge_request.merge_request_assignees.build(assignee: assignee) }
 
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
@@ -41,4 +42,13 @@ RSpec.describe MergeRequestAssignee do
   it_behaves_like 'having unique enum values'
 
   it_behaves_like 'having reviewer state'
+
+  describe 'syncs to reviewer state' do
+    before do
+      reviewer = merge_request.merge_request_reviewers.build(reviewer: assignee)
+      reviewer.update!(state: :reviewed)
+    end
+
+    it { is_expected.to have_attributes(state: 'reviewed') }
+  end
 end

spec/models/merge_request_reviewer_spec.rb

@@ -3,14 +3,24 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestReviewer do
+  let(:reviewer) { create(:user) }
   let(:merge_request) { create(:merge_request) }
 
-  subject { merge_request.merge_request_reviewers.build(reviewer: create(:user)) }
+  subject { merge_request.merge_request_reviewers.build(reviewer: reviewer) }
 
   it_behaves_like 'having unique enum values'
 
   it_behaves_like 'having reviewer state'
 
+  describe 'syncs to assignee state' do
+    before do
+      assignee = merge_request.merge_request_assignees.build(assignee: reviewer)
+      assignee.update!(state: :reviewed)
+    end
+
+    it { is_expected.to have_attributes(state: 'reviewed') }
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
     it { is_expected.to belong_to(:reviewer).class_name('User').inverse_of(:merge_request_reviewers) }