From 1196031b904d8a2a48dbab6977ae4ec606e2b709 Mon Sep 17 00:00:00 2001
From: Samantha Ming <sming@gitlab.com>
Date: Tue, 28 Jul 2020 23:32:59 -0700
Subject: [PATCH 1/2] Add Commented by to MR widget

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/10294
---
 .../components/approvals/approvals_list.vue   | 24 +++++++++----
 .../approvals/approvals_list_spec.js          | 36 ++++++++++++++++---
 locale/gitlab.pot                             |  3 ++
 3 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
index 5ecb5be90ff2f4..0f9207caf4a9aa 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
@@ -92,11 +92,12 @@ export default {
 <template>
   <table class="table m-0">
     <thead class="thead-white text-nowrap">
-      <tr class="d-none d-sm-table-row">
+      <tr class="d-none d-md-table-row">
         <th class="w-0"></th>
         <th class="w-25">{{ s__('MRApprovals|Approvers') }}</th>
         <th class="w-50"></th>
         <th>{{ s__('MRApprovals|Approvals') }}</th>
+        <th>{{ s__('MRApprovals|Commented by') }}</th>
         <th>{{ s__('MRApprovals|Approved by') }}</th>
       </tr>
     </thead>
@@ -110,7 +111,7 @@ export default {
       <tr v-for="rule in rules" :key="rule.id">
         <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
         <td :colspan="rule.rule_type === $options.ruleTypeAnyApprover ? 2 : 1">
-          <div class="d-none d-sm-block js-name">
+          <div class="d-none d-md-block js-name">
             <empty-rule-name
               v-if="rule.rule_type === $options.ruleTypeAnyApprover"
               :eligible-approvers-docs-path="eligibleApproversDocsPath"
@@ -131,7 +132,7 @@ export default {
               :security-approvals-help-page-path="securityApprovalsHelpPagePath"
             />
           </div>
-          <div class="d-flex d-sm-none flex-column js-summary">
+          <div class="d-flex d-md-none flex-column js-summary">
             <empty-rule-name
               v-if="rule.rule_type === $options.ruleTypeAnyApprover"
               :eligible-approvers-docs-path="eligibleApproversDocsPath"
@@ -144,6 +145,14 @@ export default {
               :img-size="24"
               empty-text=""
             />
+            <div v-if="rule.commented_by.length > 0" class="mt-2">
+              <span>{{ s__('MRApprovals|Commented by') }}</span>
+              <user-avatar-list
+                class="d-inline-block align-middle"
+                :items="rule.commented_by"
+                :img-size="24"
+              />
+            </div>
             <div v-if="rule.approved_by.length" class="mt-2">
               <span>{{ s__('MRApprovals|Approved by') }}</span>
               <user-avatar-list
@@ -156,14 +165,17 @@ export default {
         </td>
         <td
           v-if="rule.rule_type !== $options.ruleTypeAnyApprover"
-          class="d-none d-sm-table-cell js-approvers"
+          class="d-none d-md-table-cell js-approvers"
         >
           <div><user-avatar-list :items="rule.approvers" :img-size="24" empty-text="" /></div>
         </td>
-        <td class="w-0 d-none d-sm-table-cell text-nowrap js-pending">
+        <td class="w-0 d-none d-md-table-cell text-nowrap js-pending">
           {{ pendingApprovalsText(rule) }}
         </td>
-        <td class="d-none d-sm-table-cell js-approved-by">
+        <td class="d-none d-md-table-cell js-commented-by">
+          <user-avatar-list :items="rule.commented_by" :img-size="24" empty-text="" />
+        </td>
+        <td class="d-none d-md-table-cell js-approved-by">
           <user-avatar-list :items="rule.approved_by" :img-size="24" empty-text="" />
         </td>
       </tr>
diff --git a/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js b/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
index 8e903d1352a1b9..dd7e54e6221914 100644
--- a/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
+++ b/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
@@ -9,6 +9,7 @@ const testRuleApproved = () => ({
   name: 'Lorem',
   approvals_required: 2,
   approved_by: [{ id: 1 }, { id: 2 }, { id: 3 }],
+  commented_by: [{ id: 1 }, { id: 2 }, { id: 3 }],
   approvers: testApprovers(),
   approved: true,
 });
@@ -17,6 +18,7 @@ const testRuleUnapproved = () => ({
   name: 'Ipsum',
   approvals_required: 1,
   approved_by: [],
+  commented_by: [],
   approvers: testApprovers(),
   approved: false,
 });
@@ -25,6 +27,7 @@ const testRuleOptional = () => ({
   name: 'Dolar',
   approvals_required: 0,
   approved_by: [{ id: 1 }],
+  commented_by: [{ id: 1 }],
   approvers: testApprovers(),
   approved: false,
 });
@@ -35,6 +38,7 @@ const testRuleFallback = () => ({
   rule_type: 'any_approver',
   approvals_required: 3,
   approved_by: [{ id: 1 }, { id: 2 }],
+  commented_by: [{ id: 1 }, { id: 2 }],
   approvers: [],
   approved: false,
 });
@@ -44,6 +48,7 @@ const testRuleCodeOwner = () => ({
   fallback: true,
   approvals_required: 3,
   approved_by: [{ id: 1 }, { id: 2 }],
+  commented_by: [{ id: 1 }, { id: 2 }],
   approvers: [],
   approved: false,
   rule_type: 'code_owner',
@@ -165,6 +170,18 @@ describe('EE MRWidget approvals list', () => {
       );
     });
 
+    it('renders commented by user avatar list', () => {
+      const commentedRow = findRowElement(row, 'commented-by');
+      const commentedBy = commentedRow.find(UserAvatarList);
+
+      expect(commentedBy.props()).toEqual(
+        expect.objectContaining({
+          items: rule.commented_by,
+          emptyText: '',
+        }),
+      );
+    });
+
     describe('summary text', () => {
       let summary;
 
@@ -191,10 +208,20 @@ describe('EE MRWidget approvals list', () => {
         );
       });
 
+      it('renders commented by list', () => {
+        const commentedBy = summary.findAll(UserAvatarList).at(1);
+
+        expect(commentedBy.exists()).toBe(true);
+        expect(commentedBy.props()).toEqual(
+          expect.objectContaining({
+            items: rule.commented_by,
+          }),
+        );
+      });
+
       it('renders approved by list', () => {
-        const approvedBy = summary.findAll(UserAvatarList).at(1);
+        const approvedBy = summary.findAll(UserAvatarList).at(2);
 
-        expect(approvedBy.exists()).toBe(true);
         expect(approvedBy.props()).toEqual(
           expect.objectContaining({
             items: rule.approved_by,
@@ -281,8 +308,9 @@ describe('EE MRWidget approvals list', () => {
       const summary = findRowElement(row, 'summary');
       const lists = summary.findAll(UserAvatarList);
 
-      expect(lists).toHaveLength(1);
-      expect(lists.at(0).props('items')).toEqual(rule.approved_by);
+      expect(lists).toHaveLength(2);
+      expect(lists.at(0).props('items')).toEqual(rule.commented_by);
+      expect(lists.at(1).props('items')).toEqual(rule.approved_by);
     });
   });
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 3b30832fa98047..97da258d9845c4 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -14507,6 +14507,9 @@ msgstr ""
 msgid "MRApprovals|Approvers"
 msgstr ""
 
+msgid "MRApprovals|Commented by"
+msgstr ""
+
 msgid "MRDiff|Show changes only"
 msgstr ""
 
-- 
GitLab


From a1f7787177cbaa7f2461675a93fe91aeaca79430 Mon Sep 17 00:00:00 2001
From: Samantha Ming <sming@gitlab.com>
Date: Tue, 11 Aug 2020 15:42:53 -0700
Subject: [PATCH 2/2] Update to use feature flag

- Add feature flag on BE
- Update component & test to use feature flag
---
 .../projects/merge_requests_controller.rb     |  1 +
 .../components/approvals/approvals_list.vue   | 38 +++++++++++++----
 .../approvals/approvals_list_spec.js          | 41 ++++++++++++++++++-
 3 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 25620852caca1f..7f21d9418dafdd 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -38,6 +38,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:batch_suggestions, @project, default_enabled: true)
     push_frontend_feature_flag(:auto_expand_collapsed_diffs, @project, default_enabled: true)
     push_frontend_feature_flag(:hide_jump_to_next_unresolved_in_threads, @project)
+    push_frontend_feature_flag(:approvals_commented_by, @project, default_enabled: true)
   end
 
   before_action do
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
index 0f9207caf4a9aa..8f060c24aa2049 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_list.vue
@@ -6,6 +6,7 @@ import { RULE_TYPE_CODE_OWNER, RULE_TYPE_ANY_APPROVER } from 'ee/approvals/const
 import { sprintf, __, s__ } from '~/locale';
 import UserAvatarList from '~/vue_shared/components/user_avatar/user_avatar_list.vue';
 import ApprovedIcon from './approved_icon.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 
 export default {
   components: {
@@ -14,6 +15,7 @@ export default {
     ApprovalCheckRulePopover,
     EmptyRuleName,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     approvalRules: {
       type: Array,
@@ -92,12 +94,15 @@ export default {
 <template>
   <table class="table m-0">
     <thead class="thead-white text-nowrap">
-      <tr class="d-none d-md-table-row">
+      <tr
+        :class="glFeatures.approvalsCommentedBy ? 'd-md-table-row' : 'd-sm-table-row'"
+        class="d-none"
+      >
         <th class="w-0"></th>
         <th class="w-25">{{ s__('MRApprovals|Approvers') }}</th>
         <th class="w-50"></th>
         <th>{{ s__('MRApprovals|Approvals') }}</th>
-        <th>{{ s__('MRApprovals|Commented by') }}</th>
+        <th v-if="glFeatures.approvalsCommentedBy">{{ s__('MRApprovals|Commented by') }}</th>
         <th>{{ s__('MRApprovals|Approved by') }}</th>
       </tr>
     </thead>
@@ -111,7 +116,10 @@ export default {
       <tr v-for="rule in rules" :key="rule.id">
         <td class="w-0"><approved-icon :is-approved="rule.approved" /></td>
         <td :colspan="rule.rule_type === $options.ruleTypeAnyApprover ? 2 : 1">
-          <div class="d-none d-md-block js-name">
+          <div
+            :class="glFeatures.approvalsCommentedBy ? 'd-md-block' : 'd-sm-block'"
+            class="d-none js-name"
+          >
             <empty-rule-name
               v-if="rule.rule_type === $options.ruleTypeAnyApprover"
               :eligible-approvers-docs-path="eligibleApproversDocsPath"
@@ -132,7 +140,10 @@ export default {
               :security-approvals-help-page-path="securityApprovalsHelpPagePath"
             />
           </div>
-          <div class="d-flex d-md-none flex-column js-summary">
+          <div
+            :class="glFeatures.approvalsCommentedBy ? 'd-md-none' : 'd-sm-none'"
+            class="d-flex flex-column js-summary"
+          >
             <empty-rule-name
               v-if="rule.rule_type === $options.ruleTypeAnyApprover"
               :eligible-approvers-docs-path="eligibleApproversDocsPath"
@@ -145,7 +156,10 @@ export default {
               :img-size="24"
               empty-text=""
             />
-            <div v-if="rule.commented_by.length > 0" class="mt-2">
+            <div
+              v-if="glFeatures.approvalsCommentedBy && rule.commented_by.length > 0"
+              class="mt-2"
+            >
               <span>{{ s__('MRApprovals|Commented by') }}</span>
               <user-avatar-list
                 class="d-inline-block align-middle"
@@ -165,14 +179,22 @@ export default {
         </td>
         <td
           v-if="rule.rule_type !== $options.ruleTypeAnyApprover"
-          class="d-none d-md-table-cell js-approvers"
+          :class="glFeatures.approvalsCommentedBy ? 'd-md-table-cell' : 'd-sm-table-cell'"
+          class="d-none js-approvers"
         >
           <div><user-avatar-list :items="rule.approvers" :img-size="24" empty-text="" /></div>
         </td>
-        <td class="w-0 d-none d-md-table-cell text-nowrap js-pending">
+        <td
+          :class="glFeatures.approvalsCommentedBy ? 'd-md-table-cell' : 'd-sm-table-cell'"
+          class="w-0 d-none text-nowrap js-pending"
+        >
           {{ pendingApprovalsText(rule) }}
         </td>
-        <td class="d-none d-md-table-cell js-commented-by">
+        <td
+          v-if="glFeatures.approvalsCommentedBy"
+          :class="glFeatures.approvalsCommentedBy ? 'd-md-table-cell' : 'd-sm-table-cell'"
+          class="d-none js-commented-by"
+        >
           <user-avatar-list :items="rule.commented_by" :img-size="24" empty-text="" />
         </td>
         <td class="d-none d-md-table-cell js-approved-by">
diff --git a/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js b/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
index dd7e54e6221914..ed40052de03e64 100644
--- a/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
+++ b/ee/spec/frontend/vue_mr_widget/components/approvals/approvals_list_spec.js
@@ -59,9 +59,16 @@ const testRules = () => [testRuleApproved(), testRuleUnapproved(), testRuleOptio
 describe('EE MRWidget approvals list', () => {
   let wrapper;
 
-  const createComponent = (props = {}) => {
+  const createComponent = (props = {}, options = {}) => {
+    const { approvalsCommentedBy = true } = options;
+
     wrapper = shallowMount(ApprovalsList, {
       propsData: props,
+      provide: {
+        glFeatures: {
+          approvalsCommentedBy,
+        },
+      },
     });
   };
 
@@ -211,7 +218,6 @@ describe('EE MRWidget approvals list', () => {
       it('renders commented by list', () => {
         const commentedBy = summary.findAll(UserAvatarList).at(1);
 
-        expect(commentedBy.exists()).toBe(true);
         expect(commentedBy.props()).toEqual(
           expect.objectContaining({
             items: rule.commented_by,
@@ -355,4 +361,35 @@ describe('EE MRWidget approvals list', () => {
       expect(ruleSection.at(1).text()).toEqual(rule.section);
     });
   });
+
+  describe('approvalsCommentedBy feature flag is off', () => {
+    let row;
+
+    beforeEach(() => {
+      const rule = testRuleApproved();
+      createComponent(
+        {
+          approvalRules: [rule],
+        },
+        { approvalsCommentedBy: false },
+      );
+      row = findRows().at(0);
+    });
+
+    it('does not render commented by in table head', () => {
+      expect(wrapper.find('thead').text()).not.toContain('Commented by');
+    });
+
+    it('does not render commented by in summary text', () => {
+      const summary = findRowElement(row, 'summary');
+
+      expect(summary.findAll(UserAvatarList).length).toBe(2);
+    });
+
+    it('does not render commented by in user avatar list', () => {
+      const commentedBy = findRowElement(row, 'commented-by');
+
+      expect(commentedBy.exists()).toBe(false);
+    });
+  });
 });
-- 
GitLab