Remove trailing comma from weight system notes

We added a comma to the end of these to disambiguate between the value and the
timestamp after it. In the following release, we added a dot between the note
and the timestamp, so the comma was redundant.

This uses a background migration to find notes that are:

1. System notes.
2. That are about weight.
3. That end in a comma.

And remove the comma and clear the cached HTML. There are about 15,000 of these
on GitLab.com.

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180629191052) do
+ActiveRecord::Schema.define(version: 20180702114215) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

ee/app/models/ee/note.rb

@@ -11,7 +11,7 @@ module EE
       noteable.is_a?(Epic)
     end
 
-    # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6347
+    # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6793
     def note
       raw_note = super
 
@@ -20,7 +20,7 @@ module EE
       raw_note.delete(',')
     end
 
-    # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6347
+    # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6783
     def note_html
       raw_note_html = super
 

ee/changelogs/unreleased/migrate-weight-system-notes.yml

+---
+title: Fix weight system notes ending in commas
+merge_request:
+author:
+type: fixed

ee/db/migrate/20180702114215_schedule_weight_system_note_comma_cleanup.rb

+class ScheduleWeightSystemNoteCommaCleanup < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RemoveCommaFromWeightSystemNotes'.freeze
+  BATCH_SIZE = 1000
+  DELAY_INTERVAL = 5.minutes.to_i
+
+  class Note < ActiveRecord::Base
+    self.table_name = 'notes'
+
+    include ::EachBatch
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:system_note_metadata, :action, where: "action = 'weight'")
+
+    weight_system_notes =
+      Note
+        .joins('INNER JOIN system_note_metadata ON notes.id = system_note_metadata.note_id')
+        .where(system: true)
+        .where(system_note_metadata: { action: 'weight' })
+        .where("notes.note LIKE '%,'")
+
+    weight_system_notes.each_batch(of: BATCH_SIZE) do |relation, index|
+      ids = relation.pluck(:id)
+      delay = index * DELAY_INTERVAL
+
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, [ids])
+    end
+
+    remove_concurrent_index(:system_note_metadata, :action, where: "action = 'weight'")
+  end
+
+  def down
+  end
+end

ee/lib/gitlab/background_migration/remove_comma_from_weight_system_notes.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class RemoveCommaFromWeightSystemNotes
+      include ::Gitlab::Database::ArelMethods
+
+      def perform(note_ids)
+        notes_table = Arel::Table.new(:notes)
+
+        update_values = [
+          [notes_table[:note], Arel.sql("TRIM(TRAILING ',' FROM note)")],
+          [notes_table[:note_html], nil]
+        ]
+
+        update = arel_update_manager
+                   .table(notes_table)
+                   .set(update_values)
+                   .where(notes_table[:id].in(note_ids))
+
+        ActiveRecord::Base.connection.execute(update.to_sql)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/background_migration/remove_comma_from_weight_system_notes_spec.rb

+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::RemoveCommaFromWeightSystemNotes, :migration, schema: 20180702114215 do
+  RSpec::Matchers.define_negated_matcher :not_change, :change
+
+  describe '#perform' do
+    let(:notes) { table(:notes) }
+    let(:system_note_metadata) { table(:system_note_metadata) }
+
+    def create_system_note(note, metadata_action)
+      notes.create(note: note, note_html: note, system: true).tap do |system_note|
+        system_note_metadata.create(note_id: system_note.id, action: metadata_action)
+      end
+    end
+
+    it 'processes all notes in the batch' do
+      weight_note_1 = create_system_note('changed weight to 1,', 'weight')
+      weight_note_2 = create_system_note('changed weight to 2,', 'weight')
+      weight_note_3 = create_system_note('changed weight to 3,', 'weight')
+
+      expect { subject.perform([weight_note_1.id, weight_note_2.id]) }
+        .to change { weight_note_1.reload.note }
+        .and change { weight_note_1.reload.note_html }
+        .and change { weight_note_2.reload.note }
+        .and change { weight_note_2.reload.note_html }
+        .and not_change { weight_note_3.reload.note }
+        .and not_change { weight_note_3.reload.note_html }
+    end
+  end
+end

ee/spec/migrations/schedule_weight_system_note_comma_cleanup_spec.rb

+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '../../db/migrate/20180702114215_schedule_weight_system_note_comma_cleanup.rb')
+
+describe ScheduleWeightSystemNoteCommaCleanup, :migration do
+  describe '#up' do
+    let(:notes) { table(:notes) }
+    let(:system_note_metadata) { table(:system_note_metadata) }
+
+    def create_system_note(id, note, metadata_action)
+      notes.create!(id: id, note: note, system: true).tap do |system_note|
+        system_note_metadata.create!(note_id: system_note.id, action: metadata_action)
+      end
+    end
+
+    before do
+      stub_const("#{described_class}::BATCH_SIZE", 1)
+
+      # Should both be scheduled
+      create_system_note(1, 'changed weight to 5,', 'weight')
+      create_system_note(2, 'changed weight to 5,', 'weight')
+
+      # Should not be scheduled - no trailing comma
+      create_system_note(3, 'removed the weight', 'weight')
+
+      # Should not be scheduled - not a weight note
+      create_system_note(4, 'changed title from 5, to 5,', 'title')
+
+      # Should not be scheduled - not a system note
+      notes.create(id: 5, note: 'changed weight to 5,')
+    end
+
+    it 'schedules delayed background migrations in batches' do
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(described_class::MIGRATION).to be_scheduled_delayed_migration(described_class::DELAY_INTERVAL, [1])
+          expect(described_class::MIGRATION).to be_scheduled_delayed_migration(described_class::DELAY_INTERVAL * 2, [2])
+          expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        end
+      end
+    end
+  end
+end

ee/spec/models/ee/note_spec.rb

 require 'spec_helper'
 
 describe EE::Note do
-  # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6347
+  # Remove with https://gitlab.com/gitlab-org/gitlab-ee/issues/6793
   describe "#note and #note_html overrides for weight" do
     using RSpec::Parameterized::TableSyntax