Merge branch '353350-disallow-manual-cadence-creation' into 'master'

Prevent creation or conversion into manual iteration cadences

See merge request gitlab-org/gitlab!82352

ee/app/models/ee/iteration.rb

@@ -221,7 +221,8 @@ 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
+    # TODO: this method should be removed when manual iteration management is removed.
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/356069 the deprecation issue.
     def set_iterations_cadence
       return if iterations_cadence
       # For now we support only group iterations
@@ -307,19 +308,25 @@ module EE
       iterations_cadence.update_iteration_sequences
     end
 
-    # TODO: this method should be removed as part of https://gitlab.com/gitlab-org/gitlab/-/issues/296099
+    # TODO: this method should be removed when manual iteration management is removed.
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/356069 the deprecation issue.
     def find_or_create_default_cadence
+      default_cadence = ::Iterations::Cadence.order(id: :asc).find_by(group: group, automatic: false)
+      return default_cadence if default_cadence
+
       cadence_title = "#{group.name} Iterations"
       start_date = self.start_date || Date.today
 
-      ::Iterations::Cadence.create_with(
+      # We need to skip validation as manual cadence creation is deprecated and not allowed.
+      # A manual cadence is created here so the iterations feature is not affected during the deprecation period.
+      ::Iterations::Cadence.new(
+        group: group,
         title: cadence_title,
         start_date: start_date,
         automatic: false,
-        # set to 0, i.e. unspecified when creating default iterations as we do validate for presence.
-        iterations_in_advance: 0,
-        duration_in_weeks: 0
-      ).order(id: :asc).safe_find_or_create_by!(group: group)
+        iterations_in_advance: 2,
+        duration_in_weeks: 2
+      ).tap { |new_cadence| new_cadence.save!(validate: false) }
     end
 
     # TODO: remove this as part of https://gitlab.com/gitlab-org/gitlab/-/issues/296100

ee/app/models/iterations/cadence.rb

@@ -23,6 +23,8 @@ module Iterations
     validates :automatic, inclusion: [true, false]
     validates :description, length: { maximum: 5000 }
 
+    validate :cadence_is_automatic
+
     after_commit :ensure_iterations_in_advance, on: [:create, :update], if: :changed_iterations_automation_fields?
 
     scope :with_groups, -> (group_ids) { where(group_id: group_ids) }
@@ -84,5 +86,15 @@ module Iterations
         WHERE t.id=sprints.id AND (sprints.sequence IS DISTINCT FROM t.row_number)
       SQL
     end
+
+    private
+
+    def cadence_is_automatic
+      return unless changes.key?(:automatic)
+
+      unless automatic?
+        errors.add(:base, _('Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed.'))
+      end
+    end
   end
 end

ee/spec/features/groups/iterations/user_creates_iteration_in_cadence_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe 'User creates iteration in a cadence', :js do
   let_it_be(:now) { Time.zone.now }
   let_it_be(:group) { create(:group) }
   let_it_be(:user) { create(:group_member, :maintainer, user: create(:user), group: group ).user }
-  let_it_be(:cadence) { create(:iterations_cadence, group: group, automatic: false, duration_in_weeks: 0) }
+  let_it_be(:cadence) { build(:iterations_cadence, group: group, automatic: false, duration_in_weeks: 0).tap { |cadence| cadence.save!(validate: false) } }
 
   before do
     stub_licensed_features(iterations: true)

ee/spec/finders/iterations/cadences_finder_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Iterations::CadencesFinder do
   let_it_be(:automatic_iterations_cadence) { create(:iterations_cadence, group: group, automatic: true, duration_in_weeks: 1, title: 'one week iterations') }
   let_it_be(:active_sub_group_iterations_cadence) { create(:iterations_cadence, group: sub_group, active: true, duration_in_weeks: 1, title: 'one week iterations') }
   let_it_be(:inactive_sub_group_iterations_cadence) { create(:iterations_cadence, group: sub_group, active: false, duration_in_weeks: 2, title: 'two weeks iterations') }
-  let_it_be(:non_automatic_sub_group_iterations_cadence) { create(:iterations_cadence, group: sub_group, automatic: false, duration_in_weeks: 1, title: 'one week iterations') }
+  let_it_be(:non_automatic_sub_group_iterations_cadence) { build(:iterations_cadence, group: sub_group, automatic: false, duration_in_weeks: 1, title: 'one week iterations').tap { |cadence| cadence.save!(validate: false) } }
   let_it_be(:current_group) { group }
 
   subject { described_class.new(user, current_group, params).execute }

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

@@ -32,6 +32,41 @@ RSpec.describe ::Iterations::Cadence do
 
       it { is_expected.not_to validate_presence_of(:start_date) }
     end
