Move weight note model to EE

Write system note if weight tracking feature
flag is not set

Make comment more generic

Fix scope for SystemNoteMetadata constructor
call

and add specs

Add specs and fix missing include on ResourceWeightEvent

Fix schema.rb on_delete clause

Add specs for note count by feature flag

Add specs for services

app/models/resource_weight_event.rb

 # frozen_string_literal: true
 
 class ResourceWeightEvent < ApplicationRecord
+  include Gitlab::Utils::StrongMemoize
+
   validates :user, presence: true
   validates :issue, presence: true
 

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.
+# We store events about issuable label changes and weight changes in a separate
+# table (not as other system notes), but we still want to display notes about
+# label changes and weight changes as classic system notes in UI.  This service
+# generates "synthetic" notes for label event changes.
 
 module ResourceEvents
   class BaseSyntheticNotesBuilderService

db/schema.rb

@@ -4755,7 +4755,7 @@ ActiveRecord::Schema.define(version: 2019_12_18_225624) do
   add_foreign_key "resource_label_events", "merge_requests", on_delete: :cascade
   add_foreign_key "resource_label_events", "users", on_delete: :nullify
   add_foreign_key "resource_weight_events", "issues", on_delete: :cascade
-  add_foreign_key "resource_weight_events", "users", on_delete: :cascade
+  add_foreign_key "resource_weight_events", "users", on_delete: :nullify
   add_foreign_key "reviews", "merge_requests", on_delete: :cascade
   add_foreign_key "reviews", "projects", on_delete: :cascade
   add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify

ee/app/models/weight_note.rb

+# frozen_string_literal: true
+
+module EE
+  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
+end

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

@@ -29,10 +29,13 @@ module EE
       end
 
       def handle_weight_change
-        return unless weight_changes_tracking_enabled?
         return unless issuable.previous_changes.include?('weight')
 
-        EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.now).execute
+        if weight_changes_tracking_enabled?
+          EE::ResourceEvents::ChangeWeightService.new([issuable], current_user, Time.now).execute
+        else
+          ::SystemNoteService.change_weight_note(issuable, issuable.project, current_user)
+        end
       end
 
       def weight_changes_tracking_enabled?

ee/spec/requests/api/issues_spec.rb

@@ -345,10 +345,20 @@ describe API::Issues, :mailer do
       expect(json_response['message']['weight']).to be_present
     end
 
-    it 'adds a note when the weight is changed' do
+    it 'creates a ResourceWeightEvent' do
       expect do
         put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
       end.to change { ResourceWeightEvent.count }.by(1)
+    end
+
+    it 'does not create a system note' do
+      expect do
+        put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
+      end.not_to change { Note.count }
+    end
+
+    it 'adds a note when the weight is changed' do
+      put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
 
       expect(response).to have_gitlab_http_status(200)
       expect(json_response['weight']).to eq(9)
@@ -367,6 +377,24 @@ describe API::Issues, :mailer do
         expect(issue.reload.read_attribute(:weight)).to be_nil
       end
     end
+
+    context 'when issue weight tracking feature flag is not active' do
+      before do
+        stub_feature_flags(track_issue_weight_change_events: false)
+      end
+
+      it 'does not create a ResourceWeightEvent' do
+        expect do
+          put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
+        end.not_to change { ResourceWeightEvent.count }
+      end
+
+      it 'creates a system note' do
+        expect do
+          put api("/projects/#{project.id}/issues/#{issue.iid}", user), params: { weight: 9 }
+        end.to change { Note.count }.by(1)
+      end
+    end
   end
 
   describe 'PUT /projects/:id/issues/:issue_id to update epic' do

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

@@ -70,6 +70,10 @@ describe Issuable::CommonSystemNotesService do
         expect(event.weight).to eq(5)
         expect(event.user_id).to eq(user.id)
       end
+
+      it 'does not create a system note' do
+        expect { subject }.not_to change { Note.count }
+      end
     end
 
     context 'when resource weight event tracking is disabled' do
@@ -80,6 +84,12 @@ describe Issuable::CommonSystemNotesService do
       it 'does not created a resource weight event' do
         expect { subject }.not_to change { ResourceWeightEvent.count }
       end
+
+      it 'does create a system note' do
+        expect { subject }.to change { Note.count }.from(0).to(1)
+
+        expect(Note.first.note).to eq('changed weight to **5**')
+      end
     end
   end
 end

