Merge branch '25171-fix-mr-features-settings-hidden-when-builds-are-disabled-ee' into 'master'

Remove wrong '.builds-feature' class from the MR settings fieldset

EE MR for gitlab-org/gitlab-ce!7930

See merge request !947

app/views/projects/_issues_settings.html.haml

 %fieldset.features.append-bottom-0.issues-feature
+  %hr
   %h5.prepend-top-0
     Issues
   .form-group

app/views/projects/_merge_request_merge_settings.html.haml

+- form = local_assigns.fetch(:form)
+
+.form-group
+  .checkbox.builds-feature
+    = form.label :only_allow_merge_if_build_succeeds do
+      = form.check_box :only_allow_merge_if_build_succeeds
+      %strong Only allow merge requests to be merged if the build succeeds
+      %br
+      %span.descr
+        Builds need to be configured to enable this feature.
+        = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_when_build_succeeds', anchor: 'only-allow-merge-requests-to-be-merged-if-the-build-succeeds')
+  .checkbox
+    = form.label :only_allow_merge_if_all_discussions_are_resolved do
+      = form.check_box :only_allow_merge_if_all_discussions_are_resolved
+      %strong Only allow merge requests to be merged if all discussions are resolved

app/views/projects/_merge_request_settings.html.haml

-%fieldset.merge-requests-feature.append-bottom-default
-  %h5.prepend-top-0
-    Merge requests
-
-  .form-group
-    = label_tag :merge_method_merge, class: 'label-light' do
-      Merge method
-    .radio
-      = label_tag :project_merge_method_merge do
-        = f.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.
-
-    .radio
-      = label_tag :project_merge_method_rebase_merge do
-        = f.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.
-
-    .radio
-      = label_tag :project_merge_method_ff do
-        = f.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 is given the option to rebase.
-
-  .form-group
-    = f.label :merge_requests_template, class: 'label-light' do
-      Description template
-    = f.text_area :merge_requests_template, class: "form-control", rows: 3
+- form = local_assigns.fetch(:form)
 
