Move setting iteration cadence to service

Setting iteration cadence, looks up an existing cadence within
the group and if none found creates one, this happens in a
transaction so that it is threadsafe. So we are moving this
out of the before_validation callback to avoid subtransactions.

ee/app/models/ee/iteration.rb

@@ -48,7 +48,6 @@ module EE
       validate :validate_group
       validate :uniqueness_of_title, if: :title_changed?
 
-      before_validation :set_iterations_cadence, unless: -> { project_id.present? }
       before_save :set_iteration_state
       before_destroy :check_if_can_be_destroyed
 
@@ -174,6 +173,22 @@ module EE
       (due_date - start_date + 1).to_i
     end
 
+    # TODO: this method should be removed as part of https://gitlab.com/gitlab-org/gitlab/-/issues/296099
+    def set_iterations_cadence
+      return if iterations_cadence
+      # For now we support only group iterations
+      # issue to clarify project iterations: https://gitlab.com/gitlab-org/gitlab/-/issues/299864
+      return unless group
+
+      # we need this as we use the cadence to validate the dates overlap for this iteration,
+      # so in the case this runs before background migration we need to first set all iterations
+      # in this group to a cadence before we can validate the dates overlap.
+      default_cadence = find_or_create_default_cadence
+      group.iterations.where(iterations_cadence_id: nil).update_all(iterations_cadence_id: default_cadence.id)
+
+      self.iterations_cadence = default_cadence
+    end
+
     private
 
     def last_iteration_in_cadence?
@@ -236,22 +251,6 @@ module EE
       self.state = self.class.compute_state(start_date, due_date)
     end
 
-    # TODO: this method should be removed as part of https://gitlab.com/gitlab-org/gitlab/-/issues/296099
-    def set_iterations_cadence
-      return if iterations_cadence
-      # For now we support only group iterations
-      # issue to clarify project iterations: https://gitlab.com/gitlab-org/gitlab/-/issues/299864
-      return unless group
-
-      # we need this as we use the cadence to validate the dates overlap for this iteration,
-      # so in the case this runs before background migration we need to first set all iterations
-      # in this group to a cadence before we can validate the dates overlap.
-      default_cadence = find_or_create_default_cadence
-      group.iterations.where(iterations_cadence_id: nil).update_all(iterations_cadence_id: default_cadence.id)
-
-      self.iterations_cadence = default_cadence
-    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/services/iterations/create_service.rb

@@ -18,6 +18,7 @@ module Iterations
           parent.feature_available?(:iterations) && can?(current_user, :create_iteration, parent)
 
       iteration = parent.iterations.new(params)
+      iteration.set_iterations_cadence
 
       if iteration.save
         ::ServiceResponse.success(message: _('New iteration created'), payload: { iteration: iteration })

ee/lib/bulk_imports/groups/pipelines/iterations_pipeline.rb

@@ -16,7 +16,7 @@ module BulkImports
 
           raise ::BulkImports::Pipeline::NotAllowedError unless authorized?
 
-          context.group.iterations.create!(data)
+          ::Iterations::CreateService.new(context.group, context.current_user, data).execute
         end
 
         private

ee/spec/factories/iterations.rb

@@ -11,6 +11,7 @@ FactoryBot.define do
     due_date { generate(:sequential_date) }
 
     transient do
+      iterations_cadence { nil }
       project { nil }
       group { nil }
       project_id { nil }
@@ -57,6 +58,13 @@ FactoryBot.define do
       else
         iteration.group = create(:group)
       end
+
+      if evaluator.iterations_cadence.present?
+        iteration.iterations_cadence = evaluator.iterations_cadence
+      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
+      end
     end
 
     factory :upcoming_iteration, traits: [:upcoming]

ee/spec/models/ee/iteration_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe Iteration do
   let(:set_cadence) { nil }
 
   let_it_be(:group) { create(:group) }
+  let_it_be(:iteration_cadence) { create(:iterations_cadence, group: group) }
   let_it_be(:project) { create(:project, group: group) }
 
   describe "#iid" do
@@ -36,61 +37,6 @@ RSpec.describe Iteration do
     end
   end
 
