Merge branch '229046-iterations-charts-refactoring' into 'master'

Refactor BurnupChartService to handle iterations

See merge request gitlab-org/gitlab!41280

app/models/concerns/timebox.rb

@@ -75,8 +75,8 @@ module Timebox
 
     scope :within_timeframe, -> (start_date, end_date) do
       where('start_date is not NULL or due_date is not NULL')
-          .where('start_date is NULL or start_date <= ?', end_date)
-          .where('due_date is NULL or due_date >= ?', start_date)
+        .where('start_date is NULL or start_date <= ?', end_date)
+        .where('due_date is NULL or due_date >= ?', start_date)
     end
 
     strip_attributes :title
@@ -195,6 +195,10 @@ module Timebox
     end
   end
 
+  def weight_available?
+    resource_parent&.feature_available?(:issue_weights)
+  end
+
   private
 
   def timebox_format_reference(format = :iid)

changelogs/unreleased/229046-iterations-charts-refactoring.yml

+---
+title: Add index to resource_iteration_events for add actions
+merge_request: 41280
+author:
+type: other

db/migrate/20200903092241_add_index_to_resource_iteration_events_add_events.rb

+# frozen_string_literal: true
+
+class AddIndexToResourceIterationEventsAddEvents < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_resource_iteration_events_on_iteration_id_and_add_action'
+  ADD_ACTION = '1'
+
+  def up
+    # Index add iteration events
+    add_concurrent_index :resource_iteration_events, :iteration_id, where: "action = #{ADD_ACTION}", name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :resource_iteration_events, :iteration_id, name: INDEX_NAME
+  end
+end

db/schema_migrations/20200903092241

+e6c3d5352feed1adc82b14218a6f47fa55df9e0add8a59228d128e4e7f39614b
\ No newline at end of file

db/structure.sql

@@ -20817,6 +20817,8 @@ CREATE INDEX index_resource_iteration_events_on_merge_request_id ON public.resou
 
 CREATE INDEX index_resource_iteration_events_on_user_id ON public.resource_iteration_events USING btree (user_id);
 
+CREATE INDEX index_resource_iterationn_events_on_iteration_id_and_add_action ON public.resource_iteration_events USING btree (iteration_id) WHERE (action = 1);
+
 CREATE INDEX index_resource_label_events_issue_id_label_id_action ON public.resource_label_events USING btree (issue_id, label_id, action);
 
 CREATE INDEX index_resource_label_events_on_epic_id ON public.resource_label_events USING btree (epic_id);

ee/app/graphql/resolvers/milestone_burnup_time_series_resolver.rb

@@ -9,7 +9,7 @@ module Resolvers
     def resolve(*args)
       return [] unless milestone.burnup_charts_available?
 
-      response = Milestones::BurnupChartService.new(milestone).execute
+      response = TimeboxBurnupChartService.new(milestone).execute
 
       raise GraphQL::ExecutionError, response.message if response.error?
 

ee/app/models/ee/iteration.rb

@@ -42,6 +42,14 @@ module EE
       self.title
     end
 
+    def supports_timebox_charts?
+      resource_parent&.feature_available?(:iterations) && weight_available?
+    end
+
+    def burnup_charts_available?
+      ::Feature.enabled?(:iteration_charts, resource_parent)
+    end
+
     private
 
     def timebox_format_reference(format = :id)

ee/app/models/ee/milestone.rb

@@ -10,14 +10,12 @@ module EE
       has_many :boards
     end
 
-    def weight_available?
-      resource_parent&.feature_available?(:issue_weights)
-    end
-
     def supports_milestone_charts?
       resource_parent&.feature_available?(:milestone_charts) && weight_available?
     end
 
+    alias_method :supports_timebox_charts?, :supports_milestone_charts?
+
     def burnup_charts_available?
       ::Feature.enabled?(:burnup_charts, resource_parent)
     end

ee/app/services/milestones/burnup_chart_service.rb → ee/app/services/timebox_burnup_chart_service.rb

 # frozen_string_literal: true
 
-# This service computes the milestone's daily total number of issues and their weights.
-# For each day, this returns the totals for all issues that are assigned to the milestone at that point in time.
-# This represents the scope for this milestone. This also returns separate totals for closed issues which represent the completed work.
+# This service computes the timebox's(milestone, iteration) daily total number of issues and their weights.
+# For each day, this returns the totals for all issues that are assigned to the timebox(milestone, iteration) at that point in time.
+# This represents the scope for this timebox(milestone, iteration). This also returns separate totals for closed issues which represent the completed work.
 #
 # This is implemented by iterating over all relevant resource events ordered by time. We need to do this
 # so that we can keep track of the issue's state during that point in time and handle the events based on that.
 
