Use ResourceLabelEvent for tracking label changes

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

@@ -24,12 +24,13 @@ export default {
       required: true,
     },
     noteId: {
-      type: Number,
+      type: String,
       required: true,
     },
     noteUrl: {
       type: String,
-      required: true,
+      required: false,
+      default: '',
     },
     accessLevel: {
       type: String,
@@ -225,11 +226,11 @@ export default {
             Report as abuse
           </a>
         </li>
-        <li>
+        <li v-if="noteUrl">
           <button
             :data-clipboard-text="noteUrl"
             type="button"
-            css-class="btn-default btn-transparent"
+            class="btn-default btn-transparent js-btn-copy-note-link"
           >
             Copy link
           </button>

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

@@ -25,7 +25,7 @@ export default {
       required: true,
     },
     noteId: {
-      type: Number,
+      type: String,
       required: true,
     },
     canAwardEmoji: {

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

@@ -20,9 +20,9 @@ export default {
       default: '',
     },
     noteId: {
-      type: Number,
+      type: String,
       required: false,
-      default: 0,
+      default: '',
     },
     markdownVersion: {
       type: Number,
@@ -67,7 +67,10 @@ export default {
       'getUserDataByProp',
     ]),
     noteHash() {
-      return `#note_${this.noteId}`;
+      if (this.noteId) {
+        return `#note_${this.noteId}`;
+      }
+      return '#';
     },
     markdownPreviewPath() {
       return this.getNoteableDataByProp('preview_note_path');

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

@@ -9,7 +9,8 @@ export default {
   props: {
     author: {
       type: Object,
-      required: true,
+      required: false,
+      default: () => ({}),
     },
     createdAt: {
       type: String,
@@ -21,7 +22,7 @@ export default {
       default: '',
     },
     noteId: {
-      type: Number,
+      type: String,
       required: true,
     },
     includeToggle: {
@@ -72,7 +73,10 @@ export default {
         {{ __('Toggle discussion') }}
       </button>
     </div>
-    <a :href="author.path">
+    <a
+      v-if="Object.keys(author).length"
+      :href="author.path"
+    >
       <span class="note-header-author-name">{{ author.name }}</span>
       <span
         v-if="author.status_tooltip_html"
@@ -81,6 +85,9 @@ export default {
         @{{ author.username }}
       </span>
     </a>
+    <span v-else>
+      {{ __('A deleted user') }}
+    </span>
     <span class="note-headline-light">
       <span class="note-headline-meta">
         <template v-if="actionText">

app/controllers/concerns/issuable_actions.rb

@@ -95,6 +95,7 @@ module IssuableActions
       .includes(:noteable)
       .fresh
+    notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user).execute(notes)
     notes = prepare_notes_for_rendering(notes)
     notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }

app/controllers/concerns/notes_actions.rb

@@ -18,6 +18,7 @@ module NotesActions
     notes = notes_finder.execute
       .inc_relations_for_view
+    notes = ResourceEvents::MergeIntoNotesService.new(noteable, current_user, last_fetched_at: current_fetched_at).execute(notes)
     notes = prepare_notes_for_rendering(notes)
     notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }

app/helpers/notes_helper.rb

@@ -108,7 +108,7 @@ module NotesHelper
   end
   def noteable_note_url(note)
-    Gitlab::UrlBuilder.build(note)
+    Gitlab::UrlBuilder.build(note) if note.id
   end
   def form_resources

app/models/concerns/issuable.rb

@@ -109,10 +109,6 @@ module Issuable
       false
     end
-    def etag_caching_enabled?
-      false
-    end
-
     def has_multiple_assignees?
       assignees.count > 1
     end

app/models/concerns/noteable.rb

@@ -82,4 +82,23 @@ module Noteable
   def lockable?
     [MergeRequest, Issue].include?(self.class)
   end
+
+  def etag_caching_enabled?
+    false
+  end
+
+  def expire_note_etag_cache
+    return unless discussions_rendered_on_frontend?
+    return unless etag_caching_enabled?
+
+    Gitlab::EtagCaching::Store.new.touch(note_etag_key)
+  end
+
+  def note_etag_key
+    Gitlab::Routing.url_helpers.project_noteable_notes_path(
+      project,
+      target_type: self.class.name.underscore,
+      target_id: id
+    )
+  end
 end

app/models/label_note.rb

+# frozen_string_literal: true
+
+class LabelNote < Note
+  attr_accessor :resource_parent
+  attr_reader :events
+
+  def self.from_events(events, resource: nil, resource_parent: nil)
+    resource ||= events.first.issuable
+
+    attrs = {
+      system: true,
+      author: events.first.user,
+      created_at: events.first.created_at,
+      discussion_id: events.first.discussion_id,
+      noteable: resource,
+      system_note_metadata: SystemNoteMetadata.new(action: 'label'),
+      events: events,
+      resource_parent: resource_parent
+    }
+
+    if resource_parent.is_a?(Project)
+      attrs[:project_id] = resource_parent.id
+    end
+
+    LabelNote.new(attrs)
+  end
+
+  def events=(events)
+    @events = events
+
+    update_outdated_markdown
+  end
+
+  def cached_html_up_to_date?(markdown_field)
+    true
+  end
+
+  def note
+    @note ||= note_text
+  end
+
+  def note_html
+    @note_html ||= "<p dir=\"auto\">#{note_text(html: true)}</p>"
+  end
+
+  def project
+    resource_parent if resource_parent.is_a?(Project)
+  end
+
+  def group
+    resource_parent if resource_parent.is_a?(Group)
+  end
+
+  private
+
+  def update_outdated_markdown
+    events.each do |event|
+      if event.outdated_markdown?
+        event.refresh_invalid_reference
+      end
+    end
+  end
+
+  def note_text(html: false)
+    added = labels_str('added', label_refs_by_action('add', html))
+    removed = labels_str('removed', label_refs_by_action('remove', html))
+
+    [added, removed].compact.join(' and ')
+  end
+
+  # returns string containing added/removed labels including
+  # count of deleted labels:
+  #
+  # added ~1 ~2 + 1 deleted label
+  # added 3 deleted labels
+  # added ~1 ~2 labels
+  def labels_str(prefix, label_refs)
+    existing_refs = label_refs.select { |ref| ref.present? }.sort
+    refs_str = existing_refs.empty? ? nil : existing_refs.join(' ')
+
+    deleted = label_refs.count - existing_refs.count
+    deleted_str = deleted == 0 ? nil : "#{deleted} deleted"
+
+    return nil unless refs_str || deleted_str
+
+    label_list_str = [refs_str, deleted_str].compact.join(' + ')
+    suffix = 'label'.pluralize(deleted > 0 ? deleted : existing_refs.count)
+
+    "#{prefix} #{label_list_str} #{suffix}"
+  end
+
+  def label_refs_by_action(action, html)
+    field = html ? :reference_html : :reference
+
+    events.select { |e| e.action == action }.map(&field)
+  end
+end

app/models/note.rb

@@ -389,18 +389,7 @@ class Note < ActiveRecord::Base
   end
   def expire_etag_cache
-    return unless noteable&.discussions_rendered_on_frontend?
-    return unless noteable&.etag_caching_enabled?
-
-    Gitlab::EtagCaching::Store.new.touch(etag_key)
-  end
-
-  def etag_key
-    Gitlab::Routing.url_helpers.project_noteable_notes_path(
-      project,
-      target_type: noteable_type.underscore,
-      target_id: noteable_id
-    )
+    noteable&.expire_note_etag_cache
   end
   def touch(*args)

app/models/resource_label_event.rb

@@ -3,33 +3,122 @@
 # This model is not used yet, it will be used for:
 # https://gitlab.com/gitlab-org/gitlab-ce/issues/48483
 class ResourceLabelEvent < ActiveRecord::Base
+  include Importable
+  include Gitlab::Utils::StrongMemoize
+  include CacheMarkdownField
+
+  cache_markdown_field :reference
+
   belongs_to :user
   belongs_to :issue
   belongs_to :merge_request
   belongs_to :label
-  validates :user, presence: true, on: :create
-  validates :label, presence: true, on: :create
+  scope :created_after, ->(time) { where('created_at > ?', time) }
+
+  validates :user, presence: { unless: :importing? }, on: :create
+  validates :label, presence: { unless: :importing? }, on: :create
   validate :exactly_one_issuable
+  after_save :expire_etag_cache
+  after_destroy :expire_etag_cache
+
   enum action: {
     add: 1,
     remove: 2
   }
-  def self.issuable_columns
-    %i(issue_id merge_request_id).freeze
+  def self.issuable_attrs
+    %i(issue merge_request).freeze
   end
   def issuable
     issue || merge_request
   end
+  # create same discussion id for all actions with the same user and time
+  def discussion_id(resource = nil)
+    strong_memoize(:discussion_id) do
+      Digest::SHA1.hexdigest([self.class.name, created_at, user_id].join("-"))
+    end
+  end
+
+  def project
+    issuable.project
+  end
+
+  def group
+    issuable.group if issuable.respond_to?(:group)
+  end
+
+  def outdated_markdown?
+    return true if label_id.nil? && reference.present?
+
+    reference.nil? || latest_cached_markdown_version != cached_markdown_version
+  end
+
+  def banzai_render_context(field)
+    super.merge(pipeline: 'label', only_path: true)
+  end
+
+  def refresh_invalid_reference
+    # label_id could be nullified on label delete
+    self.reference = '' if label_id.nil?
+
+    # reference is not set for events which were not rendered yet
+    self.reference ||= label_reference
+
+    if changed?
+      save
+    elsif invalidated_markdown_cache?
+      refresh_markdown_cache!
+    end
+  end
+
   private
+  def label_reference
+    if local_label?
+      label.to_reference(format: :id)
+    elsif label.is_a?(GroupLabel)
+      label.to_reference(label.group, target_project: resource_parent, format: :id)
+    else
+      label.to_reference(resource_parent, format: :id)
+    end
+  end
+
   def exactly_one_issuable
-    if self.class.issuable_columns.count { |attr| self[attr] } != 1
-      errors.add(:base, "Exactly one of #{self.class.issuable_columns.join(', ')} is required")
+    issuable_count = self.class.issuable_attrs.count { |attr| self["#{attr}_id"] }
+
+    return true if issuable_count == 1
+
+    # if none of issuable IDs is set, check explicitly if nested issuable
+    # object is set, this is used during project import
+    if issuable_count == 0 && importing?
+      issuable_count = self.class.issuable_attrs.count { |attr| self.public_send(attr) } # rubocop:disable GitlabSecurity/PublicSend
+
+      return true if issuable_count == 1
     end
+
+    errors.add(:base, "Exactly one of #{self.class.issuable_attrs.join(', ')} is required")
+  end
+
+  def expire_etag_cache
+    issuable.expire_note_etag_cache
+  end
+
+  def local_label?
+    params = { include_ancestor_groups: true }
+    if resource_parent.is_a?(Project)
+      params[:project_id] = resource_parent.id
+    else
+      params[:group_id] = resource_parent.id
+    end
+
+    LabelsFinder.new(nil, params).execute(skip_authorization: true).where(id: label.id).any?
+  end
+
+  def resource_parent
+    issuable.project || issuable.group
   end
 end

app/serializers/note_entity.rb

@@ -4,6 +4,12 @@ class NoteEntity < API::Entities::Note
   include RequestAwareEntity
   include NotesHelper
+  expose :id do |note|
+    # resource events are represented as notes too, but don't
+    # have ID, discussion ID is used for them instead
+    note.id ? note.id.to_s : note.discussion_id
+  end
+
   expose :type
   expose :author, using: NoteUserEntity
@@ -46,8 +52,8 @@ class NoteEntity < API::Entities::Note
   expose :emoji_awardable?, as: :emoji_awardable
   expose :award_emoji, if: -> (note, _) { note.emoji_awardable? }, using: AwardEmojiEntity
-  expose :report_abuse_path do |note|
-    new_abuse_report_path(user_id: note.author.id, ref_url: Gitlab::UrlBuilder.build(note))
+  expose :report_abuse_path, if: -> (note, _) { note.author_id } do |note|
+    new_abuse_report_path(user_id: note.author_id, ref_url: Gitlab::UrlBuilder.build(note))
   end
   expose :noteable_note_url do |note|

app/serializers/project_note_entity.rb

 # frozen_string_literal: true
 class ProjectNoteEntity < NoteEntity
-  expose :human_access do |note|
+  expose :human_access, if: -> (note, _) { note.project.present? } do |note|
     note.project.team.human_max_access(note.author_id)
   end
@@ -9,7 +9,7 @@ class ProjectNoteEntity < NoteEntity
     toggle_award_emoji_project_note_path(note.project, note.id)
   end
-  expose :path do |note|
+  expose :path, if: -> (note, _) { note.id } do |note|
     project_note_path(note.project, note)
   end

app/services/issuable/common_system_notes_service.rb

@@ -55,7 +55,9 @@ module Issuable
       added_labels = issuable.labels - old_labels
       removed_labels = old_labels - issuable.labels
-      SystemNoteService.change_label(issuable, issuable.project, current_user, added_labels, removed_labels)
+      ResourceEvents::ChangeLabelsService
+        .new(issuable, current_user)
+        .execute(added_labels: added_labels, removed_labels: removed_labels)
     end
     def create_title_change_note(old_title)

app/services/issues/move_service.rb

@@ -36,6 +36,7 @@ module Issues
     def update_new_issue
       rewrite_notes
+      copy_resource_label_events
       rewrite_issue_award_emoji
       add_note_moved_from
     end
@@ -96,6 +97,18 @@ module Issues
       end
     end
+    def copy_resource_label_events
+      @old_issue.resource_label_events.find_in_batches do |batch|
+        events = batch.map do |event|
+          event.attributes
+            .except('id', 'reference', 'reference_html')
+            .merge('issue_id' => @new_issue.id, 'created_at' => event.created_at)
+        end
+
+        Gitlab::Database.bulk_insert(ResourceLabelEvent.table_name, events)
+      end
+    end
+
     def rewrite_issue_award_emoji
       rewrite_award_emoji(@old_issue, @new_issue)
     end

app/services/labels/promote_service.rb

@@ -13,6 +13,7 @@ module Labels
         label_ids_for_merge(new_label).find_in_batches(batch_size: BATCH_SIZE) do |batched_ids|
           update_issuables(new_label, batched_ids)
+          update_resource_label_events(new_label, batched_ids)
           update_issue_board_lists(new_label, batched_ids)
           update_priorities(new_label, batched_ids)
           subscribe_users(new_label, batched_ids)
@@ -52,6 +53,12 @@ module Labels
         .update_all(label_id: new_label)
     end
+    def update_resource_label_events(new_label, label_ids)
+      ResourceLabelEvent
+        .where(label: label_ids)
+        .update_all(label_id: new_label)
+    end
+
     def update_issue_board_lists(new_label, label_ids)
       List
         .where(label: label_ids)

app/services/resource_events/change_labels_service.rb

 # frozen_string_literal: true
-# This service is not used yet, it will be used for:
-# https://gitlab.com/gitlab-org/gitlab-ce/issues/48483
 module ResourceEvents
   class ChangeLabelsService
     attr_reader :resource, :user
@@ -25,6 +23,7 @@ module ResourceEvents
       end
       Gitlab::Database.bulk_insert(ResourceLabelEvent.table_name, labels)
+      resource.expire_note_etag_cache
     end
     private

app/services/resource_events/merge_into_notes_service.rb

+# frozen_string_literal: true
+
+# We store events about issuable label changes in a separate table (not as
+# other system notes), but we still want to display notes about label changes
+# as classic system notes in UI.  This service generates "synthetic" notes for
+# label event changes and merges them with classic notes and sorts them by
+# creation time.
+
+module ResourceEvents
+  class MergeIntoNotesService
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :resource, :current_user, :params
+
+    def initialize(resource, current_user, params = {})
+      @resource = resource
+      @current_user = current_user
+      @params = params
+    end
+
+    def execute(notes = [])
+      (notes + label_notes).sort_by { |n| n.created_at }
+    end
+
+    private
+
+    def label_notes
+      label_events_by_discussion_id.map do |discussion_id, events|
+        LabelNote.from_events(events, resource: resource, resource_parent: resource_parent)
+      end
+    end
+
+    def label_events_by_discussion_id
+      return [] unless resource.respond_to?(:resource_label_events)
+
+      events = resource.resource_label_events.includes(:label, :user)
+      events = since_fetch_at(events)
+
+      events.group_by { |event| event.discussion_id }
+    end
+
+    def since_fetch_at(events)
+      return events unless params[:last_fetched_at].present?
+
+      last_fetched_at = Time.at(params.fetch(:last_fetched_at).to_i)
+      events.created_after(last_fetched_at - NotesFinder::FETCH_OVERLAP)
+    end
+
+    def resource_parent
+      strong_memoize(:resource_parent) do
+        resource.project || resource.group
+      end
+    end
+  end
+end

app/services/system_note_service.rb

@@ -98,47 +98,6 @@ module SystemNoteService
     create_note(NoteSummary.new(issue, project, author, body, action: 'assignee'))
   end
-  # Called when one or more labels on a Noteable are added and/or removed
-  #
-  # noteable       - Noteable object
-  # project        - Project owning noteable
-  # author         - User performing the change
-  # added_labels   - Array of Labels added
-  # removed_labels - Array of Labels removed
-  #
-  # Example Note text:
-  #
-  #   "added ~1 and removed ~2 ~3 labels"
-  #
-  #   "added ~4 label"
-  #
-  #   "removed ~5 label"
-  #
-  # Returns the created Note object
-  def change_label(noteable, project, author, added_labels, removed_labels)
-    labels_count = added_labels.count + removed_labels.count
-
-    references     = ->(label) { label.to_reference(format: :id) }
-    added_labels   = added_labels.map(&references).join(' ')
-    removed_labels = removed_labels.map(&references).join(' ')
-
-    text_parts = []
-
-    if added_labels.present?
-      text_parts << "added #{added_labels}"
-      text_parts << 'and' if removed_labels.present?
-    end
-
-    if removed_labels.present?
-      text_parts << "removed #{removed_labels}"
-    end
-
-    text_parts << 'label'.pluralize(labels_count)
-    body = text_parts.join(' ')
-
-    create_note(NoteSummary.new(noteable, project, author, body, action: 'label'))
-  end
-
   # Called when the milestone of a Noteable is changed
   #
   # noteable  - Noteable object