-  .form-group
-    = f.label :approvals_before_merge, class: 'label-light' do
-      Approvals required
-    = f.number_field :approvals_before_merge, class: "form-control", min: 0
-    .help-block
-      Number of users to approve a merge request before it can be accepted. 0 - approving is disabled
-
-  .form-group.reset-approvals-on-push
-    .checkbox
-      = f.label :reset_approvals_on_push do
-        = f.check_box :reset_approvals_on_push
-        %span.descr Reset approvals on push
-        .help-block Approvals are reset when new data is pushed to the merge request
-
-  .form-group
-    = f.label :approver_ids, class: 'label-light' do
-      Approvers
-    = users_select_tag("project[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
-    .help-block
-      Add an approver suggestion for each merge request
-    = f.label :approver_group_ids, class: 'label-light' do
-      Approver groups
-    - skip_groups = @project.approver_groups.pluck(:group_id)
-    = groups_select_tag('project[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true }, class: 'input-large')
-    .help-block
-      Add a group as an approver suggestion for each merge request
-
-    .panel.panel-default.prepend-top-10
-      .panel-heading
-        Approvers
-      %ul.well-list.approver-list
-        - @project.approvers.each do |approver|
-          %li.approver
-            = link_to approver.user.name, approver.user
-            .pull-right
-              = link_to namespace_project_approver_path(@project.namespace, @project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
-                = icon("sign-out")
-                Remove
-        - @project.approver_groups.each do |approver_group|
-          %li.approver-group
-            Group:
-            = link_to approver_group.group.name, approver_group.group
-            .pull-right
-              = link_to namespace_project_approver_group_path(@project.namespace, @project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
-                = icon("sign-out")
-                Remove
-        - if @project.approvers.empty? && @project.approver_groups.empty?
-          %li There are no approvers
+%fieldset.features.merge-requests-feature.append-bottom-default
+  %hr
+  %h5.prepend-top-0
+    Merge Requests
 
-  .form-group.builds-feature
-    .checkbox
-      = f.label :only_allow_merge_if_build_succeeds do
-        = f.check_box :only_allow_merge_if_build_succeeds
-        %strong Only allow merge requests to be merged if the build succeeds
-      .help-block
-        Builds need to be configured to enable this feature.
-        = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_when_build_succeeds', anchor: 'only-allow-merge-requests-to-be-merged-if-the-build-succeeds')
-    .checkbox
-      = f.label :only_allow_merge_if_all_discussions_are_resolved do
-        = f.check_box :only_allow_merge_if_all_discussions_are_resolved
-        %strong Only allow merge requests to be merged if all discussions are resolved
+  = render 'projects/ee/merge_request_settings', form: form, project: @project
 
-:javascript
-  new UsersSelect();
-  new GroupsSelect();
+  = render 'projects/merge_request_merge_settings', form: form

app/views/projects/edit.html.haml

@@ -120,11 +120,10 @@
                 %span.descr Enable Container Registry for this project
                 = link_to icon('question-circle'), help_page_path('user/project/container_registry'), target: '_blank'
 
-        %hr
         = render 'issues_settings', f: f
-        %hr
 
-        = render 'merge_request_settings', f: f
+        = render 'merge_request_settings', form: f
+
         %hr
         %fieldset.features.append-bottom-default
           %h5.prepend-top-0

app/views/projects/ee/_merge_request_settings.html.haml

+- form = local_assigns.fetch(:form)
+- project = local_assigns.fetch(: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.
+
+  .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.
+
+  .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 is given the option to rebase.
+
+.form-group
+  = form.label :merge_requests_template, class: 'label-light' do
+    Description template
+  = form.text_area :merge_requests_template, class: "form-control", rows: 3
+
+.form-group
+  = form.label :approvals_before_merge, class: 'label-light' do
+    Approvals required
+  = form.number_field :approvals_before_merge, class: "form-control", min: 0
+  .help-block
+    Number of users to approve a merge request before it can be accepted. 0 - approving is disabled
+
+.form-group.reset-approvals-on-push
+  .checkbox
+    = form.label :reset_approvals_on_push do
+      = form.check_box :reset_approvals_on_push
+      %span.descr Reset approvals on push
+      .help-block Approvals are reset when new data is pushed to the merge request
+
+.form-group
+  = form.label :approver_ids, class: 'label-light' do
+    Approvers
+  = users_select_tag("project[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
+  .help-block
+    Add an approver suggestion for each merge request
+  = form.label :approver_group_ids, class: 'label-light' do
+    Approver groups
+  - skip_groups = project.approver_groups.pluck(:group_id)
+  = groups_select_tag('project[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true }, class: 'input-large')
+  .help-block
+    Add a group as an approver suggestion for each merge request
+
+  .panel.panel-default.prepend-top-10
+    .panel-heading
+      Approvers
+    %ul.well-list.approver-list
+      - project.approvers.each do |approver|
+        %li.approver
+          = link_to approver.user.name, approver.user
+          .pull-right
+            = link_to namespace_project_approver_path(project.namespace, project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+              = icon("sign-out")
+              Remove
+      - project.approver_groups.each do |approver_group|
+        %li.approver-group
+          Group:
+          = link_to approver_group.group.name, approver_group.group
+          .pull-right
+            = link_to namespace_project_approver_group_path(project.namespace, project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
+              = icon("sign-out")
+              Remove
+      - if project.approvers.empty? && project.approver_groups.empty?
+        %li There are no approvers
+
+:javascript
+  new UsersSelect();
+  new GroupsSelect();

changelogs/unreleased/25171-fix-mr-features-settings-hidden-when-builds-are-disabled.yml

+---
+title: Remove wrong '.builds-feature' class from the MR settings fieldset
+merge_request: 7930
+author: 

spec/features/projects/project_settings_spec.rb

 require 'spec_helper'
 
 describe 'Edit Project Settings', feature: true do
-  include WaitForAjax
-
   let(:user) { create(:user) }
   let(:project) { create(:empty_project, path: 'gitlab', name: 'sample') }
 
@@ -23,45 +21,6 @@ describe 'Edit Project Settings', feature: true do
       expect(page).to have_content "Name can contain only letters, digits, emojis, '_', '.', dash, space. It must start with letter, digit, emoji or '_'."
       expect(page).to have_button 'Save changes'
     end
-
-    it 'adds approver group' do
-      group = create :group
-      approver = create :user
-      group.add_developer(approver)
-      group.add_developer(user)
-
-      visit edit_namespace_project_path(project.namespace, project)
-
-      find('#s2id_project_approver_group_ids .select2-input').click
-
-      wait_for_ajax
-
-      expect(find('.select2-results')).to have_content(group.name)
-
-      find('.select2-results').click
-
-      click_button 'Save changes'
-
-      expect(page).to have_css('.approver-list li.approver-group', count: 1)
-    end
-
-    it 'removes approver group' do
-      group = create :group
-      approver = create :user
-      group.add_developer(approver)
-      group.add_developer(user)
-      create :approver_group, group: group, target: project
-
-      visit edit_namespace_project_path(project.namespace, project)
-
-      expect(find('.approver-list')).to have_content(group.name)
-
-      within('.approver-list') do
-        click_on "Remove"
-      end
-
-      expect(find('.approver-list')).not_to have_content(group.name)
-    end
   end
 
   describe 'Rename repository' do

spec/features/projects/settings/ee/merge_requests_settings_spec.rb

+require 'spec_helper'
+
+describe 'Project settings > [EE] Merge Requests', feature: true, js: true do
+  include GitlabRoutingHelper
+  include WaitForAjax
+
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project) }
+  let(:group) { create(:group) }
+  let(:approver) { create(:user) }
+
+  before do
+    login_as(user)
+    project.team << [user, :master]
+    group.add_developer(approver)
+    group.add_developer(user)
+  end
+
+  scenario 'adds approver group' do
+    visit edit_project_path(project)
+
+    find('#s2id_project_approver_group_ids .select2-input').click
+
+    wait_for_ajax
+
+    expect(find('.select2-results')).to have_content(group.name)
+
+    find('.select2-results').click
+
+    click_button 'Save changes'
+
+    expect(page).to have_css('.approver-list li.approver-group', count: 1)
+  end
+
+  context 'with an approver group' do
+    before do
+      create(:approver_group, group: group, target: project)
+    end
+
+    scenario 'removes approver group' do
+      visit edit_project_path(project)
+
+      expect(find('.approver-list')).to have_content(group.name)
+
+      within('.approver-list') do
+        click_on "Remove"
+      end
+
+      expect(find('.approver-list')).not_to have_content(group.name)
+    end
+  end
+end

spec/features/projects/settings/merge_requests_settings_spec.rb

+require 'spec_helper'
+
+feature 'Project settings > Merge Requests', feature: true, js: true do
+  include GitlabRoutingHelper
+
+  let(:project) { create(:empty_project, :public) }
+  let(:user) { create(:user) }
+
+  background do
+    project.team << [user, :master]
+    login_as(user)
+  end
+
+  context 'when Merge Request and Builds are initially enabled' do
+    before do
+      project.project_feature.update_attribute('merge_requests_access_level', ProjectFeature::ENABLED)
+    end
+
+    context 'when Builds are initially enabled' do
+      before do
+        project.project_feature.update_attribute('builds_access_level', ProjectFeature::ENABLED)
+        visit edit_project_path(project)
+      end
+
+      scenario 'shows the Merge Requests settings' do
+        expect(page).to have_content('Only allow merge requests to be merged if the build succeeds')
+        expect(page).to have_content('Only allow merge requests to be merged if all discussions are resolved')
+
+        select 'Disabled', from: "project_project_feature_attributes_merge_requests_access_level"
+
+        expect(page).not_to have_content('Only allow merge requests to be merged if the build succeeds')
+        expect(page).not_to have_content('Only allow merge requests to be merged if all discussions are resolved')
+      end
+    end
+
+    context 'when Builds are initially disabled' do
+      before do
+        project.project_feature.update_attribute('builds_access_level', ProjectFeature::DISABLED)
+        visit edit_project_path(project)
+      end
+
+      scenario 'shows the Merge Requests settings that do not depend on Builds feature' do
+        expect(page).not_to have_content('Only allow merge requests to be merged if the build succeeds')
+        expect(page).to have_content('Only allow merge requests to be merged if all discussions are resolved')
+
+        select 'Everyone with access', from: "project_project_feature_attributes_builds_access_level"
+
+        expect(page).to have_content('Only allow merge requests to be merged if the build succeeds')
+        expect(page).to have_content('Only allow merge requests to be merged if all discussions are resolved')
+      end
+    end
+  end
+
+  context 'when Merge Request are initially disabled' do
+    before do
+      project.project_feature.update_attribute('merge_requests_access_level', ProjectFeature::DISABLED)
+      visit edit_project_path(project)
+    end
+
+    scenario 'does not show the Merge Requests settings' do
+      expect(page).not_to have_content('Only allow merge requests to be merged if the build succeeds')
+      expect(page).not_to have_content('Only allow merge requests to be merged if all discussions are resolved')
+
+      select 'Everyone with access', from: "project_project_feature_attributes_merge_requests_access_level"
+
+      expect(page).to have_content('Only allow merge requests to be merged if the build succeeds')
+      expect(page).to have_content('Only allow merge requests to be merged if all discussions are resolved')
+    end
+  end
+end