Merge branch '1979-move-mr-approval-settings' into 'master'

Move merge request approval settings

See merge request gitlab-org/gitlab-ee!8493

app/views/projects/edit.html.haml

@@ -95,6 +95,8 @@
         = render 'projects/merge_request_settings', form: f
         = f.submit 'Save changes', class: "btn btn-success qa-save-merge-request-changes"
 
+  = render_if_exists 'projects/merge_request_approvals_settings', expanded: expanded
+
   = render_if_exists 'projects/service_desk_settings'
 
   %section.settings.no-animate{ class: ('expanded' if expanded) }

doc/user/project/merge_requests/merge_request_approvals.md

@@ -17,8 +17,8 @@ Merge request approvals enable enforced code review by requiring specified peopl
 
 To edit the merge request approvals:
 
-1. Navigate to your project's **Settings > General** and expand the
-   **Merge requests settings**
+1. Navigate to your project's **Settings > General** and expand
+   **Merge request approvals**
 1. Search for users or groups that will be [eligible to approve](#eligible-approvers)
    merge requests and click the **Add** button to add them as approvers
 1. Set the minimum number of required approvals under the "Approvals required"
@@ -109,8 +109,8 @@ level and change them later when creating or editing the merge request.
 
 First, you have to enable this option in the project's settings:
 
-1. Navigate to your project's **Settings > General** and expand the
-   **Merge requests settings**
+1. Navigate to your project's **Settings > General** and expand
+   **Merge request approvals**
 1. Tick the "Can override approvers and approvals required per merge request"
    checkbox
 
@@ -155,8 +155,8 @@ If approvals are [set at the project level](#editing-approvals),
 you can choose whether all approvals on a merge request are removed when
 new commits are pushed to the source branch of the merge request:
 
-1. Navigate to your project's **Settings > General** and expand the
-   **Merge requests settings**
+1. Navigate to your project's **Settings > General** and expand
+   **Merge request approvals**
 1. Tick the "Can override approvers and approvals required per merge request"
    checkbox
 
@@ -179,8 +179,8 @@ enabling it [at the project level](#editing-approvals). Authors
 also need to be included in the approvers list in order to be able to
 approve their merge request.
 
-1. Navigate to your project's **Settings > General** and expand the
-   **Merge requests settings**
+1. Navigate to your project's **Settings > General** and expand
+   **Merge request approvals**
 1. Tick the "Enable self approval of merge requests" checkbox
 1. Click **Save changes**
 

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

-- return unless project.feature_available?(:merge_request_approvers)
-- can_override_approvers = project.can_override_approvers?
-
-.form-group
-  %strong= _('Merge request approvals')
-  = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
-
-.nested-settings
-  .form-group
-    = form.label :approver_ids, class: 'label-bold' do
-      Approvers
-    = hidden_field_tag "project[approver_ids]"
-    = hidden_field_tag "project[approver_group_ids]"
-    .input-group.input-btn-group
-      = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" }
-      %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' }
-        Add
-    .form-text.text-muted
-      Add users or groups who are allowed to approve every merge request
-
-    .card.prepend-top-10.js-current-approvers
-      .load-wrapper.hidden
-        = icon('spinner spin', class: 'approver-list-loader')
-      .card-header
-        Approvers
-        %span.badge.badge-pill
-          - ids = []
-          - project.approvers.each do |user|
-            - ids << user.user_id
-          - project.approver_groups.each do |group|
-            - group.users.each do |user|
-              - unless ids.include?(user.id)
-                - ids << user.id
-          = ids.count
-      %ul.content-list.approver-list
-        - project.approvers.each do |approver|
-          %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
-            = link_to approver.user.name, approver.user
-            .float-right
-              %button{ href: project_approver_path(project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn btn-remove js-approver-remove", title: 'Remove approver' }
-                = icon("trash")
-        - project.approver_groups.each do |approver_group|
-          %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } }
-            %span
-              %span.light
-                Group:
-              = link_to approver_group.group.name, approver_group.group
-              %span.badge.badge-pill
-                = approver_group.group.members.count
-            .float-right
-              %button{ href: project_approver_group_path(project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, class: "btn btn-remove js-approver-remove", title: 'Remove group' }
-                = icon("trash")
-        - if project.approvers.empty? && project.approver_groups.empty?
-          %li There are no approvers
-
-  .form-group
-    = form.label :approvals_before_merge, class: 'label-bold' do
-      Approvals required
-    = form.number_field :approvals_before_merge, class: "form-control", min: 0
-    .form-text.text-muted
-      Set number of approvers required before open merge requests can be merged
-
-  .form-group
-    .form-check
-      = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true)
-      = form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
-        %strong Can override approvers and approvals required per merge request
-        = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank'
-
-  .form-group.reset-approvals-on-push
-    .form-check
-      = form.check_box :reset_approvals_on_push, class: 'form-check-input'
-      = form.label :reset_approvals_on_push, class: 'form-check-label' do
-        %strong Remove all approvals in a merge request when new commits are pushed to its source branch
-
-  .form-group.self-approval
-    .form-check
-      = form.check_box :merge_requests_author_approval, class: 'form-check-input'
-      = form.label :merge_requests_author_approval, class: 'form-check-label' do
-        %strong Enable self approval of merge requests
-        = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals",
-          anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'
-
+- return unless @project.feature_available?(:merge_request_approvers)
+
+%section.settings.merge-requests-feature.no-animate#js-merge-request-approval-settings{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:merge_requests_access_level) == 0)] }
+  .settings-header
+    %h4
+      = _("Merge request approvals")
+    %button.btn.js-settings-toggle{ type: 'button' }
+      = expanded ? _("Collapse") : _("Expand")
+    %p
+      = _("Customize your merge request approval settings.")
+      = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
+  .settings-content
+    = form_for [@project.namespace.becomes(Namespace), @project], remote: true, html: { class: "merge-request-approval-settings-form" }, authenticity_token: true do |f|
+      %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-approval-settings' }
+      = render 'projects/merge_request_approvals_settings_form', form: f, project: @project
+      = f.submit _("Save changes"), class: "btn btn-success"

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

+- can_override_approvers = project.can_override_approvers?
+
+.form-group
+  = form.label :approver_ids, class: 'label-bold' do
+    = _("Approvers")
+  = hidden_field_tag "project[approver_ids]"
+  = hidden_field_tag "project[approver_group_ids]"
+  .input-group.input-btn-group
+    = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" }
+    %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' }
+      = _("Add")
+  .form-text.text-muted
+    = _("Add users or groups who are allowed to approve every merge request")
+
+  .card.prepend-top-10.js-current-approvers
+    .load-wrapper.hidden
+      = icon('spinner spin', class: 'approver-list-loader')
+    .card-header
+      = _("Approvers")
+      %span.badge.badge-pill
+        - ids = []
+        - project.approvers.each do |user|
+          - ids << user.user_id
+        - project.approver_groups.each do |group|
+          - group.users.each do |user|
+            - unless ids.include?(user.id)
+              - ids << user.id
+        = ids.count
+    %ul.content-list.approver-list
+      - project.approvers.each do |approver|
+        %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
+          = link_to approver.user.name, approver.user
+          .float-right
+            - confirm_message = _("Are you sure you want to remove approver %{name}?") % { name: approver.user.name }
+            %button{ href: project_approver_path(project, approver), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove approver") }
+              = icon("trash")
+      - project.approver_groups.each do |approver_group|
+        %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } }
+          %span
+            %span.light
+              = _("Group:")
+            = link_to approver_group.group.name, approver_group.group
+            %span.badge.badge-pill
+              = approver_group.group.members.count
+          .float-right
+            - confirm_message = _("Are you sure you want to remove group %{name}?") % { name: approver_group.group.name }
+            %button{ href: project_approver_group_path(project, approver_group), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove group") }
+              = icon("trash")
+      - if project.approvers.empty? && project.approver_groups.empty?
+        %li= _("There are no approvers")
+
+.form-group
+  = form.label :approvals_before_merge, class: 'label-bold' do
+    = _("Approvals required")
+  = form.number_field :approvals_before_merge, class: "form-control", min: 0
+  .form-text.text-muted
+    = _("Set number of approvers required before open merge requests can be merged")
+
+.form-group
+  .form-check
+    = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true)
+    = form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
+      %strong= _("Can override approvers and approvals required per merge request")
+      = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'overriding-the-merge-request-approvals-default-settings'), target: '_blank'
+
+.form-group.reset-approvals-on-push
+  .form-check
+    = form.check_box :reset_approvals_on_push, class: 'form-check-input'
+    = form.label :reset_approvals_on_push, class: 'form-check-label' do
+      %strong= _("Remove all approvals in a merge request when new commits are pushed to its source branch")
+
+.form-group.self-approval
+  .form-check
+    = form.check_box :merge_requests_author_approval, class: 'form-check-input'
+    = form.label :merge_requests_author_approval, class: 'form-check-label' do
+      %strong= _("Enable self approval of merge requests")
+      = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals",
+        anchor: 'allowing-merge-request-authors-to-approve-their-own-merge-requests'), target: '_blank'

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

