Merge branch 'ph/updatedStateByUser' into 'master'

Added updated_state_by_user to reviewers and assignees

See merge request gitlab-org/gitlab!75927

app/models/concerns/merge_request_reviewer_state.rb

@@ -14,6 +14,14 @@ module MergeRequestReviewerState
       presence: true,
       inclusion: { in: self.states.keys }
 
+    belongs_to :updated_state_by, class_name: 'User', foreign_key: :updated_state_by_user_id
+
     after_initialize :set_state, unless: :persisted?
+
+    def attention_requested_by
+      return unless attention_requested?
+
+      updated_state_by
+    end
   end
 end

app/models/merge_request.rb

@@ -1936,10 +1936,18 @@ class MergeRequest < ApplicationRecord
     merge_request_assignees.find_by(user_id: user.id)
   end
 
+  def merge_request_assignees_with(user_ids)
+    merge_request_assignees.where(user_id: user_ids)
+  end
+
   def find_reviewer(user)
     merge_request_reviewers.find_by(user_id: user.id)
   end
 
+  def merge_request_reviewers_with(user_ids)
+    merge_request_reviewers.where(user_id: user_ids)
+  end
+
   def enabled_reports
     {
       sast: report_type_enabled?(:sast),

app/services/merge_requests/base_service.rb

@@ -61,6 +61,8 @@ module MergeRequests
 
       unless new_reviewers.include?(current_user)
         remove_attention_requested(merge_request, current_user)
+
+        merge_request.merge_request_reviewers_with(new_reviewers).update_all(updated_state_by_user_id: current_user.id)
       end
     end
 

app/services/merge_requests/handle_assignees_change_service.rb

@@ -21,6 +21,8 @@ module MergeRequests
       merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees)
       merge_request_activity_counter.track_assignees_changed_action(user: current_user)
 
+      merge_request.merge_request_assignees_with(new_assignees).update_all(updated_state_by_user_id: current_user.id)
+
       execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks]
 
       unless new_assignees.include?(current_user)

app/services/merge_requests/toggle_attention_requested_service.rb

@@ -59,7 +59,8 @@ module MergeRequests
     end
 
     def update_state(reviewer_or_assignee)
-      reviewer_or_assignee&.update(state: reviewer_or_assignee&.attention_requested? ? :reviewed : :attention_requested)
+      reviewer_or_assignee&.update(state: reviewer_or_assignee&.attention_requested? ? :reviewed : :attention_requested,
+        updated_state_by: current_user)
     end
   end
 end

db/migrate/20211203160952_add_updated_state_by_user_id_to_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddUpdatedStateByUserIdToMergeRequestReviewers < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    add_column :merge_request_reviewers, :updated_state_by_user_id, :bigint
+  end
+end

db/migrate/20211203161149_add_index_to_merge_request_reviewers_updated_state_by_user_id.rb

+# frozen_string_literal: true
+
+class AddIndexToMergeRequestReviewersUpdatedStateByUserId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_on_merge_request_reviewers_updated_state_by_user_id'
+
+  def up
+    add_concurrent_index :merge_request_reviewers, :updated_state_by_user_id, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :merge_request_reviewers, INDEX_NAME
+  end
+end

db/migrate/20211203161840_add_updated_state_by_user_id_to_merge_request_assignees.rb

+# frozen_string_literal: true
+
+class AddUpdatedStateByUserIdToMergeRequestAssignees < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    add_column :merge_request_assignees, :updated_state_by_user_id, :bigint
+  end
+end

db/migrate/20211203161942_add_index_to_merge_request_assignees_updated_state_by_user_id.rb

+# frozen_string_literal: true
+
+class AddIndexToMergeRequestAssigneesUpdatedStateByUserId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_on_merge_request_assignees_updated_state_by_user_id'
+
+  def up
+    add_concurrent_index :merge_request_assignees, :updated_state_by_user_id, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :merge_request_assignees, INDEX_NAME
+  end
+end

db/migrate/20220105152547_add_foreign_key_to_updated_state_by_user_id_to_merge_request_assignees.rb

