Merge branch '231244-move-approval-rule-in-mr' into 'master'

Collapse approval rules and move it under reviewer

See merge request gitlab-org/gitlab!47475

app/controllers/projects/merge_requests/creations_controller.rb

@@ -11,6 +11,11 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
   before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path]
   before_action :build_merge_request, except: [:create]
 
+  before_action do
+    push_frontend_feature_flag(:merge_request_reviewers, @project)
+    push_frontend_feature_flag(:mr_collapsed_approval_rules, @project)
+  end
+
   def new
     define_new_vars
   end

app/controllers/projects/merge_requests_controller.rb

@@ -49,6 +49,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   before_action do
     push_frontend_feature_flag(:vue_issuable_sidebar, @project.group)
+    push_frontend_feature_flag(:merge_request_reviewers, @project)
+    push_frontend_feature_flag(:mr_collapsed_approval_rules, @project)
   end
 
   around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions]

app/views/shared/issuable/_form.html.haml

@@ -32,7 +32,7 @@
         = form.label :confidential, class: 'form-check-label' do
           This issue is confidential and should only be visible to team members with at least Reporter access.
 
-= render 'shared/issuable/form/metadata', issuable: issuable, form: form, project: project
+= render 'shared/issuable/form/metadata', issuable: issuable, form: form, project: project, presenter: presenter
 
 = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form
 

app/views/shared/issuable/form/_metadata.html.haml

 - project = local_assigns.fetch(:project)
 - issuable = local_assigns.fetch(:issuable)
+- presenter = local_assigns.fetch(:presenter)
 
 - return unless can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
 
@@ -14,7 +15,7 @@
 
     - if issuable.allows_reviewers?
       .form-group.row.merge-request-reviewer
-        = render "shared/issuable/form/metadata_issuable_reviewer", issuable: issuable, form: form, has_due_date: has_due_date
+        = render "shared/issuable/form/metadata_issuable_reviewer", issuable: issuable, form: form, has_due_date: has_due_date, presenter: presenter
 
     = render_if_exists "shared/issuable/form/epic", issuable: issuable, form: form, project: project
 

app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml

 = form.label :reviewer_id, issuable.allows_multiple_reviewers? ? _('Reviewers') : _('Reviewer'), class: "col-form-label #{has_due_date ? "col-md-2 col-lg-4" : "col-sm-2"}"
-.col-sm-10{ class: ("col-md-8" if has_due_date) }
+.col-sm-10.gl-mb-2{ class: ("col-md-8" if has_due_date) }
   .issuable-form-select-holder.selectbox
     - issuable.reviewers.each do |reviewer|
       = hidden_field_tag "#{issuable.to_ability_name}[reviewer_ids][]", reviewer.id, id: nil, data: { meta: reviewer.name, avatar_url: reviewer.avatar_url, name: reviewer.name, username: reviewer.username }
@@ -8,3 +8,5 @@
       = hidden_field_tag "#{issuable.to_ability_name}[reviewer_ids][]", 0, id: nil, data: { meta: '' }
 
     = dropdown_tag(users_dropdown_label(issuable.reviewers), options: reviewers_dropdown_options(issuable.to_ability_name))
+  - if Feature.enabled?(:mr_collapsed_approval_rules, @project)
+    = render_if_exists 'shared/issuable/approver_suggestion', issuable: issuable, presenter: presenter

config/feature_flags/development/mr_collapsed_approval_rules.yml

+---
+name: mr_collapsed_approval_rules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/47475
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/284052
+milestone: '13.6'
+type: development
+group: group::source code
+default_enabled: false

ee/app/assets/javascripts/approvals/components/mr_edit/app.vue

 <script>
