From 0bd68a6ec02cad50296c58502c3fc96e94c532bc Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 4 Sep 2024 20:20:05 +0000 Subject: [PATCH 1/3] Add mergeability check for scheduled merge --- .../detailed_merge_status_enum.rb | 3 + app/models/merge_request.rb | 4 +- .../mergeability/check_merge_time_service.rb | 29 +++++++ doc/api/graphql/reference/index.md | 2 + doc/api/merge_requests.md | 1 + spec/models/merge_request_spec.rb | 3 +- .../check_merge_time_service_spec.rb | 77 +++++++++++++++++++ 7 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 app/services/merge_requests/mergeability/check_merge_time_service.rb create mode 100644 spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb diff --git a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb index ce934458aec842..231cbbee617c6c 100644 --- a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb +++ b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb @@ -66,6 +66,9 @@ class DetailedMergeStatusEnum < BaseEnum value 'SECURITY_POLICIES_EVALUATING', value: :security_policy_evaluation, description: 'All security policies must be evaluated.' + value 'MERGE_TIME', + value: :merge_time, + description: 'Merge request must be merged after the configured time.' end end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 699af3ec43cd25..e3ac518209fc63 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1297,7 +1297,8 @@ def skipped_mergeable_checks(options = {}) skip_locked_paths_check: merge_when_checks_pass_strat, skip_jira_check: merge_when_checks_pass_strat, skip_locked_lfs_files_check: merge_when_checks_pass_strat, - skip_security_policy_check: merge_when_checks_pass_strat + skip_security_policy_check: merge_when_checks_pass_strat, + skip_merge_time_check: merge_when_checks_pass_strat } end @@ -1325,6 +1326,7 @@ def self.mergeable_state_checks # [ ::MergeRequests::Mergeability::CheckOpenStatusService, + ::MergeRequests::Mergeability::CheckMergeTimeService, ::MergeRequests::Mergeability::CheckDraftStatusService, ::MergeRequests::Mergeability::CheckCommitsStatusService, ::MergeRequests::Mergeability::CheckDiscussionsStatusService, diff --git a/app/services/merge_requests/mergeability/check_merge_time_service.rb b/app/services/merge_requests/mergeability/check_merge_time_service.rb new file mode 100644 index 00000000000000..d7834f30908dbc --- /dev/null +++ b/app/services/merge_requests/mergeability/check_merge_time_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class CheckMergeTimeService < CheckBaseService + identifier :merge_time + description 'Checks whether the specified merge time is active' + + def execute + merge_after = merge_request.merge_schedule&.merge_after + return inactive if merge_after.nil? + + if merge_after.future? + failure + else + success + end + end + + def skip? + params[:skip_merge_time_check].present? + end + + def cacheable? + false + end + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 1e683ceb048590..a840a27e060301 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -36665,6 +36665,7 @@ Detailed representation of whether a GitLab merge request can be merged. | `LOCKED_LFS_FILES` | Merge request includes locked LFS files. | | `LOCKED_PATHS` | Merge request includes locked paths. | | `MERGEABLE` | Branch can be merged. | +| `MERGE_TIME` | Merge request must be merged after the configured time. | | `NEED_REBASE` | Merge request needs to be rebased. | | `NOT_APPROVED` | Merge request must be approved before merging. | | `NOT_OPEN` | Merge request must be open before merging. | @@ -37477,6 +37478,7 @@ Representation of mergeability check identifier. | `LOCKED_LFS_FILES` | Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author. | | `LOCKED_PATHS` | Checks whether the merge request contains locked paths. | | `MERGE_REQUEST_BLOCKED` | Checks whether the merge request is blocked. | +| `MERGE_TIME` | Checks whether the specified merge time is active. | | `NEED_REBASE` | Checks whether the merge request needs to be rebased. | | `NOT_APPROVED` | Checks whether the merge request is approved. | | `NOT_OPEN` | Checks whether the merge request is open. | diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index b61888e6e28675..9be6c73a632bb0 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -909,6 +909,7 @@ Use `detailed_merge_status` instead of `merge_status` to account for all potenti [Require associated Jira issue for merge requests to be merged](../integration/jira/issues.md#require-associated-jira-issue-for-merge-requests-to-be-merged). - `mergeable`: The branch can merge cleanly into the target branch. - `merge_request_blocked`: Blocked by another merge request. + - `merge_time`: Must be merged after the configured time. - `need_rebase`: The merge request must be rebased. - `not_approved`: Approval is required before merge. - `not_open`: The merge request must be open before merge. diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2fab1f07887caf..ce5d3f788a6201 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3945,7 +3945,8 @@ def set_compare(merge_request) is_expected.to include(skip_approved_check: skip_checks, skip_draft_check: skip_checks, skip_blocked_check: skip_checks, skip_discussions_check: skip_checks, skip_external_status_check: skip_checks, skip_requested_changes_check: skip_checks, - skip_jira_check: skip_checks, skip_security_policy_check: skip_checks) + skip_jira_check: skip_checks, skip_security_policy_check: skip_checks, + skip_merge_time_check: skip_checks) end end end diff --git a/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb b/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb new file mode 100644 index 00000000000000..fac0ab11780153 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckMergeTimeService, feature_category: :code_review_workflow do + subject(:check_merge_time) { described_class.new(merge_request: merge_request, params: params) } + + let_it_be(:merge_request) { build(:merge_request) } + + let(:params) { { skip_merge_time_check: skip_check } } + let(:skip_check) { false } + + it_behaves_like 'mergeability check service', :merge_time, 'Checks whether the specified merge time is active' + + describe '#execute' do + let(:result) { check_merge_time.execute } + + before do + merge_request.merge_schedule = build( + :merge_request_merge_schedule, + merge_request: merge_request, + merge_after: merge_after + ) + end + + context 'when merge_after is not set' do + let(:merge_after) { nil } + + it 'returns a check result with status inactive' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS + expect(result.payload[:identifier]).to eq(:merge_time) + end + end + + context 'when merge_after is in the future' do + let(:merge_after) { 1.minute.from_now } + + it 'returns a check result with status failed' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + expect(result.payload[:identifier]).to eq(:merge_time) + end + end + + context 'when merge_after is in the past' do + let(:merge_after) { 1.minute.ago } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + expect(result.payload[:identifier]).to eq(:merge_time) + end + end + end + + describe '#skip?' do + context 'when skip check param is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_merge_time.skip?).to eq true + end + end + + context 'when skip check param is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_merge_time.skip?).to eq false + end + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_merge_time.cacheable?).to eq false + end + end +end -- GitLab From 05037dcfe94e1db3dfc2f29686440cb36052ef4d Mon Sep 17 00:00:00 2001 From: Niklas Date: Tue, 1 Oct 2024 17:11:04 +0000 Subject: [PATCH 2/3] Update documentation language --- .../types/merge_requests/detailed_merge_status_enum.rb | 2 +- .../merge_requests/mergeability/check_merge_time_service.rb | 2 +- doc/api/graphql/reference/index.md | 4 ++-- doc/api/merge_requests.md | 2 +- .../mergeability/check_merge_time_service_spec.rb | 3 ++- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb index 231cbbee617c6c..1dfc21f833444f 100644 --- a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb +++ b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb @@ -68,7 +68,7 @@ class DetailedMergeStatusEnum < BaseEnum description: 'All security policies must be evaluated.' value 'MERGE_TIME', value: :merge_time, - description: 'Merge request must be merged after the configured time.' + description: 'Merge request may not be merged until after the specified time.' end end end diff --git a/app/services/merge_requests/mergeability/check_merge_time_service.rb b/app/services/merge_requests/mergeability/check_merge_time_service.rb index d7834f30908dbc..cfdd60044247e5 100644 --- a/app/services/merge_requests/mergeability/check_merge_time_service.rb +++ b/app/services/merge_requests/mergeability/check_merge_time_service.rb @@ -4,7 +4,7 @@ module MergeRequests module Mergeability class CheckMergeTimeService < CheckBaseService identifier :merge_time - description 'Checks whether the specified merge time is active' + description 'Checks whether the merge is blocked due to a scheduled merge time' def execute merge_after = merge_request.merge_schedule&.merge_after diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index a840a27e060301..24179e14c4f951 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -36665,7 +36665,7 @@ Detailed representation of whether a GitLab merge request can be merged. | `LOCKED_LFS_FILES` | Merge request includes locked LFS files. | | `LOCKED_PATHS` | Merge request includes locked paths. | | `MERGEABLE` | Branch can be merged. | -| `MERGE_TIME` | Merge request must be merged after the configured time. | +| `MERGE_TIME` | Merge request may not be merged until after the specified time. | | `NEED_REBASE` | Merge request needs to be rebased. | | `NOT_APPROVED` | Merge request must be approved before merging. | | `NOT_OPEN` | Merge request must be open before merging. | @@ -37478,7 +37478,7 @@ Representation of mergeability check identifier. | `LOCKED_LFS_FILES` | Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author. | | `LOCKED_PATHS` | Checks whether the merge request contains locked paths. | | `MERGE_REQUEST_BLOCKED` | Checks whether the merge request is blocked. | -| `MERGE_TIME` | Checks whether the specified merge time is active. | +| `MERGE_TIME` | Checks whether the merge is blocked due to a scheduled merge time. | | `NEED_REBASE` | Checks whether the merge request needs to be rebased. | | `NOT_APPROVED` | Checks whether the merge request is approved. | | `NOT_OPEN` | Checks whether the merge request is open. | diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 9be6c73a632bb0..b154d67f739d72 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -909,7 +909,7 @@ Use `detailed_merge_status` instead of `merge_status` to account for all potenti [Require associated Jira issue for merge requests to be merged](../integration/jira/issues.md#require-associated-jira-issue-for-merge-requests-to-be-merged). - `mergeable`: The branch can merge cleanly into the target branch. - `merge_request_blocked`: Blocked by another merge request. - - `merge_time`: Must be merged after the configured time. + - `merge_time`: May not be merged until after the specified time. - `need_rebase`: The merge request must be rebased. - `not_approved`: Approval is required before merge. - `not_open`: The merge request must be open before merge. diff --git a/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb b/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb index fac0ab11780153..ba931501a3e082 100644 --- a/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_merge_time_service_spec.rb @@ -10,7 +10,8 @@ let(:params) { { skip_merge_time_check: skip_check } } let(:skip_check) { false } - it_behaves_like 'mergeability check service', :merge_time, 'Checks whether the specified merge time is active' + it_behaves_like 'mergeability check service', :merge_time, + 'Checks whether the merge is blocked due to a scheduled merge time' describe '#execute' do let(:result) { check_merge_time.execute } -- GitLab From 75fb43135cb804a643d14f19e8f2c8aa81e9749c Mon Sep 17 00:00:00 2001 From: Niklas Date: Tue, 1 Oct 2024 18:39:45 +0000 Subject: [PATCH 3/3] Fix N+1 query spec --- app/graphql/resolvers/concerns/resolves_merge_requests.rb | 2 ++ .../graphql/resolvers/concerns/ee/resolves_merge_requests.rb | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/graphql/resolvers/concerns/resolves_merge_requests.rb b/app/graphql/resolvers/concerns/resolves_merge_requests.rb index 7e21e3617f4d99..84a9fcbb227ea1 100644 --- a/app/graphql/resolvers/concerns/resolves_merge_requests.rb +++ b/app/graphql/resolvers/concerns/resolves_merge_requests.rb @@ -62,6 +62,8 @@ def preloads diff_stats_summary: [:metrics], approved_by: [:approved_by_users], merge_after: [:merge_schedule], + mergeable: [:merge_schedule], + detailed_merge_status: [:merge_schedule], milestone: [:milestone], security_auto_fix: [:author], head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }], diff --git a/ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb b/ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb index 3f8abaa64c53a4..2391c1a95a0afb 100644 --- a/ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb +++ b/ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb @@ -8,14 +8,14 @@ module ResolvesMergeRequests def preloads super.tap do |h| - h[:mergeable] = [ + h[:mergeable] += [ *approved_mergeability_check_preloads, *blocked_by_other_mrs_mergeability_check_preloads, *commits_status_mergeability_check_preloads, *security_policy_evaluation_check_preloads ] - h[:detailed_merge_status] = [ + h[:detailed_merge_status] += [ *approved_mergeability_check_preloads, *blocked_by_other_mrs_mergeability_check_preloads, *commits_status_mergeability_check_preloads, -- GitLab