Merge branch 'fix-persisting-multiple-vsa-default-stages' into 'master'

Fix persisting multiple default VSA stages

See merge request gitlab-org/gitlab!38072

db/migrate/20200728080250_replace_unique_index_on_cycle_analytics_stages.rb

+# frozen_string_literal: true
+
+class ReplaceUniqueIndexOnCycleAnalyticsStages < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  OLD_INDEX_NAME = 'index_analytics_ca_group_stages_on_group_id_and_name'
+  NEW_INDEX_NAME = 'index_group_stages_on_group_id_group_value_stream_id_and_name'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:analytics_cycle_analytics_group_stages,
+                         [:group_id, :group_value_stream_id, :name],
+                         unique: true,
+                         name: NEW_INDEX_NAME)
+
+    remove_concurrent_index_by_name :analytics_cycle_analytics_group_stages, OLD_INDEX_NAME
+  end
+
+  def down
+    # Removing duplicated records (group_id, name) that would prevent re-creating the old index.
+    execute <<-SQL
+      DELETE FROM analytics_cycle_analytics_group_stages
+      USING (
+        SELECT group_id, name, MIN(id) as min_id
+        FROM analytics_cycle_analytics_group_stages
+        GROUP BY group_id, name
+        HAVING COUNT(id) > 1
+      ) as analytics_cycle_analytics_group_stages_name_duplicates
+      WHERE analytics_cycle_analytics_group_stages_name_duplicates.group_id = analytics_cycle_analytics_group_stages.group_id
+      AND analytics_cycle_analytics_group_stages_name_duplicates.name = analytics_cycle_analytics_group_stages.name
+      AND analytics_cycle_analytics_group_stages_name_duplicates.min_id <> analytics_cycle_analytics_group_stages.id
+    SQL
+
+    add_concurrent_index(:analytics_cycle_analytics_group_stages,
+                         [:group_id, :name],
+                         unique: true,
+                         name: OLD_INDEX_NAME)
+
+    remove_concurrent_index_by_name :analytics_cycle_analytics_group_stages, NEW_INDEX_NAME
+  end
+end

db/schema_migrations/20200728080250

+546555a009e8923ea8b976ce38d882d387407fb03e7bbcb9c760df53bafd1f91
\ No newline at end of file

db/structure.sql

@@ -18880,8 +18880,6 @@ CREATE INDEX index_analytics_ca_group_stages_on_end_event_label_id ON public.ana
 
 CREATE INDEX index_analytics_ca_group_stages_on_group_id ON public.analytics_cycle_analytics_group_stages USING btree (group_id);
 
-CREATE UNIQUE INDEX index_analytics_ca_group_stages_on_group_id_and_name ON public.analytics_cycle_analytics_group_stages USING btree (group_id, name);
-
 CREATE INDEX index_analytics_ca_group_stages_on_relative_position ON public.analytics_cycle_analytics_group_stages USING btree (relative_position);
 
 CREATE INDEX index_analytics_ca_group_stages_on_start_event_label_id ON public.analytics_cycle_analytics_group_stages USING btree (start_event_label_id);
@@ -19654,6 +19652,8 @@ CREATE INDEX index_group_group_links_on_shared_with_group_id ON public.group_gro
 
 CREATE INDEX index_group_import_states_on_group_id ON public.group_import_states USING btree (group_id);
 
+CREATE UNIQUE INDEX index_group_stages_on_group_id_group_value_stream_id_and_name ON public.analytics_cycle_analytics_group_stages USING btree (group_id, group_value_stream_id, name);
+
 CREATE UNIQUE INDEX index_group_wiki_repositories_on_disk_path ON public.group_wiki_repositories USING btree (disk_path);
 
 CREATE INDEX index_group_wiki_repositories_on_shard_id ON public.group_wiki_repositories USING btree (shard_id);

ee/app/models/analytics/cycle_analytics/group_stage.rb

@@ -6,6 +6,7 @@ module Analytics
       include Analytics::CycleAnalytics::Stage
 
       validates :group, presence: true
+      validates :name, uniqueness: { scope: [:group_id, :group_value_stream_id] }
       belongs_to :group
       belongs_to :value_stream, class_name: 'Analytics::CycleAnalytics::GroupValueStream', foreign_key: :group_value_stream_id
 

ee/app/services/analytics/cycle_analytics/stages/base_service.rb

@@ -39,7 +39,7 @@ module Analytics
         end
 
         def persist_default_stages!
