Merge branch 'ajk-reviewer-widget-approved' into 'master'

Show which reviewers have approved an MR

See merge request gitlab-org/gitlab!54365

app/assets/javascripts/sidebar/components/reviewers/uncollapsed_reviewer_list.vue

 <script>
 import { GlButton, GlTooltipDirective, GlIcon } from '@gitlab/ui';
+import { sprintf, s__ } from '~/locale';
 import ReviewerAvatarLink from './reviewer_avatar_link.vue';
 
 const LOADING_STATE = 'loading';
@@ -50,6 +51,9 @@ export default {
     },
   },
   methods: {
+    approvedByTooltipTitle(user) {
+      return sprintf(s__('MergeRequest|Approved by @%{username}'), user);
+    },
     toggleShowLess() {
       this.showLess = !this.showLess;
     },
@@ -57,6 +61,7 @@ export default {
       this.loadingStates[userId] = LOADING_STATE;
       this.$emit('request-review', { userId, callback: this.requestReviewComplete });
     },
+
     requestReviewComplete(userId, success) {
       if (success) {
         this.loadingStates[userId] = SUCCESS_STATE;
@@ -85,11 +90,20 @@ export default {
       <reviewer-avatar-link :user="user" :root-path="rootPath" :issuable-type="issuableType">
         <div class="gl-ml-3">@{{ user.username }}</div>
       </reviewer-avatar-link>
+      <gl-icon
+        v-if="user.approved"
+        v-gl-tooltip.left
+        :size="16"
+        :title="approvedByTooltipTitle(user)"
+        name="status-success"
+        class="float-right gl-my-2 gl-ml-2 gl-text-green-500"
+        data-testid="re-approved"
+      />
       <gl-icon
         v-if="loadingStates[user.id] === $options.SUCCESS_STATE"
         :size="24"
         name="check"
-        class="float-right gl-text-green-500"
+        class="float-right gl-py-2 gl-mr-2 gl-text-green-500"
         data-testid="re-request-success"
       />
       <gl-button

app/assets/javascripts/sidebar/queries/sidebarDetailsMR.query.graphql

+query mergeRequestSidebarDetails($fullPath: ID!, $iid: String!) {
+  project(fullPath: $fullPath) {
+    mergeRequest(iid: $iid) {
+      iid # currently unused.
+    }
+  }
+}

app/assets/javascripts/sidebar/services/sidebar_service.js

-import sidebarDetailsQuery from 'ee_else_ce/sidebar/queries/sidebarDetails.query.graphql';
+import sidebarDetailsIssueQuery from 'ee_else_ce/sidebar/queries/sidebarDetails.query.graphql';
 import { convertToGraphQLId } from '~/graphql_shared/utils';
 import createGqClient, { fetchPolicies } from '~/lib/graphql';
 import axios from '~/lib/utils/axios_utils';
 import reviewerRereviewMutation from '../queries/reviewer_rereview.mutation.graphql';
+import sidebarDetailsMRQuery from '../queries/sidebarDetailsMR.query.graphql';
+
+const queries = {
+  merge_request: sidebarDetailsMRQuery,
+  issue: sidebarDetailsIssueQuery,
+};
 
 export const gqClient = createGqClient(
   {},
@@ -20,6 +26,7 @@ export default class SidebarService {
       this.projectsAutocompleteEndpoint = endpointMap.projectsAutocompleteEndpoint;
       this.fullPath = endpointMap.fullPath;
       this.iid = endpointMap.iid;
+      this.issuableType = endpointMap.issuableType;
 
       SidebarService.singleton = this;
     }
@@ -31,7 +38,7 @@ export default class SidebarService {
     return Promise.all([
       axios.get(this.endpoint),
       gqClient.query({
-        query: sidebarDetailsQuery,
+        query: this.sidebarDetailsQuery(),
         variables: {
           fullPath: this.fullPath,
           iid: this.iid.toString(),
@@ -40,6 +47,10 @@ export default class SidebarService {
     ]);
   }
 
+  sidebarDetailsQuery() {
+    return queries[this.issuableType];
+  }
+
   update(key, data) {
     return axios.put(this.endpoint, { [key]: data });
   }

app/assets/javascripts/sidebar/sidebar_mediator.js

@@ -22,6 +22,7 @@ export default class SidebarMediator {
       projectsAutocompleteEndpoint: options.projectsAutocompleteEndpoint,
       fullPath: options.fullPath,
       iid: options.iid,
+      issuableType: options.issuableType,
     });
     SidebarMediator.singleton = this;
   }

app/helpers/issuables_helper.rb

@@ -388,7 +388,8 @@ module IssuablesHelper
       iid: issuable[:iid],
       severity: issuable[:severity],
       timeTrackingLimitToHours: Gitlab::CurrentSettings.time_tracking_limit_to_hours,
-      createNoteEmail: issuable[:create_note_email]
+      createNoteEmail: issuable[:create_note_email],
+      issuableType: issuable[:type]
     }
   end
 

app/serializers/merge_request_user_entity.rb

@@ -4,6 +4,10 @@ class MergeRequestUserEntity < ::API::Entities::UserBasic
   include UserStatusTooltip
   include RequestAwareEntity
 
+  def self.satisfies(*methods)
+    ->(_, options) { methods.all? { |m| options[:merge_request].try(m) } }
+  end
+
   expose :can_merge do |reviewer, options|
     options[:merge_request]&.can_be_merged_by?(reviewer)
   end
@@ -12,11 +16,17 @@ class MergeRequestUserEntity < ::API::Entities::UserBasic
     request.current_user&.can?(:update_merge_request, options[:merge_request])
   end
 
-  expose :reviewed, if: -> (_, options) { options[:merge_request] && options[:merge_request].allows_reviewers? } do |reviewer, options|
+  expose :reviewed, if: satisfies(:present?, :allows_reviewers?) do |reviewer, options|
     reviewer = options[:merge_request].find_reviewer(reviewer)
 
     reviewer&.reviewed?
   end
+
+  expose :approved, if: satisfies(:present?) do |user, options|
+    # This approach is preferred over MergeRequest#approved_by? since this
+    # makes one query per merge request, whereas #approved_by? makes one per user
+    options[:merge_request].approvals.any? { |app| app.user_id == user.id }
+  end
 end
 
 MergeRequestUserEntity.prepend_if_ee('EE::MergeRequestUserEntity')

changelogs/unreleased/ajk-reviewer-widget-approved.yml

+---
+title: Show icon next to reviewers who have approved
+merge_request: 54365
+author:
+type: changed

doc/user/project/merge_requests/getting_started.md

@@ -164,6 +164,13 @@ the author of the merge request can request a new review from the reviewer:
 GitLab creates a new [to-do item](../../todos.md) for the reviewer, and sends
 them a notification email.
 
+#### Approval status
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/292936) in GitLab 13.10.
+
+If a user in the reviewer list has approved the merge request, a green tick symbol is
+shown to the right of their name.
+
 ### Merge requests to close issues
 
 If the merge request is being created to resolve an issue, you can

ee/app/assets/javascripts/sidebar/sidebar_mediator.js

@@ -9,7 +9,7 @@ export default class SidebarMediator extends CESidebarMediator {
   }
 
   processFetchedData(restData, graphQlData) {
-    super.processFetchedData(restData);
+    super.processFetchedData(restData, graphQlData);
     this.store.setWeightData(restData);
     this.store.setEpicData(restData);
     this.store.setStatusData(graphQlData);

locale/gitlab.pot

@@ -18874,6 +18874,9 @@ msgstr ""
 msgid "MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}"
 msgstr ""
 
+msgid "MergeRequest|Approved by @%{username}"
+msgstr ""
+
 msgid "MergeRequest|Compare %{target} and %{source}"
 msgstr ""
 

spec/frontend/sidebar/components/reviewers/uncollapsed_reviewer_list_spec.js

@@ -7,6 +7,8 @@ import userDataMock from '../../user_data_mock';
 describe('UncollapsedReviewerList component', () => {
   let wrapper;
 
+  const reviewerApprovalIcons = () => wrapper.findAll('[data-testid="re-approved"]');
+
   function createComponent(props = {}) {
     const propsData = {
       users: [],
@@ -58,19 +60,29 @@ describe('UncollapsedReviewerList component', () => {
       const user = userDataMock();
 
       createComponent({
-        users: [user, { ...user, id: 2, username: 'hello-world' }],
+        users: [user, { ...user, id: 2, username: 'hello-world', approved: true }],
       });
     });
 
-    it('only has one user', () => {
+    it('has both users', () => {
       expect(wrapper.findAll(ReviewerAvatarLink).length).toBe(2);
     });
 
-    it('shows one user with avatar, username and author name', () => {
+    it('shows both users with avatar, username and author name', () => {
       expect(wrapper.text()).toContain(`@root`);
       expect(wrapper.text()).toContain(`@hello-world`);
     });
 
+    it('renders approval icon', () => {
+      expect(reviewerApprovalIcons().length).toBe(1);
+    });
+
+    it('shows that hello-world approved', () => {
+      const icon = reviewerApprovalIcons().at(0);
+
+      expect(icon.attributes('title')).toEqual('Approved by @hello-world');
+    });
+
     it('renders re-request loading icon', async () => {
       await wrapper.setData({ loadingStates: { 2: 'loading' } });
 

spec/frontend/sidebar/user_data_mock.js

@@ -10,4 +10,5 @@ export default () => ({
   can_merge: true,
   can_update_merge_request: true,
   reviewed: true,
+  approved: false,
 });

spec/serializers/merge_request_user_entity_spec.rb

@@ -3,19 +3,22 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestUserEntity do
-  let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
-  let(:request) { EntityRequest.new(project: project, current_user: user) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let(:request) { EntityRequest.new(project: merge_request.target_project, current_user: user) }
 
   let(:entity) do
-    described_class.new(user, request: request)
+    described_class.new(user, request: request, merge_request: merge_request)
   end
 
-  context 'as json' do
+  describe '#as_json' do
     subject { entity.as_json }
 
     it 'exposes needed attributes' do
-      expect(subject).to include(:id, :name, :username, :state, :avatar_url, :web_url, :can_merge)
+      is_expected.to include(
+        :id, :name, :username, :state, :avatar_url, :web_url,
+        :can_merge, :can_update_merge_request, :reviewed, :approved
+      )
     end
 
     context 'when `status` is not preloaded' do
@@ -24,6 +27,22 @@ RSpec.describe MergeRequestUserEntity do
       end
     end
 
+    context 'when the user has not approved the merge-request' do
+      it 'exposes that the user has not approved the MR' do
+        expect(subject).to include(approved: false)
+      end
+    end
+
+    context 'when the user has approved the merge-request' do
+      before do
+        merge_request.approvals.create!(user: user)
+      end
+
+      it 'exposes that the user has approved the MR' do
+        expect(subject).to include(approved: true)
+      end
+    end
+
     context 'when `status` is preloaded' do
       before do
         user.create_status!(availability: :busy)
@@ -35,5 +54,27 @@ RSpec.describe MergeRequestUserEntity do
         expect(subject[:availability]).to eq('busy')
       end
     end
+
+    describe 'performance' do
+      let_it_be(:user_a) { create(:user) }
+      let_it_be(:user_b) { create(:user) }
+      let_it_be(:merge_request_b) { create(:merge_request) }
+
+      it 'is linear in the number of merge requests' do
+        pending "See: https://gitlab.com/gitlab-org/gitlab/-/issues/322549"
+        baseline = ActiveRecord::QueryRecorder.new do
+          ent = described_class.new(user_a, request: request, merge_request: merge_request)
+          ent.as_json
+        end
+
+        expect do
+          a = described_class.new(user_a, request: request, merge_request: merge_request_b)
+          b = described_class.new(user_b, request: request, merge_request: merge_request_b)
+
+          a.as_json
+          b.as_json
+        end.not_to exceed_query_limit(baseline)
+      end
+    end
   end
 end