Generate weight change notes as synthetic notes

We are adding weich change tracking events similarly to label events
and in order to keep consistent and not duplicate system notes
we are generating the notes out of tracked weight change events.

app/models/resource_weight_event.rb

@@ -8,4 +8,17 @@ class ResourceWeightEvent < ApplicationRecord
   belongs_to :issue
 
   scope :by_issue, ->(issue) { where(issue_id: issue.id) }
+  scope :created_after, ->(time) { where('created_at > ?', time) }
+
+  def discussion_id(resource = nil)
+    strong_memoize(:discussion_id) do
+      Digest::SHA1.hexdigest(discussion_id_key.join("-"))
+    end
+  end
+
+  private
+
+  def discussion_id_key
+    [self.class.name, created_at, user_id]
+  end
 end

app/models/weight_note.rb

+# frozen_string_literal: true
+
+class WeightNote < Note
+  attr_accessor :resource_parent, :event
+
+  def self.from_event(event, resource: nil, resource_parent: nil)
+    resource ||= event.issue
+
+    attrs = {
+      system: true,
+      author: event.user,
+      created_at: event.created_at,
+      noteable: resource,
+      event: event,
+      system_note_metadata: SystemNoteMetadata.new(action: 'weight'),
+      resource_parent: resource_parent
+    }
+
+    if resource_parent.is_a?(Project)
+      attrs[:project_id] = resource_parent.id
+    end
+
+    WeightNote.new(attrs)
+  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 note_text(html: false)
+    weight_text = html ? "<strong>#{event.weight}</strong>" : event.weight
+    event.weight ? "changed weight to #{weight_text}" : 'removed the weight'
+  end
+end

app/services/resource_events/base_synthetic_notes_builder_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.
+
+module ResourceEvents
+  class BaseSyntheticNotesBuilderService
+    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
+      synthetic_notes
+    end
+
+    private
+
+    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/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.
+# We store events about issuable label changes and weight changes in separate tables (not as
+# other system notes), but we still want to display notes about label and weight changes
+# as classic system notes in UI.  This service merges synthetic label and weight notes
+# with classic notes and sorts them by creation time.
 
 module ResourceEvents
   class MergeIntoNotesService
@@ -19,39 +18,15 @@ module ResourceEvents
     end
 
     def execute(notes = [])
-      (notes + label_notes).sort_by { |n| n.created_at }
+      (notes + synthetic_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
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def label_events_by_discussion_id
-      return [] unless resource.respond_to?(:resource_label_events)
-
-      events = resource.resource_label_events.includes(:label, user: :status)
-      events = since_fetch_at(events)
-
-      events.group_by { |event| event.discussion_id }
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
-    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
+    def synthetic_notes
+      SyntheticLabelNotesBuilderService.new(resource, current_user, params).execute
     end
   end
 end
+
+ResourceEvents::MergeIntoNotesService.prepend_if_ee('EE::ResourceEvents::MergeIntoNotesService')

app/services/resource_events/synthetic_label_notes_builder_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.
+
+module ResourceEvents
+  class SyntheticLabelNotesBuilderService < BaseSyntheticNotesBuilderService
+    private
+
+    def synthetic_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: :status) # rubocop: disable CodeReuse/ActiveRecord
+      events = since_fetch_at(events)
+
+      events.group_by { |event| event.discussion_id }
+    end
+  end
+end

db/migrate/20191210211253_create_resource_weight_event.rb

@@ -5,7 +5,7 @@ class CreateResourceWeightEvent < ActiveRecord::Migration[5.2]
 
   def change
     create_table :resource_weight_events do |t|
-      t.references :user, null: false, foreign_key: { on_delete: :cascade },
+      t.references :user, null: false, foreign_key: { on_delete: :nullify },
                    index: { name: 'index_resource_weight_events_on_user_id' }
       t.references :issue, null: false, foreign_key: { on_delete: :cascade },
                    index: false

ee/app/services/ee/issuable/common_system_notes_service.rb

@@ -11,7 +11,7 @@ module EE
         super
 
         ActiveRecord::Base.no_touching do
-          handle_weight_change_note
+          handle_weight_change
           handle_date_change_note if is_update
         end
       end
@@ -28,26 +28,15 @@ module EE
         end
       end
 
