Merge branch 'roll-over-issues' into 'master'

Roll-over issues from one iteration to another

See merge request gitlab-org/gitlab!62350

app/models/issue.rb

@@ -23,6 +23,7 @@ class Issue < ApplicationRecord
   include IssueAvailableFeatures
   include Todoable
   include FromUnion
+  include EachBatch
 
   extend ::Gitlab::Utils::Override
 

app/policies/base_policy.rb

@@ -27,6 +27,10 @@ class BasePolicy < DeclarativePolicy::Base
   with_options scope: :user, score: 0
   condition(:security_bot) { @user&.security_bot? }
 
+  desc "User is automation bot"
+  with_options scope: :user, score: 0
+  condition(:automation_bot) { @user&.automation_bot? }
+
   desc "User email is unconfirmed or user account is locked"
   with_options scope: :user, score: 0
   condition(:inactive) { @user&.confirmation_required_on_sign_in? || @user&.access_locked? }

app/policies/concerns/policy_actor.rb

@@ -53,6 +53,10 @@ module PolicyActor
     false
   end
 
+  def automation_bot?
+    false
+  end
+
   def deactivated?
     false
   end

config/sidekiq_queues.yml

@@ -196,6 +196,8 @@
   - 2
 - - issue_rebalancing
   - 1
+- - iterations
+  - 1
 - - jira_connect
   - 1
 - - jira_importer

ee/app/models/ee/iteration.rb

@@ -22,6 +22,8 @@ module EE
     prepended do
       include AtomicInternalId
       include Timebox
+      include EachBatch
+      include AfterCommitQueue
 
       attr_accessor :skip_future_date_validation
       attr_accessor :skip_project_validation
@@ -64,6 +66,7 @@ module EE
 
       scope :start_date_passed, -> { where('start_date <= ?', Date.current).where('due_date >= ?', Date.current) }
       scope :due_date_passed, -> { where('due_date < ?', Date.current) }
+      scope :with_cadence, -> { preload([iterations_cadence: :group]) }
 
       state_machine :state_enum, initial: :upcoming do
         event :start do
@@ -74,6 +77,12 @@ module EE
           transition [:upcoming, :started] => :closed
         end
 
+        after_transition any => [:closed] do |iteration|
+          iteration.run_after_commit do
+            Iterations::RollOverIssuesWorker.perform_async([iteration.id]) if iteration.iterations_cadence&.can_roll_over?
+          end
+        end
+
         state :upcoming, value: Iteration::STATE_ENUM_MAP[:upcoming]
         state :started, value: Iteration::STATE_ENUM_MAP[:started]
         state :closed, value: Iteration::STATE_ENUM_MAP[:closed]

ee/app/models/iterations/cadence.rb

@@ -50,6 +50,10 @@ module Iterations
       active? && automatic? && duration_in_weeks.to_i > 0 && iterations_in_advance.to_i > 0
     end
 
+    def can_roll_over?
+      active? && automatic? && roll_over?
+    end
+
     def duration_in_days
       duration_in_weeks * 7
     end

ee/app/policies/ee/group_policy.rb

@@ -216,6 +216,10 @@ module EE
         enable :admin_iteration_cadence
       end
 
+      rule { (automation_bot | developer) & iterations_available }.policy do
+        enable :rollover_issues
+      end
+
       rule { reporter & epics_available }.policy do
         enable :create_epic
         enable :admin_epic

ee/app/services/iterations/roll_over_issues_service.rb

