Added review started reviewer state

Added a new state to reviewers, `REVIEW_STARTED`, which will get set
when a reviewer creates a draft note for the first time.

#457990

app/assets/javascripts/sidebar/components/reviewers/uncollapsed_reviewer_list.vue

@@ -28,6 +28,11 @@ const REVIEW_STATE_ICONS = {
     name: 'dash-circle',
     title: __('Awaiting review'),
   },
+  REVIEW_STARTED: {
+    name: 'status_running',
+    class: 'gl-text-blue-500',
+    title: __('Reviewer started review'),
+  },
 };
 
 export default {

app/graphql/types/merge_request_review_state_enum.rb

@@ -15,5 +15,7 @@ class MergeRequestReviewStateEnum < BaseEnum
       description: 'Merge request reviewer has approved the changes.'
     value 'UNAPPROVED', value: 'unapproved',
       description: 'Merge request reviewer removed their approval of the changes.'
+    value 'REVIEW_STARTED', value: 'review_started',
+      description: 'Merge request reviewer has started a review.'
   end
 end

app/models/concerns/merge_request_reviewer_state.rb

@@ -9,7 +9,8 @@ module MergeRequestReviewerState
       reviewed: 1,
       requested_changes: 2,
       approved: 3,
-      unapproved: 4
+      unapproved: 4,
+      review_started: 5
     }
 
     validates :state,

app/services/draft_notes/create_service.rb

@@ -36,11 +36,22 @@ def execute
         merge_request_activity_counter.track_create_review_note_action(user: current_user)
       end
 
+      after_execute
+
       draft_note
     end
 
     private
 
+    def after_execute
+      # Update reviewer state to `REVIEW_STARTED` when a new review has started
+      return unless draft_notes.one?
+
+      ::MergeRequests::UpdateReviewerStateService
+        .new(project: merge_request.project, current_user: current_user)
+        .execute(merge_request, 'review_started')
+    end
+
     def base_error(text)
       DraftNote.new.tap do |draft|
         draft.errors.add(:base, text)

app/services/draft_notes/destroy_service.rb

@@ -10,10 +10,21 @@ def execute(draft = nil)
       clear_highlight_diffs_cache(Array.wrap(drafts))
 
       drafts.is_a?(DraftNote) ? drafts.destroy! : drafts.delete_all
+
+      after_execute
     end
 
     private
 
+    def after_execute
+      # Update reviewer state to `UNREVIEWED` when a new review was deleted
+      return unless draft_notes.empty?
+
+      ::MergeRequests::UpdateReviewerStateService
+        .new(project: merge_request.project, current_user: current_user)
+        .execute(merge_request, 'unreviewed')
+    end
+
     def clear_highlight_diffs_cache(drafts)
       merge_request.diffs.clear_cache if unfolded_drafts?(drafts)
     end

doc/api/graphql/reference/index.md

@@ -35474,6 +35474,7 @@ State of a review of a GitLab merge request.
 | <a id="mergerequestreviewstateapproved"></a>`APPROVED` | Merge request reviewer has approved the changes. |
 | <a id="mergerequestreviewstaterequested_changes"></a>`REQUESTED_CHANGES` | Merge request reviewer has requested changes. |
 | <a id="mergerequestreviewstatereviewed"></a>`REVIEWED` | Merge request reviewer has reviewed. |
+| <a id="mergerequestreviewstatereview_started"></a>`REVIEW_STARTED` | Merge request reviewer has started a review. |
 | <a id="mergerequestreviewstateunapproved"></a>`UNAPPROVED` | Merge request reviewer removed their approval of the changes. |
 | <a id="mergerequestreviewstateunreviewed"></a>`UNREVIEWED` | Awaiting review from merge request reviewer. |

locale/gitlab.pot

@@ -45137,6 +45137,9 @@ msgstr ""
 msgid "Reviewer requested changes"
 msgstr ""