+import { uniqueId } from 'lodash';
+import { GlIcon, GlButton, GlCollapse, GlCollapseToggleDirective } from '@gitlab/ui';
 import App from '../app.vue';
 import MrRules from './mr_rules.vue';
 import MrRulesHiddenInputs from './mr_rules_hidden_inputs.vue';
 
 export default {
   components: {
+    GlIcon,
+    GlButton,
+    GlCollapse,
     App,
     MrRules,
     MrRulesHiddenInputs,
   },
+  directives: {
+    CollapseToggle: GlCollapseToggleDirective,
+  },
+  data() {
+    return {
+      collapseId: uniqueId('approval-rules-expandable-section-'),
+      isCollapsed: false,
+    };
+  },
+  computed: {
+    toggleIcon() {
+      return this.isCollapsed ? 'chevron-down' : 'chevron-right';
+    },
+    isCollapseFeatureEnabled() {
+      return gon.features?.mergeRequestReviewers && gon.features?.mrCollapsedApprovalRules;
+    },
+  },
 };
 </script>
 
 <template>
+  <div v-if="isCollapseFeatureEnabled" class="gl-mt-2">
+    <gl-button v-collapse-toggle="collapseId" variant="link" button-text-classes="flex">
+      <gl-icon :name="toggleIcon" class="mr-1" />
+      <span>{{ s__('ApprovalRule|Approval rules') }}</span>
+    </gl-button>
+
+    <gl-collapse
+      :id="collapseId"
+      v-model="isCollapsed"
+      class="gl-mt-3 gl-ml-5 gl-mb-5 gl-transition-medium"
+    >
       <app>
         <mr-rules slot="rules" />
         <mr-rules-hidden-inputs slot="footer" />
       </app>
+    </gl-collapse>
+  </div>
+  <app v-else>
+    <mr-rules slot="rules" />
+    <mr-rules-hidden-inputs slot="footer" />
+  </app>
 </template>

ee/app/views/shared/issuable/_approvals.html.haml

@@ -3,7 +3,8 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.approval_feature_available?
 
-.form-group.row
+- if !Feature.enabled?(:merge_request_reviewers, @project) || !Feature.enabled?(:mr_collapsed_approval_rules, @project)
+  .form-group.row
     .col-sm-2.col-form-label
       = form.label :approver_ids, "Approval rules"
     .col-sm-10

ee/spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -39,6 +39,8 @@ RSpec.describe "User creates a merge request", :js do
 
     expect(find_field("merge_request_description").value).to eq(template_text)
 
+    click_button 'Approval rules'
+
     page.within('.js-approval-rules') do
       expect(page).to have_css("img[alt=\"#{approver.name}\"]")
     end

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

@@ -41,12 +41,16 @@ RSpec.describe 'Merge request > User edits MR with approval rules', :js do
   end
 
   it "shows approval rules" do
+    click_button 'Approval rules'
+
     names = page_rule_names.map(&:text)
 
     expect(names).to eq(mr_rule_names)
   end
 
   it "allows user to create approval rule" do
+    click_button 'Approval rules'
+
     rule_name = "Custom Approval Rule"
 
     click_button "Add approval rule"
@@ -67,6 +71,7 @@ RSpec.describe 'Merge request > User edits MR with approval rules', :js do
     before do
       group.add_developer create(:user)
 
+      click_button 'Approval rules'
       click_button "Add approval rule"
     end
 

ee/spec/features/merge_request/user_sets_approval_rules_spec.rb

@@ -25,6 +25,8 @@ RSpec.describe 'Merge request > User sets approval rules', :js do
       end
 
       it "shows approval rules from target project", :sidekiq_might_not_need_inline do
+        click_button 'Approval rules'
+
         names = page_rule_names
         regular_rules.each_with_index do |rule, idx|
           expect(names[idx]).to have_text(rule.name)
@@ -46,6 +48,8 @@ RSpec.describe 'Merge request > User sets approval rules', :js do
       end
 
       it "shows approval rules" do
