From 25c39a3371d479896cf9faff2c237215556e7538 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Tue, 5 Dec 2023 16:06:52 -0500 Subject: [PATCH 1/6] Hide "Merge immediately" button when fast-forward is not possible We've recently implemented automatic rebase on merge for fast-forward merge trains. This had the unintended consequence making the `Merge immediately` button available when it shouldn't be. In this MR, we add logic to hide this button when fast-forward merge trains are enabled and the MR cannot be merged immediately. Part of https://gitlab.com/gitlab-org/gitlab/-/issues/434070 Changelog: fixed EE: true --- .../stores/mr_widget_store.js | 1 + app/serializers/merge_request_poll_widget_entity.rb | 2 ++ .../mixins/ready_to_merge.js | 2 +- .../states/mr_widget_ready_to_merge_spec.js | 13 +++++++++++++ 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 473e8a936fd854..6254de64562b36 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -122,6 +122,7 @@ export default class MergeRequestStore { this.availableAutoMergeStrategies, ); this.ffOnlyEnabled = data.ff_only_enabled; + this.ffMergePossible = data.ff_merge_possilbe; this.isRemovingSourceBranch = this.isRemovingSourceBranch || false; this.mergeRequestState = data.state; this.isOpen = this.mergeRequestState === STATUS_OPEN; diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb index cef3f4555df72d..3374cd46729298 100644 --- a/app/serializers/merge_request_poll_widget_entity.rb +++ b/app/serializers/merge_request_poll_widget_entity.rb @@ -15,6 +15,8 @@ class MergeRequestPollWidgetEntity < Grape::Entity merge_request.project.merge_requests_ff_only_enabled end + expose :ff_merge_possible?, as: :ff_merge_possible + # User entities expose :merge_user, using: UserEntity diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js b/ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js index 36b9b8d828aafb..bd6040f162bbb7 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/mixins/ready_to_merge.js @@ -107,7 +107,7 @@ export default { }, shouldShowMergeImmediatelyDropdown() { if (this.preferredAutoMergeStrategy === MT_MERGE_STRATEGY) { - return true; + return !this.mr.ffOnlyEnabled || this.mr.ffMergePossible; } return this.isPipelineActive && !this.state.onlyAllowMergeIfPipelineSucceeds; diff --git a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js index f88f253cd5e9ca..cee13d5c3c70f2 100644 --- a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -30,6 +30,7 @@ describe('ReadyToMerge', () => { isMergeAllowed: true, onlyAllowMergeIfPipelineSucceeds: false, ffOnlyEnabled: false, + ffMergePossible: false, hasCI: false, ciStatus: null, sha: '12345678', @@ -145,6 +146,18 @@ describe('ReadyToMerge', () => { expect(findMergeImmediatelyDropdown().exists()).toBe(true); }); + it('should return false when fast-forward is required but not possible', () => { + createComponent({ + availableAutoMergeStrategies: [MT_MERGE_STRATEGY], + ffOnlyEnabled: true, + ffMergePossible: false, + headPipeline: { id: 'gid://gitlab/Pipeline/1', path: 'path/to/pipeline', active: false }, + onlyAllowMergeIfPipelineSucceeds: false, + }); + + expect(findMergeImmediatelyDropdown().exists()).toBe(false); + }); + it('should display the new merge dropdown options for merge trains when the skip trains feature flag is enabled', () => { createComponent( { -- GitLab From 651d9634e06ad79eaef25377dac2fa88bd442a20 Mon Sep 17 00:00:00 2001 From: Rudy Crespo Date: Sat, 9 Dec 2023 15:12:40 +0000 Subject: [PATCH 2/6] Fix typo in mr_widget_store.js --- .../vue_merge_request_widget/stores/mr_widget_store.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 6254de64562b36..9ce5448d86e598 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -122,7 +122,7 @@ export default class MergeRequestStore { this.availableAutoMergeStrategies, ); this.ffOnlyEnabled = data.ff_only_enabled; - this.ffMergePossible = data.ff_merge_possilbe; + this.ffMergePossible = data.ff_merge_possible; this.isRemovingSourceBranch = this.isRemovingSourceBranch || false; this.mergeRequestState = data.state; this.isOpen = this.mergeRequestState === STATUS_OPEN; -- GitLab From f56123786a1b789134c7d3ea48f7a9e097883062 Mon Sep 17 00:00:00 2001 From: Rudy Crespo Date: Sat, 9 Dec 2023 15:13:27 +0000 Subject: [PATCH 3/6] Improve test description --- .../components/states/mr_widget_ready_to_merge_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js index cee13d5c3c70f2..e2dd7a3a70cc59 100644 --- a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -146,7 +146,7 @@ describe('ReadyToMerge', () => { expect(findMergeImmediatelyDropdown().exists()).toBe(true); }); - it('should return false when fast-forward is required but not possible', () => { + it('should not be rendered when fast-forward is required but not possible', () => { createComponent({ availableAutoMergeStrategies: [MT_MERGE_STRATEGY], ffOnlyEnabled: true, -- GitLab From f2313b665f8d77e4745f3c5dfe252271c06674ca Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Mon, 11 Dec 2023 22:33:23 -0500 Subject: [PATCH 4/6] Add FE test coverage; refactor to it.each --- .../states/mr_widget_ready_to_merge_spec.js | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js index e2dd7a3a70cc59..7674603d4b4b99 100644 --- a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -136,26 +136,31 @@ describe('ReadyToMerge', () => { expect(findMergeImmediatelyDropdown().exists()).toBe(true); }); - it('should return true when the merge train auto merge stategy is available', () => { - createComponent({ - availableAutoMergeStrategies: [MT_MERGE_STRATEGY], - headPipeline: { id: 'gid://gitlab/Pipeline/1', path: 'path/to/pipeline', active: false }, - onlyAllowMergeIfPipelineSucceeds: true, - }); - - expect(findMergeImmediatelyDropdown().exists()).toBe(true); - }); - - it('should not be rendered when fast-forward is required but not possible', () => { - createComponent({ - availableAutoMergeStrategies: [MT_MERGE_STRATEGY], - ffOnlyEnabled: true, - ffMergePossible: false, - headPipeline: { id: 'gid://gitlab/Pipeline/1', path: 'path/to/pipeline', active: false }, - onlyAllowMergeIfPipelineSucceeds: false, - }); + describe('with merge train auto merge strategy', () => { + it.each` + ffOnlyEnabled | ffMergePossible | isVisible + ${false} | ${false} | ${true} + ${false} | ${true} | ${true} + ${true} | ${false} | ${false} + ${true} | ${true} | ${true} + `( + 'with ffOnlyEnabled $ffOnlyEnabled and ffMergePossible $ffMergePossible should be visible: $isVisible', + ({ ffOnlyEnabled, ffMergePossible, isVisible }) => { + createComponent({ + availableAutoMergeStrategies: [MT_MERGE_STRATEGY], + headPipeline: { + id: 'gid://gitlab/Pipeline/1', + path: 'path/to/pipeline', + active: false, + }, + ffOnlyEnabled: ffOnlyEnabled, + ffMergePossible: ffMergePossible, + onlyAllowMergeIfPipelineSucceeds: true, + }); - expect(findMergeImmediatelyDropdown().exists()).toBe(false); + expect(findMergeImmediatelyDropdown().exists()).toBe(isVisible); + }, + ); }); it('should display the new merge dropdown options for merge trains when the skip trains feature flag is enabled', () => { -- GitLab From bb0a380e54917b1a4348b5a37273f1dd04f52892 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Tue, 12 Dec 2023 12:51:14 -0500 Subject: [PATCH 5/6] Fix eslint errors --- .../components/states/mr_widget_ready_to_merge_spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js index 7674603d4b4b99..5e8367212d4b9a 100644 --- a/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -153,8 +153,8 @@ describe('ReadyToMerge', () => { path: 'path/to/pipeline', active: false, }, - ffOnlyEnabled: ffOnlyEnabled, - ffMergePossible: ffMergePossible, + ffOnlyEnabled, + ffMergePossible, onlyAllowMergeIfPipelineSucceeds: true, }); -- GitLab From 408c2a34aa250009a8cdfc3bdbd22084cd6e7ce7 Mon Sep 17 00:00:00 2001 From: Hordur Freyr Yngvason Date: Tue, 12 Dec 2023 12:55:30 -0500 Subject: [PATCH 6/6] Test for presence of ff_* fields in response --- spec/serializers/merge_request_poll_widget_entity_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/serializers/merge_request_poll_widget_entity_spec.rb b/spec/serializers/merge_request_poll_widget_entity_spec.rb index 6b80609c348625..171f2324cf1fd2 100644 --- a/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_widget_entity_spec.rb @@ -22,6 +22,9 @@ .to eq(resource.default_merge_commit_message(include_description: true)) end + it { is_expected.to include(ff_only_enabled: false) } + it { is_expected.to include(ff_merge_possible: false) } + describe 'new_blob_path' do context 'when user can push to project' do it 'returns path' do -- GitLab