+msgid "Reviewer started review"
+msgstr ""
+
 msgid "Reviewers"
 msgstr ""

spec/graphql/types/merge_request_review_state_enum_spec.rb

@@ -24,6 +24,10 @@
       'UNAPPROVED' => have_attributes(
         description: 'Merge request reviewer removed their approval of the changes.',
         value: 'unapproved'
+      ),
+      'REVIEW_STARTED' => have_attributes(
+        description: 'Merge request reviewer has started a review.',
+        value: 'review_started'
       )
     )
   end

spec/services/draft_notes/create_service_spec.rb

@@ -2,9 +2,9 @@
 require 'spec_helper'
 
 RSpec.describe DraftNotes::CreateService, feature_category: :code_review_workflow do
-  let(:merge_request) { create(:merge_request) }
-  let(:project) { merge_request.target_project }
-  let(:user) { merge_request.author }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:project) { merge_request.target_project }
+  let_it_be(:user) { merge_request.author }
 
   def create_draft(params)
     described_class.new(merge_request, user, params).execute
@@ -44,6 +44,27 @@ def create_draft(params)
     expect(draft).not_to be_persisted
   end
 
+  it 'updates reviewer state on first draft note' do
+    expect_next_instance_of(
+      MergeRequests::UpdateReviewerStateService,
+      project: project, current_user: user
+    ) do |service|
+      expect(service).to receive(:execute).with(merge_request, 'review_started')
+    end
+
+    create_draft(note: 'This is a test')
+  end
+
+  context 'when a draft note has already been created' do
+    let_it_be(:note) { create(:draft_note, merge_request: merge_request, author: user) }
+
+    it 'does not update reviewer state' do
+      expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
+
+      create_draft(note: 'This is a test')
+    end
+  end
+
   context 'in a thread' do
     it 'creates a draft note with discussion_id' do
       discussion = create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion

spec/services/draft_notes/destroy_service_spec.rb

@@ -26,6 +26,19 @@ def destroy(draft_note = nil)
     expect(DraftNote.count).to eq(0)
   end
 
+  it 'updates reviewer state when all draft notes are deleted' do
+    draft = create(:draft_note, merge_request: merge_request, author: user)
+
+    expect_next_instance_of(
+      MergeRequests::UpdateReviewerStateService,
+      project: project, current_user: user
+    ) do |service|
+      expect(service).to receive(:execute).with(merge_request, 'unreviewed')
+    end
+
+    destroy(draft)
+  end
+
   context 'diff highlight cache clearing' do
     context 'when destroying all draft notes of a user' do
       it 'clears highlighting cache if unfold required for any' do

spec/services/merge_requests/update_reviewer_state_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequests::UpdateReviewerStateService, feature_category: :code_review_workflow do
+  using RSpec::Parameterized::TableSyntax
+
   let_it_be(:current_user) { create(:user) }
   let_it_be(:merge_request) { create(:merge_request, reviewers: [current_user]) }
   let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) }
@@ -37,9 +39,26 @@
         expect(result[:status]).to eq :success
       end
 
-      it 'updates reviewers state' do
-        expect(result[:status]).to eq :success
-        expect(reviewer.state).to eq 'requested_changes'
+      context 'when updating reviewer state' do
+        where(:initial_state, :new_state) do
+          'unreviewed'        | 'requested_changes'
+          'unreviewed'        | 'reviewed'
+          'unreviewed'        | 'approved'
+          'unreviewed'        | 'unapproved'
+          'unreviewed'        | 'review_started'
+          'requested_changes' | 'unreviewed'
+        end
+
+        with_them do
+          it do
+            reviewer.update!(state: initial_state)
+
+            result = service.execute(merge_request, new_state)
+
+            expect(result[:status]).to eq :success
+            expect(reviewer.reload.state).to eq new_state
+          end
+        end
       end
 
       it 'calls SystemNoteService.requested_changes' do