Refactor BurnupChartService to handle iterations

Iterations and milestones use burnup ad burndown charts based on similar
resource events. Refactored BurnupChartService to be able to handle
both timebox(milestone, iteration) events and build chart time series

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 ""