Merge branch '343621-separately-store-the-numbering-of-iterations' into 'master'

Save iteration sequence number

See merge request gitlab-org/gitlab!74352

db/migrate/20211126042235_add_sequence_column_to_sprints_table.rb

+# frozen_string_literal: true
+
+class AddSequenceColumnToSprintsTable < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    add_column :sprints, :sequence, :integer
+    execute "ALTER TABLE sprints ADD CONSTRAINT sequence_is_unique_per_iterations_cadence_id UNIQUE (iterations_cadence_id, sequence) DEFERRABLE INITIALLY DEFERRED"
+  end
+
+  def down
+    remove_column :sprints, :sequence
+  end
+end

db/post_migrate/20211130165043_backfill_sequence_column_for_sprints_table.rb

+# frozen_string_literal: true
+
+class BackfillSequenceColumnForSprintsTable < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    execute(
+      <<-SQL
+        UPDATE sprints
+        SET sequence=t.row_number
+        FROM (
+          SELECT id, row_number() OVER (PARTITION BY iterations_cadence_id ORDER BY start_date)
+          FROM sprints as s1
+          WHERE s1.iterations_cadence_id IS NOT NULL
+        ) as t
+        WHERE t.id=sprints.id AND (sprints.sequence IS NULL OR sprints.sequence <> t.row_number)
+      SQL
+    )
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20211126042235

+c6992d23fc43c26861accf7c516603802c95367460ad688d1a420a60a33833f1
\ No newline at end of file

db/schema_migrations/20211130165043

+2eece823b66fec7f5a9a5c24b93d354a47939a7cdd915349a433b7bbec6abc22
\ No newline at end of file

db/structure.sql

@@ -19648,6 +19648,7 @@ CREATE TABLE sprints (
     description_html text,
     state_enum smallint DEFAULT 1 NOT NULL,
     iterations_cadence_id integer,
+    sequence integer,
     CONSTRAINT sprints_must_belong_to_project_or_group CHECK ((((project_id <> NULL::bigint) AND (group_id IS NULL)) OR ((group_id <> NULL::bigint) AND (project_id IS NULL)))),
     CONSTRAINT sprints_title CHECK ((char_length(title) <= 255))
 );
@@ -24046,6 +24047,9 @@ ALTER TABLE ONLY sent_notifications
 ALTER TABLE ONLY sentry_issues
     ADD CONSTRAINT sentry_issues_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY sprints
+    ADD CONSTRAINT sequence_is_unique_per_iterations_cadence_id UNIQUE (iterations_cadence_id, sequence) DEFERRABLE INITIALLY DEFERRED;
+
 ALTER TABLE ONLY serverless_domain_cluster
     ADD CONSTRAINT serverless_domain_cluster_pkey PRIMARY KEY (uuid);
 

ee/app/models/ee/iteration.rb

@@ -55,6 +55,10 @@ module EE
       before_save :set_iteration_state
       before_destroy :check_if_can_be_destroyed
 
+      after_save :update_iteration_sequences, if: -> { iterations_cadence && saved_change_to_start_or_due_date? }
+      after_destroy :update_iteration_sequences, if: :iterations_cadence
+      after_commit :reset, on: [:update, :create], if: :saved_change_to_start_or_due_date?
+
       scope :due_date_order_asc, -> { order(:due_date) }
       scope :upcoming, -> { with_state(:upcoming) }
       scope :current, -> { with_state(:current) }
@@ -225,6 +229,10 @@ module EE
       start_date_changed? || due_date_changed?
     end
 
+    def saved_change_to_start_or_due_date?
+      saved_change_to_start_date? || saved_change_to_due_date?
+    end
+
     # ensure dates do not overlap with other Iterations in the same cadence tree
     def dates_do_not_overlap
       return unless iterations_cadence
@@ -254,6 +262,10 @@ module EE
       self.state = self.class.compute_state(start_date, due_date)
     end
 
+    def update_iteration_sequences
+      iterations_cadence.update_iteration_sequences
+    end
+
     # TODO: this method should be removed as part of https://gitlab.com/gitlab-org/gitlab/-/issues/296099
     def find_or_create_default_cadence
       cadence_title = "#{group.name} Iterations"

ee/app/models/iterations/cadence.rb

@@ -65,5 +65,16 @@ module Iterations
     def changed_iterations_automation_fields?
       (previous_changes.keys.map(&:to_sym) & ITERATIONS_AUTOMATION_FIELDS).present?
     end
+
+    def update_iteration_sequences
+      connection.execute <<~SQL
+        UPDATE sprints SET sequence=t.row_number
+        FROM (
+          SELECT id, row_number() OVER (ORDER BY start_date) FROM sprints
+          WHERE iterations_cadence_id = #{id}
+        ) as t
+        WHERE t.id=sprints.id AND (sprints.sequence IS DISTINCT FROM t.row_number)
+      SQL
+    end
   end
 end

ee/app/services/iterations/cadences/create_iterations_in_advance_service.rb

