Use burnup data to generate burndown charts

Also add toggle to load old data
We now also render the charts before making the network request
for both cases, which speeds up the rendering of charts
Add a flag to ensure we only fetch burndown data once

ee/app/assets/javascripts/burndown_chart/components/burn_charts.vue

@@ -40,6 +40,11 @@ export default {
       required: false,
       default: '',
     },
+    showNewOldBurndownToggle: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   apollo: {
     burnupData: {
@@ -69,7 +74,7 @@ export default {
       issuesSelected: true,
       burnupData: [],
       useLegacyBurndown: !this.glFeatures.burnupCharts,
-      showInfo: true,
+      showInfo: this.showNewOldBurndownToggle,
       error: '',
     };
   },
@@ -258,7 +263,7 @@ export default {
         </gl-button>
       </gl-button-group>
 
-      <gl-button-group v-if="glFeatures.burnupCharts">
+      <gl-button-group v-if="glFeatures.burnupCharts && showNewOldBurndownToggle">
         <gl-button
           ref="oldBurndown"
           :category="useLegacyBurndown ? 'primary' : 'secondary'"

ee/app/assets/javascripts/burndown_chart/index.js

@@ -29,6 +29,7 @@ export default () => {
     const dueDate = $chartEl.data('dueDate');
     const milestoneId = $chartEl.data('milestoneId');
     const burndownEventsPath = $chartEl.data('burndownEventsPath');
+    const isLegacy = $chartEl.data('isLegacy');
 
     // eslint-disable-next-line no-new
     new Vue({
@@ -41,6 +42,7 @@ export default () => {
       render(createElement) {
         return createElement('burn-charts', {
           props: {
+            showNewOldBurndownToggle: isLegacy,
             burndownEventsPath,
             startDate,
             dueDate,

ee/app/helpers/ee/timeboxes_helper.rb

@@ -19,28 +19,9 @@ module EE
       milestone.is_a?(EE::Milestone) && !milestone.supports_milestone_charts? && show_promotions?
     end
 
-    def show_burndown_placeholder?(milestone, warning)
-      return false if cookies['hide_burndown_message'].present?
-      return false unless milestone.supports_milestone_charts?
-
-      warning.nil? && can?(current_user, :admin_milestone, milestone.resource_parent)
-    end
-
-    def data_warning_for(burndown)
-      return unless burndown
-
-      message =
-        if burndown.empty?
-          "The burndown chart can’t be shown, as all issues assigned to this milestone were closed on an older GitLab version before data was recorded. "
-        elsif !burndown.accurate?
-          "Some issues can’t be shown in the burndown chart, as they were closed on an older GitLab version before data was recorded. "
-        end
-
-      if message
-        link = link_to "About burndown charts", help_page_path('user/project/milestones/burndown_charts.md'), class: 'burndown-docs-link'
-
-        content_tag(:div, (message + link).html_safe, id: "data-warning", class: "settings-message prepend-top-20")
-      end
+    def show_burndown_placeholder?(milestone)
+      milestone.supports_milestone_charts? &&
+        can?(current_user, :admin_milestone, milestone.resource_parent)
     end
 
     def milestone_weight_tooltip_text(weight)
@@ -50,5 +31,13 @@ module EE
         _("Weight %{weight}") % { weight: weight }
       end
     end
+
+    def first_resource_state_event
+      strong_memoize(:first_resource_state_event) { ::ResourceStateEvent.first }
+    end
+
+    def legacy_milestone?(milestone)
+      first_resource_state_event && milestone.created_at < first_resource_state_event.created_at
+    end
   end
 end

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

 - milestone = local_assigns[:milestone]
 - burndown = burndown_chart(milestone)
-- warning = data_warning_for(burndown)
 - 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)
 
-= warning
-
 - 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"),
     milestone_id: milestone.to_global_id,
+    is_legacy: legacy_milestone?(milestone),
     burndown_events_path: expose_url(burndown_endpoint) } }
 
-- elsif show_burndown_placeholder?(milestone, warning)
+- elsif show_burndown_placeholder?(milestone)
   .burndown-hint.content-block.container-fluid
     = sprite_icon('close', size: 16, css_class: 'dismiss-icon')
     .row

ee/spec/features/projects/milestones/milestone_spec.rb

@@ -42,47 +42,6 @@ RSpec.describe 'Milestones on EE' do
         end
       end
 
-      context 'when a closed issue do not have closed events' do
-        it 'shows warning' do
-          close_issue(create(:issue, issue_params))
-          close_issue(create(:issue, issue_params))
-
-          # Legacy issue: No closed events being created
-          create(:closed_issue, issue_params)
-
-          visit_milestone
-
-          expect(page).to have_selector('#data-warning', count: 1)
-          expect(page.find('#data-warning').text).to include("Some issues can’t be shown in the burndown chart")
-          expect(page).to have_selector('.burndown-chart')
-        end
-      end
-
-      context 'when all closed issues do not have closed events' do
-        it 'shows warning and hides burndown' do
-          create(:closed_issue, issue_params)
-          create(:closed_issue, issue_params)
-
-          visit_milestone
-
-          expect(page).to have_selector('#data-warning', count: 1)
-          expect(page.find('#data-warning').text).to include("The burndown chart can’t be shown")
-          expect(page).not_to have_selector('.burndown-chart')
-        end
-      end
-
-      context 'data is accurate' do
-        it 'does not show warning' do
-          create(:issue, issue_params)
-          close_issue(create(:issue, issue_params))
-
-          visit_milestone
-
-          expect(page).not_to have_selector('#data-warning')
-          expect(page).to have_selector('.burndown-chart')
-        end
-      end
-
       context 'with due & start date not set' do
         let(:milestone_without_dates) { create(:milestone, project: project) }
 

ee/spec/frontend/burndown_chart/components/burn_charts_spec.js

@@ -184,11 +184,6 @@ describe('burndown_chart', () => {
       expect(findBurnupChart().props('issuesSelected')).toBe(false);
     });
 
-    it('shows old/new burndown buttons', () => {
-      expect(findOldBurndownChartButton().exists()).toBe(true);
-      expect(findNewBurndownChartButton().exists()).toBe(true);
-    });
-
     it('uses burndown data computed from burnup data', () => {
       createComponent({
         data: {
@@ -204,15 +199,37 @@ describe('burndown_chart', () => {
       expect(openIssuesCount).toEqual([expectedCount]);
       expect(openIssuesWeight).toEqual([expectedWeight]);
     });
+  });
+
+  describe('showNewOldBurndownToggle', () => {
+    it('hides old/new burndown buttons if feature disabled', () => {
+      createComponent({ featureEnabled: false, props: { showNewOldBurndownToggle: true } });
+
+      expect(findOldBurndownChartButton().exists()).toBe(false);
+      expect(findNewBurndownChartButton().exists()).toBe(false);
+    });
+
+    it('hides old/new burndown buttons if props is false', () => {
+      createComponent({ featureEnabled: true, props: { showNewOldBurndownToggle: false } });
+
+      expect(findOldBurndownChartButton().exists()).toBe(false);
+      expect(findNewBurndownChartButton().exists()).toBe(false);
+    });
+
+    it('shows old/new burndown buttons if prop true', () => {
+      createComponent({ featureEnabled: true, props: { showNewOldBurndownToggle: true } });
+
+      expect(findOldBurndownChartButton().exists()).toBe(true);
+      expect(findNewBurndownChartButton().exists()).toBe(true);
+    });
 
     it('calls fetchLegacyBurndownEvents, but only once', () => {
+      createComponent({ featureEnabled: true, props: { showNewOldBurndownToggle: true } });
       jest.spyOn(wrapper.vm, 'fetchLegacyBurndownEvents');
       mock.onGet(defaultProps.burndownEventsPath).reply(200, []);
 
       findOldBurndownChartButton().vm.$emit('click');
 
-      findOldBurndownChartButton().vm.$emit('click');
-
       expect(wrapper.vm.fetchLegacyBurndownEvents).toHaveBeenCalledTimes(1);
     });
   });

ee/spec/helpers/timeboxes_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TimeboxesHelper do
+  describe '#show_burndown_placeholder?' do
+    let_it_be(:user) { build(:user) }
+
+    subject { helper.show_burndown_placeholder?(milestone) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    describe('milestone does not support burndown charts') do
+      let(:milestone) { double('Milestone', supports_milestone_charts?: false) }
+
+      it { is_expected.to be false }
+    end
+
+    describe('user without permission') do
+      let(:milestone) { double('Milestone', supports_milestone_charts?: true, resource_parent: 'board') }
+
+      before do
+        stub_can_admin_milestone(false)
+      end
+
+      it { is_expected.to be false }
+    end
+
+    describe('user with permission') do
+      let(:milestone) { double('Milestone', supports_milestone_charts?: true, resource_parent: 'board') }
+
+      before do
+        stub_can_admin_milestone(true)
+      end
+
+      it { is_expected.to be true }
+    end
+  end
+
+  describe '#legacy_milestone?' do
+    subject { legacy_milestone?(milestone) }
+
+    describe 'without any ResourceStateEvents' do
+      let(:milestone) { double('Milestone', created_at: Date.current) }
+
+      it { is_expected.to be_nil }
+    end
+
+    describe 'with ResourceStateEvent created before milestone' do
+      let(:milestone) { double('Milestone', created_at: Date.current) }
+
+      before do
+        create_resource_state_event(Date.yesterday)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+
+    describe 'with ResourceStateEvent created same day as milestone' do
+      let(:milestone) { double('Milestone', created_at: Date.current) }
+
+      before do
+        create_resource_state_event
+      end
+
+      it { is_expected.to eq(false) }
+    end
+
+    describe 'with ResourceStateEvent created after milestone' do
+      let(:milestone) { double('Milestone', created_at: Date.yesterday) }
+
+      before do
+        create_resource_state_event
+      end
+
+      it { is_expected.to eq(true) }
+    end
+  end
+
+  def create_resource_state_event(created_at = Date.current)
+    create(:resource_state_event, created_at: created_at)
+  end
+
+  def stub_can_admin_milestone(ability)
+    allow(helper).to receive(:can?).with(user, :admin_milestone, milestone.resource_parent).and_return(ability)
+  end
+end