-      def handle_weight_change_note
-        if !issuable.is_a?(Epic) && issuable.previous_changes.include?('weight')
-          note = create_weight_change_note
-
-          track_weight_change(note)
-        end
-      end
-
-      def create_weight_change_note
-        ::SystemNoteService.change_weight_note(issuable, issuable.project, current_user)
-      end
-
-      def track_weight_change(note)
+      def handle_weight_change
         return unless weight_changes_tracking_enabled?
+        return unless issuable.previous_changes.include?('weight')
 
-        EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, note.created_at).execute
+        EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.now).execute
       end
 
       def weight_changes_tracking_enabled?
-        ::Feature.enabled?(:track_issue_weight_change_events, issuable.project)
+        !issuable.is_a?(Epic) && ::Feature.enabled?(:track_issue_weight_change_events, issuable.project)
       end
     end
   end

ee/app/services/ee/resource_events/change_weight_service.rb

@@ -12,7 +12,10 @@ module EE
       end
 
       def execute
-        ::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes) unless resource_weight_changes.empty?
+        unless resource_weight_changes.empty?
+          ::Gitlab::Database.bulk_insert(ResourceWeightEvent.table_name, resource_weight_changes)
+          resources.each(&:expire_note_etag_cache)
+        end
       end
 
       private

ee/app/services/ee/resource_events/merge_into_notes_service.rb

+# frozen_string_literal: true
+
+module EE
+  module ResourceEvents
+    module MergeIntoNotesService
+      extend ActiveSupport::Concern
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :synthetic_notes
+      def synthetic_notes
+        super + SyntheticWeightNotesBuilderService.new(resource, current_user, params).execute
+      end
+    end
+  end
+end

ee/app/services/ee/resource_events/synthetic_weight_notes_builder_service.rb

+# frozen_string_literal: true
+
+# We store events about issue weight changes in a separate table,
+# but we still want to display notes about weight changes
+# as classic system notes in UI. This service generates "synthetic" notes for
+# weight event changes.
+
+module EE
+  module ResourceEvents
+    class SyntheticWeightNotesBuilderService < ::ResourceEvents::BaseSyntheticNotesBuilderService
+      private
+
+      def synthetic_notes
+        weight_change_events.map do |event|
+          WeightNote.from_event(event, resource: resource, resource_parent: resource_parent)
+        end
+      end
+
+      def weight_change_events
+        return [] unless resource.respond_to?(:resource_weight_events)
+
+        events = resource.resource_weight_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
+        since_fetch_at(events)
+      end
+    end
+  end
+end

ee/spec/requests/api/issues_spec.rb

@@ -348,7 +348,7 @@ describe API::Issues, :mailer do
     it 'adds a note when the weight is changed' do
       expect do
         put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
-      end.to change { Note.count }.by(1)
+      end.to change { ResourceWeightEvent.count }.by(1)
 
       expect(response).to have_gitlab_http_status(200)
       expect(json_response['weight']).to eq(9)

ee/spec/services/ee/issuable/common_system_notes_service_spec.rb

@@ -8,7 +8,20 @@ describe Issuable::CommonSystemNotesService do
   let(:issuable) { create(:issue) }
 
   context 'on issuable update' do
-    it_behaves_like 'system note creation', { weight: 5 }, 'changed weight to **5**'
+    context 'when weight is changed' do
+      before do
+        issuable.update!(weight: 5)
+      end
+
+      it 'creates a resource label event' do
+        described_class.new(project, user).execute(issuable, old_labels: [])
+        event = issuable.reload.resource_weight_events.last
+
+        expect(event).not_to be_nil
+        expect(event.weight).to eq 5
+        expect(event.user_id).to eq user.id
+      end
+    end
 
     context 'when issuable is an epic' do
       let(:timestamp) { Time.now }
@@ -40,9 +53,8 @@ describe Issuable::CommonSystemNotesService do
       issuable.save
     end
 
-    it 'creates a system note for weight' do
-      expect { subject }.to change { issuable.notes.count }.from(0).to(1)
-      expect(issuable.notes.last.note).to match('changed weight')
+    it 'does not create a common system note for weight' do
+      expect { subject }.not_to change { issuable.notes.count }
     end
 
     context 'when resource weight event tracking is enabled' do
@@ -53,11 +65,10 @@ describe Issuable::CommonSystemNotesService do
       it 'creates a resource weight event' do
         subject
 
-        note = issuable.notes.last
         event = issuable.resource_weight_events.last
 
         expect(event.weight).to eq(5)
-        expect(event.created_at).to eq(note.created_at)
+        expect(event.user_id).to eq(user.id)
       end
     end