+# frozen_string_literal: true
+
+module Iterations
+  class RollOverIssuesService
+    PermissionsError = Class.new(StandardError)
+
+    BATCH_SIZE = 100
+
+    def initialize(user, from_iteration, to_iteration)
+      @user = user
+      @from_iteration = from_iteration
+      @to_iteration = to_iteration
+    end
+
+    def execute
+      return ::ServiceResponse.error(message: _('Operation not allowed'), http_status: 403) unless can_roll_over_issues?
+
+      from_iteration.issues.opened.each_batch(of: BATCH_SIZE) do |issues|
+        add_iteration_events, remove_iteration_events = iteration_events(issues)
+
+        ActiveRecord::Base.transaction do
+          issues.update_all(sprint_id: to_iteration.id, updated_at: rolled_over_at)
+          Gitlab::Database.bulk_insert(ResourceIterationEvent.table_name, remove_iteration_events) # rubocop:disable Gitlab/BulkInsert
+          Gitlab::Database.bulk_insert(ResourceIterationEvent.table_name, add_iteration_events) # rubocop:disable Gitlab/BulkInsert
+        end
+      end
+
+      ::ServiceResponse.success
+    end
+
+    private
+
+    attr_reader :user, :from_iteration, :to_iteration
+
+    def iteration_events(issues)
+      add_iteration_events = []
+      remove_iteration_events = []
+      issues.map do |issue|
+        remove_iteration_events << common_event_attributes(issue).merge({ iteration_id: issue.sprint_id, action: ResourceTimeboxEvent.actions[:remove] })
+        add_iteration_events << common_event_attributes(issue).merge({ iteration_id: to_iteration.id, action: ResourceTimeboxEvent.actions[:add] })
+      end
+
+      [add_iteration_events, remove_iteration_events]
+    end
+
+    def common_event_attributes(issue)
+      {
+        created_at: rolled_over_at,
+        user_id: user.id,
+        issue_id: issue.id
+      }
+    end
+
+    def can_roll_over_issues?
+      user && to_iteration && from_iteration &&
+        !to_iteration.closed? && to_iteration.due_date > rolled_over_at.to_date &&
+        (user.automation_bot? || user.can?(:rollover_issues, to_iteration))
+    end
+
+    def rolled_over_at
+      @rolled_over_at ||= Time.current
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -755,6 +755,15 @@
   :tags:
   - :exclude_from_kubernetes
   - :exclude_from_gitlab_com
+- :name: iterations:iterations_roll_over_issues
+  :worker_name: Iterations::RollOverIssuesWorker
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: personal_access_tokens:personal_access_tokens_groups_policy
   :worker_name: PersonalAccessTokens::Groups::PolicyWorker
   :feature_category: :authentication_and_authorization

ee/app/workers/iterations/roll_over_issues_worker.rb

+# frozen_string_literal: true
+
+module Iterations
+  class RollOverIssuesWorker
+    include ApplicationWorker
+
+    BATCH_SIZE = 1000
+
+    idempotent!
+
+    queue_namespace :iterations
+    feature_category :issue_tracking
+
+    def perform(iteration_ids)
+      Iteration.closed.id_in(iteration_ids).each_batch(of: BATCH_SIZE) do |iterations_batch|
+        iterations_batch.with_cadence.each do |iteration|
+          cadence = iteration.iterations_cadence
+
+          next unless cadence.group.iteration_cadences_feature_flag_enabled? # keep this behind FF for now
+          next unless cadence.can_roll_over?
+
+          new_iteration = cadence.next_open_iteration(iteration.due_date)
+
+          # proactively generate some iterations in advance if no upcoming iteration found
+          # this should help prevent the case where issues roll-over is triggered but
+          # cadence did not yet generate the iterations in advance
+          unless new_iteration
+            response = Iterations::Cadences::CreateIterationsInAdvanceService.new(automation_bot, cadence).execute
+            if response.error?
+              log_error(cadence, iteration, nil, response)
+              next
+            end
+          end
+
+          response = Iterations::RollOverIssuesService.new(automation_bot, iteration, new_iteration).execute
+          log_error(cadence, iteration, new_iteration, response) if response.error?
+        end
+      end
+    end
+
+    private
+
+    def log_error(cadence, from_iteration, to_iteration, response)
+      logger.error(
+        worker: self.class.name,
+        cadence_id: cadence&.id,
+        from_iteration_id: from_iteration&.id,
+        to_iteration_id: to_iteration&.id,
+        group_id: cadence&.group&.id,
+        message: response.message
+      )
+    end
+
+    def automation_bot
+      @automation_bot_id ||= User.automation_bot
+    end
+  end
+end

ee/app/workers/iterations_update_status_worker.rb

@@ -4,6 +4,7 @@ class IterationsUpdateStatusWorker
   include ApplicationWorker
 
   sidekiq_options retry: 3
+  BATCH_SIZE = 1000
 
   idempotent!
 
@@ -18,16 +19,17 @@ class IterationsUpdateStatusWorker
   private
 
   def set_started_iterations
-    Iteration
-      .upcoming
-      .start_date_passed
-      .update_all(state_enum: ::Iteration::STATE_ENUM_MAP[:started])
+    Iteration.upcoming.start_date_passed.each_batch(of: BATCH_SIZE) do |iterations|
+      iterations.update_all(state_enum: ::Iteration::STATE_ENUM_MAP[:started], updated_at: Time.current)
+    end
   end
 
   def set_closed_iterations
