Use ResourceLabelEvent for tracking label changes

When displaying notes for an issuable, label events are
squashed and converted into Note for presenting purposes only.

18 files changed, 164 insertions, 57 deletions

diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue
index cdbbb342331..b248aa3ae08 100644
--- a/app/assets/javascripts/notes/components/note_actions.vue
+++ b/app/assets/javascripts/notes/components/note_actions.vue
@@ -24,7 +24,7 @@ export default {
       required: true,
     },
     noteId: {
-      type: Number,
+      type: String,
       required: true,
     },
     noteUrl: {
diff --git a/app/assets/javascripts/notes/components/note_header.vue b/app/assets/javascripts/notes/components/note_header.vue
index a621418cf72..5d3f923a7b4 100644
--- a/app/assets/javascripts/notes/components/note_header.vue
+++ b/app/assets/javascripts/notes/components/note_header.vue
@@ -21,7 +21,7 @@ export default {
       default: '',
     },
     noteId: {
-      type: Number,
+      type: String,
       required: true,
     },
     includeToggle: {
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 37e03d70b6f..280d0152b14 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -95,6 +95,9 @@ module IssuableActions
       .includes(:noteable)
       .fresh
 
+    # FIXME: import/export
+    notes = ResourceEvents::MergeIntoNotesService.new(issuable).execute(notes)
+
     notes = prepare_notes_for_rendering(notes)
     notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
 
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index 5127db3f5fb..68f152cbfec 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -18,6 +18,9 @@ module NotesActions
     notes = notes_finder.execute
       .inc_relations_for_view
 
+    # FIXME: created_after
+    notes = ResourceEvents::MergeIntoNotesService.new(noteable, 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) }
 
diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb
index 42c255fcd1e..48f9ecc3bbf 100644
--- a/app/models/resource_label_event.rb
+++ b/app/models/resource_label_event.rb
@@ -8,10 +8,15 @@ class ResourceLabelEvent < ActiveRecord::Base
   belongs_to :merge_request
   belongs_to :label
 
+  scope :created_after, ->(time) { where('created_at > ?', time) }
+
   validates :user, presence: true, on: :create
   validates :label, presence: true, on: :create
   validate :exactly_one_issuable
 
+  after_save :expire_etag_cache
+  after_destroy :expire_etag_cache
+
   enum action: {
     add: 1,
     remove: 2
@@ -25,6 +30,16 @@ class ResourceLabelEvent < ActiveRecord::Base
     issue || merge_request
   end
 
+  # FIXME: check again alias_method
+  def updated_at
+    created_at
+  end
+
+  # create same discussion id for all actions with the same user and time
+  def discussion_id(resource = nil)
+    Digest::SHA1.hexdigest([self.class.name, created_at, user_id].join("-"))
+  end
+
   private
 
   def exactly_one_issuable
@@ -32,4 +47,20 @@ class ResourceLabelEvent < ActiveRecord::Base
       errors.add(:base, "Exactly one of #{self.class.issuable_columns.join(', ')} is required")
     end
   end
+
+  def expire_etag_cache
+    return unless issuable&.discussions_rendered_on_frontend?
+    return unless issuable&.etag_caching_enabled?
+
+    Gitlab::EtagCaching::Store.new.touch(etag_key)
+  end
+
+  # FIXME: override for epic
+  def etag_key
+    Gitlab::Routing.url_helpers.project_noteable_notes_path(
+      project,
+      target_type: 'issue',
+      target_id: issuable.id
+    )
+  end
 end
diff --git a/app/serializers/note_entity.rb b/app/serializers/note_entity.rb
index daa5c24d0f5..07ecc335d14 100644
--- a/app/serializers/note_entity.rb
+++ b/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.present? ? note.id.to_s : note.discussion_id
+  end
+
   expose :type
 
   expose :author, using: NoteUserEntity
diff --git a/app/serializers/project_note_entity.rb b/app/serializers/project_note_entity.rb
index d7c4d0aacc6..ce9353ddbe4 100644
--- a/app/serializers/project_note_entity.rb
+++ b/app/serializers/project_note_entity.rb
@@ -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
 
diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb
index 028b350ca07..ab53c38aa3a 100644
--- a/app/services/issuable/common_system_notes_service.rb
+++ b/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)
diff --git a/app/services/labels/promote_service.rb b/app/services/labels/promote_service.rb
index 623a5f0950e..22b4b30d902 100644
--- a/app/services/labels/promote_service.rb
+++ b/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,13 @@ module Labels
         .update_all(label_id: new_label)
     end
 
+    def update_resource_label_events(new_label, label_ids)
+      # FIXME: DB - review, batch is probably not needed though (same as issuables above)
+      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)
diff --git a/app/services/resource_events/change_labels_service.rb b/app/services/resource_events/change_labels_service.rb
index 8edb0ddb3ed..950cc8d7f46 100644
--- a/app/services/resource_events/change_labels_service.rb
+++ b/app/services/resource_events/change_labels_service.rb
@@ -1,7 +1,5 @@
 # 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