@@ -11,6 +11,4 @@
     .hint
       Description parsed with #{link_to "GitLab Flavored Markdown", help_page_path('user/markdown'), target: '_blank'}.
 
-= render 'projects/merge_request_approvals_settings', project: @project, form: form
-
 = render_ce 'projects/merge_request_merge_settings', form: form

ee/changelogs/unreleased/1979-move-mr-approval-settings.yml

+---
+title: Move merge request approval settings
+merge_request: 8493
+author:
+type: changed

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

@@ -11,7 +11,7 @@ describe 'EE > Projects > Settings > User manages merge requests template' do
 
   it 'saves merge request template' do
     fill_in 'project_merge_requests_template', with: "This merge request should contain the following."
-    page.within '.merge-requests-feature' do
+    page.within '#js-merge-request-settings' do
       click_button 'Save changes'
     end
 

locale/gitlab.pot

@@ -447,6 +447,9 @@ msgstr ""
 msgid "Add user(s) to the group:"
 msgstr ""
 
+msgid "Add users or groups who are allowed to approve every merge request"
+msgstr ""
+
 msgid "Add users to group"
 msgstr ""
 
@@ -776,6 +779,12 @@ msgstr ""
 msgid "Applications"
 msgstr ""
 
+msgid "Approvals required"
+msgstr ""
+
+msgid "Approvers"
+msgstr ""
+
 msgid "Apr"
 msgstr ""
 