-    Iteration
-      .upcoming.or(Iteration.started)
-      .due_date_passed
-      .update_all(state_enum: ::Iteration::STATE_ENUM_MAP[:closed])
+    Iteration.upcoming.or(Iteration.started).due_date_passed.each_batch(of: BATCH_SIZE) do |iterations|
+      closed_iteration_ids = iterations.pluck_primary_key
+      iterations.update_all(state_enum: ::Iteration::STATE_ENUM_MAP[:closed], updated_at: Time.current)
+
+      Iterations::RollOverIssuesWorker.perform_async(closed_iteration_ids)
+    end
   end
 end

ee/spec/models/ee/iteration_spec.rb

@@ -580,4 +580,32 @@ RSpec.describe Iteration do
       end
     end
   end
+
+  context 'when closing iteration' do
+    let_it_be_with_reload(:iteration) { create(:iteration, group: group, start_date: 4.days.from_now, due_date: 1.week.from_now) }
+
+    context 'when cadence roll-over flag enabled' do
+      before do
+        iteration.iterations_cadence.update!(automatic: true, active: true, roll_over: true)
+      end
+
+      it 'triggers roll-over issues worker' do
+        expect(Iterations::RollOverIssuesWorker).to receive(:perform_async).with([iteration.id])
+
+        iteration.close!
+      end
+    end
+
+    context 'when cadence roll-over flag disabled' do
+      before do
+        iteration.iterations_cadence.update!(automatic: true, active: true, roll_over: false)
+      end
+
+      it 'triggers roll-over issues worker' do
+        expect(Iterations::RollOverIssuesWorker).not_to receive(:perform_async)
+
+        iteration.close!
+      end
+    end
+  end
 end

ee/spec/services/iterations/roll_over_issues_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Iterations::RollOverIssuesService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:closed_iteration1) { create(:closed_iteration, group: group) }
+  let_it_be(:closed_iteration2) { create(:closed_iteration, group: group) }
+  let_it_be(:started_iteration) { create(:started_iteration, group: group) }
+  let_it_be(:upcoming_iteration) { create(:upcoming_iteration, group: group) }
+  let_it_be(:open_issues) { create_list(:issue, 5, :opened, iteration: closed_iteration1)}
+  let_it_be(:closed_issues) { create_list(:issue, 5, :closed, iteration: closed_iteration1)}
+
+  let(:from_iteration) { closed_iteration1 }
+  let(:to_iteration) { started_iteration }
+
+  subject { described_class.new(user, from_iteration, to_iteration).execute }
+
+  context 'when from iteration or null iteration or both are nil' do
+    context 'when to iteration is nil' do
+      let(:to_iteration) { nil }
+
+      it { is_expected.to be_error }
+    end
+
+    context 'when from iteration is nil' do
+      let(:from_iteration) { nil }
+
+      it { is_expected.to be_error }
+    end
+
+    context 'when both from_iteration and to_iteration are nil' do
+      let(:from_iteration) { nil }
+      let(:to_iteration) { nil }
+
+      it { is_expected.to be_error }
+    end
+  end
+
+  context 'when iterations are present' do
+    context 'when issues are rolled-over to a closed iteration' do
+      let(:to_iteration) { closed_iteration2 }
+
+      it { is_expected.to be_error }
+    end
+
+    context 'when user does not have permission to roll-over issues' do
+      context 'when user is not a team member' do
+        it { is_expected.to be_error }
+      end
+
+      context 'when user is a bot other than automation bot' do
+        let(:user) { User.security_bot }
+
+        it { is_expected.to be_error }
+      end
+
+      context 'when user is a team member' do
+        before do
+          group.add_reporter(user)
+        end
+
+        it { is_expected.to be_error }
+      end
+    end
+
+    context 'when user has permissions to roll-over issues' do
+      context 'when user is a team member' do
+        before do
+          group.add_developer(user)
+        end
+
+        it { is_expected.not_to be_error }
+      end
+
+      context 'when user is the automation bot' do
+        let(:user) { User.automation_bot }
+
+        it { is_expected.not_to be_error }
+
+        it 'rolls-over issues to next iteration' do
+          expect { subject }.to change { started_iteration.reload.issues }.from([]).to(open_issues)
+                                .and(change { closed_iteration1.reload.issues }.from(open_issues + closed_issues).to(closed_issues) )
+                                .and(change(ResourceIterationEvent, :count).by(10))
+        end
+      end
+    end
+  end
+end

