Merge branch 'psi-burndown-tidier-toggle' into 'master'

Hide burndown toggle switch for new milestones

See merge request gitlab-org/gitlab!42656

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