+
+    describe 'cadence_is_automatic' do
+      context 'when creating a new cadence' do
+        it 'does not allow the creation of manul cadences' do
+          cadence = build(:iterations_cadence, automatic: false).tap { |cadence| cadence.valid? }
+
+          expect(cadence.errors.full_messages).to include(_('Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed.'))
+        end
+      end
+
+      context 'when cadence already existed as manual' do
+        let_it_be(:manual_cadence, refind: true) { build(:iterations_cadence).tap { |cadence| cadence.save!(validate: false) } }
+
+        context 'when `automatic` is not updated' do
+          it 'allows to change other attributes' do
+            manual_cadence.assign_attributes(duration_in_weeks: 2, iterations_in_advance: 4)
+
+            expect(manual_cadence).to be_valid
+          end
+        end
+      end
+
+      context 'when cadence already existed as automatic' do
+        let_it_be(:automatic_cadence, refind: true) { create(:iterations_cadence) }
+
+        context 'when changing a cadence to manual' do
+          it 'adds a validation error' do
+            automatic_cadence.assign_attributes(duration_in_weeks: 2, iterations_in_advance: 4, automatic: false)
+
+            expect(automatic_cadence).to be_invalid
+            expect(automatic_cadence.errors.full_messages).to include(_('Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed.'))
+          end
+        end
+      end
+    end
   end
 
   describe '#update_iteration_sequences', :aggregate_failures do

ee/spec/requests/api/graphql/mutations/iterations/cadences/create_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe 'Creating an iteration cadence' do
       start_date: start_date,
       duration_in_weeks: 1,
       iterations_in_advance: 1,
-      automatic: false,
+      automatic: true,
       active: false,
       roll_over: true,
       description: 'Iteration cadence description'
@@ -79,13 +79,10 @@ RSpec.describe 'Creating an iteration cadence' do
       end
 
       context 'when creating a manual iteration cadence' do
-        context 'when start date is not provided' do
         let(:attributes) { { title: 'automatic cadence', duration_in_weeks: 1, active: true, automatic: false } }
 
-          it 'creates an iteration cadence' do
-            expect { post_graphql_mutation(mutation, current_user: current_user) }.to change(Iterations::Cadence, :count).by(1)
-          end
-        end
+        it_behaves_like 'a mutation that returns errors in the response',
+                        errors: [_('Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed.')]
       end
 
       context 'when iteration_cadences feature flag is disabled' do

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

@@ -17,7 +17,6 @@ RSpec.describe 'Updating an iteration cadence' do
       start_date: start_date,
       duration_in_weeks: 1,
       iterations_in_advance: 1,
-      automatic: false,
       active: false,
       roll_over: true,
       description: description
@@ -80,7 +79,7 @@ RSpec.describe 'Updating an iteration cadence' do
         expect(iteration_cadence_hash['startDate'].to_date).to eq(start_date.to_date)
         expect(iteration_cadence_hash['durationInWeeks']).to eq(1)
         expect(iteration_cadence_hash['iterationsInAdvance']).to eq(1)
-        expect(iteration_cadence_hash['automatic']).to eq(false)
+        expect(iteration_cadence_hash['automatic']).to eq(true)
         expect(iteration_cadence_hash['active']).to eq(false)
         expect(iteration_cadence_hash['rollOver']).to eq(true)
         expect(iteration_cadence_hash['description']).to eq(description)
@@ -90,7 +89,7 @@ RSpec.describe 'Updating an iteration cadence' do
         expect(iteration_cadence.start_date).to eq(start_date.to_date)
         expect(iteration_cadence.duration_in_weeks).to eq(1)
         expect(iteration_cadence.iterations_in_advance).to eq(1)
-        expect(iteration_cadence.automatic).to eq(false)
+        expect(iteration_cadence.automatic).to eq(true)
         expect(iteration_cadence.active).to eq(false)
         expect(iteration_cadence.roll_over).to eq(true)
         expect(iteration_cadence.description).to eq(description)

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

@@ -7,7 +7,7 @@ RSpec.describe 'Creating an Iteration' do
 
   let_it_be(:current_user) { create(:user) }
   let_it_be(:group) { create(:group) }
-  let_it_be(:cadence) { create(:iterations_cadence, group: group)}
+  let_it_be(:cadence) { build(:iterations_cadence, group: group, automatic: false).tap { |cadence| cadence.save!(validate: false) } }
 
   let(:start_date) { Time.now.strftime('%F') }
   let(:end_date) { 1.day.from_now.strftime('%F') }

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