+        click_button 'Approval rules'
+
         names = page_rule_names
         rules.each.with_index do |rule, idx|
           expect(names[idx]).to have_text(rule.name)

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -11,6 +11,36 @@ RSpec.describe 'Merge request > User sets approvers', :js do
   let!(:config_selector) { '.js-approval-rules' }
   let!(:modal_selector) { '#mr-edit-approvals-create-modal' }
 
+  context 'with feature flag off' do
+    let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    def visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: false)
+      stub_feature_flags(merge_request_reviewers: merge_request_reviewers, mr_collapsed_approval_rules: mr_collapsed_approval_rules)
+      project.add_developer(user)
+      sign_in(user)
+      visit edit_project_merge_request_path(project, merge_request)
+    end
+
+    def non_collapse_approval_rules
+      expect(page).to have_button('Add approval rule')
+    end
+
+    it 'does not hide approval rules inside collapse when only merge_request_reviewers is off' do
+      visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: true)
+      non_collapse_approval_rules
+    end
+
+    it 'does not hide approval rules inside collapse when mr_collapsed_approval_rules is off' do
+      visit_mr(merge_request_reviewers: true, mr_collapsed_approval_rules: false)
+      non_collapse_approval_rules
+    end
+
+    it 'does not hide approval rules inside collapse when merge_request_reviewers and mr_collapsed_approval_rules are off' do
+      visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: false)
+      non_collapse_approval_rules
+    end
+  end
+
   context 'when editing an MR with a different author' do
     let(:author) { create(:user) }
     let(:merge_request) { create(:merge_request, author: author, source_project: project) }

ee/spec/features/merge_requests/user_resets_approvers_spec.rb

@@ -32,6 +32,8 @@ RSpec.describe 'Merge Requests > User resets approvers', :js do
   end
 
   it 'resets approvers for merge requests' do
+    click_button 'Approval rules'
+
     expect_avatar(find('.js-members'), first_user)
 
     click_button 'Reset to project defaults'

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

@@ -23,7 +23,7 @@ RSpec.describe 'Project settings > [EE] Merge Requests', :js do
   it 'adds approver' do
     visit edit_project_path(project)
 
-    open_modal(text: 'Add approval rule')
+    open_modal(text: 'Add approval rule', expand: false)
     open_approver_select
 
     expect(find('.select2-results')).to have_content(user.name)
@@ -50,7 +50,7 @@ RSpec.describe 'Project settings > [EE] Merge Requests', :js do
   it 'adds approver group' do
     visit edit_project_path(project)
 
-    open_modal(text: 'Add approval rule')
+    open_modal(text: 'Add approval rule', expand: false)
     open_approver_select
 
     expect(find('.select2-results')).to have_content(group.name)
@@ -81,7 +81,7 @@ RSpec.describe 'Project settings > [EE] Merge Requests', :js do
 
       expect_avatar(find('.js-members'), rule.approvers)
 
-      open_modal
+      open_modal(text: 'Edit', expand: false)
       remove_approver(group.name)
       click_button "Update approval rule"
       wait_for_requests

ee/spec/support/helpers/feature_approval_helper.rb

 # frozen_string_literal: true
 
 module FeatureApprovalHelper
-  def open_modal(text: 'Edit')
+  def open_modal(text: 'Edit', expand: true)
     page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
+
+    if expand
+      click_button 'Approval rules'
+    end
+
     within(config_selector) do
       click_on(text)
     end

ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb

@@ -22,8 +22,9 @@ RSpec.describe 'shared/issuable/_approvals.html.haml' do
   end
 
   context 'has no approvers' do
-    context 'can override approvers' do
+    context 'when mr_collapsed_approval_rules feature flag is off' do
       before do
+        stub_feature_flags(mr_collapsed_approval_rules: false)
         render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
       end
 

locale/gitlab.pot

@@ -3511,6 +3511,9 @@ msgid_plural "ApprovalRuleSummary|%{count} approvals required from %{membersCoun
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "ApprovalRule|Approval rules"
+msgstr ""
+
 msgid "ApprovalRule|Approvers"
 msgstr ""