Merge branch 'remove-legacy-burndown' into 'master'

Remove legacy_data? method on legacy burndown

See merge request gitlab-org/gitlab!49662

changelogs/unreleased/remove-legacy-burndown.yml

+---
+title: Remove unnecessary queries in milestone page
+merge_request: 49662
+author:
+type: performance

ee/app/helpers/ee/timeboxes_helper.rb

@@ -2,17 +2,10 @@
 
 module EE
   module TimeboxesHelper
-    def burndown_chart(milestone)
-      if milestone.supports_milestone_charts?
-        issues = milestone.issues_visible_to_user(current_user)
-        Burndown.new(issues, milestone.start_date, milestone.due_date)
-      end
-    end
-
-    def can_generate_chart?(milestone, burndown)
+    def can_generate_chart?(milestone)
       return false unless milestone.supports_milestone_charts?
 
-      burndown&.valid? && !burndown&.empty?
+      milestone.start_date.present? && milestone.due_date.present?
     end
 
     def show_burndown_charts_promotion?(milestone)

ee/app/models/burndown.rb

@@ -3,7 +3,7 @@
 class Burndown
   include Gitlab::Utils::StrongMemoize
 
-  attr_reader :issues, :start_date, :end_date, :due_date, :accurate
+  attr_reader :issues, :start_date, :end_date, :due_date
 
   def initialize(issues, start_date, due_date)
     @start_date = start_date
@@ -25,38 +25,12 @@ class Burndown
     burndown_events
   end
 
-  def empty?
-    issues.any? && legacy_data?
-  end
-
   def valid?
     start_date && due_date
   end
 
-  # If all closed issues have no closed events, mark burndown chart as containing legacy data
-  def legacy_data?
-    strong_memoize(:legacy_data) do
-      closed_events = closed_issues
-      closed_events.any? && closed_issues_events_count == 0
-    end
-  end
-
-  def accurate?
-    closed_issues.count == closed_issues_events_count
-  end
-
   private
 
-  def closed_issues
-    issues.select(&:closed?)
-  end
-
-  def closed_issues_events_count
-    strong_memoize(:closed_issues_events_count) do
-      Event.closed_action.where(target: closed_issues).count
-    end
-  end
-
   def burndown_events
     issues
       .map { |issue| burndown_events_for(issue) }

ee/app/views/shared/milestones/_burndown.html.haml

 - milestone = local_assigns[:milestone]
-- burndown = burndown_chart(milestone)
 - burndown_endpoint = milestone.group_milestone? ? api_v4_groups_milestones_burndown_events_path(id: milestone.group.id, milestone_id: milestone.id) : api_v4_projects_milestones_burndown_events_path(id: milestone.project.id, milestone_id: milestone.timebox_id)
 
-- if can_generate_chart?(milestone, burndown)
-  .burndown-chart.mb-2{ data: { start_date: burndown.start_date.strftime("%Y-%m-%d"),
-    due_date: burndown.due_date.strftime("%Y-%m-%d"),
+- if can_generate_chart?(milestone)
+  .burndown-chart.mb-2{ data: { start_date: milestone.start_date.strftime("%Y-%m-%d"),
+    due_date: milestone.due_date.strftime("%Y-%m-%d"),
     milestone_id: milestone.to_global_id,
     is_legacy: legacy_milestone?(milestone),
     burndown_events_path: expose_url(burndown_endpoint) } }

ee/spec/helpers/timeboxes_helper_spec.rb

@@ -3,6 +3,26 @@
 require 'spec_helper'
 
 RSpec.describe TimeboxesHelper do
+  describe '#can_generate_chart?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:supports_milestone_charts, :start_date, :due_date, :can_generate_chart) do
+      false | nil        | nil        | false
+      true  | Date.today | Date.today | true
+      true  | Date.today | nil        | false
+      true  | nil        | Date.today | false
+      true  | nil        | nil        | false
+    end
+
+    subject { helper.can_generate_chart?(milestone) }
+
+    let(:milestone) { double('Milestone', supports_milestone_charts?: supports_milestone_charts, start_date: start_date, due_date: due_date) }
+
+    with_them do
+      it { is_expected.to eq(can_generate_chart) }
+    end
+  end
+
   describe '#show_burndown_placeholder?' do
     let_it_be(:user) { build(:user) }
 

ee/spec/models/burndown_spec.rb

@@ -73,34 +73,6 @@ RSpec.describe Burndown do
       end
     end
 
-    it "sets attribute accurate to true" do
-      burndown = described_class.new(milestone.issues_visible_to_user(user), milestone.start_date, milestone.due_date)
-
-      expect(burndown).to be_accurate
-    end
-
-    it "is accurate with no issues" do
-      new_milestone = create(:milestone)
-      burndown = described_class.new(new_milestone.issues_visible_to_user(user), new_milestone.start_date, new_milestone.due_date)
-
-      new_milestone.project.add_maintainer(user)
-
-      expect(burndown).to be_accurate
-    end
-
-    context "when there are no closed issues" do
-      before do
-        milestone.issues.delete_all
-        create(:issue, issue_params.merge(created_at: milestone.start_date.end_of_day))
-      end
-
-      it "sets attribute empty to false" do
-        burndown = described_class.new(milestone.issues_visible_to_user(user), milestone.start_date, milestone.due_date)
-
-        expect(burndown).not_to be_empty
-      end
-    end
-
     it "ignores follow-up events with the same action" do
       create(:event, target: milestone.issues.first, created_at: milestone.start_date + 1.minute, action: :reopened)
       event1 = create(:closed_issue_event, target: milestone.issues.first, created_at: milestone.start_date + 2.minutes)
@@ -127,24 +99,6 @@ RSpec.describe Burndown do
           { created_at: Date.new(2017, 3, 3).beginning_of_day,  weight: 2, action: 'reopened' }
         ])
       end
-
-      it "sets attribute empty to true" do
-        burndown = described_class.new(milestone.issues_visible_to_user(user), milestone.start_date, milestone.due_date)
-
-        expect(burndown).to be_empty
-      end
-    end
-
-    context "when one but not all closed issues does not have a closed event" do
-      it "sets attribute accurate to false" do
-        Event.where(target: milestone.issues.closed.first, action: :closed).destroy_all # rubocop: disable Cop/DestroyAll
-        burndown = described_class.new(milestone.issues_visible_to_user(user), milestone.start_date, milestone.due_date)
-
-        aggregate_failures do
-          expect(burndown).not_to be_empty
-          expect(burndown).not_to be_accurate
-        end
-      end
     end
   end