ee/spec/workers/iterations/roll_over_issues_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Iterations::RollOverIssuesWorker do
+  let_it_be(:group1) { create(:group) }
+  let_it_be(:group2) { create(:group) }
+  let_it_be(:cadence1, reload: true) { create(:iterations_cadence, group: group1, roll_over: true, automatic: true) }
+  let_it_be(:cadence2) { create(:iterations_cadence, group: group2, roll_over: true, automatic: true) }
+  let_it_be(:closed_iteration1) { create(:closed_iteration, iterations_cadence: cadence1, group: group1, start_date: 2.weeks.ago, due_date: 1.week.ago) }
+  let_it_be(:closed_iteration2) { create(:closed_iteration, iterations_cadence: cadence2, group: group2, start_date: 2.weeks.ago, due_date: 1.week.ago) }
+  let_it_be(:started_iteration1) { create(:started_iteration, iterations_cadence: cadence1, group: group1, start_date: 2.days.ago, due_date: 5.days.from_now) }
+  let_it_be(:started_iteration2) { create(:upcoming_iteration, iterations_cadence: cadence2, group: group2, start_date: 2.days.ago, due_date: 5.days.from_now) }
+
+  let(:mock_success_service) { double('mock service', execute: ::ServiceResponse.success) }
+  let(:mock_failure_service) { double('mock service', execute: ::ServiceResponse.error(message: 'some error')) }
+
+  subject(:worker) { described_class.new }
+
+  describe '#perform' do
+    context 'when iteration cadence is not automatic' do
+      before do
+        cadence1.update!(automatic: false)
+      end
+
+      it 'exits early' do
+        expect(Iterations::RollOverIssuesService).not_to receive(:new)
+
+        worker.perform(group1.iterations)
+      end
+    end
+
+    context 'when roll-over option on iteration cadence is not enabled' do
+      before do
+        cadence1.update!(roll_over: false)
+      end
+
+      it 'exits early' do
+        expect(Iterations::RollOverIssuesService).not_to receive(:new)
+
+        worker.perform(group1.iterations)
+      end
+    end
+
+    context 'when roll-over option on iteration cadence is enabled' do
+      context 'when service fails to create future iteration' do
+        it 'logs error' do
+          expect(Iterations::RollOverIssuesService).to receive(:new).and_return(mock_failure_service).once
+          expect(worker).to receive(:log_error)
+
+          worker.perform(group1.iterations)
+        end
+      end
+
+      context 'when cadence has upcoming iteration' do
+        it 'filters out any iterations that are not closed' do
+          expect(Iterations::RollOverIssuesService).to receive(:new).and_return(mock_success_service).once
+          expect(Iterations::Cadences::CreateIterationsInAdvanceService).not_to receive(:new)
+          expect(Iteration).to receive(:closed).and_call_original.once
+
+          worker.perform(group1.iterations)
+        end
+      end
+
+      context 'when cadence does not have upcoming iteration' do
+        let_it_be(:group) { create(:group) }
+        let_it_be(:cadence) { create(:iterations_cadence, group: group, roll_over: true, automatic: true) }
+        let_it_be(:closed_iteration) { create(:closed_iteration, iterations_cadence: cadence, group: group, start_date: 2.weeks.ago, due_date: 1.week.ago) }
+
+        it 'creates a new iteration to roll-over issues' do
+          expect(Iterations::RollOverIssuesService).to receive(:new).and_return(mock_success_service).once
+          expect(Iterations::Cadences::CreateIterationsInAdvanceService).to receive(:new).and_return(mock_success_service)
+          expect(Iteration).to receive(:closed).and_call_original.once
+
+          worker.perform(cadence.iterations)
+        end
+
+        context 'when service fails to create future iteration' do
+          it 'logs error and exits early' do
+            expect(Iterations::RollOverIssuesService).not_to receive(:new)
+            expect(Iterations::Cadences::CreateIterationsInAdvanceService).to receive(:new).and_return(mock_failure_service)
+            expect(worker).to receive(:log_error)
+
+            worker.perform(cadence.iterations)
+          end
+        end
+      end
+
+      it 'avoids N+1 database queries' do
+        # warm-up
+        User.automation_bot # this create the automation bot user record
+        worker.send(:automation_bot) # this will trigger the check and initiate the @automation_bot instance var
+
+        representative = group1.iterations.closed.first
+        control_count = ActiveRecord::QueryRecorder.new { worker.perform(representative) }.count
+
+        # for each iteration 2 extra queries are needed:
+        # - find the next open iteration
+        # - select the open issues to be moved
+        # so we have 2 extra closed iterations compared to control count so we need 4 more queries
+        iteration_ids = [group1.iterations.closed.pluck(:id) + group2.iterations.closed.pluck(:id)].flatten
+
+        expect { worker.perform(iteration_ids) }.not_to exceed_query_limit(control_count + 4)
+      end
+
+      context 'with batches' do
+        before do
+          stub_const("#{described_class}::BATCH_SIZE", 1)
+        end
+
+        it "run in batches" do
+          expect(Iterations::RollOverIssuesService).to receive(:new).and_return(mock_success_service).twice
+          expect(Iteration).to receive(:closed).and_call_original.exactly(3).times
+
+          iteration_ids = [group1.iterations.closed.pluck(:id) + group2.iterations.closed.pluck(:id)].flatten
+          worker.perform(iteration_ids)
+        end
+      end
+    end
+  end
+
+  include_examples 'an idempotent worker' do
+    let(:job_args) { [group1.iterations] }
+  end
+end