-  describe 'setting iteration cadence' do
-    let_it_be(:iterations_cadence) { create(:iterations_cadence, group: group, start_date: 10.days.ago) }
-
-    let(:iteration) { create(:iteration, group: group, iterations_cadence: set_cadence, start_date: 2.days.from_now) }
-
-    context 'when iterations_cadence is set correctly' do
-      let(:set_cadence) { iterations_cadence}
-
-      it 'does not change the iterations_cadence' do
-        expect(iteration.iterations_cadence).to eq(iterations_cadence)
-      end
-    end
-
-    context 'when iterations_cadence exists for the group' do
-      let(:set_cadence) { nil }
-
-      it 'sets the iterations_cadence to the existing record' do
-        expect(iteration.iterations_cadence).to eq(iterations_cadence)
-      end
-    end
-
-    context 'when iterations_cadence does not exists for the group' do
-      let_it_be(:group) { create(:group, name: 'Test group')}
-
-      let(:iteration) { build(:iteration, group: group, iterations_cadence: set_cadence) }
-
-      it 'creates a default iterations_cadence and uses it for the iteration' do
-        expect { iteration.save! }.to change { Iterations::Cadence.count }.by(1)
-      end
-
-      it 'sets the newly created iterations_cadence to the record' do
-        iteration.save!
-
-        expect(iteration.iterations_cadence).to eq(Iterations::Cadence.last)
-      end
-
-      it 'creates the iterations_cadence with the correct attributes' do
-        iteration.save!
-
-        cadence = Iterations::Cadence.last
-
-        expect(cadence.reload.start_date).to eq(iteration.start_date)
-        expect(cadence.title).to eq('Test group Iterations')
-      end
-    end
-
-    context 'when iteration is a project iteration' do
-      it 'does not set the iterations_cadence' do
-        iteration = create(:iteration, iterations_cadence: nil, project: project, skip_project_validation: true)
-
-        expect(iteration.reload.iterations_cadence).to be_nil
-      end
-    end
-  end
-
   describe '.reference_pattern' do
     subject { described_class.reference_pattern }
 
@@ -190,7 +136,7 @@ RSpec.describe Iteration do
   end
 
   context 'Validations' do
-    subject { build(:iteration, group: group, start_date: start_date, due_date: due_date) }
+    subject { build(:iteration, group: group, iterations_cadence: iteration_cadence, start_date: start_date, due_date: due_date) }
 
     describe 'when iteration belongs to project' do
       subject { build(:iteration, project: project, start_date: Time.current, due_date: 1.day.from_now) }
@@ -202,7 +148,7 @@ RSpec.describe Iteration do
     end
 
     describe '#dates_do_not_overlap' do
-      let_it_be(:existing_iteration) { create(:iteration, group: group, start_date: 4.days.from_now, due_date: 1.week.from_now) }
+      let_it_be(:existing_iteration) { create(:iteration, group: group, iterations_cadence: iteration_cadence, start_date: 4.days.from_now, due_date: 1.week.from_now) }
 
       context 'when no Iteration dates overlap' do
         let(:start_date) { 2.weeks.from_now }
@@ -273,6 +219,7 @@ RSpec.describe Iteration do
 
           context 'different group' do
             let(:group) { create(:group) }
+            let(:iteration_cadence) { create(:iterations_cadence, group: group) }
 
             it { is_expected.to be_valid }
 
@@ -283,8 +230,9 @@ RSpec.describe Iteration do
 
           context 'sub-group' do
             let(:subgroup) { create(:group, parent: group) }
+            let(:subgroup_ic) { create(:iterations_cadence, group: subgroup) }
 
-            subject { build(:iteration, group: subgroup, start_date: start_date, due_date: due_date) }
+            subject { build(:iteration, group: subgroup, iterations_cadence: subgroup_ic, start_date: start_date, due_date: due_date) }
 
             it { is_expected.to be_valid }
           end

ee/spec/models/ee/list_spec.rb

@@ -64,7 +64,7 @@ RSpec.describe List do
   end
 
   context 'when it is an iteration type' do
-    let(:iteration) { build(:iteration, title: 'awesome-iteration') }
+    let(:iteration) { build(:iteration, title: 'awesome-iteration', group: create(:group)) }
 
     subject { described_class.new(list_type: :iteration, iteration: iteration, board: board) }
 

