Merge branch '6309-faulty-username-tooltip' into 'master'

Resolve "Bug: Avatar Icons have wrong tooltip in approval"

Closes #6309

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

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.vue

 <script>
 import Flash from '~/flash';
-import LinkToMemberAvatar from 'ee/vue_shared/components/link_to_member_avatar.vue';
+import Icon from '~/vue_shared/components/icon.vue';
+import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue';
+
 import { s__ } from '~/locale';
 import eventHub from '~/vue_merge_request_widget/event_hub';
 
 export default {
   name: 'ApprovalsFooter',
   components: {
-    LinkToMemberAvatar,
+    Icon,
+    UserAvatarLink,
   },
   props: {
     mr: {
@@ -84,22 +87,22 @@ export default {
     <div class="approvers-prefix">
       <p>{{ approvedByText }}</p>
       <div class="approvers-list">
-        <link-to-member-avatar
-          v-for="(approver, index) in approvedBy"
-          :key="index"
-          :avatar-size="20"
-          :avatar-url="approver.user.avatar_url"
-          :display-name="approver.user.name"
-          :profile-url="approver.user.web_url"
-          :show-tooltip="true"
-          extra-link-class="approver-avatar js-approver-list-member"
+        <user-avatar-link
+          v-for="approver in approvedBy"
+          :key="approver.user.username"
+          class="js-approver-list-member"
+          :img-size="20"
+          :img-src="approver.user.avatar_url"
+          :img-alt="approver.user.name"
+          :link-href="approver.user.web_url"
+          :tooltip-text="approver.user.name"
+          tooltip-placement="bottom"
         />
-        <link-to-member-avatar
+        <icon
           v-for="n in approvalsLeft"
           :key="n"
-          :avatar-size="20"
-          :clickable="false"
-          :show-tooltip="false"
+          name="dotted-circle"
+          class="avatar avatar-placeholder s20"
         />
       </div>
       <button

ee/app/assets/javascripts/vue_shared/components/link_to_member_avatar.vue

-<script>
-// Analogue of link_to_member_avatar in app/helpers/projects_helper.rb
-import pendingAvatarSvg from 'ee_icons/_icon_dotted_circle.svg';
-
-export default {
-  props: {
-    avatarUrl: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    profileUrl: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    displayName: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    extraAvatarClass: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    extraLinkClass: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    showTooltip: {
-      type: Boolean,
-      required: false,
-      default: true,
-    },
-    clickable: {
-      type: Boolean,
-      required: false,
-      default: true,
-    },
-    tooltipContainer: {
-      type: String,
-      required: false,
-      default: 'body',
-    },
-    avatarSize: {
-      type: Number,
-      required: false,
-      default: 32,
-    },
-  },
-  data() {
-    return {
-      avatarBaseClass: 'avatar avatar-inline',
-      pendingAvatarSvg,
-    };
-  },
-  computed: {
-    avatarSizeClass() {
-      return `s${this.avatarSize}`;
-    },
-    avatarHtmlClass() {
-      return `${this.avatarSizeClass} ${this.avatarBaseClass} avatar-placeholder`;
-    },
-    tooltipClass() {
-      return this.showTooltip ? 'has-tooltip' : '';
-    },
-    avatarClass() {
-      return `${this.avatarBaseClass} ${this.avatarSizeClass} ${this.extraAvatarClass}`;
-    },
-    disabledClass() {
-      return !this.clickable ? 'disabled' : '';
-    },
-    linkClass() {
-      return `author-link ${this.tooltipClass} ${this.extraLinkClass} ${this.disabledClass}`;
-    },
-  },
-};
-</script>
-
-<template>
-  <div class="link-to-member-avatar">
-    <a
-      :href="profileUrl"
-      :class="linkClass"
-      :title="displayName"
-      :data-container="tooltipContainer"
-    >
-      <img
-        v-if="avatarUrl"
-        :class="avatarClass"
-        :src="avatarUrl"
-        :width="avatarSize"
-        :height="avatarSize"
-        :alt="displayName"
-      />
-      <span
-        v-else
-        :class="avatarHtmlClass"
-        :width="avatarSize"
-        :height="avatarSize"
-        v-html="pendingAvatarSvg"
-      >
-      </span>
-    </a>
-  </div>
-</template>

ee/app/assets/stylesheets/pages/merge_requests.scss

-.link-to-member-avatar {
-  .disabled {
-    pointer-events: none;
-    cursor: default;
-  }
-
-  .avatar {
-    margin-bottom: 0;
-    margin-left: 7px;
-    display: block;
-  }
-}
-
 .approvals-body {
   @include media-breakpoint-up(md) {
     display: flex;
@@ -41,12 +28,11 @@
   .approvers-list {
     display: flex;
     align-items: center;
-
-    .link-to-member-avatar:not(:first-child) {
-      img {
-        margin-left: 0;
-      }
+    margin-left: 7px;
   }
+
+  .avatar-placeholder {
+    color: $gray-darkest;
   }
 
   .unapprove-btn {
@@ -63,9 +49,5 @@
       outline: none;
     }
   }
-
-  .approver-avatar {
-    position: relative;
-  }
 }
 

ee/app/views/projects/merge_requests/widget/open/_approvals_body.html.haml

-%approvals-body{ ':user-can-approve' => 'approvals.user_can_approve', ':user-has-approved' => 'approvals.user_has_approved', ':approved-by' => 'approvals.approved_by', ':approvals-left' => 'approvals.approvals_left', ':suggested-approvers' => 'approvals.suggested_approvers' }

ee/app/views/projects/merge_requests/widget/open/_approvals_footer.html.haml

-%approvals-footer{ 'pending-avatar-svg' => custom_icon('icon_dotted_circle'), 'checkmark-svg' => custom_icon('icon_checkmark'), ':user-can-approve' => 'approvals.user_can_approve', ':user-has-approved' => 'approvals.user_has_approved', ':approved-by' => 'approvals.approved_by', ':approvals-left' => 'approvals.approvals_left' }

ee/app/views/shared/icons/_icon_dotted_circle.svg

-<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 27 27"><path fill="#bfbfbf" fill-rule="evenodd" d="m13.5 26.5c1.412 0 2.794-.225 4.107-.662l-.316-.949c-1.212.403-2.487.611-3.792.611v1m6.06-1.495c1.234-.651 2.355-1.498 3.321-2.504l-.721-.692c-.892.929-1.928 1.711-3.067 2.312l.467.884m4.66-4.147c.79-1.149 1.391-2.418 1.777-3.762l-.961-.276c-.356 1.24-.911 2.411-1.64 3.471l.824.567m2.184-5.761c.063-.518.096-1.041.097-1.568 0-.896-.085-1.758-.255-2.603l-.98.197c.157.78.236 1.576.236 2.405-.001.486-.031.97-.09 1.448l.993.122m-.738-6.189c-.493-1.307-1.195-2.523-2.075-3.605l-.776.631c.812.999 1.46 2.122 1.916 3.327l.935-.353m-3.539-5.133c-1.043-.926-2.229-1.68-3.512-2.229l-.394.919c1.184.507 2.279 1.203 3.242 2.058l.664-.748m-5.463-2.886c-1.012-.253-2.058-.384-3.119-.388-.378 0-.717.013-1.059.039l.077.997c.316-.024.629-.036.98-.036.979.003 1.944.124 2.879.358l.243-.97m-6.238-.022c-1.361.33-2.653.878-3.832 1.619l.532.847c1.089-.684 2.281-1.189 3.536-1.494l-.236-.972m-5.517 2.878c-1.047.922-1.94 2.01-2.643 3.212l.864.504c.649-1.112 1.474-2.114 2.441-2.966l-.661-.75m-3.54 5.076c-.499 1.293-.789 2.664-.854 4.072l.999.046c.06-1.3.328-2.564.788-3.758l-.933-.36m-.78 6.202c.163 1.396.549 2.744 1.14 4l.905-.425c-.545-1.16-.902-2.404-1.052-3.692l-.993.116m2.177 5.814c.788 1.151 1.756 2.169 2.866 3.01l.606-.796c-1.025-.78-1.919-1.721-2.646-2.783l-.825.565m4.665 4.164c1.23.65 2.559 1.1 3.943 1.328l.162-.987c-1.278-.21-2.503-.625-3.638-1.225l-.468.884m6.02 1.501c.024 0 .024 0 .048 0v-1c-.022 0-.022 0-.044 0l-.004 1"/></svg>
\ No newline at end of file

ee/changelogs/unreleased/6309-faulty-username-tooltip.yml

+---
+title: Ensure that avatars in approvals have correct tooltip
+merge_request: 6269
+author:
+type: fixed

ee/spec/features/merge_request/user_approves_spec.rb

@@ -27,14 +27,14 @@ describe 'Merge request > User approves', :js do
         approve_merge_request
 
         expect(page).to have_content('Approved by')
-        expect(page).to have_css('.approver-avatar')
+        expect(page).to have_css('.js-approver-list-member')
       end
 
       it 'I am able to unapprove' do
         approve_merge_request
         unapprove_merge_request
 
-        expect(page).to have_no_css('.approver-avatar')
+        expect(page).to have_no_css('.js-approver-list-member')
       end
     end
 
@@ -48,14 +48,14 @@ describe 'Merge request > User approves', :js do
         approve_merge_request
 
         expect(page).to have_content('Approved by')
-        expect(page).to have_css('.approver-avatar')
+        expect(page).to have_css('.js-approver-list-member')
       end
 
       it 'I am able to unapprove' do
         approve_merge_request
         unapprove_merge_request
 
-        expect(page).to have_no_css('.approver-avatar')
+        expect(page).to have_no_css('.js-approver-list-member')
       end
     end
 

ee/spec/javascripts/approvals/approvals_footer_spec.js

 import Vue from 'vue';
-import pendingAvatarSvg from 'ee_icons/_icon_dotted_circle.svg';
 import ApprovalsFooter from 'ee/vue_merge_request_widget/components/approvals/approvals_footer.vue';
 import { TEST_HOST } from 'spec/test_constants';
 
@@ -13,8 +12,7 @@ describe('Approvals Footer Component', () => {
     userCanApprove: false,
     userHasApproved: true,
     approvedBy: [],
-    approvalsLeft: 1,
-    pendingAvatarSvg,
+    approvalsLeft: 3,
   };
 
   beforeEach(() => {
@@ -58,17 +56,46 @@ describe('Approvals Footer Component', () => {
   });
 
   describe('approvers list', () => {
+    const avatarUrl = `${TEST_HOST}/dummy.jpg`;
+
     it('shows link to member avatar for for each approver', done => {
-      vm.approvedBy.push({
+      vm.approvedBy = [
+        {
           user: {
-          avatar_url: `${TEST_HOST}/dummy.jpg`,
+            username: 'Tanuki',
+            avatar_url: avatarUrl,
           },
-      });
+        },
+      ];
 
       Vue.nextTick(() => {
         const memberImage = document.querySelector('.approvers-list img');
 
-        expect(memberImage.src).toMatch(/dummy\.jpg$/);
+        expect(memberImage.src).toContain(avatarUrl);
+        done();
+      });
+    });
+
+    it('allows to add multiple approvers withoutd duplicate-key errors', done => {
+      vm.approvedBy = [
+        {
+          user: {
+            username: 'Tanuki',
+            avatar_url: avatarUrl,
+          },
+        },
+        {
+          user: {
+            username: 'Tanuki2',
+            avatar_url: avatarUrl,
+          },
+        },
+      ];
+
+      Vue.nextTick(() => {
+        const approvers = document.querySelectorAll('.approvers-list img');
+
+        expect(approvers.length).toBe(2);
         done();
       });
     });

ee/spec/javascripts/vue_shared/components/link_to_member_avatar_spec.js

-import Vue from 'vue';
-import linkToMemberAvatar from 'ee/vue_shared/components/link_to_member_avatar.vue';
-
-describe('Link To Members Components', () => {
-  let vm;
-  const propsData = {
-    avatarSize: 32,
-    avatarUrl: 'myavatarurl.com',
-    profileUrl: 'profileUrl.com',
-    displayName: 'mydisplayname',
-    extraAvatarClass: 'myextraavatarclass',
-    extraLinkClass: 'myextralinkclass',
-    showTooltip: true,
-  };
-
-  beforeEach(() => {
-    setFixtures('<div id="mock-container"></div>');
-
-    const LinkToMembersComponent = Vue.extend(linkToMemberAvatar);
-
-    vm = new LinkToMembersComponent({
-      el: '#mock-container',
-      propsData,
-    }).$mount();
-  });
-
-  afterEach(() => {
-    vm.$destroy();
-  });
-
-  it('should default to the body as tooltip container', () => {
-    expect(vm.tooltipContainer).toBe('body');
-  });
-
-  it('should return a defined Vue component', () => {
-    expect(vm).toBeDefined();
-    expect(vm.$data).toBeDefined();
-  });
-
-  it('should have <a> children', () => {
-    const componentLink = vm.$el.querySelector('a');
-
-    expect(componentLink).not.toBeNull();
-    expect(componentLink.getAttribute('href')).toBe(propsData.profileUrl);
-  });
-
-  it('should show a <img> if the avatarUrl is set', () => {
-    const avatarImg = vm.$el.querySelector('a img');
-
-    expect(avatarImg).not.toBeNull();
-    expect(avatarImg.getAttribute('src')).toBe(propsData.avatarUrl);
-  });
-
-  it('should fallback to a <svg> if the avatarUrl is not set', done => {
-    vm.avatarUrl = undefined;
-
-    Vue.nextTick(() => {
-      const avatarImg = vm.$el.querySelector('a svg');
-
-      expect(avatarImg).not.toBeNull();
-      done();
-    });
-  });
-
-  it('should correctly compute computed values', () => {
-    const correctVals = {
-      disabledClass: '',
-      avatarSizeClass: 's32',
-      avatarHtmlClass: 's32 avatar avatar-inline avatar-placeholder',
-      avatarClass: 'avatar avatar-inline s32 myextraavatarclass',
-      tooltipClass: 'has-tooltip',
-      linkClass: 'author-link has-tooltip myextralinkclass ',
-    };
-
-    Object.keys(correctVals).forEach(computedKey => {
-      const expectedVal = correctVals[computedKey];
-      const actualComputed = vm[computedKey];
-
-      expect(actualComputed).toBe(expectedVal);
-    });
-  });
-});