@@ -6,7 +6,7 @@ RSpec.describe Iterations::Cadences::CreateIterationsInAdvanceService do
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group) }
   let_it_be(:inactive_cadence) { create(:iterations_cadence, group: group, active: false, automatic: true, start_date: 2.weeks.ago) }
-  let_it_be(:manual_cadence) { create(:iterations_cadence, group: group, active: true, automatic: false, start_date: 2.weeks.ago) }
+  let_it_be(:manual_cadence) { build(:iterations_cadence, group: group, active: true, automatic: false, start_date: 2.weeks.ago).tap { |cadence| cadence.save!(validate: false) } }
   let_it_be_with_reload(:automated_cadence) { create(:iterations_cadence, group: group, active: true, automatic: true, start_date: 2.weeks.ago) }
 
   subject { described_class.new(user, cadence).execute }

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

@@ -50,36 +50,13 @@ RSpec.describe Iterations::Cadences::CreateService do
         end
 
         context 'create manual cadence' do
-          context 'when duration_in_weeks: nil, start_date: nil and iterations_in_advance: nil' do
           before do
             params.merge!(automatic: false, duration_in_weeks: nil, iterations_in_advance: nil, start_date: nil)
           end
 
-            it 'creates an iteration cadence' do
-              expect(response).to be_success
-              expect(iteration_cadence).to be_persisted
-              expect(iteration_cadence.title).to eq('My iteration cadence')
-              expect(iteration_cadence.duration_in_weeks).to be_nil
-              expect(iteration_cadence.iterations_in_advance).to be_nil
-              expect(iteration_cadence.active).to eq(true)
-              expect(iteration_cadence.automatic).to eq(false)
-              expect(iteration_cadence.start_date).to be_nil
-            end
-          end
-
-          context 'with out list of values for duration_in_weeks, iterations_in_advance' do
-            before do
-              params.merge!(automatic: false, duration_in_weeks: 15, iterations_in_advance: 15)
-            end
-
-            it 'does not create an iteration cadence but returns errors' do
-              expect(response.error?).to be_truthy
-              expect(errors).to match([
-                "Duration in weeks is not included in the list",
-                "Iterations in advance is not included in the list"
-              ])
-            end
-          end
+          it_behaves_like 'does not create an interation cadence', [
+            _('Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed.')
+          ]
         end
 
         context 'create automatic cadence' do

ee/spec/services/iterations/create_service_spec.rb

@@ -147,8 +147,9 @@ RSpec.describe Iterations::CreateService do
       context 'when iteration_cadences FF is disabled' do
         let_it_be(:user) { create(:user) }
         let_it_be(:group) { create(:group) }
-        let_it_be(:cadences) { create_list(:iterations_cadence, 2, group: group) }
-        let_it_be(:other_iteration) { create(:iteration, iterations_cadence: cadences.second) }
+        let_it_be(:first_legacy_cadence) { build(:iterations_cadence, group: group, automatic: false).tap { |cadence| cadence.save!(validate: false) } }
+        let_it_be(:automatic_cadence) { create(:iterations_cadence, group: group) }
+        let_it_be(:other_iteration) { create(:iteration, iterations_cadence: automatic_cadence) }
         let_it_be(:parent, refind: true) { group }
 
         let(:params) { base_params }
@@ -163,12 +164,12 @@ RSpec.describe Iterations::CreateService do
           expect(response).to be_success
           expect(saved_iteration).to be_persisted
           expect(saved_iteration.title).to eq('v2.1.9')
-          expect(saved_iteration.iterations_cadence_id).to eq(ordered_cadences.first.id)
+          expect(saved_iteration.iterations_cadence_id).to eq(first_legacy_cadence.id)
         end
 
         it 'does not update the iterations from the non-default cadences' do
           expect(response).to be_success
-          expect(other_iteration.iterations_cadence_id).to eq(ordered_cadences.second.id)
+          expect(other_iteration.iterations_cadence_id).to eq(automatic_cadence.id)
         end
       end
     end

ee/spec/workers/iterations/roll_over_issues_worker_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Iterations::RollOverIssuesWorker do
   describe '#perform' do
     context 'when iteration cadence is not automatic' do
       before do
-        cadence1.update!(automatic: false)
+        cadence1.update_columns(automatic: false)
       end
 
       it 'exits early' do

locale/gitlab.pot

@@ -22700,6 +22700,9 @@ msgstr ""
 msgid "Manual"
 msgstr ""
 
+msgid "Manual iteration cadences are deprecated. Only automatic iteration cadences are allowed."
+msgstr ""
+
 msgid "ManualOrdering|Couldn't save the order of the issues"
 msgstr ""