Merge branch '195871-fix-duplicate-weight-change-notes' into 'master'

Fix rendering of duplicate system notes for weight changes

See merge request gitlab-org/gitlab!26014

app/models/issue.rb

@@ -305,6 +305,10 @@ class Issue < ApplicationRecord
     labels.map(&:hook_attrs)
   end
 
+  def previous_updated_at
+    previous_changes['updated_at']&.first || updated_at
+  end
+
   private
 
   def ensure_metrics

app/models/milestone_note.rb

@@ -12,6 +12,7 @@ class MilestoneNote < ::Note
         created_at: event.created_at,
         noteable: resource,
         milestone: event.milestone,
+        discussion_id: event.discussion_id,
         event: event,
         system_note_metadata: ::SystemNoteMetadata.new(action: 'milestone'),
         resource_parent: resource_parent

app/models/resource_event.rb

@@ -21,7 +21,7 @@ class ResourceEvent < ApplicationRecord
   private
 
   def discussion_id_key
-    [self.class.name, created_at, user_id]
+    [self.class.name, id, user_id]
   end
 
   def exactly_one_issuable

app/models/resource_label_event.rb

@@ -103,6 +103,10 @@ class ResourceLabelEvent < ResourceEvent
   def resource_parent
     issuable.project || issuable.group
   end
+
+  def discussion_id_key
+    [self.class.name, created_at, user_id]
+  end
 end
 
 ResourceLabelEvent.prepend_if_ee('EE::ResourceLabelEvent')

changelogs/unreleased/195871-fix-duplicate-weight-change-notes.yml

+---
+title: Ensure weight changes no longer render duplicate system notes
+merge_request: 26014
+author:
+type: fixed

ee/app/models/concerns/ee/weight_eventable.rb

@@ -7,5 +7,19 @@ module EE
     included do
       has_many :resource_weight_events
     end
+
+    def previous_weight
+      previous_changes['weight']&.first
+    end
+
+    # We want to know if resource already had a weight but no weight tracking events(i.e. no resource_weight_events record)
+    # in which case we will create a historical resource_weight_event record with last known weight. That is to try and
+    # populate more data when tracking changes, without having a huge migration parsing weight change notes.
+    #
+    # At some point this should be removed, when we will have enough historical data and it will be of no use:
+    # https://gitlab.com/gitlab-org/gitlab/issues/208785
+    def first_weight_event?
+      previous_weight.present? && resource_weight_events.none?
+    end
   end
 end

ee/app/models/weight_note.rb

@@ -13,6 +13,7 @@ module EE
         created_at: event.created_at,
         noteable: resource,
         event: event,
+        discussion_id: event.discussion_id,
         system_note_metadata: ::SystemNoteMetadata.new(action: 'weight'),
         resource_parent: resource_parent
       }

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

@@ -25,18 +25,10 @@ module EE
           changes = []
           base_data = { user_id: user.id, issue_id: resource.id }
 
-          changes << base_data.merge({ weight: previous_weight(resource), created_at: resource.updated_at }) if first_weight_event?(resource)
+          changes << base_data.merge({ weight: resource.previous_weight, created_at: resource.previous_updated_at }) if resource.first_weight_event?
           changes << base_data.merge({ weight: resource.weight, created_at: event_created_at })
         end.flatten
       end
-
-      def previous_weight(resource)
-        resource.previous_changes['weight']&.first
-      end
-
-      def first_weight_event?(resource)
-        previous_weight(resource) && ResourceWeightEvent.by_issue(resource).none?
-      end
     end
   end
 end

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

@@ -19,7 +19,7 @@ module EE
       def weight_change_events
         return [] unless resource.respond_to?(:resource_weight_events)
 
-        events = resource.resource_weight_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
+        events = resource.resource_weight_events.includes(user: :status).order(:id) # rubocop: disable CodeReuse/ActiveRecord
         since_fetch_at(events)
       end
     end

