diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4e5d1a596ecb78e5d3386ab90acaf85c67cdd105..a31528925e1b2d201f4de236776bf8b26e5edc93 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1252,7 +1252,8 @@ def skipped_mergeable_checks(options = {}) skip_blocked_check: merge_when_checks_pass_strat, skip_discussions_check: merge_when_checks_pass_strat, skip_external_status_check: merge_when_checks_pass_strat, - skip_requested_changes_check: merge_when_checks_pass_strat + skip_requested_changes_check: merge_when_checks_pass_strat, + skip_jira_check: merge_when_checks_pass_strat } end @@ -1264,6 +1265,7 @@ def skipped_mergeable_checks(options = {}) # skip_blocked_check # skip_external_status_check # skip_requested_changes_check + # skip_jira_check def mergeable?(check_mergeability_retry_lease: false, skip_rebase_check: false, **mergeable_state_check_params) return false unless mergeable_state?(**mergeable_state_check_params) @@ -1303,6 +1305,7 @@ def self.all_mergeability_checks # skip_blocked_check # skip_external_status_check # skip_requested_changes_check + # skip_jira_check def mergeable_state?(**params) results = check_mergeability_states(checks: self.class.mergeable_state_checks, **params) @@ -2283,6 +2286,16 @@ def temporarily_unapproved? false end + def has_jira_issue_keys? + return false unless project&.jira_integration&.reference_pattern + + Atlassian::JiraIssueKeyExtractor.has_keys?( + title, + description, + custom_regex: project.jira_integration.reference_pattern + ) + end + private def check_mergeability_states(checks:, execute_all: false, **params) diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 53761ecc3ca10f66f7a7287b2facc80510a8cf05..331c6af09d3e787ccc8344e3b31e3a9aa7a2e2e9 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -1,17 +1,12 @@ import CEGetStateKey from '~/vue_merge_request_widget/stores/get_state_key'; import { MWCP_MERGE_STRATEGY } from '~/vue_merge_request_widget/constants'; import { stateKey as CEStateKey } from '~/vue_merge_request_widget/stores/state_maps'; -import { stateKey } from './state_maps'; export default function getStateKey() { if (this.isGeoSecondaryNode) { return 'geoSecondaryNode'; } - if (this.jiraAssociation?.enforced && this.jiraAssociation?.issue_keys.length === 0) { - return stateKey.jiraAssociationMissing; - } - if (!this.autoMergeEnabled && this.preferredAutoMergeStrategy === MWCP_MERGE_STRATEGY) { return CEStateKey.readyToMerge; } diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 9bdf6236a10c69b4a2c54269c02fddcc09e28679..ee1e9b891e2471f9565490e1a39e19fd340833b5 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -2,11 +2,9 @@ import { stateToComponentMap as ceStateMap } from '~/vue_merge_request_widget/st export const stateKey = { policyViolation: 'policyViolation', - jiraAssociationMissing: 'jiraAssociationMissing', }; export const stateToComponentMap = { ...ceStateMap, geoSecondaryNode: 'mr-widget-geo-secondary-node', policyViolation: 'mr-widget-policy-violation', - jiraAssociationMissing: 'mr-widget-jira-association-missing', }; diff --git a/ee/app/events/merge_requests/jira_title_description_update_event.rb b/ee/app/events/merge_requests/jira_title_description_update_event.rb new file mode 100644 index 0000000000000000000000000000000000000000..91e7e71e1215c3c10e2c88d8f0fdaab1642e40ea --- /dev/null +++ b/ee/app/events/merge_requests/jira_title_description_update_event.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class JiraTitleDescriptionUpdateEvent < Gitlab::EventStore::Event + def schema + { + 'type' => 'object', + 'required' => %w[ + current_user_id + merge_request_id + ], + 'properties' => { + 'current_user_id' => { 'type' => 'integer' }, + 'merge_request_id' => { 'type' => 'integer' } + } + } + end + end +end diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index 15294918b1b3eb1607751af453872f2de9d9a3b8..3628932055f70778caad2602f1336db44bfffa6e 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -10,6 +10,7 @@ def handle_changes(merge_request, options) super handle_override_requested_changes(merge_request, merge_request.previous_changes) + handle_title_and_desc_edits(merge_request, merge_request.previous_changes.keys) end private @@ -84,6 +85,22 @@ def handle_override_requested_changes(merge_request, changed_fields) ) ) end + + def handle_title_and_desc_edits(merge_request, changed_fields) + return unless ::Feature.enabled?(:merge_when_checks_pass, merge_request.project) + + fields = %w[title description] + + return unless changed_fields.any? { |field| fields.include?(field) } + + return unless merge_request.has_jira_issue_keys? + + ::Gitlab::EventStore.publish( + ::MergeRequests::JiraTitleDescriptionUpdateEvent.new( + data: { current_user_id: current_user.id, merge_request_id: merge_request.id } + ) + ) + end end end end diff --git a/ee/app/services/merge_requests/mergeability/check_jira_status_service.rb b/ee/app/services/merge_requests/mergeability/check_jira_status_service.rb index cb3551ca722fafa7c5c4ceadda0ad253ef8dc7cb..a36eb37a1f608a39173e7d75a5cd762a49b3a8d8 100644 --- a/ee/app/services/merge_requests/mergeability/check_jira_status_service.rb +++ b/ee/app/services/merge_requests/mergeability/check_jira_status_service.rb @@ -9,7 +9,7 @@ class CheckJiraStatusService < CheckBaseService def execute return inactive unless merge_request.project.prevent_merge_without_jira_issue? - if has_jira_issue_keys? + if merge_request.has_jira_issue_keys? success else failure(reason: identifier) @@ -17,22 +17,12 @@ def execute end def skip? - false + params[:skip_jira_check].present? end def cacheable? false end - - private - - def has_jira_issue_keys? - Atlassian::JiraIssueKeyExtractor.has_keys?( - merge_request.title, - merge_request.description, - custom_regex: merge_request.project.jira_integration.reference_pattern - ) - end end end end diff --git a/ee/lib/ee/gitlab/event_store.rb b/ee/lib/ee/gitlab/event_store.rb index fa6c5ce5d593cc75a2cf403132c19e7a82c68127..3162ac5956d07d762cca8326f51af236e73d4f72 100644 --- a/ee/lib/ee/gitlab/event_store.rb +++ b/ee/lib/ee/gitlab/event_store.rb @@ -33,6 +33,8 @@ def configure!(store) store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::ExternalStatusCheckPassedEvent store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::UnblockedStateEvent + store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, + to: ::MergeRequests::JiraTitleDescriptionUpdateEvent store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::OverrideRequestedChangesStateEvent store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::ApprovedEvent diff --git a/ee/spec/frontend/license_compliance/store/get_state_key_spec.js b/ee/spec/frontend/license_compliance/store/get_state_key_spec.js index c68b11b87d67c3fc5f7f8015197f2e763fd6e589..111a945ff2778da1d336d538a5822b12eaa9ea42 100644 --- a/ee/spec/frontend/license_compliance/store/get_state_key_spec.js +++ b/ee/spec/frontend/license_compliance/store/get_state_key_spec.js @@ -7,25 +7,6 @@ describe('getStateKey', () => { commitsCount: 2, }; - describe('jiraAssociationMissing', () => { - const createContext = (enforced, hasIssues) => ({ - ...canMergeContext, - jiraAssociation: { - enforced, - issue_keys: hasIssues ? [1] : [], - }, - }); - - it.each` - scenario | enforced | hasIssues | state - ${'enforced without issues'} | ${true} | ${false} | ${'jiraAssociationMissing'} - `('when $scenario, state should equal $state', ({ enforced, hasIssues, state }) => { - const bound = getStateKey.bind(createContext(enforced, hasIssues)); - - expect(bound()).toBe(state); - }); - }); - describe('AutoMergeStrategy "merge_when_checks_pass"', () => { const createContext = (detailedMergeStatus, preferredAutoMergeStrategy, autoMergeEnabled) => ({ ...canMergeContext, diff --git a/ee/spec/lib/ee/gitlab/event_store_spec.rb b/ee/spec/lib/ee/gitlab/event_store_spec.rb index 606e85ee3517e32692e26494ad73d990eafd74e8..f63a1ac3f5684fbeec5ec952336a3da17d6bc934 100644 --- a/ee/spec/lib/ee/gitlab/event_store_spec.rb +++ b/ee/spec/lib/ee/gitlab/event_store_spec.rb @@ -12,6 +12,7 @@ ::Ci::PipelineCreatedEvent, ::Repositories::KeepAroundRefsCreatedEvent, ::MergeRequests::ApprovedEvent, + ::MergeRequests::JiraTitleDescriptionUpdateEvent, ::MergeRequests::ApprovalsResetEvent, ::MergeRequests::DraftStateChangeEvent, ::MergeRequests::UnblockedStateEvent, diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb index 44d7ab43aa28fb04246b79627d01b22bc45eb187..6eb5f8326195b021d9acd663cd7173898a5406f9 100644 --- a/ee/spec/services/ee/merge_requests/update_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb @@ -577,5 +577,85 @@ def update_merge_request(opts) ) end end + + describe 'JiraTitleDescriptionUpdateEvent' do + let(:has_jira_key) { true } + + before do + allow(merge_request).to receive(:has_jira_issue_keys?).and_return(has_jira_key) + end + + context 'when the merge_when_checks_pass feature flag is on' do + context 'when the description changes' do + context 'when the description or title has a jira key' do + it 'publishes a JiraTitleDescriptionUpdateEvent' do + expected_data = { + current_user_id: user.id, + merge_request_id: merge_request.id + } + + expect do + update_merge_request(description: 'New description') + end.to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent).with(expected_data) + end + end + + context 'when the description or title does not have a jira key' do + let(:has_jira_key) { false } + + it 'does not publish a JiraTitleDescriptionUpdateEvent' do + expect do + update_merge_request(description: 'New description') + end.not_to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent) + end + end + end + + context 'when the title changes' do + it 'publishes a JiraTitleDescriptionUpdateEvent' do + expected_data = { + current_user_id: user.id, + merge_request_id: merge_request.id + } + + expect do + update_merge_request(title: 'New title') + end.to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent).with(expected_data) + end + + context 'when the description or title does not have a jira key' do + let(:has_jira_key) { false } + + it 'does not publish a JiraTitleDescriptionUpdateEvent' do + expect do + update_merge_request(description: 'New description') + end.not_to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent) + end + end + end + end + + context 'when the merge_when_checks_pass feature flag is off' do + before do + stub_feature_flags(merge_when_checks_pass: false) + end + + context 'when the description changes' do + it 'does not publish a JiraTitleDescriptionUpdateEvent' do + expect do + update_merge_request(description: 'New description') + end.not_to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent) + end + end + + context 'when the title changes' do + it 'publishes a JiraTitleDescriptionUpdateEvent' do + expect do + update_merge_request(title: 'New title') + end.not_to publish_event(MergeRequests::JiraTitleDescriptionUpdateEvent) + end + end + end + end end end diff --git a/ee/spec/services/merge_requests/mergeability/check_jira_status_service_spec.rb b/ee/spec/services/merge_requests/mergeability/check_jira_status_service_spec.rb index 5400675ff76838d737629614aa2b9ff60ed96b86..cd7950a7b45708739f82bc3053fa15046e7c619c 100644 --- a/ee/spec/services/merge_requests/mergeability/check_jira_status_service_spec.rb +++ b/ee/spec/services/merge_requests/mergeability/check_jira_status_service_spec.rb @@ -8,7 +8,8 @@ let_it_be(:jira_integration) { create(:jira_integration) } let_it_be(:project) { build(:project) } let_it_be(:merge_request) { build(:merge_request, source_project: project) } - let(:params) { {} } + let(:params) { { skip_jira_check: skip_check } } + let(:skip_check) { false } it_behaves_like 'mergeability check service', :jira_association_missing, 'Checks whether the title or description references a Jira issue.' @@ -72,8 +73,20 @@ end describe '#skip?' do - it 'returns false' do - expect(check_jira_status.skip?).to eq false + context 'when skip check param is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_jira_status.skip?).to eq true + end + end + + context 'when skip check param is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_jira_status.skip?).to eq false + end end end diff --git a/ee/spec/workers/merge_requests/process_auto_merge_from_event_worker_spec.rb b/ee/spec/workers/merge_requests/process_auto_merge_from_event_worker_spec.rb index 548147bc93947cf9f685a9a7842833f69e5d03de..1059c832e692a2331c2dd6cb49fff47eb34d1e43 100644 --- a/ee/spec/workers/merge_requests/process_auto_merge_from_event_worker_spec.rb +++ b/ee/spec/workers/merge_requests/process_auto_merge_from_event_worker_spec.rb @@ -2,7 +2,6 @@ require 'spec_helper' -# TODO - Currently in EE because the events are only from EE code at the moment RSpec.describe MergeRequests::ProcessAutoMergeFromEventWorker, feature_category: :code_review_workflow do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } @@ -11,47 +10,6 @@ let(:data) { { current_user_id: user.id, merge_request_id: merge_request_id } } - shared_examples 'process auto merge from event worker' do - it_behaves_like 'subscribes to event' do - it 'calls AutoMergeService' do - expect_next_instance_of( - AutoMergeService, - project, user - ) do |service| - expect(service).to receive(:process).with(merge_request) - end - - consume_event(subscriber: described_class, event: event) - end - - context 'when the merge request does not exist' do - let(:merge_request_id) { -1 } - - it 'logs and does not call AutoMergeService' do - expect(Sidekiq.logger).to receive(:info).with( - hash_including('message' => 'Merge request not found.', 'merge_request_id' => merge_request_id) - ) - expect(AutoMergeService).not_to receive(:new) - - expect { consume_event(subscriber: described_class, event: event) } - .not_to raise_exception - end - end - - context 'when feature flag "merge_when_checks_pass" is disabled' do - before do - stub_feature_flags(merge_when_checks_pass: false) - end - - it "doesn't call AutoMergeService" do - expect(AutoMergeService).not_to receive(:new) - - consume_event(subscriber: described_class, event: event) - end - end - end - end - it_behaves_like 'process auto merge from event worker' do let(:event) { MergeRequests::ApprovedEvent.new(data: data) } end @@ -75,4 +33,8 @@ it_behaves_like 'process auto merge from event worker' do let(:event) { ::MergeRequests::DiscussionsResolvedEvent.new(data: data) } end + + it_behaves_like 'process auto merge from event worker' do + let(:event) { MergeRequests::JiraTitleDescriptionUpdateEvent.new(data: data) } + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 666243b809eae0a468238e8bb3c9d017065a229d..bf61a7a664ea8e68ce245987d2b7dcb150da9071 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3752,17 +3752,18 @@ def set_compare(merge_request) let(:options) { { auto_merge_requested: true, auto_merge_strategy: auto_merge_strategy } } where(:auto_merge_strategy, :skip_approved_check, :skip_draft_check, :skip_blocked_check, - :skip_discussions_check, :skip_external_status_check, :skip_requested_changes_check) do - '' | false | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true + :skip_discussions_check, :skip_external_status_check, :skip_requested_changes_check, :skip_jira_check) do + '' | false | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true | true end with_them do it do is_expected.to include(skip_approved_check: skip_approved_check, skip_draft_check: skip_draft_check, skip_blocked_check: skip_blocked_check, skip_discussions_check: skip_discussions_check, - skip_external_status_check: skip_external_status_check, skip_requested_changes_check: skip_requested_changes_check) + skip_external_status_check: skip_external_status_check, skip_requested_changes_check: skip_requested_changes_check, + skip_jira_check: skip_jira_check) end end end @@ -6581,6 +6582,52 @@ def transition! it { is_expected.to eq(false) } end + describe '#has_jira_issue_keys?' do + let(:merge_request) { build_stubbed(:merge_request) } + + subject(:has_jira_issue_keys) { merge_request.has_jira_issue_keys? } + + context 'when project has jira integration' do + let(:jira_integration) { build(:jira_integration) } + + before do + allow(merge_request.project).to receive(:jira_integration).and_return(jira_integration) + end + + context 'when the merge request title has a key' do + before do + merge_request.title = 'PROJECT-1' + end + + it 'returns true' do + expect(has_jira_issue_keys).to be_truthy + end + end + + context 'when the merge request title has a key' do + before do + merge_request.description = 'PROJECT-1' + end + + it 'returns true' do + expect(has_jira_issue_keys).to be_truthy + end + end + + context 'when the merge request does not have a key' do + it 'returns false' do + expect(has_jira_issue_keys).to be_falsey + end + end + end + + context 'when project does not have jira integration' do + it 'returns false' do + expect(has_jira_issue_keys).to be_falsey + end + end + end + describe '#allows_multiple_assignees?' do let(:merge_request) { build_stubbed(:merge_request) } diff --git a/spec/support/shared_examples/workers/auto_merge_from_event_shared_examples.rb b/spec/support/shared_examples/workers/auto_merge_from_event_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..9db3ef43a85cd8bc898ed399fc2ccb3438471730 --- /dev/null +++ b/spec/support/shared_examples/workers/auto_merge_from_event_shared_examples.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'process auto merge from event worker' do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, merge_user: user) } + let(:merge_request_id) { merge_request.id } + + let(:data) { { current_user_id: user.id, merge_request_id: merge_request_id } } + + it_behaves_like 'subscribes to event' do + it 'calls AutoMergeService' do + expect_next_instance_of( + AutoMergeService, + project, user + ) do |service| + expect(service).to receive(:process).with(merge_request) + end + + consume_event(subscriber: described_class, event: event) + end + + context 'when the merge request does not exist' do + let(:merge_request_id) { -1 } + + it 'logs and does not call AutoMergeService' do + expect(Sidekiq.logger).to receive(:info).with( + hash_including('message' => 'Merge request not found.', 'merge_request_id' => merge_request_id) + ) + expect(AutoMergeService).not_to receive(:new) + + expect { consume_event(subscriber: described_class, event: event) } + .not_to raise_exception + end + end + + context 'when feature flag "merge_when_checks_pass" is disabled' do + before do + stub_feature_flags(merge_when_checks_pass: false) + end + + it "doesn't call AutoMergeService" do + expect(AutoMergeService).not_to receive(:new) + + consume_event(subscriber: described_class, event: event) + end + end + end +end