@@ -15,7 +15,11 @@ module Iterations
         return ::ServiceResponse.error(message: _('Cadence is not automated'), http_status: 422) unless cadence.can_be_automated?
 
         update_existing_iterations!
-        ::ApplicationRecord.legacy_bulk_insert(Iteration.table_name, build_new_iterations) # rubocop:disable Gitlab/BulkInsert
+
+        Iteration.transaction do
+          ::ApplicationRecord.legacy_bulk_insert(Iteration.table_name, build_new_iterations) # rubocop:disable Gitlab/BulkInsert
+          cadence.update_iteration_sequences
+        end
 
         cadence.update!(last_run_date: compute_last_run_date)
 

ee/spec/factories/iterations.rb

@@ -43,6 +43,12 @@ FactoryBot.define do
       end
     end
 
+    trait(:with_due_date) do
+      after(:stub, :build) do |iteration, evaluator|
+        iteration.due_date = evaluator.start_date + 4.days if evaluator.start_date.present?
+      end
+    end
+
     after(:build, :stub) do |iteration, evaluator|
       if evaluator.group
         iteration.group = evaluator.group
@@ -61,6 +67,9 @@ FactoryBot.define do
 
       if evaluator.iterations_cadence.present?
         iteration.iterations_cadence = evaluator.iterations_cadence
+        # TODO https://gitlab.com/gitlab-org/gitlab/-/issues/296100
+        # group_id will be removed from sprints and we won't need this feature.
+        iteration.group = evaluator.iterations_cadence.group unless evaluator.group.present?
       else
         iteration.iterations_cadence = iteration.group.iterations_cadences.first || create(:iterations_cadence, group: iteration.group) if iteration.group
         iteration.iterations_cadence = create(:iterations_cadence, group_id: iteration.group_id) if iteration.group_id

ee/spec/models/ee/iterations/cadence_spec.rb

@@ -33,4 +33,40 @@ RSpec.describe ::Iterations::Cadence do
       it { is_expected.not_to validate_presence_of(:start_date) }
     end
   end
+
+  describe '#update_iteration_sequences', :aggregate_failures do
+    using RSpec::Parameterized::TableSyntax
+
+    let_it_be(:group) { create(:group) }
+    let_it_be(:iterations_cadence) { create(:iterations_cadence, group: group) }
+
+    let(:expected_sequence) { (1..iterations_cadence.iterations.size).to_a }
+    let(:ordered_iterations) { iterations_cadence.iterations.order(:start_date) }
+
+    context 'an iteration is created or updated' do
+      where(:start_date, :expected_ordered_title) do
+        1.week.ago       | lazy { %w[iteration a b] }
+        Date.today       | lazy { %w[iteration a b] }
+        2.weeks.from_now | lazy { %w[a iteration b] }
+        4.weeks.from_now | lazy { %w[a b iteration] }
+      end
+
+      with_them do
+        before do
+          Iteration.insert_all!([
+            {  sequence: nil, title: 'iteration', start_date: start_date, due_date: start_date + 4.days, iterations_cadence_id: iterations_cadence.id, iid: 1, created_at: Time.zone.now, updated_at: Time.zone.now },
+            {  sequence: nil, title: 'a', start_date: 1.week.from_now, due_date: 1.week.from_now + 4.days, iterations_cadence_id: iterations_cadence.id, iid: 2, created_at: Time.zone.now, updated_at: Time.zone.now },
+            {  sequence: nil, title: 'b', start_date: 3.weeks.from_now, due_date: 3.weeks.from_now + 4.days, iterations_cadence_id: iterations_cadence.id, iid: 3, created_at: Time.zone.now, updated_at: Time.zone.now }
+          ])
+        end
+
+        it 'sequence numbers are correctly updated' do
+          iterations_cadence.update_iteration_sequences
+
+          expect(ordered_iterations.map(&:sequence)).to eq(expected_sequence)
+          expect(ordered_iterations.map(&:title)).to eq(expected_ordered_title)
+        end
+      end
+    end
+  end
 end

ee/spec/services/iterations/cadences/create_iterations_in_advance_service_spec.rb

@@ -44,6 +44,8 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
 
         context 'with automatic and active cadence' do
           let(:cadence) { automated_cadence }
+          let(:ordered_iterations) { automated_cadence.iterations.order(:start_date) }
+          let(:ordered_sequences) { ordered_iterations.map(&:sequence) }
 
           it 'does not return error' do
             expect(subject).not_to be_error
@@ -58,10 +60,22 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
           end
 
           context 'when new iterations need to be created' do
+            shared_examples "creating iterations with sequences" do
+              let(:sequences) { (1..cadence.iterations.size).to_a }
+
+              it 'creates iterations with correct sequences' do
+                subject
+
+                expect(ordered_sequences).to eq(sequences)
+              end
+            end
+
             context 'when no iterations exist' do
-              it 'creates new iterations' do
+              it 'creates new iterations', :aggregate_failures do
                 expect { subject }.to change(Iteration, :count).by(3)
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
 
             context 'when cadence start date is in future' do