ee/spec/requests/api/graphql/mutations/iterations/update_spec.rb

@@ -7,7 +7,8 @@ RSpec.describe 'Updating an Iteration' do
 
   let_it_be(:current_user) { create(:user) }
   let_it_be(:group) { create(:group) }
-  let_it_be(:iteration) { create(:iteration, group: group) }
+  let_it_be(:cadence) { create(:iterations_cadence, group: group) }
+  let_it_be(:iteration) { create(:iteration, group: group, iterations_cadence: cadence) }
 
   let(:start_date) { 1.day.from_now.strftime('%F') }
   let(:end_date) { 5.days.from_now.strftime('%F') }
@@ -106,7 +107,7 @@ RSpec.describe 'Updating an Iteration' do
         end
 
         context 'when another iteration with given dates overlap' do
-          let_it_be(:another_iteration) { create(:iteration, group: group, start_date: start_date.strftime('%F'), due_date: end_date.strftime('%F') ) }
+          let_it_be(:another_iteration) { create(:iteration, group: group, iterations_cadence: cadence, start_date: start_date.strftime('%F'), due_date: end_date.strftime('%F') ) }
 
           it_behaves_like 'a mutation that returns errors in the response',
                           errors: ["Dates cannot overlap with other existing Iterations within this iterations cadence"]

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

@@ -9,7 +9,7 @@ RSpec.describe Iterations::Cadences::DestroyService do
   let_it_be(:project) { create(:project, :repository, group: group) }
   let_it_be(:user) { create(:user) }
   let_it_be(:iteration_cadence, refind: true) { create(:iterations_cadence, group: group, start_date: Date.today, duration_in_weeks: 1, iterations_in_advance: 2) }
-  let_it_be(:iteration) { create(:current_iteration, group: group, start_date: 2.days.ago, due_date: 5.days.from_now) }
+  let_it_be(:iteration) { create(:current_iteration, group: group, iterations_cadence: iteration_cadence, start_date: 2.days.ago, due_date: 5.days.from_now) }
   let_it_be(:iteration_list, refind: true) { create(:iteration_list, iteration: iteration) }
   let_it_be(:iteration_event, refind: true) { create(:resource_iteration_event, iteration: iteration) }
   let_it_be(:board) { create(:board, iteration: iteration, group: group) }

ee/spec/services/iterations/create_service_spec.rb

@@ -95,8 +95,55 @@ RSpec.describe Iterations::CreateService do
   end
 
   context 'for groups' do
-    let_it_be(:parent, refind: true) { create(:group) }
+    let_it_be(:group) { create(:group) }
 
-    it_behaves_like 'iterations create service'
+    context 'group without cadences' do
+      let_it_be(:parent, refind: true) { group }
+
+      it_behaves_like 'iterations create service'
+    end
+
+    context 'group with a cadence' do
+      let_it_be(:cadence) { create(:iterations_cadence, group: group) }
+      let_it_be(:parent, refind: true) { group }
+
+      it_behaves_like 'iterations create service'
+    end
+
+    context 'group with multiple cadences' do
+      let_it_be(:cadence) { create_list(:iterations_cadence, 2, group: group) }
+      let_it_be(:parent, refind: true) { group }
+
+      it_behaves_like 'iterations create service'
+
+      context 'with specific cadence being passed as param' do
+        let_it_be(:user) { create(:user) }
+
+        let(:params) do
+          {
+            title: 'v2.1.9',
+            description: 'Patch release to fix security issue',
+            start_date: Time.current.to_s,
+            due_date: 1.day.from_now.to_s,
+            iterations_cadence_id: group.iterations_cadences.last.id
+          }
+        end
+
+        let(:response) { described_class.new(parent, user, params).execute }
+        let(:iteration) { response.payload[:iteration] }
+
+        before do
+          parent.add_developer(user)
+        end
+
+        context 'valid params' do
+          it 'creates an iteration' do
+            expect(response.success?).to be_truthy
+            expect(iteration).to be_persisted
+            expect(iteration.iterations_cadence_id).to eq(group.iterations_cadences.last.id)
+          end
+        end
+      end
+    end
   end
 end