ee/spec/workers/iterations_update_status_worker_spec.rb

@@ -3,44 +3,50 @@
 require 'spec_helper'
 
 RSpec.describe IterationsUpdateStatusWorker do
+  let_it_be(:closed_iteration1, reload: true) { create(:iteration, :skip_future_date_validation, start_date: 20.days.ago, due_date: 11.days.ago) }
+  let_it_be(:started_iteration1, reload: true) { create(:iteration, :skip_future_date_validation, start_date: 10.days.ago, due_date: 3.days.ago) }
+  let_it_be(:started_iteration2) { create(:iteration, :skip_future_date_validation, start_date: 2.days.ago, due_date: 5.days.from_now) }
+  let_it_be(:upcoming_iteration) { create(:iteration, start_date: 11.days.from_now, due_date: 13.days.from_now) }
+
   subject(:worker) { described_class.new }
 
   describe '#perform' do
-    context 'when iterations are in `upcoming` state' do
-      let_it_be(:closed_iteration1) { create(:iteration, :skip_future_date_validation, start_date: 20.days.ago, due_date: 11.days.ago) }
-      let_it_be(:closed_iteration2) { create(:iteration, :skip_future_date_validation, start_date: 10.days.ago, due_date: 3.days.ago) }
-      let_it_be(:started_iteration) { create(:iteration, :skip_future_date_validation, start_date: Date.current, due_date: 10.days.from_now) }
-      let_it_be(:upcoming_iteration) { create(:iteration, start_date: 11.days.from_now, due_date: 13.days.from_now) }
+    before do
+      started_iteration1.update_column(:state_enum, 2)
+      closed_iteration1.update_column(:state_enum, 1)
+    end
+
+    it 'schedules an issues roll-over job' do
+      expect(Iterations::RollOverIssuesWorker).to receive(:perform_async)
+
+      worker.perform
+    end
 
+    context 'when iterations with passed due dates are in `upcoming`, `started` or `closes` states' do
       it 'updates the status of iterations that require it', :aggregate_failures do
-        expect(closed_iteration1.state).to eq('closed')
-        expect(closed_iteration2.state).to eq('closed')
-        expect(started_iteration.state).to eq('started')
+        expect(closed_iteration1.state).to eq('upcoming')
+        expect(started_iteration1.state).to eq('started')
+        expect(started_iteration2.state).to eq('started')
         expect(upcoming_iteration.state).to eq('upcoming')
 
-        closed_iteration2.update!(state: 'upcoming')
         worker.perform
 
         expect(closed_iteration1.reload.state).to eq('closed')
-        expect(closed_iteration2.reload.state).to eq('closed')
-        expect(started_iteration.reload.state).to eq('started')
+        expect(started_iteration1.reload.state).to eq('closed')
+        expect(started_iteration2.reload.state).to eq('started')
         expect(upcoming_iteration.reload.state).to eq('upcoming')
       end
-    end
 
-    context 'when iterations are in `started` state' do
-      let_it_be(:iteration1) { create(:iteration, :skip_future_date_validation, start_date: 10.days.ago, due_date: 2.days.ago) }
-      let_it_be(:iteration2) { create(:iteration, :skip_future_date_validation, start_date: 1.day.ago, due_date: Date.today, state_enum: ::Iteration::STATE_ENUM_MAP[:started]) }
+      context 'in batches' do
+        before do
+          stub_const("#{described_class}::BATCH_SIZE", 1)
+        end
 
-      it 'updates from started to closed when due date is past, does not touch others', :aggregate_failures do
-        expect(iteration1.state).to eq('closed')
-        expect(iteration2.state).to eq('started')
-
-        iteration1.update!(state: 'started')
-        worker.perform
+        it "run in batches" do
+          expect(Iterations::RollOverIssuesWorker).to receive(:perform_async).twice
 
-        expect(iteration1.reload.state).to eq('closed')
-        expect(iteration2.reload.state).to eq('started')
+          worker.perform
+        end
       end
     end
   end