-class Milestones::BurnupChartService
+class TimeboxBurnupChartService
   include Gitlab::Utils::StrongMemoize
 
   EVENT_COUNT_LIMIT = 50_000
 
-  def initialize(milestone)
-    @milestone = milestone
+  def initialize(timebox)
+    @timebox = timebox
   end
 
   def execute
-    return ServiceResponse.error(message: _('Milestone does not support burnup charts')) unless milestone.supports_milestone_charts?
-    return ServiceResponse.error(message: _('Milestone must have a start and due date')) if milestone.start_date.blank? || milestone.due_date.blank?
+    return ServiceResponse.error(message: _('%{timebox_type} does not support burnup charts' % { timebox_type: timebox_type })) unless timebox.supports_timebox_charts?
+    return ServiceResponse.error(message: _('%{timebox_type} must have a start and due date' % { timebox_type: timebox_type })) if timebox.start_date.blank? || timebox.due_date.blank?
     return ServiceResponse.error(message: _('Burnup chart could not be generated due to too many events')) if resource_events.num_tuples > EVENT_COUNT_LIMIT
 
     @issue_states = {}
@@ -26,8 +26,8 @@ class Milestones::BurnupChartService
 
     resource_events.each do |event|
       case event['event_type']
-      when 'milestone'
-        handle_milestone_event(event)
+      when 'timebox'
+        handle_resource_timebox_event(event)
       when 'state'
         handle_state_event(event)
       when 'weight'
@@ -40,25 +40,25 @@ class Milestones::BurnupChartService
 
   private
 
-  attr_reader :milestone, :issue_states, :chart_data
+  attr_reader :timebox, :issue_states, :chart_data
 
-  def handle_milestone_event(event)
+  def handle_resource_timebox_event(event)
     issue_state = find_issue_state(event['issue_id'])
 
-    return if issue_state[:milestone] == milestone.id && event['action'] == ResourceMilestoneEvent.actions[:add] && event['value'] == milestone.id
+    return if issue_state[:timebox] == timebox.id && event['action'] == ResourceTimeboxEvent.actions[:add] && event['value'] == timebox.id
 
-    if event['action'] == ResourceMilestoneEvent.actions[:add] && event['value'] == milestone.id
-      handle_add_milestone_event(event)
-    elsif issue_state[:milestone] == milestone.id
-      # If the issue is currently assigned to the milestone, then treat any event here as a removal.
-      # We do not have a separate `:remove` event when replacing milestones with another one.
-      handle_remove_milestone_event(event)
+    if event['action'] == ResourceTimeboxEvent.actions[:add] && event['value'] == timebox.id
+      handle_add_timebox_event(event)
+    elsif issue_state[:timebox] == timebox.id
+      # If the issue is currently assigned to the timebox(milestone or iteration), then treat any event here as a removal.
+      # We do not have a separate `:remove` event when replacing timebox(milestone or iteration) with another one.
+      handle_remove_timebox_event(event)
     end
 
-    issue_state[:milestone] = event['value']
+    issue_state[:timebox] = event['value']
   end
 
-  def handle_add_milestone_event(event)
+  def handle_add_timebox_event(event)
     issue_state = find_issue_state(event['issue_id'])
 
     increment_scope(event['created_at'], issue_state[:weight])
@@ -68,7 +68,7 @@ class Milestones::BurnupChartService
     end
   end
 
-  def handle_remove_milestone_event(event)
+  def handle_remove_timebox_event(event)
     issue_state = find_issue_state(event['issue_id'])
 
     decrement_scope(event['created_at'], issue_state[:weight])
@@ -83,7 +83,7 @@ class Milestones::BurnupChartService
     old_state = issue_state[:state]
     issue_state[:state] = event['value']
 
-    return if issue_state[:milestone] != milestone.id
+    return if issue_state[:timebox] != timebox.id
 
     if old_state == ResourceStateEvent.states[:closed] && event['value'] == ResourceStateEvent.states[:reopened]
       decrement_completed(event['created_at'], issue_state[:weight])
@@ -97,7 +97,7 @@ class Milestones::BurnupChartService
     old_weight = issue_state[:weight]
     issue_state[:weight] = event['value'] || 0
 