@@ -803,6 +812,12 @@ msgstr ""
 msgid "Are you sure you want to remove %{group_name}?"
 msgstr ""
 
+msgid "Are you sure you want to remove approver %{name}?"
+msgstr ""
+
+msgid "Are you sure you want to remove group %{name}?"
+msgstr ""
+
 msgid "Are you sure you want to remove this identity?"
 msgstr ""
 
@@ -1405,6 +1420,9 @@ msgstr ""
 msgid "Callback URL"
 msgstr ""
 
+msgid "Can override approvers and approvals required per merge request"
+msgstr ""
+
 msgid "Can't find HEAD commit for this branch"
 msgstr ""
 
@@ -2555,6 +2573,9 @@ msgstr ""
 msgid "Customize how Google Code email addresses and usernames are imported into GitLab. In the next step, you'll be able to select the projects you want to import."
 msgstr ""
 
+msgid "Customize your merge request approval settings."
+msgstr ""
+
 msgid "Customize your pipeline configuration, view your pipeline status and coverage report."
 msgstr ""
 
@@ -3025,6 +3046,9 @@ msgstr ""
 msgid "Enable reCAPTCHA or Akismet and set IP limits."
 msgstr ""
 
+msgid "Enable self approval of merge requests"
+msgstr ""
+
 msgid "Enable the Performance Bar for a given group."
 msgstr ""
 
@@ -4099,6 +4123,9 @@ msgstr ""
 msgid "Group name"
 msgstr ""
 
+msgid "Group:"
+msgstr ""
+
 msgid "Group: %{group_name}"
 msgstr ""
 
@@ -6831,6 +6858,12 @@ msgstr ""
 msgid "Remove Runner"
 msgstr ""
 
+msgid "Remove all approvals in a merge request when new commits are pushed to its source branch"
+msgstr ""
+
+msgid "Remove approver"
+msgstr ""
+
 msgid "Remove avatar"
 msgstr ""
 
@@ -7385,6 +7418,9 @@ msgstr ""
 msgid "Set notification email for abuse reports."
 msgstr ""
 
+msgid "Set number of approvers required before open merge requests can be merged"
+msgstr ""
+
 msgid "Set requirements for a user to sign-in. Enable mandatory two-factor authentication."
 msgstr ""
 
@@ -8112,6 +8148,9 @@ msgstr ""
 msgid "The value lying at the midpoint of a series of observed values. E.g., between 3, 5, 9, the median is 5. Between 3, 5, 7, 8, the median is (5+7)/2 = 6."
 msgstr ""
 
+msgid "There are no approvers"
+msgstr ""
+
 msgid "There are no archived projects yet"
 msgstr ""
 

spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb

@@ -10,19 +10,19 @@ describe 'Projects > Settings > User manages merge request settings' do
   end
 
   it 'shows "Merge commit" strategy' do
-    page.within '.merge-requests-feature' do
+    page.within '#js-merge-request-settings' do
       expect(page).to have_content 'Merge commit'
     end
   end
 
   it 'shows "Merge commit with semi-linear history " strategy' do
-    page.within '.merge-requests-feature' do
+    page.within '#js-merge-request-settings' do
       expect(page).to have_content 'Merge commit with semi-linear history'
     end
   end
 
   it 'shows "Fast-forward merge" strategy' do
-    page.within '.merge-requests-feature' do
+    page.within '#js-merge-request-settings' do
       expect(page).to have_content 'Fast-forward merge'
     end
   end