Merge branch 'mycroft-note-badge' into 'master'

Display Contributor and Author badges on notes

Closes #16534

See merge request gitlab-org/gitlab!40198

app/assets/javascripts/notes/components/note_actions.vue

 <script>
 import { mapGetters } from 'vuex';
 import { GlLoadingIcon, GlTooltipDirective, GlIcon } from '@gitlab/ui';
-import { __ } from '~/locale';
+import { __, sprintf } from '~/locale';
 import resolvedStatusMixin from '~/batch_comments/mixins/resolved_status';
 import ReplyButton from './note_actions/reply_button.vue';
 import eventHub from '~/sidebar/event_hub';
 import Api from '~/api';
 import { deprecatedCreateFlash as flash } from '~/flash';
+import { splitCamelCase } from '../../lib/utils/text_utility';
 
 export default {
   name: 'NoteActions',
@@ -47,6 +48,26 @@ export default {
       required: false,
       default: null,
     },
+    isAuthor: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    isContributor: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    noteableType: {
+      type: String,
+      required: false,
+      default: '',
+    },
+    projectName: {
+      type: String,
+      required: false,
+      default: '',
+    },
     showReply: {
       type: Boolean,
       required: true,
@@ -121,6 +142,9 @@ export default {
     targetType() {
       return this.getNoteableData.targetType;
     },
+    noteableDisplayName() {
+      return splitCamelCase(this.noteableType).toLowerCase();
+    },
     assignees() {
       return this.getNoteableData.assignees || [];
     },
@@ -130,6 +154,22 @@ export default {
     canAssign() {
       return this.getNoteableData.current_user?.can_update && this.isIssue;
     },
+    displayAuthorBadgeText() {
+      return sprintf(__('This user is the author of this %{noteable}.'), {
+        noteable: this.noteableDisplayName,
+      });
+    },
+    displayMemberBadgeText() {
+      return sprintf(__('This user is a %{access} of the %{name} project.'), {
+        access: this.accessLevel.toLowerCase(),
+        name: this.projectName,
+      });
+    },
+    displayContributorBadgeText() {
+      return sprintf(__('This user has previously committed to the %{name} project.'), {
+        name: this.projectName,
+      });
+    },
   },
   methods: {
     onEdit() {
@@ -175,7 +215,24 @@ export default {
 
 <template>
   <div class="note-actions">
-    <span v-if="accessLevel" class="note-role user-access-role">{{ accessLevel }}</span>
+    <span
+      v-if="isAuthor"
+      class="note-role user-access-role has-tooltip d-none d-md-inline-block"
+      :title="displayAuthorBadgeText"
+      >{{ __('Author') }}</span
+    >
+    <span
+      v-if="accessLevel"
+      class="note-role user-access-role has-tooltip"
+      :title="displayMemberBadgeText"
+      >{{ accessLevel }}</span
+    >
+    <span
+      v-else-if="isContributor"
+      class="note-role user-access-role has-tooltip"
+      :title="displayContributorBadgeText"
+      >{{ __('Contributor') }}</span
+    >
     <div v-if="canResolve" class="note-actions-item">
       <button
         ref="resolveButton"

app/assets/javascripts/notes/components/noteable_note.vue

@@ -389,6 +389,10 @@ export default {
           :note-id="note.id"
           :note-url="note.noteable_note_url"
           :access-level="note.human_access"
+          :is-contributor="note.is_contributor"
+          :is-author="note.is_noteable_author"
+          :project-name="note.project_name"
+          :noteable-type="note.noteable_type"
           :show-reply="showReplyButton"
           :can-edit="note.current_user.can_edit"
           :can-award-emoji="note.current_user.can_award_emoji"

app/controllers/concerns/renders_notes.rb

@@ -5,7 +5,6 @@ module RendersNotes
   def prepare_notes_for_rendering(notes, noteable = nil)
     preload_noteable_for_regular_notes(notes)
     preload_max_access_for_authors(notes, @project)
-    preload_first_time_contribution_for_authors(noteable, notes)
     preload_author_status(notes)
     Notes::RenderService.new(current_user).execute(notes)
 
@@ -19,7 +18,8 @@ module RendersNotes
     return unless project
 
     user_ids = notes.map(&:author_id)
-    project.team.max_member_access_for_user_ids(user_ids)
+    access = project.team.max_member_access_for_user_ids(user_ids).select { |k, v| v == Gitlab::Access::NO_ACCESS }.keys
+    project.team.contribution_check_for_user_ids(access)
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
@@ -28,12 +28,6 @@ module RendersNotes
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def preload_first_time_contribution_for_authors(noteable, notes)
-    return unless noteable.is_a?(Issuable) && noteable.first_contribution?
-
-    notes.each {|n| n.specialize_for_first_contribution!(noteable)}
-  end
-
   # rubocop: disable CodeReuse/ActiveRecord
   def preload_author_status(notes)
     ActiveRecord::Associations::Preloader.new.preload(notes, { author: :status })

app/helpers/issuables_helper.rb

@@ -205,6 +205,12 @@ module IssuablesHelper
       author_output
     end
 
+    if access = project.team.human_max_access(issuable.author_id)
+      output << content_tag(:span, access, class: "user-access-role has-tooltip d-none d-xl-inline-block gl-ml-3 ", title: _("This user is a %{access} of the %{name} project.") % { access: access.downcase, name: project.name })
+    elsif project.team.contributor?(issuable.author_id)
+      output << content_tag(:span, _("Contributor"), class: "user-access-role has-tooltip d-none d-xl-inline-block gl-ml-3", title: _("This user has previously committed to the %{name} project.") % { name: project.name })
+    end
+
     output << content_tag(:span, (sprite_icon('first-contribution', css_class: 'gl-icon gl-vertical-align-middle') if issuable.first_contribution?), class: 'has-tooltip gl-ml-2', title: _('1st contribution!'))
 
     output << content_tag(:span, (issuable.task_status if issuable.tasks?), id: "task_status", class: "d-none d-sm-none d-md-inline-block gl-ml-3")

app/helpers/notes_helper.rb

@@ -85,6 +85,10 @@ module NotesHelper
     note.project.team.max_member_access(note.author_id)
   end
 
+  def note_human_max_access(note)
+    note.project.team.human_max_access(note.author_id)
+  end
+
   def discussion_path(discussion)
     if discussion.for_merge_request?
       return unless discussion.diff_discussion?

app/models/merge_request.rb

@@ -1603,7 +1603,7 @@ class MergeRequest < ApplicationRecord
   def first_contribution?
     return false if project.team.max_member_access(author_id) > Gitlab::Access::GUEST
 
-    project.merge_requests.merged.where(author_id: author_id).empty?
+    !project.merge_requests.merged.exists?(author_id: author_id)
   end
 
   # TODO: remove once production database rename completes

app/models/note.rb

@@ -20,20 +20,6 @@ class Note < ApplicationRecord
   include ThrottledTouch
   include FromUnion
 
-  module SpecialRole
-    FIRST_TIME_CONTRIBUTOR = :first_time_contributor
-
-    class << self
-      def values
-        constants.map {|const| self.const_get(const, false)}
-      end
-
-      def value?(val)
-        values.include?(val)
-      end
-    end
-  end
-
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
 
   redact_field :note
@@ -60,9 +46,6 @@ class Note < ApplicationRecord
   # Attribute used to store the attributes that have been changed by quick actions.
   attr_accessor :commands_changes
 
-  # A special role that may be displayed on issuable's discussions
-  attr_reader :special_role
-
   default_value_for :system, false
 
   attr_mentionable :note, pipeline: :note
@@ -220,10 +203,6 @@ class Note < ApplicationRecord
         .where(noteable_type: type, noteable_id: ids)
     end
 
-    def has_special_role?(role, note)
-      note.special_role == role
-    end
-
     def search(query)
       fuzzy_search(query, [:note])
     end
@@ -342,20 +321,20 @@ class Note < ApplicationRecord
     noteable.author_id == user.id
   end
 
-  def special_role=(role)
-    raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.value?(role)
+  def contributor?
+    return false unless ::Feature.enabled?(:show_contributor_on_note, project)
 
-    @special_role = role
+    project&.team&.contributor?(self.author_id)
   end
 
-  def has_special_role?(role)
-    self.class.has_special_role?(role, self)
-  end
+  def noteable_author?(noteable)
+    return false unless ::Feature.enabled?(:show_author_on_note, project)
 
-  def specialize_for_first_contribution!(noteable)
-    return unless noteable.author_id == self.author_id
+    noteable.author == self.author
+  end
 
-    self.special_role = Note::SpecialRole::FIRST_TIME_CONTRIBUTOR
+  def project_name
+    project&.name
   end
 
   def confidential?(include_noteable: false)

app/models/project_team.rb

@@ -178,6 +178,40 @@ class ProjectTeam
     max_member_access_for_user_ids([user_id])[user_id]
   end
 
+  def contribution_check_for_user_ids(user_ids)
+    user_ids = user_ids.uniq
+    key = "contribution_check_for_users:#{project.id}"
+
+    Gitlab::SafeRequestStore[key] ||= {}
+    contributors = Gitlab::SafeRequestStore[key] || {}
+
+    user_ids -= contributors.keys
+
+    return contributors if user_ids.empty?
+
+    resource_contributors = project.merge_requests
+                                   .merged
+                                   .where(author_id: user_ids, target_branch: project.default_branch.to_s)
+                                   .pluck(:author_id)
+                                   .product([true]).to_h
+
+    contributors.merge!(resource_contributors)
+
+    missing_resource_ids = user_ids - resource_contributors.keys
+
+    missing_resource_ids.each do |resource_id|
+      contributors[resource_id] = false
+    end
+
+    contributors
+  end
+
+  def contributor?(user_id)
+    return false if max_member_access(user_id) >= Gitlab::Access::GUEST
+
+    contribution_check_for_user_ids([user_id])[user_id]
+  end
+
   private
 
   def fetch_members(level = nil)

app/serializers/note_entity.rb

@@ -46,6 +46,10 @@ class NoteEntity < API::Entities::Note
     SystemNoteHelper.system_note_icon_name(note)
   end
 
+  expose :is_noteable_author do |note|
+    note.noteable_author?(request.noteable)
+  end
+
   expose :discussion_id do |note|
     note.discussion_id(request.noteable)
   end

app/serializers/project_note_entity.rb

@@ -5,6 +5,14 @@ class ProjectNoteEntity < NoteEntity
     note.project.team.human_max_access(note.author_id)
   end
 
+  expose :is_contributor, if: -> (note, _) { note.project.present? } do |note|
+    note.contributor?
+  end
+
+  expose :project_name, if: -> (note, _) { note.project.present? } do |note|
+    note.project.name
+  end
+
   expose :toggle_award_path, if: -> (note, _) { note.emoji_awardable? } do |note|
     toggle_award_emoji_project_note_path(note.project, note.id)
   end

app/views/projects/notes/_actions.html.haml

-- access = note_max_access_for_user(note)
-- if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR)
-  %span.note-role.note-role-special.has-tooltip{ title: _("This is the author's first Merge Request to this project.") }
-    = sprite_icon('first-contribution', css_class: 'gl-icon gl-vertical-align-top')
-- if access.nonzero?
-  %span.note-role.user-access-role= Gitlab::Access.human_access(access)
+- access = note_human_max_access(note)
+- if note.noteable_author?(@noteable)
+  %span{ class: 'note-role user-access-role has-tooltip d-none d-md-inline-block', title: _("This user is the author of this %{noteable}.") % { noteable: @noteable.human_class_name } }= _("Author")
+- if access
+  %span{ class: 'note-role user-access-role has-tooltip', title: _("This user is a %{access} of the %{name} project.") % { access: access.downcase, name: note.project_name } }= access
+- elsif note.contributor?
+  %span{ class: 'note-role user-access-role has-tooltip', title: _("This user has previously committed to the %{name} project.") % { name: note.project_name } }= _("Contributor")
 
 - if note.resolvable?
   - can_resolve = can?(current_user, :resolve_note, note)

changelogs/unreleased/display-contributor-and-author-badges-on-notes.yml

+---
+title: Display Contributor and Author badges on notes
+merge_request: 40198
+author: Mycroft Kang @TaehyeokKang
+type: added

config/feature_flags/development/show_author_on_note.yml

+---
+name: show_author_on_note
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40198
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/250282
+group: group::project management
+type: development
+default_enabled: false
\ No newline at end of file

config/feature_flags/development/show_contributor_on_note.yml

+---
+name: show_contributor_on_note
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40198
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/249179
+group: group::project management
+type: development
+default_enabled: false
\ No newline at end of file

locale/gitlab.pot

@@ -7005,6 +7005,9 @@ msgstr ""
 msgid "Contributions per group member"
 msgstr ""
 
+msgid "Contributor"
+msgstr ""
+
 msgid "Contributors"
 msgstr ""
 
@@ -25843,9 +25846,6 @@ msgstr ""
 msgid "This is a security log of important events involving your account."
 msgstr ""
 
-msgid "This is the author's first Merge Request to this project."
-msgstr ""
-
 msgid "This is the highest peak of users on your installation since the license started."
 msgstr ""
 
@@ -26083,6 +26083,15 @@ msgstr ""
 msgid "This user has no identities"
 msgstr ""
 
+msgid "This user has previously committed to the %{name} project."
+msgstr ""
+
+msgid "This user is a %{access} of the %{name} project."
+msgstr ""
+
+msgid "This user is the author of this %{noteable}."
+msgstr ""
+
 msgid "This user will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches."
 msgstr ""
 

spec/fixtures/api/schemas/entities/discussion.json

@@ -60,6 +60,9 @@
           "resolve_with_issue_path": { "type": "string" },
           "cached_markdown_version": { "type": "integer" },
           "human_access": { "type": ["string", "null"] },
+          "is_noteable_author": { "type": "boolean" },
+          "is_contributor": { "type": "boolean" },
+          "project_name": { "type": "string" },
           "toggle_award_path": { "type": "string" },
           "path": { "type": "string" },
           "commands_changes": { "type": "object", "additionalProperties": true },

spec/frontend/notes/components/note_actions_spec.js

@@ -35,8 +35,12 @@ describe('noteActions', () => {
       canEdit: true,
       canAwardEmoji: true,
       canReportAsAbuse: true,
+      isAuthor: true,
+      isContributor: false,
+      noteableType: 'MergeRequest',
       noteId: '539',
       noteUrl: `${TEST_HOST}/group/project/-/merge_requests/1#note_1`,
+      projectName: 'project',
       reportAbusePath: `${TEST_HOST}/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26`,
       showReply: false,
     };
@@ -60,15 +64,43 @@ describe('noteActions', () => {
       wrapper = shallowMountNoteActions(props);
     });
 
+    it('should render noteable author badge', () => {
+      expect(
+        wrapper
+          .findAll('.note-role')
+          .at(0)
+          .text()
+          .trim(),
+      ).toEqual('Author');
+    });
+
     it('should render access level badge', () => {
       expect(
         wrapper
-          .find('.note-role')
+          .findAll('.note-role')
+          .at(1)
           .text()
           .trim(),
       ).toEqual(props.accessLevel);
     });
 
+    it('should render contributor badge', () => {
+      wrapper.setProps({
+        accessLevel: null,
+        isContributor: true,
+      });
+
+      return wrapper.vm.$nextTick().then(() => {
+        expect(
+          wrapper
+            .findAll('.note-role')
+            .at(1)
+            .text()
+            .trim(),
+        ).toBe('Contributor');
+      });
+    });
+
     it('should render emoji link', () => {
       expect(wrapper.find('.js-add-award').exists()).toBe(true);
       expect(wrapper.find('.js-add-award').attributes('data-position')).toBe('right');

spec/models/note_spec.rb

@@ -286,6 +286,56 @@ RSpec.describe Note do
     end
   end
 
+  describe "noteable_author?" do
+    let(:user1) { create(:user) }
+    let(:user2) { create(:user) }
+    let(:project) { create(:project, :public, :repository) }
+
+    context 'when note is on commit' do
+      let(:noteable) { create(:commit, project: project, author: user1) }
+
+      context 'if user is the noteable author' do
+        let(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user1) }
+        let(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user1) }
+
+        it 'returns true' do
+          expect(note.noteable_author?(noteable)).to be true
+          expect(diff_note.noteable_author?(noteable)).to be true
+        end
+      end
+
+      context 'if user is not the noteable author' do
+        let(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user2) }
+        let(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user2) }
+
+        it 'returns false' do
+          expect(note.noteable_author?(noteable)).to be false
+          expect(diff_note.noteable_author?(noteable)).to be false
+        end
+      end
+    end
+
+    context 'when note is on issue' do
+      let(:noteable) { create(:issue, project: project, author: user1) }
+
+      context 'if user is the noteable author' do
+        let(:note) { create(:note, noteable: noteable, author: user1, project: project) }
+
+        it 'returns true' do
+          expect(note.noteable_author?(noteable)).to be true
+        end
+      end
+
+      context 'if user is not the noteable author' do
+        let(:note) { create(:note, noteable: noteable, author: user2, project: project) }
+
+        it 'returns false' do
+          expect(note.noteable_author?(noteable)).to be false
+        end
+      end
+    end
+  end
+
   describe "edited?" do
     let(:note) { build(:note, updated_by_id: nil, created_at: Time.current, updated_at: Time.current + 5.hours) }
 
@@ -1228,22 +1278,6 @@ RSpec.describe Note do
     end
   end
 
-  describe '#special_role=' do
-    let(:role) { Note::SpecialRole::FIRST_TIME_CONTRIBUTOR }
-
-    it 'assigns role' do
-      subject.special_role = role
-
-      expect(subject.special_role).to eq(role)
-    end
-
-    it 'does not assign unknown role' do
-      expect { subject.special_role = :bogus }.to raise_error(/Role is undefined/)
-
-      expect(subject.special_role).to be_nil
-    end
-  end
-
   describe '#parent' do
     it 'returns project for project notes' do
       project = create(:project)

spec/models/project_team_spec.rb

@@ -3,6 +3,8 @@
 require "spec_helper"
 
 RSpec.describe ProjectTeam do
+  include ProjectForksHelper
+
   let(:maintainer) { create(:user) }
   let(:reporter) { create(:user) }
   let(:guest) { create(:user) }
@@ -237,6 +239,35 @@ RSpec.describe ProjectTeam do
     end
   end
 
+  describe '#contributor?' do
+    let(:project) { create(:project, :public, :repository) }
+
+    context 'when user is a member of project' do
+      before do
+        project.add_maintainer(maintainer)
+        project.add_reporter(reporter)
+        project.add_guest(guest)
+      end
+
+      it { expect(project.team.contributor?(maintainer.id)).to be false }
+      it { expect(project.team.contributor?(reporter.id)).to be false }
+      it { expect(project.team.contributor?(guest.id)).to be false }
+    end
+
+    context 'when user has at least one merge request merged into default_branch' do
+      let(:contributor) { create(:user) }
+      let(:user_without_access) { create(:user) }
+      let(:first_fork_project) { fork_project(project, contributor, repository: true) }
+
+      before do
+        create(:merge_request, :merged, author: contributor, target_project: project, source_project: first_fork_project, target_branch: project.default_branch.to_s)
+      end
+
+      it { expect(project.team.contributor?(contributor.id)).to be true }
+      it { expect(project.team.contributor?(user_without_access.id)).to be false }
+    end
+  end
+
   describe '#max_member_access' do
     let(:requester) { create(:user) }
 
@@ -366,6 +397,66 @@ RSpec.describe ProjectTeam do
     end
   end
 
+  describe '#contribution_check_for_user_ids', :request_store do
+    let(:project) { create(:project, :public, :repository) }
+    let(:contributor) { create(:user) }
+    let(:second_contributor) { create(:user) }
+    let(:user_without_access) { create(:user) }
+    let(:first_fork_project) { fork_project(project, contributor, repository: true) }
+    let(:second_fork_project) { fork_project(project, second_contributor, repository: true) }
+
+    let(:users) do
+      [contributor, second_contributor, user_without_access].map(&:id)
+    end
+
+    let(:expected) do
+      {
+        contributor.id => true,
+        second_contributor.id => true,
+        user_without_access.id => false
+      }
+    end
+
+    before do
+      create(:merge_request, :merged, author: contributor, target_project: project, source_project: first_fork_project, target_branch: project.default_branch.to_s)
+      create(:merge_request, :merged, author: second_contributor, target_project: project, source_project: second_fork_project, target_branch: project.default_branch.to_s)
+    end
+
+    def contributors(users)
+      project.team.contribution_check_for_user_ids(users)
+    end
+
+    it 'does not perform extra queries when asked for users who have already been found' do
+      contributors(users)
+
+      expect { contributors([contributor.id]) }.not_to exceed_query_limit(0)
+
+      expect(contributors([contributor.id])).to eq(expected)
+    end
+
+    it 'only requests the extra users when uncached users are passed' do
+      new_contributor = create(:user)
+      new_fork_project = fork_project(project, new_contributor, repository: true)
+      second_new_user = create(:user)
+      all_users = users + [new_contributor.id, second_new_user.id]
+      create(:merge_request, :merged, author: new_contributor, target_project: project, source_project: new_fork_project, target_branch: project.default_branch.to_s)
+
+      expected_all = expected.merge(new_contributor.id => true,
+                                    second_new_user.id => false)
+
+      contributors(users)
+
+      queries = ActiveRecord::QueryRecorder.new { contributors(all_users) }
+
+      expect(queries.count).to eq(1)
+      expect(contributors([new_contributor.id])).to eq(expected_all)
+    end
+
+    it 'returns correct contributors' do
+      expect(contributors(users)).to eq(expected)
+    end
+  end
+
   shared_examples 'max member access for users' do
     let(:project) { create(:project) }
     let(:group) { create(:group) }
@@ -438,9 +529,9 @@ RSpec.describe ProjectTeam do
       it 'does not perform extra queries when asked for users who have already been found' do
         access_levels(users)
 
-        expect { access_levels(users) }.not_to exceed_query_limit(0)
+        expect { access_levels([maintainer.id]) }.not_to exceed_query_limit(0)
 
-        expect(access_levels(users)).to eq(expected)
+        expect(access_levels([maintainer.id])).to eq(expected)
       end
 
       it 'only requests the extra users when uncached users are passed' do