-    return if issue_state[:milestone] != milestone.id
+    return if issue_state[:timebox] != timebox.id
 
     add_chart_data(event['created_at'], :scope_weight, issue_state[:weight] - old_weight)
 
@@ -128,7 +128,7 @@ class Milestones::BurnupChartService
 
   def add_chart_data(timestamp, field, value)
     date = timestamp.to_date
-    date = milestone.start_date if date < milestone.start_date
+    date = timebox.start_date if date < timebox.start_date
 
     if chart_data.empty?
       chart_data.push({
@@ -150,7 +150,7 @@ class Milestones::BurnupChartService
 
   def find_issue_state(issue_id)
     issue_states[issue_id] ||= {
-      milestone: nil,
+      timebox: nil,
       weight: 0,
       state: ResourceStateEvent.states[:opened]
     }
@@ -159,16 +159,16 @@ class Milestones::BurnupChartService
   # rubocop: disable CodeReuse/ActiveRecord
   def resource_events
     strong_memoize(:resource_events) do
-      union = Gitlab::SQL::Union.new([milestone_events, state_events, weight_events]) # rubocop: disable Gitlab/Union
+      union = Gitlab::SQL::Union.new([resource_timebox_events, state_events, weight_events]) # rubocop: disable Gitlab/Union
 
       ActiveRecord::Base.connection.execute("(#{union.to_sql}) ORDER BY created_at LIMIT #{EVENT_COUNT_LIMIT + 1}")
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def milestone_events
-    ResourceMilestoneEvent.by_issue_ids_and_created_at_earlier_or_equal_to(issue_ids, end_time)
-      .select('\'milestone\' AS event_type, created_at, milestone_id AS value, action, issue_id')
+  def resource_timebox_events
+    resource_timebox_event_class.by_issue_ids_and_created_at_earlier_or_equal_to(issue_ids, end_time)
+      .select("'timebox' AS event_type, created_at, #{timebox_fk} AS value, action, issue_id")
   end
 
   def state_events
@@ -186,17 +186,34 @@ class Milestones::BurnupChartService
     # We find all issues that have this milestone added before this milestone's due date.
     # We cannot just filter by `issues.milestone_id` because there might be issues that have
     # since been moved to other milestones and we still need to include these in this graph.
-    ResourceMilestoneEvent
+    resource_timebox_event_class
       .select(:issue_id)
       .where({
-        milestone_id: milestone.id,
+        "#{timebox_fk}": timebox.id,
         action: :add
-      })
+       })
       .where('created_at <= ?', end_time)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
   def end_time
-    @end_time ||= milestone.due_date.end_of_day
+    @end_time ||= timebox.due_date.end_of_day
+  end
+
+  def timebox_type
+    timebox.class.name
+  end
+
+  def timebox_fk
+    timebox_type.downcase.foreign_key
+  end
+
+  def resource_timebox_event_class
+    case timebox
+    when Milestone then ResourceMilestoneEvent
+    when Iteration then ResourceIterationEvent
+    else
+      raise ArgumentError, 'Cannot handle timebox type'
+    end
   end
 end

ee/spec/graphql/resolvers/milestone_burnup_time_series_resolver_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe Resolvers::MilestoneBurnupTimeSeriesResolver do
 
     context 'when the service returns an error' do
       before do
-        stub_const('Milestones::BurnupChartService::EVENT_COUNT_LIMIT', 1)
+        stub_const('TimeboxBurnupChartService::EVENT_COUNT_LIMIT', 1)
       end
 
       it 'raises a GraphQL exception' do

locale/gitlab.pot

@@ -773,6 +773,12 @@ msgstr ""
 msgid "%{timebox_name} should belong either to a project or a group."
 msgstr ""
 
+msgid "%{timebox_type} does not support burnup charts"
+msgstr ""
+
+msgid "%{timebox_type} must have a start and due date"
+msgstr ""
+
 msgid "%{title} %{operator} %{threshold}"
 msgstr ""
 
@@ -15857,18 +15863,12 @@ msgid_plural "Milestones"
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "Milestone does not support burnup charts"
-msgstr ""
-
 msgid "Milestone lists not available with your current license"
 msgstr ""
 
 msgid "Milestone lists show all issues from the selected milestone."
 msgstr ""
 
-msgid "Milestone must have a start and due date"
-msgstr ""
-
 msgid "MilestoneSidebar|Closed:"
 msgstr ""