ee/spec/features/issues/move_issue_resource_weight_events_spec.rb

@@ -41,14 +41,14 @@ describe 'Resource weight events', :js do
         visit project_issue_path(target_project, issue)
         wait_for_all_requests
 
-        expect(page).to have_content 'changed weight to 2'
-        expect(page).to have_content 'changed weight to 3'
+        expect(page).to have_content('changed weight to 2', count: 1)
+        expect(page).to have_content('changed weight to 3', count: 1)
 
         visit project_issue_path(project, issue)
         wait_for_all_requests
 
-        expect(page).to have_content 'changed weight to 2'
-        expect(page).to have_content 'changed weight to 3'
+        expect(page).to have_content('changed weight to 2', count: 1)
+        expect(page).to have_content('changed weight to 3', count: 1)
         expect(page).to have_content 'Closed'
       end
     end

ee/spec/features/issues/resource_weight_events_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'issue resource weight events', :js do
+  let(:user)     { create(:user) }
+  let(:project)  { create(:project, :public) }
+  let(:issue)    { create(:issue, project: project, author: user) }
+
+  context 'when user displays the issue' do
+    let!(:note) { create(:note_on_issue, author: user, project: project, noteable: issue, note: 'some note') }
+    let!(:event1) { create(:resource_weight_event, issue: issue, weight: 1) }
+    let!(:event2) { create(:resource_weight_event, issue: issue, weight: 5) }
+
+    before do
+      visit project_issue_path(project, issue)
+      wait_for_all_requests
+    end
+
+    it 'shows both notes and resource weight event synthetic notes' do
+      page.within('#notes') do
+        expect(find("#note_#{note.id}")).to have_content 'some note'
+        expect(find("#note_#{event1.discussion_id}")).to have_content 'changed weight to 1', count: 1
+        expect(find("#note_#{event2.discussion_id}")).to have_content 'changed weight to 5', count: 1
+      end
+    end
+  end
+end

ee/spec/models/concerns/ee/weight_eventable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::WeightEventable do
+  subject { build(:issue) }
+
+  describe 'associations' do
+    it { is_expected.to have_many(:resource_weight_events) }
+  end
+
+  describe '#first_weight_event?' do
+    it 'returns false as it has no weight changes' do
+      allow(subject).to receive(:previous_changes).and_return({ 'weight' => nil })
+
+      expect(subject.first_weight_event?).to be false
+    end
+
+    it 'returns false as it has no previous weight' do
+      allow(subject).to receive(:previous_changes).and_return({ 'weight' => [nil, 3] })
+
+      expect(subject.first_weight_event?).to be false
+    end
+
+    it 'returns false as it has already a resoure_weight_event' do
+      create(:resource_weight_event, issue: subject)
+      allow(subject).to receive(:previous_changes).and_return({ 'weight' => [nil, 3] })
+
+      expect(subject.first_weight_event?).to be false
+    end
+
+    it 'returns true as the previous weight exists and there is no resoure_weight_event record' do
+      allow(subject).to receive(:previous_changes).and_return({ 'weight' => [3, 4] })
+
+      expect(subject.first_weight_event?).to be true
+    end
+  end
+end

ee/spec/services/ee/resource_events/change_weight_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe EE::ResourceEvents::ChangeWeightService do
   let_it_be(:user) { create(:user) }
 
   let(:issue) { create(:issue, weight: 3) }
-  let(:created_at_time) { Time.new(2019, 1, 1, 12, 30, 48) }
+  let(:created_at_time) { Time.utc(2019, 1, 1, 12, 30, 48, '123.123'.to_r) }
 
   subject { described_class.new([issue], user, created_at_time).execute }
 
@@ -35,14 +35,16 @@ RSpec.describe EE::ResourceEvents::ChangeWeightService do
   context 'when there is no existing weight event record' do
     before do
       ResourceWeightEvent.delete_all