-          persisted_default_stages = parent.cycle_analytics_stages.default_stages
+          persisted_default_stages = parent.cycle_analytics_stages.by_value_stream(value_stream).default_stages
 
           # make sure that we persist default stages only once
           stages_to_persist = build_default_stages.select do |new_default_stage|

ee/changelogs/unreleased/fix-persisting-multiple-vsa-default-stages.yml

+---
+title: Fix persisting multiple default Value Stream Analytics stages per value stream
+merge_request: 38072
+author:
+type: fixed

ee/spec/models/analytics/cycle_analytics/group_stage_spec.rb

@@ -3,6 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Analytics::CycleAnalytics::GroupStage do
+  describe 'uniqueness validation on name' do
+    subject { build(:cycle_analytics_group_stage) }
+
+    it { is_expected.to validate_uniqueness_of(:name).scoped_to([:group_id, :group_value_stream_id]) }
+  end
+
   describe 'associations' do
     it { is_expected.to belong_to(:group) }
     it { is_expected.to belong_to(:value_stream) }

ee/spec/services/analytics/cycle_analytics/stages/create_service_spec.rb

@@ -52,8 +52,8 @@ RSpec.describe Analytics::CycleAnalytics::Stages::CreateService do
   end
 
   describe 'persistence of default stages' do
-    let(:persisted_stages) { group.cycle_analytics_stages }
-    let(:customized_stages) { group.cycle_analytics_stages.where(custom: true) }
+    let(:persisted_stages) { value_stream.stages }
+    let(:customized_stages) { value_stream.stages.where(custom: true) }
     let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages.all }
     let(:expected_stage_count) { default_stages.count + customized_stages.count }
 
@@ -81,6 +81,19 @@ RSpec.describe Analytics::CycleAnalytics::Stages::CreateService do
           expect(group.cycle_analytics_stages.count).to eq(expected_stage_count)
         end
       end
+
+      context 'when creating a stage for the second value stream' do
+        before do
+          first_value_stream = create(:cycle_analytics_group_value_stream, group: group)
+          described_class.new(parent: group, params: params.merge(name: 'other stage', value_stream: first_value_stream), current_user: user).execute
+        end
+
+        it 'persists the new stage and the default stages for the second value streams' do
+          subject
+
+          expect(value_stream.stages.count).to eq(Gitlab::Analytics::CycleAnalytics::DefaultStages.all.size + 1)
+        end
+      end
     end
 
     context 'when params are invalid' do

spec/migrations/20200728080250_replace_unique_index_on_cycle_analytics_stages_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200728080250_replace_unique_index_on_cycle_analytics_stages.rb')
+
+RSpec.describe ReplaceUniqueIndexOnCycleAnalyticsStages, :migration, schema: 20200728080250 do
+  let(:namespaces) { table(:namespaces) }
+  let(:group_value_streams) { table(:analytics_cycle_analytics_group_value_streams) }
+  let(:group_stages) { table(:analytics_cycle_analytics_group_stages) }
+
+  let(:group) { namespaces.create!(type: 'Group', name: 'test', path: 'test') }
+
+  let(:value_stream_1) { group_value_streams.create!(group_id: group.id, name: 'vs1') }
+  let(:value_stream_2) { group_value_streams.create!(group_id: group.id, name: 'vs2') }
+
+  let(:duplicated_stage_1) { group_stages.create!(group_id: group.id, group_value_stream_id: value_stream_1.id, name: 'stage', start_event_identifier: 1, end_event_identifier: 1) }
+  let(:duplicated_stage_2) { group_stages.create!(group_id: group.id, group_value_stream_id: value_stream_2.id, name: 'stage', start_event_identifier: 1, end_event_identifier: 1) }
+
+  let(:stage_record) { group_stages.create!(group_id: group.id, group_value_stream_id: value_stream_2.id, name: 'other stage', start_event_identifier: 1, end_event_identifier: 1) }
+
+  describe '#down' do
+    subject { described_class.new.down }
+
+    before do
+      described_class.new.up
+
+      duplicated_stage_1
+      duplicated_stage_2
+      stage_record
+    end
+
+    it 'removes duplicated stage records' do
+      subject
+
+      stage = group_stages.find_by_id(duplicated_stage_2.id)
+      expect(stage).to be_nil
+    end
+
+    it 'does not change the first duplicated stage record' do
+      expect { subject }.not_to change { duplicated_stage_1.reload.attributes }
+    end
+
+    it 'does not change not duplicated stage record' do
+      expect { subject }.not_to change { stage_record.reload.attributes }
+    end
+  end
+end