From de49ab37b096c3e9378b30ada1361178a982d011 Mon Sep 17 00:00:00 2001
From: Samantha Ming <sming@gitlab.com>
Date: Wed, 11 Nov 2020 20:47:38 -0800
Subject: [PATCH] Collapse approval rules and move it under reviewer

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/232817
---
 .../merge_requests/creations_controller.rb    |  5 +++
 .../projects/merge_requests_controller.rb     |  2 +
 app/views/shared/issuable/_form.html.haml     |  2 +-
 .../shared/issuable/form/_metadata.html.haml  |  3 +-
 .../_metadata_issuable_reviewer.html.haml     |  4 +-
 .../mr_collapsed_approval_rules.yml           |  8 ++++
 .../approvals/components/mr_edit/app.vue      | 41 ++++++++++++++++++-
 .../shared/issuable/_approvals.html.haml      | 11 ++---
 .../user_creates_merge_request_spec.rb        |  2 +
 .../user_edits_approval_rules_mr_spec.rb      |  5 +++
 .../user_sets_approval_rules_spec.rb          |  4 ++
 .../merge_request/user_sets_approvers_spec.rb | 30 ++++++++++++++
 .../user_resets_approvers_spec.rb             |  2 +
 .../settings/merge_requests_settings_spec.rb  |  6 +--
 .../helpers/feature_approval_helper.rb        |  7 +++-
 .../issuable/_approvals.html.haml_spec.rb     |  3 +-
 locale/gitlab.pot                             |  3 ++
 17 files changed, 124 insertions(+), 14 deletions(-)
 create mode 100644 config/feature_flags/development/mr_collapsed_approval_rules.yml

diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 3e077c1af3772c..453928e251f424 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/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
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 627d578643f31e..c2bb441d640e18 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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]
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index c0aba0eef7f98a..16d9d7dae5a7e4 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/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
 
diff --git a/app/views/shared/issuable/form/_metadata.html.haml b/app/views/shared/issuable/form/_metadata.html.haml
index 459eb112e4fd6b..366e819d252c4c 100644
--- a/app/views/shared/issuable/form/_metadata.html.haml
+++ b/app/views/shared/issuable/form/_metadata.html.haml
@@ -1,5 +1,6 @@
 - 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
 
diff --git a/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml b/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml
index 5ff3a6781ad823..a4e2ce035cc79a 100644
--- a/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml
+++ b/app/views/shared/issuable/form/_metadata_issuable_reviewer.html.haml
@@ -1,5 +1,5 @@
 = 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
diff --git a/config/feature_flags/development/mr_collapsed_approval_rules.yml b/config/feature_flags/development/mr_collapsed_approval_rules.yml
new file mode 100644
index 00000000000000..5fca48ddc3ab86
--- /dev/null
+++ b/config/feature_flags/development/mr_collapsed_approval_rules.yml
@@ -0,0 +1,8 @@
+---
+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
diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue
index 68c9db7d3c4c57..dd5fff61b0f95e 100644
--- a/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue
+++ b/ee/app/assets/javascripts/approvals/components/mr_edit/app.vue
@@ -1,19 +1,58 @@
 <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>
-  <app>
+  <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>
diff --git a/ee/app/views/shared/issuable/_approvals.html.haml b/ee/app/views/shared/issuable/_approvals.html.haml
index 40b8ed64561d72..127f50848ab8f4 100644
--- a/ee/app/views/shared/issuable/_approvals.html.haml
+++ b/ee/app/views/shared/issuable/_approvals.html.haml
@@ -3,8 +3,9 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.approval_feature_available?
 
-.form-group.row
-  .col-sm-2.col-form-label
-    = form.label :approver_ids, "Approval rules"
-  .col-sm-10
-    = render_if_exists 'shared/issuable/approver_suggestion', issuable: issuable, presenter: presenter
+- 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
+      = render_if_exists 'shared/issuable/approver_suggestion', issuable: issuable, presenter: presenter
diff --git a/ee/spec/features/merge_request/user_creates_merge_request_spec.rb b/ee/spec/features/merge_request/user_creates_merge_request_spec.rb
index c11aab1c45e0ad..299afcbae9de13 100644
--- a/ee/spec/features/merge_request/user_creates_merge_request_spec.rb
+++ b/ee/spec/features/merge_request/user_creates_merge_request_spec.rb
@@ -39,6 +39,8 @@
 
     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
diff --git a/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb b/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb
index 65520a8a65b2d1..6fcb94e22e05bf 100644
--- a/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb
+++ b/ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb
@@ -41,12 +41,16 @@ def add_approval_rule_member(type, name)
   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 @@ def add_approval_rule_member(type, name)
     before do
       group.add_developer create(:user)
 
+      click_button 'Approval rules'
       click_button "Add approval rule"
     end
 
diff --git a/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb
index 34c7d1c9948cda..a95ae19d49466f 100644
--- a/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb
+++ b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb
@@ -25,6 +25,8 @@ def page_rule_names
       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 @@ def page_rule_names
       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)
diff --git a/ee/spec/features/merge_request/user_sets_approvers_spec.rb b/ee/spec/features/merge_request/user_sets_approvers_spec.rb
index 12df752bef31a9..5470a0abb0aeb1 100644
--- a/ee/spec/features/merge_request/user_sets_approvers_spec.rb
+++ b/ee/spec/features/merge_request/user_sets_approvers_spec.rb
@@ -11,6 +11,36 @@
   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) }
diff --git a/ee/spec/features/merge_requests/user_resets_approvers_spec.rb b/ee/spec/features/merge_requests/user_resets_approvers_spec.rb
index f787d9fc3b5c61..15e872f3a233b5 100644
--- a/ee/spec/features/merge_requests/user_resets_approvers_spec.rb
+++ b/ee/spec/features/merge_requests/user_resets_approvers_spec.rb
@@ -32,6 +32,8 @@
   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'
diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb
index 701edce2240b38..4efb445239f831 100644
--- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb
+++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb
@@ -23,7 +23,7 @@
   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 @@
   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 @@
 
       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
diff --git a/ee/spec/support/helpers/feature_approval_helper.rb b/ee/spec/support/helpers/feature_approval_helper.rb
index 1cc1830cd90ded..ae8f24be496e21 100644
--- a/ee/spec/support/helpers/feature_approval_helper.rb
+++ b/ee/spec/support/helpers/feature_approval_helper.rb
@@ -1,8 +1,13 @@
 # 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
diff --git a/ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb b/ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb
index 7c7eb619838401..73558401720bb3 100644
--- a/ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb
+++ b/ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb
@@ -22,8 +22,9 @@
   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
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index d93d2dbb8a31ba..7284bc9d8b4047 100644
--- a/locale/gitlab.pot
+++ b/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 ""
 
-- 
GitLab