diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb index fab7a227e7d71f774fd6b13e74d337b84d64aeae..9e1e381c568c57f65992977b75edfc1e097703df 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/app/services/draft_notes/publish_service.rb @@ -59,7 +59,8 @@ def create_note_from_draft(draft, skip_capture_diff_note_position: false, skip_k note_params = draft.publish_params.merge(skip_keep_around_commits: skip_keep_around_commits) note = Notes::CreateService.new(draft.project, draft.author, note_params).execute( skip_capture_diff_note_position: skip_capture_diff_note_position, - skip_merge_status_trigger: skip_merge_status_trigger + skip_merge_status_trigger: skip_merge_status_trigger, + skip_set_reviewed: true ) set_discussion_resolve_status(note, draft) diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 39d0d0a79233d2cd9da26e541f663d666634d8f0..9f554544c5292223498d3a92511b6c94700b4f53 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -4,7 +4,7 @@ module Notes class CreateService < ::Notes::BaseService include IncidentManagement::UsageData - def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: false) + def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: false, skip_set_reviewed: false) note = Notes::BuildService.new(project, current_user, params.except(:merge_request_diff_head_sha)).execute # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440 @@ -38,7 +38,8 @@ def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: f when_saved( note, skip_capture_diff_note_position: skip_capture_diff_note_position, - skip_merge_status_trigger: skip_merge_status_trigger + skip_merge_status_trigger: skip_merge_status_trigger, + skip_set_reviewed: skip_set_reviewed ) end end @@ -79,7 +80,9 @@ def update_discussions(note) end end - def when_saved(note, skip_capture_diff_note_position: false, skip_merge_status_trigger: false) + def when_saved( + note, skip_capture_diff_note_position: false, skip_merge_status_trigger: false, + skip_set_reviewed: false) todo_service.new_note(note, current_user) clear_noteable_diffs_cache(note) Suggestions::CreateService.new(note).execute @@ -87,6 +90,8 @@ def when_saved(note, skip_capture_diff_note_position: false, skip_merge_status_t track_event(note, current_user) if note.for_merge_request? && note.start_of_discussion? + set_reviewed(note) unless skip_set_reviewed + if !skip_capture_diff_note_position && note.diff_note? Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion) end @@ -208,6 +213,11 @@ def track_note_creation_in_ipynb(note) def track_note_creation_visual_review(note) Gitlab::Tracking.event('Notes::CreateService', 'execute', **tracking_data_for(note)) end + + def set_reviewed(note) + ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user) + .execute(note.noteable) + end end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 582f5037e483e4f6ce05b085ddeb1b77f169586e..593b67be5d564bd9c8f9b445719e12dd6c879a76 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -2910,6 +2910,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: merge_requests_set_reviewer_reviewed + :worker_name: MergeRequests::SetReviewerReviewedWorker + :feature_category: :code_review_workflow + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: merge_requests_update_head_pipeline :worker_name: MergeRequests::UpdateHeadPipelineWorker :feature_category: :code_review_workflow diff --git a/app/workers/merge_requests/set_reviewer_reviewed_worker.rb b/app/workers/merge_requests/set_reviewer_reviewed_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..2f15bf3b8795c261a6cf1e8c1f177acb64ae9e85 --- /dev/null +++ b/app/workers/merge_requests/set_reviewer_reviewed_worker.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module MergeRequests + class SetReviewerReviewedWorker + include Gitlab::EventStore::Subscriber + + data_consistency :always + feature_category :code_review_workflow + urgency :low + idempotent! + + def handle_event(event) + current_user_id = event.data[:current_user_id] + merge_request_id = event.data[:merge_request_id] + current_user = User.find_by_id(current_user_id) + merge_request = MergeRequest.find_by_id(merge_request_id) + + if !current_user + logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id)) + elsif !merge_request + logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id)) + else + project = merge_request.source_project + + ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user) + .execute(merge_request) + end + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 2f66e4d25dc4056d07e487297bfde06ea24a6c6f..cd5d1cdca2ebd521d48cac22af5fac445c867d85 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -333,6 +333,8 @@ - 1 - - merge_requests_resolve_todos_after_approval - 1 +- - merge_requests_set_reviewer_reviewed + - 1 - - merge_requests_stream_approval_audit_event - 1 - - merge_requests_sync_code_owner_approval_rules diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb index c017396c8e870a638d7e105e21f8a423972dfbe2..ce71ee594f24b9dd63024ac4d7d6fc7ba287a455 100644 --- a/lib/gitlab/event_store.rb +++ b/lib/gitlab/event_store.rb @@ -60,6 +60,7 @@ def self.configure!(store) store.subscribe ::MergeRequests::CreateApprovalNoteWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::MergeRequests::ResolveTodosAfterApprovalWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::MergeRequests::ExecuteApprovalHooksWorker, to: ::MergeRequests::ApprovedEvent + store.subscribe ::MergeRequests::SetReviewerReviewedWorker, to: ::MergeRequests::ApprovedEvent store.subscribe ::Ml::ExperimentTracking::AssociateMlCandidateToPackageWorker, to: ::Packages::PackageCreatedEvent, if: -> (event) { ::Ml::ExperimentTracking::AssociateMlCandidateToPackageWorker.handles_event?(event) } diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 7a31fd6a77ddc4724dadffcc108e0f11c52c49f0..b1438516dde0dddcb4bc7edd7326fab6458ee80e 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -172,6 +172,25 @@ create(:merge_request, source_project: project_with_repo, target_project: project_with_repo) end + let(:new_opts) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) } + + it 'calls MergeRequests::MarkReviewerReviewedService service' do + expect_next_instance_of( + MergeRequests::MarkReviewerReviewedService, + project: project_with_repo, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + described_class.new(project_with_repo, user, new_opts).execute + end + + it 'does not call MergeRequests::MarkReviewerReviewedService service when skip_set_reviewed is true' do + expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new) + + described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true) + end + context 'noteable highlight cache clearing' do let(:position) do Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", diff --git a/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb b/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..942cf8e87e9e52822658a1fdc213bc420883a963 --- /dev/null +++ b/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::SetReviewerReviewedWorker, feature_category: :source_code_management do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + let(:data) { { current_user_id: user.id, merge_request_id: merge_request.id } } + let(:approved_event) { MergeRequests::ApprovedEvent.new(data: data) } + + it_behaves_like 'subscribes to event' do + let(:event) { approved_event } + end + + it 'calls MergeRequests::MarkReviewerReviewedService' do + expect_next_instance_of( + MergeRequests::MarkReviewerReviewedService, + project: project, current_user: user + ) do |service| + expect(service).to receive(:execute).with(merge_request) + end + + consume_event(subscriber: described_class, event: approved_event) + end + + shared_examples 'when object does not exist' do + it 'logs and does not call MergeRequests::MarkReviewerReviewedService' do + expect(Sidekiq.logger).to receive(:info).with(hash_including(log_payload)) + expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new) + + expect { consume_event(subscriber: described_class, event: approved_event) } + .not_to raise_exception + end + end + + context 'when the user does not exist' do + before do + user.destroy! + end + + it_behaves_like 'when object does not exist' do + let(:log_payload) { { 'message' => 'Current user not found.', 'current_user_id' => user.id } } + end + end + + context 'when the merge request does not exist' do + before do + merge_request.destroy! + end + + it_behaves_like 'when object does not exist' do + let(:log_payload) { { 'message' => 'Merge request not found.', 'merge_request_id' => merge_request.id } } + end + end +end