Merge branch 'ph/defaultAttentionRequiredToAssigneesReviewers' into 'master'

Defaults reviewer state to attention_required

See merge request gitlab-org/gitlab!73295

app/models/concerns/merge_request_reviewer_state.rb

@@ -13,5 +13,13 @@ module MergeRequestReviewerState
     validates :state,
       presence: true,
       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_required
+      end
+    end
   end
 end

spec/graphql/types/user_merge_request_interaction_type_spec.rb

@@ -78,7 +78,7 @@ RSpec.describe GitlabSchema.types['UserMergeRequestInteraction'] do
         merge_request.reviewers << user
       end
 
-      it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['UNREVIEWED'].value) }
+      it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['ATTENTION_REQUIRED'].value) }
 
       it 'implies not reviewed' do
         expect(resolve(:reviewed)).to be false
@@ -87,7 +87,8 @@ RSpec.describe GitlabSchema.types['UserMergeRequestInteraction'] do
 
     context 'when the user has provided a review' do
       before do
-        merge_request.merge_request_reviewers.create!(reviewer: user, state: MergeRequestReviewer.states['reviewed'])
+        reviewer = merge_request.merge_request_reviewers.create!(reviewer: user)
+        reviewer.update!(state: MergeRequestReviewer.states['reviewed'])
       end
 
       it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['REVIEWED'].value) }

spec/models/merge_request_assignee_spec.rb

@@ -39,4 +39,6 @@ RSpec.describe MergeRequestAssignee do
   end
 
   it_behaves_like 'having unique enum values'
+
+  it_behaves_like 'having reviewer state'
 end

spec/models/merge_request_reviewer_spec.rb

@@ -9,6 +9,8 @@ RSpec.describe MergeRequestReviewer do
 
   it_behaves_like 'having unique enum values'
 
+  it_behaves_like 'having reviewer state'
+
   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) }

spec/models/users/merge_request_interaction_spec.rb

@@ -61,7 +61,7 @@ RSpec.describe ::Users::MergeRequestInteraction do
         merge_request.reviewers << user
       end
 
-      it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['UNREVIEWED'].value) }
+      it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['ATTENTION_REQUIRED'].value) }
 
       it 'implies not reviewed' do
         expect(interaction).not_to be_reviewed
@@ -70,7 +70,8 @@ RSpec.describe ::Users::MergeRequestInteraction do
 
     context 'when the user has provided a review' do
       before do
-        merge_request.merge_request_reviewers.create!(reviewer: user, state: MergeRequestReviewer.states['reviewed'])
+        reviewer = merge_request.merge_request_reviewers.create!(reviewer: user)
+        reviewer.update!(state: MergeRequestReviewer.states['reviewed'])
       end
 
       it { is_expected.to eq(Types::MergeRequestReviewStateEnum.values['REVIEWED'].value) }

spec/requests/api/graphql/project/merge_request_spec.rb

@@ -347,7 +347,7 @@ RSpec.describe 'getting merge request information nested in a project' do
         expect(interaction_data).to contain_exactly a_hash_including(
           'canMerge' => false,
           'canUpdate' => can_update,
-          'reviewState' => unreviewed,
+          'reviewState' => attention_required,
           'reviewed' => false,
           'approved' => false
         )
@@ -380,8 +380,8 @@ RSpec.describe 'getting merge request information nested in a project' do
     describe 'scalability' do
       let_it_be(:other_users) { create_list(:user, 3) }
 
-      let(:unreviewed) do
-        { 'reviewState' => 'UNREVIEWED' }
+      let(:attention_required) do
+        { 'reviewState' => 'ATTENTION_REQUIRED' }
       end
 
       let(:reviewed) do
@@ -413,9 +413,9 @@ RSpec.describe 'getting merge request information nested in a project' do
           expect { post_graphql(query) }.not_to exceed_query_limit(baseline)
 
           expect(interaction_data).to contain_exactly(
-            include(unreviewed),
-            include(unreviewed),
-            include(unreviewed),
+            include(attention_required),
+            include(attention_required),
+            include(attention_required),
             include(reviewed)
           )
         end
@@ -444,7 +444,7 @@ RSpec.describe 'getting merge request information nested in a project' do
 
   it_behaves_like 'when requesting information about MR interactions' do
     let(:field) { :reviewers }
-    let(:unreviewed) { 'UNREVIEWED' }
+    let(:attention_required) { 'ATTENTION_REQUIRED' }
     let(:can_update) { false }
 
     def assign_user(user)
@@ -454,7 +454,7 @@ RSpec.describe 'getting merge request information nested in a project' do
 
   it_behaves_like 'when requesting information about MR interactions' do
     let(:field) { :assignees }
-    let(:unreviewed) { nil }
+    let(:attention_required) { nil }
     let(:can_update) { true } # assignees can update MRs
 
     def assign_user(user)

spec/support/shared_examples/models/reviewer_state_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'having reviewer state' do
+  describe 'mr_attention_requests feature flag is disabled' do
+    before do
+      stub_feature_flags(mr_attention_requests: false)
+    end
+
+    it { is_expected.to have_attributes(state: 'unreviewed') }
+  end
+
+  describe 'mr_attention_requests feature flag is enabled' do
+    it { is_expected.to have_attributes(state: 'attention_required') }
+  end
+end