ee/spec/services/ee/resource_events/merge_into_notes_service_spec.rb

@@ -3,13 +3,19 @@
 require 'spec_helper'
 
 describe ResourceEvents::MergeIntoNotesService do
-  def create_event(params)
+  def create_label_event(params)
     event_params = { action: :add, label: label, issue: resource,
                      user: user }
 
     create(:resource_label_event, event_params.merge(params))
   end
 
+  def create_weight_event(params, weight = resource.weight)
+    event_params = { issue: resource, user: user }
+
+    create(:resource_weight_event, event_params.merge(params))
+  end
+
   set(:project) { create(:project) }
   set(:user) { create(:user) }
   set(:resource) { create(:issue, project: project) }
@@ -26,15 +32,15 @@ describe ResourceEvents::MergeIntoNotesService do
     it 'squashes events with same time and author into single note but scoped labels are separated' do
       user2 = create(:user)
 
-      create_event(created_at: time)
-      create_event(created_at: time, label: label2, action: :remove)
-      create_event(created_at: time, label: scoped_label_group1_1, action: :remove)
-      create_event(created_at: time, label: scoped_label_group1_2, action: :add)
-      create_event(created_at: time, label: scoped_label_group2_2, action: :remove)
-      create_event(created_at: time, label: scoped_label_group2_1, action: :add)
-      create_event(created_at: time, label: scoped_label_group3_1, action: :add)
-      create_event(created_at: time, user: user2)
-      create_event(created_at: 1.day.ago, label: label2)
+      create_label_event(created_at: time)
+      create_label_event(created_at: time, label: label2, action: :remove)
+      create_label_event(created_at: time, label: scoped_label_group1_1, action: :remove)
+      create_label_event(created_at: time, label: scoped_label_group1_2, action: :add)
+      create_label_event(created_at: time, label: scoped_label_group2_2, action: :remove)
+      create_label_event(created_at: time, label: scoped_label_group2_1, action: :add)
+      create_label_event(created_at: time, label: scoped_label_group3_1, action: :add)
+      create_label_event(created_at: time, user: user2)
+      create_label_event(created_at: 1.day.ago, label: label2)
 
       notes = described_class.new(resource, user).execute
 
@@ -56,10 +62,10 @@ describe ResourceEvents::MergeIntoNotesService do
     context 'scoped labels' do
       context 'when all labels are automatically removed' do
         it 'adds "automatically removed" message' do
-          create_event(created_at: time, label: scoped_label_group1_1, action: :add)
-          create_event(created_at: time, label: scoped_label_group1_2, action: :remove)
-          create_event(created_at: time, label: scoped_label_group2_1, action: :add)
-          create_event(created_at: time, label: scoped_label_group2_2, action: :remove)
+          create_label_event(created_at: time, label: scoped_label_group1_1, action: :add)
+          create_label_event(created_at: time, label: scoped_label_group1_2, action: :remove)
+          create_label_event(created_at: time, label: scoped_label_group2_1, action: :add)
+          create_label_event(created_at: time, label: scoped_label_group2_2, action: :remove)
 
           note = described_class.new(resource, user).execute.first.note
 
@@ -72,9 +78,9 @@ describe ResourceEvents::MergeIntoNotesService do
 
       context 'when any of the labels is manually removed' do
         it 'adds "removed" message' do
-          create_event(created_at: time, label: scoped_label_group1_1, action: :add)
-          create_event(created_at: time, label: scoped_label_group1_2, action: :remove)
-          create_event(created_at: time, label: scoped_label_group2_1, action: :remove)
+          create_label_event(created_at: time, label: scoped_label_group1_1, action: :add)
+          create_label_event(created_at: time, label: scoped_label_group1_2, action: :remove)
+          create_label_event(created_at: time, label: scoped_label_group2_1, action: :remove)
 
           note = described_class.new(resource, user).execute.first.note
 
@@ -85,5 +91,21 @@ describe ResourceEvents::MergeIntoNotesService do
         end
       end
     end
+
+    context 'with weight events' do
+      it 'includes the expected notes' do
+        create_weight_event(created_at: time, weight: 3)
+        create_weight_event(created_at: time, weight: 1)
+        create_weight_event(created_at: time, weight: 5)
+
+        notes = described_class.new(resource, user).execute
+
+        expect(notes.size).to eq(3)
+
+        expect(notes[0].note).to eq('changed weight to 3')
+        expect(notes[1].note).to eq('changed weight to 1')
+        expect(notes[2].note).to eq('changed weight to 5')
+      end
+    end
   end
 end