+# frozen_string_literal: true
+
+class AddForeignKeyToUpdatedStateByUserIdToMergeRequestAssignees < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_request_assignees, :users, column: :updated_state_by_user_id, on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_request_assignees, column: :updated_state_by_user_id
+    end
+  end
+end

db/migrate/20220105153149_add_foreign_key_to_updated_state_by_user_id_to_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+class AddForeignKeyToUpdatedStateByUserIdToMergeRequestReviewers < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :merge_request_reviewers, :users, column: :updated_state_by_user_id, on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_request_reviewers, column: :updated_state_by_user_id
+    end
+  end
+end

db/schema_migrations/20211203160952

+f25c65dfcb5b7b4a663cc4c792ffd985f6afd3156036485a5a43a791ee799e7b
\ No newline at end of file

db/schema_migrations/20211203161149

+01482a299a7dac9d3f786f0dbe4650c686911bf788467146d3e9a91eafd0fc32
\ No newline at end of file

db/schema_migrations/20211203161840

+1b895e979ba2f1696559179c46c000e349da2d1ab94c968dd95103f188425103
\ No newline at end of file

db/schema_migrations/20211203161942

+62432b2679cafa381671c9555f503867c254a7b3734e10cf634b34998d5fb5a3
\ No newline at end of file

db/schema_migrations/20220105152547

+0f1ea41fae57710e0e05c9b71a14800394c4c57e37a39e92be49c50120d7d2ee
\ No newline at end of file

db/schema_migrations/20220105153149

+8194c695a809f2eb29e5033f089c1d20874f61731a4289026f2d550854e7097d
\ No newline at end of file

db/structure.sql

@@ -16236,7 +16236,8 @@ CREATE TABLE merge_request_assignees (
     user_id integer NOT NULL,
     merge_request_id integer NOT NULL,
     created_at timestamp with time zone,
-    state smallint DEFAULT 0 NOT NULL
+    state smallint DEFAULT 0 NOT NULL,
+    updated_state_by_user_id bigint
 );
 
 CREATE SEQUENCE merge_request_assignees_id_seq
@@ -16462,7 +16463,8 @@ CREATE TABLE merge_request_reviewers (
     user_id bigint NOT NULL,
     merge_request_id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
-    state smallint DEFAULT 0 NOT NULL
+    state smallint DEFAULT 0 NOT NULL,
+    updated_state_by_user_id bigint
 );
 
 CREATE SEQUENCE merge_request_reviewers_id_seq