@@ -80,12 +94,14 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
 
                 expect(automated_cadence.reload.last_run_date).to eq(automated_cadence.iterations.last(3).first.due_date)
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
 
             context 'when advanced iterations exist but cadence needs to create more' do
-              let_it_be(:current_iteration) { create(:iteration, group: group, iterations_cadence: automated_cadence, start_date: 3.days.ago, due_date: (1.week - 3.days).from_now)}
-              let_it_be(:next_iteration1) { create(:iteration, group: group, iterations_cadence: automated_cadence, start_date: current_iteration.due_date + 1.day, due_date: current_iteration.due_date + 1.week)}
-              let_it_be(:next_iteration2) { create(:iteration, group: group, iterations_cadence: automated_cadence, start_date: next_iteration1.due_date + 1.day, due_date: next_iteration1.due_date + 1.week)}
+              let_it_be(:current_iteration) { create(:iteration, iterations_cadence: automated_cadence, start_date: 3.days.ago, due_date: (1.week - 3.days).from_now)}
+              let_it_be(:next_iteration1) { create(:iteration, iterations_cadence: automated_cadence, start_date: current_iteration.due_date + 1.day, due_date: current_iteration.due_date + 1.week)}
+              let_it_be(:next_iteration2) { create(:iteration, iterations_cadence: automated_cadence, start_date: next_iteration1.due_date + 1.day, due_date: next_iteration1.due_date + 1.week)}
 
               before do
                 automated_cadence.update!(iterations_in_advance: 3, duration_in_weeks: 3)
@@ -102,6 +118,8 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
                 expect(next_iteration2.reload.start_date).to eq(next_iteration1.due_date + 1.day)
                 expect(next_iteration2.reload.due_date).to eq(next_iteration1.due_date + 3.weeks)
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
 
             context 'when advanced iterations exist but cadence changes duration to a smaller one' do
@@ -124,6 +142,8 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
                 expect(next_iteration2.reload.start_date).to eq(next_iteration1.due_date + 1.day)
                 expect(next_iteration2.reload.due_date).to eq(next_iteration1.due_date + 1.week)
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
 
             context 'when cadence start date changes to a future date with existing iteration' do
@@ -136,6 +156,8 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
               it 'creates new iterations' do
                 expect { subject }.to change(Iteration, :count).by(2)
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
 
             context 'when cadence has iterations but all are in the past' do
@@ -176,6 +198,8 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
 
                 expect(group.reload.iterations.order(:start_date).map(&:state)).to eq(%w[closed closed current upcoming upcoming])
               end
+
+              it_behaves_like "creating iterations with sequences"
             end
           end
         end

spec/migrations/20211130165043_backfill_sequence_column_for_sprints_table_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe BackfillSequenceColumnForSprintsTable, :migration, schema: 20211126042235 do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:sprints) { table(:sprints) }
+  let(:iterations_cadences) { table(:iterations_cadences) }
+
+  let!(:group) { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:cadence_1) { iterations_cadences.create!(group_id: group.id, title: "cadence 1") }
+  let!(:cadence_2) { iterations_cadences.create!(group_id: group.id, title: "cadence 2") }
+  let!(:iteration_1) { sprints.create!(id: 1, group_id: group.id, iterations_cadence_id: cadence_1.id, start_date: Date.new(2021, 11, 1), due_date: Date.new(2021, 11, 5), iid: 1, title: 'a' ) }
+  let!(:iteration_2) { sprints.create!(id: 2, group_id: group.id, iterations_cadence_id: cadence_1.id, start_date: Date.new(2021, 12, 1), due_date: Date.new(2021, 12, 5), iid: 2, title: 'b') }
+  let!(:iteration_3) { sprints.create!(id: 3, group_id: group.id, iterations_cadence_id: cadence_2.id, start_date: Date.new(2021, 12, 1), due_date: Date.new(2021, 12, 5), iid: 4, title: 'd') }
+  let!(:iteration_4) { sprints.create!(id: 4, group_id: group.id, iterations_cadence_id: nil, start_date: Date.new(2021, 11, 15), due_date: Date.new(2021, 11, 20), iid: 3, title: 'c') }
+
+  describe '#up' do
+    it "correctly sets the sequence attribute with idempotency" do
+      migration.up
+
+      expect(iteration_1.reload.sequence).to be 1
+      expect(iteration_2.reload.sequence).to be 2
+      expect(iteration_3.reload.sequence).to be 1
+      expect(iteration_4.reload.sequence).to be nil
+
+      iteration_5 = sprints.create!(id: 5, group_id: group.id, iterations_cadence_id: cadence_1.id, start_date: Date.new(2022, 1, 1), due_date: Date.new(2022, 1, 5), iid: 1, title: 'e' )
+
+      migration.down
+      migration.up
+
+      expect(iteration_1.reload.sequence).to be 1
+      expect(iteration_2.reload.sequence).to be 2
+      expect(iteration_5.reload.sequence).to be 3
+      expect(iteration_3.reload.sequence).to be 1
+      expect(iteration_4.reload.sequence).to be nil
+    end
+  end
+end