diff --git a/app/services/resource_events/merge_into_notes_service.rb b/app/services/resource_events/merge_into_notes_service.rb
new file mode 100644
index 00000000000..fe68d591e35
--- /dev/null
+++ b/app/services/resource_events/merge_into_notes_service.rb
@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+module ResourceEvents
+  class MergeIntoNotesService
+    FETCH_OVERLAP = 5.seconds
+
+    attr_reader :resource, :params
+
+    def initialize(resource, params = {})
+      @resource = resource
+      @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|
+        note_from_label_events(discussion_id, events)
+      end
+    end
+
+    def note_from_label_events(discussion_id, events)
+      text = change_label_text(events)
+      issuable = events.first.issuable
+      attrs = {
+        system: true,
+        author: events.first.user,
+        created_at: events.first.created_at,
+        discussion_id: discussion_id,
+        note: text,
+        noteable: issuable,
+        system_note_metadata: SystemNoteMetadata.new(action: 'label')
+      }
+
+      if issuable.respond_to?(:project_id)
+        attrs[:project_id] = issuable.project_id
+      end
+
+      Note.new(attrs)
+    end
+
+    def label_events_by_discussion_id
+      return [] unless resource.respond_to?(:resource_label_events)
+
+      events = resource.resource_label_events.includes(:label)
+      events = since_fetch_at(events)
+
+      events.group_by { |event| event.discussion_id }
+    end
+
+    # FIXME: take from system_note, refactor
+    def change_label_text(events)
+      added_labels = events.select { |e| e.action == 'add' && e.label }.map(&:label)
+      removed_labels = events.select { |e| e.action == 'remove' && e.label }.map(&:label)
+      added_unknown_labels_count = events.select { |e| e.action == 'add' && e.label.nil? }.map(&:label).count
+      removed_unknown_labels_count = events.select { |e| e.action == 'remove' && e.label.nil? }.map(&:label).count
+      labels_count = added_labels.count + removed_labels.count + added_unknown_labels_count + removed_unknown_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 << " + #{added_unknown_labels_count} deleted" if added_unknown_labels_count > 0
+        text_parts << 'and' if removed_labels.present?
+      end
+
+      if removed_labels.present?
+        text_parts << "removed #{removed_labels}"
+        text_parts << " + #{removed_unknown_labels_count} deleted" if removed_unknown_labels_count > 0
+      end
+
+      text_parts << 'label'.pluralize(labels_count)
+      text_parts.join(' ')
+    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 - FETCH_OVERLAP)
+    end
+  end
+end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index dda89830179..3ea81445798 100644
--- a/app/services/system_note_service.rb
+++ b/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
diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb
index 13c34e3473a..53b89f585a8 100644
--- a/lib/api/discussions.rb
+++ b/lib/api/discussions.rb
@@ -31,6 +31,8 @@ module API
             .includes(:noteable)
             .fresh
 
+          notes = ResourceEvents::MergeIntoNotesService.new(noteable).execute(notes)
+
           notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
           discussions = Kaminari.paginate_array(Discussion.build_collection(notes, noteable))
 
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 39923e6d5b5..940aa59e96d 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -37,13 +37,13 @@ module API
           # page can have less elements than :per_page even if
           # there's more than one page.
           raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
-          notes =
-            # paginate() only works with a relation. This could lead to a
-            # mismatch between the pagination headers info and the actual notes
-            # array returned, but this is really a edge-case.
-            paginate(raw_notes)
-            .reject { |n| n.cross_reference_not_visible_for?(current_user) }
-          present notes, with: Entities::Note
+
+          raw_notes = ResourceEvents::MergeIntoNotesService.new(noteable).execute(raw_notes)
+          raw_notes = raw_notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+
+          notes = Kaminari.paginate_array(raw_notes)
+
+          present paginate(notes), with: Entities::Note
         end
 
         desc "Get a single #{noteable_type.to_s.downcase} note" do
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index f69f98a78a3..9b6a3dadfad 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -19,6 +19,10 @@ project_tree:
     - milestone:
       - events:
         - :push_event_payload
+    - resource_label_events:
+      - :user
+      - label:
+          :priorities
     - :issue_assignees
   - snippets:
     - :award_emoji
diff --git a/spec/javascripts/notes/components/note_actions_spec.js b/spec/javascripts/notes/components/note_actions_spec.js
index 52cc42cb53d..55d30d69f31 100644
--- a/spec/javascripts/notes/components/note_actions_spec.js
+++ b/spec/javascripts/notes/components/note_actions_spec.js
@@ -28,7 +28,7 @@ describe('issue_note_actions component', () => {
         canEdit: true,
         canAwardEmoji: true,
         canReportAsAbuse: true,
-        noteId: 539,
+        noteId: '539',
         noteUrl: 'https://localhost:3000/group/project/merge_requests/1#note_1',
         reportAbusePath:
           '/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26',
diff --git a/spec/javascripts/notes/components/note_awards_list_spec.js b/spec/javascripts/notes/components/note_awards_list_spec.js
index 9d98ba219da..082f98f22f5 100644
--- a/spec/javascripts/notes/components/note_awards_list_spec.js
+++ b/spec/javascripts/notes/components/note_awards_list_spec.js
@@ -30,7 +30,7 @@ describe('note_awards_list component', () => {
       propsData: {
         awards: awardsMock,
         noteAuthorId: 2,
-        noteId: 545,
+        noteId: '545',
         canAwardEmoji: true,
         toggleAwardPath: '/gitlab-org/gitlab-ce/notes/545/toggle_award_emoji',
       },
diff --git a/spec/javascripts/notes/components/note_form_spec.js b/spec/javascripts/notes/components/note_form_spec.js
index 95d400ab3df..a938744ab46 100644
--- a/spec/javascripts/notes/components/note_form_spec.js
+++ b/spec/javascripts/notes/components/note_form_spec.js
@@ -19,7 +19,7 @@ describe('issue_note_form component', () => {
     props = {
       isEditing: false,
       noteBody: 'Magni suscipit eius consectetur enim et ex et commodi.',
-      noteId: 545,
+      noteId: '545',
     };
 
     vm = new Component({