@@ -27232,8 +27234,12 @@ CREATE INDEX index_on_label_links_all_columns ON label_links USING btree (target
 
 CREATE INDEX index_on_merge_request_assignees_state ON merge_request_assignees USING btree (state) WHERE (state = 2);
 
+CREATE INDEX index_on_merge_request_assignees_updated_state_by_user_id ON merge_request_assignees USING btree (updated_state_by_user_id);
+
 CREATE INDEX index_on_merge_request_reviewers_state ON merge_request_reviewers USING btree (state) WHERE (state = 2);
 
+CREATE INDEX index_on_merge_request_reviewers_updated_state_by_user_id ON merge_request_reviewers USING btree (updated_state_by_user_id);
+
 CREATE INDEX index_on_merge_requests_for_latest_diffs ON merge_requests USING btree (target_project_id) INCLUDE (id, latest_merge_request_diff_id);
 
 COMMENT ON INDEX index_on_merge_requests_for_latest_diffs IS 'Index used to efficiently obtain the oldest merge request for a commit SHA';
@@ -29678,6 +29684,9 @@ ALTER TABLE ONLY epics
 ALTER TABLE ONLY ci_pipelines
     ADD CONSTRAINT fk_3d34ab2e06 FOREIGN KEY (pipeline_schedule_id) REFERENCES ci_pipeline_schedules(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY merge_request_reviewers
+    ADD CONSTRAINT fk_3d674b9f23 FOREIGN KEY (updated_state_by_user_id) REFERENCES users(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY ci_pipeline_schedule_variables
     ADD CONSTRAINT fk_41c35fda51 FOREIGN KEY (pipeline_schedule_id) REFERENCES ci_pipeline_schedules(id) ON DELETE CASCADE;
 
@@ -30002,6 +30011,9 @@ ALTER TABLE ONLY merge_request_metrics
 ALTER TABLE ONLY dast_profile_schedules
     ADD CONSTRAINT fk_aef03d62e5 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY merge_request_assignees
+    ADD CONSTRAINT fk_af036e3261 FOREIGN KEY (updated_state_by_user_id) REFERENCES users(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY analytics_cycle_analytics_group_stages
     ADD CONSTRAINT fk_analytics_cycle_analytics_group_stages_group_value_stream_id FOREIGN KEY (group_value_stream_id) REFERENCES analytics_cycle_analytics_group_value_streams(id) ON DELETE CASCADE;
 

spec/models/merge_request_assignee_spec.rb

@@ -51,4 +51,24 @@ RSpec.describe MergeRequestAssignee do
 
     it { is_expected.to have_attributes(state: 'reviewed') }
   end
+
+  describe '#attention_requested_by' do
+    let(:current_user) { create(:user) }
+
+    before do
+      subject.update!(updated_state_by: current_user)
+    end
+
+    context 'attention requested' do
+      it { expect(subject.attention_requested_by).to eq(current_user) }
+    end
+
+    context 'attention requested' do
+      before do
+        subject.update!(state: :reviewed)
+      end
+
+      it { expect(subject.attention_requested_by).to eq(nil) }
+    end
+  end
 end

spec/models/merge_request_reviewer_spec.rb

@@ -25,4 +25,24 @@ RSpec.describe MergeRequestReviewer 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) }
   end
+
+  describe '#attention_requested_by' do
+    let(:current_user) { create(:user) }
+
+    before do
+      subject.update!(updated_state_by: current_user)
+    end
+
+    context 'attention requested' do
+      it { expect(subject.attention_requested_by).to eq(current_user) }
+    end
+
+    context 'attention requested' do
+      before do
+        subject.update!(state: :reviewed)
+      end
+
+      it { expect(subject.attention_requested_by).to eq(nil) }
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -5109,4 +5109,34 @@ RSpec.describe MergeRequest, factory_default: :keep do
       let!(:model) { create(:merge_request, head_pipeline: parent) }
     end
   end
+
+  describe '#merge_request_reviewers_with' do
+    let_it_be(:reviewer1) { create(:user) }
+    let_it_be(:reviewer2) { create(:user) }
+
+    before do
+      subject.update!(reviewers: [reviewer1, reviewer2])
+    end
+
+    it 'returns reviewers' do
+      reviewers = subject.merge_request_reviewers_with([reviewer1.id])
+
+      expect(reviewers).to match_array([subject.merge_request_reviewers[0]])
+    end
+  end
+
+  describe '#merge_request_assignees_with' do
+    let_it_be(:assignee1) { create(:user) }
+    let_it_be(:assignee2) { create(:user) }
+
+    before do
+      subject.update!(assignees: [assignee1, assignee2])
+    end
+
+    it 'returns assignees' do
+      assignees = subject.merge_request_assignees_with([assignee1.id])
+
+      expect(assignees).to match_array([subject.merge_request_assignees[0]])
+    end
+  end
 end

spec/services/merge_requests/handle_assignees_change_service_spec.rb

@@ -95,6 +95,12 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do
       execute
     end
 
+    it 'updates attention requested by of assignee' do
+      execute
+
+      expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user)
+    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/toggle_attention_requested_service_spec.rb

@@ -59,6 +59,13 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do
         expect(reviewer.state).to eq 'attention_requested'
       end
 
+      it 'adds who toggled attention' do
+        service.execute
+        reviewer.reload
+
+        expect(reviewer.updated_state_by).to eq current_user
+      end
+
       it 'creates a new todo for the reviewer' do
         expect(todo_service).to receive(:create_attention_requested_todo).with(merge_request, current_user, user)
 

spec/services/merge_requests/update_service_spec.rb

@@ -215,6 +215,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
 
               MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request)
             end
+
+            it 'updates attention requested by of reviewer' do
+              opts[:reviewers] = [user2]
+
+              MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request)
+
+              expect(merge_request.find_reviewer(user2).updated_state_by).to eq(user)
+            end
           end
 
           context 'when reviewers did not change' do