From ba4743b612d0fdcbc28e43bad118fa234b646711 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 15 Sep 2017 16:24:14 -0500 Subject: [PATCH 1/2] Move fast-forward merge request settings around for CE port See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14272 --- ...ge_request_fast_forward_settings.html.haml | 13 +++++ .../_merge_request_settings.html.haml | 13 ++++- app/views/projects/edit.html.haml | 2 +- ...ge_request_fast_forward_settings.html.haml | 14 +++++ .../_merge_request_rebase_settings.html.haml | 14 +++++ .../ee/_merge_request_settings.html.haml | 54 ++++++------------- 6 files changed, 71 insertions(+), 39 deletions(-) create mode 100644 app/views/projects/_merge_request_fast_forward_settings.html.haml create mode 100644 ee/app/views/projects/ee/_merge_request_fast_forward_settings.html.haml create mode 100644 ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml diff --git a/app/views/projects/_merge_request_fast_forward_settings.html.haml b/app/views/projects/_merge_request_fast_forward_settings.html.haml new file mode 100644 index 00000000000000..9d357293a2f0cc --- /dev/null +++ b/app/views/projects/_merge_request_fast_forward_settings.html.haml @@ -0,0 +1,13 @@ +- form = local_assigns.fetch(:form) +- project = local_assigns.fetch(:project) + +.radio + = label_tag :project_merge_method_ff do + = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" + %strong Fast-forward merge + %br + %span.descr + No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. + %br + %span.descr + When fast-forward merge is not possible, the user must first rebase locally. diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index ccbf4e70404c11..939750ed1e5ba2 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -1,5 +1,16 @@ - form = local_assigns.fetch(:form) -= render 'projects/ee/merge_request_settings', form: form, project: @project +.form-group + = label_tag :merge_method_merge, class: 'label-light' do + Merge method + .radio + = label_tag :project_merge_method_merge do + = form.radio_button :merge_method, :merge, class: "js-merge-method-radio" + %strong Merge commit + %br + %span.descr + A merge commit is created for every merge, and merging is allowed as long as there are no conflicts. + + = render 'merge_request_fast_forward_settings', project: @project, form: form = render 'projects/merge_request_merge_settings', form: form diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 8a4130fd526cd9..0a716159babb2a 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -97,7 +97,7 @@ = render 'shared/promotions/promote_mr_features' = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { multipart: true, class: "merge-request-settings-form" }, authenticity_token: true do |f| - = render 'merge_request_settings', form: f + = render 'projects/ee/merge_request_settings', form: f = f.submit 'Save changes', class: "btn btn-save" = render 'projects/ee/service_desk_settings' diff --git a/ee/app/views/projects/ee/_merge_request_fast_forward_settings.html.haml b/ee/app/views/projects/ee/_merge_request_fast_forward_settings.html.haml new file mode 100644 index 00000000000000..85e0c54614060c --- /dev/null +++ b/ee/app/views/projects/ee/_merge_request_fast_forward_settings.html.haml @@ -0,0 +1,14 @@ +- form = local_assigns.fetch(:form) +- project = local_assigns.fetch(:project) + +.radio + = label_tag :project_merge_method_ff do + = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" + %strong Fast-forward merge + %br + %span.descr + No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. + - if project.feature_available?(:merge_request_rebase) + %br + %span.descr + When fast-forward merge is not possible, the user is given the option to rebase. diff --git a/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml b/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml new file mode 100644 index 00000000000000..85e0c54614060c --- /dev/null +++ b/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml @@ -0,0 +1,14 @@ +- form = local_assigns.fetch(:form) +- project = local_assigns.fetch(:project) + +.radio + = label_tag :project_merge_method_ff do + = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" + %strong Fast-forward merge + %br + %span.descr + No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. + - if project.feature_available?(:merge_request_rebase) + %br + %span.descr + When fast-forward merge is not possible, the user is given the option to rebase. diff --git a/ee/app/views/projects/ee/_merge_request_settings.html.haml b/ee/app/views/projects/ee/_merge_request_settings.html.haml index aef37b02fd680f..c003f442ba09db 100644 --- a/ee/app/views/projects/ee/_merge_request_settings.html.haml +++ b/ee/app/views/projects/ee/_merge_request_settings.html.haml @@ -1,43 +1,21 @@ - form = local_assigns.fetch(:form) -- project = local_assigns.fetch(:project) -- if project.feature_available?(:merge_request_rebase) || project.feature_available?(:fast_forward_merge) - .form-group - = label_tag :merge_method_merge, class: 'label-light' do - Merge method - .radio - = label_tag :project_merge_method_merge do - = form.radio_button :merge_method, :merge, class: "js-merge-method-radio" - %strong Merge commit - %br - %span.descr - A merge commit is created for every merge, and merging is allowed as long as there are no conflicts. +.form-group + = label_tag :merge_method_merge, class: 'label-light' do + Merge method + .radio + = label_tag :project_merge_method_merge do + = form.radio_button :merge_method, :merge, class: "js-merge-method-radio" + %strong Merge commit + %br + %span.descr + A merge commit is created for every merge, and merging is allowed as long as there are no conflicts. + + - if @project.feature_available?(:merge_request_rebase) + = render 'projects/ee/merge_request_rebase_settings', project: @project, form: form - - if project.feature_available?(:merge_request_rebase) - .radio - = label_tag :project_merge_method_rebase_merge do - = form.radio_button :merge_method, :rebase_merge, class: "js-merge-method-radio" - %strong Merge commit with semi-linear history - %br - %span.descr - A merge commit is created for every merge, but merging is only allowed if fast-forward merge is possible. - This way you could make sure that if this merge request would build, after merging to target branch it would also build. - %br - %span.descr - When fast-forward merge is not possible, the user is given the option to rebase. + = render 'projects/ee/merge_request_fast_forward_settings', project: @project, form: form - - if project.feature_available?(:fast_forward_merge) - .radio - = label_tag :project_merge_method_ff do - = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" - %strong Fast-forward merge - %br - %span.descr - No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. - - if project.feature_available?(:merge_request_rebase) - %br - %span.descr - When fast-forward merge is not possible, the user is given the option to rebase. - if @project.feature_available?(:issuable_default_templates) .form-group @@ -48,4 +26,6 @@ .hint Description parsed with #{link_to "GitLab Flavored Markdown", help_page_path('user/markdown'), target: '_blank'}. -= render 'projects/ee/merge_request_approvals_settings', project: project, form: form += render 'projects/ee/merge_request_approvals_settings', project: @project, form: form + += render 'projects/merge_request_merge_settings', form: form -- GitLab From 7ca2e0d16e192dfda6ad4311599dfbc6ccefa77b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 18 Sep 2017 17:08:09 -0500 Subject: [PATCH 2/2] Move fast-forward MR widget states around for CE port See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14272 --- .../components/states/mr_widget_conflicts.js | 51 ++++++---- .../stores/mr_widget_store.js | 1 + .../stores/mr_widget_store.js | 1 - .../_merge_request_rebase_settings.html.haml | 17 ++-- .../states/mr_widget_conflicts_spec.js | 99 ++++++++++++------- .../states/mr_widget_ready_to_merge_spec.js | 80 +++++++++------ 6 files changed, 157 insertions(+), 92 deletions(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js index f9cb79a0bc12d4..f2245a532f4e88 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_conflicts.js @@ -10,27 +10,40 @@ export default { }, template: `
- +
- - There are merge conflicts. - - Resolve these conflicts or ask someone with write access to this repository to merge it locally + +
`, 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 46a2c35e4e3505..dc3e557520846d 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 @@ -59,6 +59,7 @@ export default class MergeRequestStore { this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false; this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false; this.mergePath = data.merge_path; + this.ffOnlyEnabled = data.ff_only_enabled; this.statusPath = data.status_path; this.emailPatchesPath = data.email_patches_path; this.plainDiffPath = data.plain_diff_path; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index c241b375d008a3..810469942ab303 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -28,7 +28,6 @@ export default class MergeRequestStore extends CEMergeRequestStore { this.rebaseInProgress = data.rebase_in_progress; this.approvalsLeft = !data.approved; this.rebasePath = data.rebase_path; - this.ffOnlyEnabled = data.ff_only_enabled; } initGeo(data) { diff --git a/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml b/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml index 85e0c54614060c..54e0b73d24c3fa 100644 --- a/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml +++ b/ee/app/views/projects/ee/_merge_request_rebase_settings.html.haml @@ -1,14 +1,13 @@ - form = local_assigns.fetch(:form) -- project = local_assigns.fetch(:project) .radio - = label_tag :project_merge_method_ff do - = form.radio_button :merge_method, :ff, class: "js-merge-method-radio" - %strong Fast-forward merge + = label_tag :project_merge_method_rebase_merge do + = form.radio_button :merge_method, :rebase_merge, class: "js-merge-method-radio" + %strong Merge commit with semi-linear history %br %span.descr - No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. - - if project.feature_available?(:merge_request_rebase) - %br - %span.descr - When fast-forward merge is not possible, the user is given the option to rebase. + A merge commit is created for every merge, but merging is only allowed if fast-forward merge is possible. + This way you could make sure that if this merge request would build, after merging to target branch it would also build. + %br + %span.descr + When fast-forward merge is not possible, the user is given the option to rebase. diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js index 3b7b7d936624df..d4b18703b258d6 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_conflicts_spec.js @@ -2,17 +2,16 @@ import Vue from 'vue'; import conflictsComponent from '~/vue_merge_request_widget/components/states/mr_widget_conflicts'; const path = '/conflicts'; -const createComponent = () => { +const createComponent = (customConfig = {}) => { const Component = Vue.extend(conflictsComponent); + const config = Object.assign({ + mr: {}, + }, customConfig); + return new Component({ el: document.createElement('div'), - propsData: { - mr: { - canMerge: true, - conflictResolutionPath: path, - }, - }, + propsData: config, }); }; @@ -27,44 +26,78 @@ describe('MRWidgetConflicts', () => { }); describe('template', () => { - it('should have correct elements', () => { - const el = createComponent().$el; - const resolveButton = el.querySelector('.js-resolve-conflicts-button'); - const mergeButton = el.querySelector('.mr-widget-body .btn'); - const mergeLocallyButton = el.querySelector('.js-merge-locally-button'); - - expect(el.textContent).toContain('There are merge conflicts'); - expect(el.textContent).not.toContain('ask someone with write access'); - expect(el.querySelector('.btn-success').disabled).toBeTruthy(); - expect(resolveButton.textContent).toContain('Resolve conflicts'); - expect(resolveButton.getAttribute('href')).toEqual(path); - expect(mergeButton.textContent).toContain('Merge'); - expect(mergeLocallyButton.textContent).toContain('Merge locally'); + describe('when allowed to merge', () => { + let vm; + + beforeEach(() => { + vm = createComponent({ + mr: { + canMerge: true, + conflictResolutionPath: path, + }, + }); + }); + + it('should tell you about conflicts without bothering other people', () => { + expect(vm.$el.textContent).toContain('There are merge conflicts'); + expect(vm.$el.textContent).not.toContain('ask someone with write access'); + }); + + it('should allow you to resolve the conflicts', () => { + const resolveButton = vm.$refs.resolveConflictsButton; + + expect(resolveButton.textContent).toContain('Resolve conflicts'); + expect(resolveButton.getAttribute('href')).toEqual(path); + }); + + it('should have merge buttons', () => { + const mergeButton = vm.$refs.statusIcon.$refs.mergeButton; + const mergeLocallyButton = vm.$refs.mergeLocallyButton; + + expect(mergeButton.textContent).toContain('Merge'); + expect(mergeButton.disabled).toBeTruthy(); + expect(mergeButton.classList.contains('btn-success')).toBeTruthy(); + expect(mergeLocallyButton.textContent).toContain('Merge locally'); + }); }); describe('when user does not have permission to merge', () => { let vm; beforeEach(() => { - vm = createComponent(); - vm.mr.canMerge = false; + vm = createComponent({ + mr: { + canMerge: false, + }, + }); }); - it('should show proper message', (done) => { - Vue.nextTick(() => { - expect(vm.$el.textContent).toContain('ask someone with write access'); - done(); - }); + it('should show proper message', () => { + expect(vm.$el.textContent).toContain('ask someone with write access'); + }); + + it('should not have action buttons', () => { + expect(vm.$refs.statusIcon.$refs.mergeButton).toBeDefined(); + expect(vm.$refs.resolveConflictsButton).toBeUndefined(); + expect(vm.$refs.mergeLocallyButton).toBeUndefined(); }); + }); + + describe('when fast-forward merge enabled', () => { + let vm; - it('should not have action buttons', (done) => { - Vue.nextTick(() => { - expect(vm.$el.querySelectorAll('.btn').length).toBe(1); - expect(vm.$el.querySelector('.js-resolve-conflicts-button')).toEqual(null); - expect(vm.$el.querySelector('.js-merge-locally-button')).toEqual(null); - done(); + beforeEach(() => { + vm = createComponent({ + mr: { + ffOnlyEnabled: true, + }, }); }); + + it('should tell you to rebase locally', () => { + expect(vm.$el.textContent).toContain('Fast-forward merge is not possible.'); + expect(vm.$el.textContent).toContain('To merge this request, first rebase locally'); + }); }); }); }); diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js index c607c9746a4c21..2e84d8db946f2b 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js @@ -169,36 +169,6 @@ describe('MRWidgetReadyToMerge', () => { expect(vm.isMergeButtonDisabled).toBeTruthy(); }); }); - - describe('Remove source branch checkbox', () => { - describe('when user can merge but cannot delete branch', () => { - it('isRemoveSourceBranchButtonDisabled should be true', () => { - expect(vm.isRemoveSourceBranchButtonDisabled).toBe(true); - }); - - it('should be disabled in the rendered output', () => { - const checkboxElement = vm.$el.querySelector('#remove-source-branch-input'); - expect(checkboxElement.getAttribute('disabled')).toBe('disabled'); - }); - }); - - describe('when user can merge and can delete branch', () => { - beforeEach(() => { - this.customVm = createComponent({ - mr: { canRemoveSourceBranch: true }, - }); - }); - - it('isRemoveSourceBranchButtonDisabled should be false', () => { - expect(this.customVm.isRemoveSourceBranchButtonDisabled).toBe(false); - }); - - it('should be enabled in rendered output', () => { - const checkboxElement = this.customVm.$el.querySelector('#remove-source-branch-input'); - expect(checkboxElement.getAttribute('disabled')).toBeNull(); - }); - }); - }); }); describe('methods', () => { @@ -419,4 +389,54 @@ describe('MRWidgetReadyToMerge', () => { }); }); }); + + describe('Remove source branch checkbox', () => { + describe('when user can merge but cannot delete branch', () => { + it('isRemoveSourceBranchButtonDisabled should be true', () => { + expect(vm.isRemoveSourceBranchButtonDisabled).toBe(true); + }); + + it('should be disabled in the rendered output', () => { + const checkboxElement = vm.$el.querySelector('#remove-source-branch-input'); + expect(checkboxElement.getAttribute('disabled')).toBe('disabled'); + }); + }); + + describe('when user can merge and can delete branch', () => { + beforeEach(() => { + this.customVm = createComponent({ + mr: { canRemoveSourceBranch: true }, + }); + }); + + it('isRemoveSourceBranchButtonDisabled should be false', () => { + expect(this.customVm.isRemoveSourceBranchButtonDisabled).toBe(false); + }); + + it('should be enabled in rendered output', () => { + const checkboxElement = this.customVm.$el.querySelector('#remove-source-branch-input'); + expect(checkboxElement.getAttribute('disabled')).toBeNull(); + }); + }); + }); + + describe('Commit message area', () => { + it('when using merge commits, should show "Modify commit message" button', () => { + const customVm = createComponent({ + mr: { ffOnlyEnabled: false }, + }); + + expect(customVm.$refs.fastForwardMessage).toBeUndefined(); + expect(customVm.$refs.modifyCommitMessageButton).toBeDefined(); + }); + + it('when fast-forward merge is enabled, only show fast-forward message', () => { + const customVm = createComponent({ + mr: { ffOnlyEnabled: true }, + }); + + expect(customVm.$refs.fastForwardMessage).toBeDefined(); + expect(customVm.$refs.modifyCommitMessageButton).toBeUndefined(); + }); + }); }); -- GitLab