-      issue.update(weight: 5)
+      issue.update(weight: 5, updated_at: 10.seconds.ago)
     end
 
     it 'creates the expected event records' do
+      prev_update_at = issue.previous_changes['updated_at']&.first
+
       expect { subject }.to change { ResourceWeightEvent.count }.by(2)
 
       record = ResourceWeightEvent.first
-      expect_event_record(record, weight: 3, created_at: issue.reload.updated_at)
+      expect_event_record(record, weight: 3, created_at: prev_update_at)
 
       record = ResourceWeightEvent.last
       expect_event_record(record, weight: 5, created_at: created_at_time)
@@ -53,7 +55,7 @@ RSpec.describe EE::ResourceEvents::ChangeWeightService do
     expect(record.issue).to eq(issue)
     expect(record.user).to eq(user)
     expect(record.weight).to eq(weight)
-    expect(record.created_at).to eq(created_at)
+    expect(record.created_at).to be_like_time(created_at)
   end
 
   describe 'bulk issue weight updates' do

spec/models/issue_spec.rb

@@ -425,16 +425,16 @@ describe Issue do
     let(:issue) { create(:issue, title: 'testing-issue') }
 
     it 'starts with the issue iid' do
-      expect(issue.to_branch_name).to match /\A#{issue.iid}-[A-Za-z\-]+\z/
+      expect(issue.to_branch_name).to match(/\A#{issue.iid}-[A-Za-z\-]+\z/)
     end
 
     it "contains the issue title if not confidential" do
-      expect(issue.to_branch_name).to match /testing-issue\z/
+      expect(issue.to_branch_name).to match(/testing-issue\z/)
     end
 
     it "does not contain the issue title if confidential" do
       issue = create(:issue, title: 'testing-issue', confidential: true)
-      expect(issue.to_branch_name).to match /confidential-issue\z/
+      expect(issue.to_branch_name).to match(/confidential-issue\z/)
     end
 
     context 'issue title longer than 100 characters' do
@@ -932,4 +932,33 @@ describe Issue do
   end
 
   it_behaves_like 'versioned description'
+
+  describe "#previous_updated_at" do
+    let_it_be(:updated_at) { Time.new(2012, 01, 06) }
+    let_it_be(:issue) { create(:issue, updated_at: updated_at) }
+
+    it 'returns updated_at value if updated_at did not change at all' do
+      allow(issue).to receive(:previous_changes).and_return({})
+
+      expect(issue.previous_updated_at).to eq(updated_at)
+    end
+
+    it 'returns updated_at value if `previous_changes` has nil value for `updated_at`' do
+      allow(issue).to receive(:previous_changes).and_return({ 'updated_at' => nil })
+
+      expect(issue.previous_updated_at).to eq(updated_at)
+    end
+
+    it 'returns updated_at value if previous updated_at value is not present' do
+      allow(issue).to receive(:previous_changes).and_return({ 'updated_at' => [nil, Time.new(2013, 02, 06)] })
+
+      expect(issue.previous_updated_at).to eq(updated_at)
+    end
+
+    it 'returns previous updated_at when present' do
+      allow(issue).to receive(:previous_changes).and_return({ 'updated_at' => [Time.new(2013, 02, 06), Time.new(2013, 03, 06)] })
+
+      expect(issue.previous_updated_at).to eq(Time.new(2013, 02, 06))
+    end
+  end
 end

spec/models/resource_weight_event_spec.rb

@@ -67,7 +67,7 @@ RSpec.describe ResourceWeightEvent, type: :model do
 
     it 'returns the expected id' do
       allow(Digest::SHA1).to receive(:hexdigest)
-                               .with("ResourceWeightEvent-2019-12-30 00:00:00 UTC-#{user1.id}")
+                               .with("ResourceWeightEvent-#{event.id}-#{user1.id}")
                                .and_return('73d167c478')
 
       expect(event.discussion_id).to eq('73d167c478')