ee/spec/services/ee/resource_events/synthetic_weight_notes_builder_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::ResourceEvents::SyntheticWeightNotesBuilderService do
+  describe '#execute' do
+    let!(:user) { create(:user) }
+
+    let!(:issue) { create(:issue, author: user) }
+
+    let!(:event1) { create(:resource_weight_event, issue: issue) }
+    let!(:event2) { create(:resource_weight_event, issue: issue) }
+    let!(:event3) { create(:resource_weight_event, issue: issue) }
+
+    it 'returns the expected synthetic notes' do
+      notes = EE::ResourceEvents::SyntheticWeightNotesBuilderService.new(issue, user).execute
+
+      expect(notes.size).to eq(3)
+    end
+  end
+end

spec/models/resource_weight_event_spec.rb

@@ -3,6 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe ResourceWeightEvent, type: :model do
+  let_it_be(:user1) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+
+  let_it_be(:issue1) { create(:issue, author: user1) }
+  let_it_be(:issue2) { create(:issue, author: user1) }
+  let_it_be(:issue3) { create(:issue, author: user2) }
+
   describe 'validations' do
     it { is_expected.not_to allow_value(nil).for(:user) }
     it { is_expected.not_to allow_value(nil).for(:issue) }
@@ -15,13 +22,6 @@ RSpec.describe ResourceWeightEvent, type: :model do
   end
 
   describe '.by_issue' do
-    let_it_be(:user1) { create(:user) }
-    let_it_be(:user2) { create(:user) }
-
-    let_it_be(:issue1) { create(:issue, author: user1) }
-    let_it_be(:issue2) { create(:issue, author: user1) }
-    let_it_be(:issue3) { create(:issue, author: user2) }
-
     let_it_be(:event1) { create(:resource_weight_event, issue: issue1) }
     let_it_be(:event2) { create(:resource_weight_event, issue: issue2) }
     let_it_be(:event3) { create(:resource_weight_event, issue: issue1) }
@@ -38,4 +38,38 @@ RSpec.describe ResourceWeightEvent, type: :model do
       expect(events).to be_empty
     end
   end
+
+  describe '.created_after' do
+    let!(:created_at1) { 1.day.ago }
+    let!(:created_at2) { 2.days.ago }
+    let!(:created_at3) { 3.days.ago }
+
+    let!(:event1) { create(:resource_weight_event, issue: issue1, created_at: created_at1) }
+    let!(:event2) { create(:resource_weight_event, issue: issue2, created_at: created_at2) }
+    let!(:event3) { create(:resource_weight_event, issue: issue2, created_at: created_at3) }
+
+    it 'returns the expected events' do
+      events = ResourceWeightEvent.created_after(created_at3)
+
+      expect(events).to contain_exactly(event1, event2)
+    end
+
+    it 'returns no events if time is after last record time' do
+      events = ResourceWeightEvent.created_after(1.minute.ago)
+
+      expect(events).to be_empty
+    end
+  end
+
+  describe '#discussion_id' do
+    let_it_be(:event) { create(:resource_weight_event, issue: issue1, created_at: Time.utc(2019, 12, 30)) }
+
+    it 'returns the expected id' do
+      allow(Digest::SHA1).to receive(:hexdigest)
+                               .with("ResourceWeightEvent-2019-12-30 00:00:00 UTC-#{user1.id}")
+                               .and_return('73d167c478')
+
+      expect(event.discussion_id).to eq('73d167c478')
+    end
+  end
 end

spec/services/resource_events/synthetic_label_notes_builder_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ResourceEvents::SyntheticLabelNotesBuilderService do
+  describe '#execute' do
+    let!(:user) { create(:user) }
+
+    let!(:issue) { create(:issue, author: user) }
+
+    let!(:event1) { create(:resource_label_event, issue: issue) }
+    let!(:event2) { create(:resource_label_event, issue: issue) }
+    let!(:event3) { create(:resource_label_event, issue: issue) }
+
+    it 'returns the expected synthetic notes' do
+      notes = ResourceEvents::SyntheticLabelNotesBuilderService.new(issue, user).execute
+
+      expect(notes.size).to eq(3)
+    end
+  end
+end