Merge branch 'bvl-burndown-chart-license-check' into 'master'

Add License check for Burndown Charts

Closes #2578

See merge request !2219

app/controllers/projects/milestones_controller.rb

@@ -45,8 +45,10 @@ class Projects::MilestonesController < Projects::ApplicationController
   end
 
   def show
+    if @project.feature_available?(:burndown_charts, current_user)
       @burndown = Burndown.new(@milestone)
     end
+  end
 
   def create
     @milestone = Milestones::CreateService.new(project, current_user, milestone_params).execute

app/helpers/milestones_helper.rb

@@ -133,6 +133,19 @@ module MilestonesHelper
     end
   end
 
+  def can_generate_chart?(burndown)
+    return unless @project.feature_available?(:burndown_charts, current_user)
+
+    burndown&.valid? && !burndown&.empty?
+  end
+
+  def show_burndown_placeholder?(warning)
+    return false if cookies['hide_burndown_message'].present?
+    return false unless @project.feature_available?(:burndown_charts, current_user)
+
+    warning.nil? && can?(current_user, :admin_milestone, @project)
+  end
+
   def milestone_merge_request_tab_path(milestone)
     if @project
       merge_requests_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json)

app/models/license.rb

 class License < ActiveRecord::Base
   include ActionView::Helpers::NumberHelper
 
-  DEPLOY_BOARD_FEATURE = 'GitLab_DeployBoard'.freeze
-  FILE_LOCK_FEATURE = 'GitLab_FileLocks'.freeze
-  GEO_FEATURE = 'GitLab_Geo'.freeze
   AUDITOR_USER_FEATURE = 'GitLab_Auditor_User'.freeze
-  SERVICE_DESK_FEATURE = 'GitLab_ServiceDesk'.freeze
-  OBJECT_STORAGE_FEATURE = 'GitLab_ObjectStorage'.freeze
+  BURNDOWN_CHARTS_FEATURE = 'BurndownCharts'.freeze
+  DEPLOY_BOARD_FEATURE = 'GitLab_DeployBoard'.freeze
   ELASTIC_SEARCH_FEATURE = 'GitLab_ElasticSearch'.freeze
-  RELATED_ISSUES_FEATURE = 'RelatedIssues'.freeze
   EXPORT_ISSUES_FEATURE  = 'GitLab_ExportIssues'.freeze
+  FILE_LOCK_FEATURE = 'GitLab_FileLocks'.freeze
+  GEO_FEATURE = 'GitLab_Geo'.freeze
   MERGE_REQUEST_REBASE_FEATURE = 'GitLab_MergeRequestRebase'.freeze
   MERGE_REQUEST_SQUASH_FEATURE = 'GitLab_MergeRequestSquash'.freeze
   FAST_FORWARD_MERGE_FEATURE = 'GitLab_FastForwardMerge'.freeze
+  OBJECT_STORAGE_FEATURE = 'GitLab_ObjectStorage'.freeze
+  RELATED_ISSUES_FEATURE = 'RelatedIssues'.freeze
+  SERVICE_DESK_FEATURE = 'GitLab_ServiceDesk'.freeze
 
   FEATURE_CODES = {
     geo: GEO_FEATURE,
@@ -22,10 +23,11 @@ class License < ActiveRecord::Base
     elastic_search: ELASTIC_SEARCH_FEATURE,
     related_issues: RELATED_ISSUES_FEATURE,
     # Features that make sense to Namespace:
-    fast_forward_merge: FAST_FORWARD_MERGE_FEATURE,
+    burndown_charts: BURNDOWN_CHARTS_FEATURE,
     deploy_board: DEPLOY_BOARD_FEATURE,
-    file_lock: FILE_LOCK_FEATURE,
     export_issues: EXPORT_ISSUES_FEATURE,
+    fast_forward_merge: FAST_FORWARD_MERGE_FEATURE,
+    file_lock: FILE_LOCK_FEATURE,
     merge_request_rebase: MERGE_REQUEST_REBASE_FEATURE,
     merge_request_squash: MERGE_REQUEST_SQUASH_FEATURE
   }.freeze
@@ -36,12 +38,13 @@ class License < ActiveRecord::Base
   EARLY_ADOPTER_PLAN = 'early_adopter'.freeze
 
   EES_FEATURES = [
+    { BURNDOWN_CHARTS_FEATURE => 1 },
     { ELASTIC_SEARCH_FEATURE => 1 },
-    { RELATED_ISSUES_FEATURE => 1 },
-    { FAST_FORWARD_MERGE_FEATURE => 1 },
     { EXPORT_ISSUES_FEATURE => 1 },
+    { FAST_FORWARD_MERGE_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
-    { MERGE_REQUEST_SQUASH_FEATURE => 1 }
+    { MERGE_REQUEST_SQUASH_FEATURE => 1 },
+    { RELATED_ISSUES_FEATURE => 1 }
   ].freeze
 
   EEP_FEATURES = [
@@ -68,16 +71,17 @@ class License < ActiveRecord::Base
   EARLY_ADOPTER_FEATURES = [
     # TODO: Add EES features
     # https://gitlab.com/gitlab-org/gitlab-ee/issues/2335)
+    { AUDITOR_USER_FEATURE => 1 },
+    { BURNDOWN_CHARTS_FEATURE => 1 },
     { DEPLOY_BOARD_FEATURE => 1 },
+    { EXPORT_ISSUES_FEATURE => 1 },
+    { FAST_FORWARD_MERGE_FEATURE => 1 },
     { FILE_LOCK_FEATURE => 1 },
     { GEO_FEATURE => 1 },
-    { AUDITOR_USER_FEATURE => 1 },
-    { SERVICE_DESK_FEATURE => 1 },
-    { OBJECT_STORAGE_FEATURE => 1 },
-    { FAST_FORWARD_MERGE_FEATURE => 1 },
-    { EXPORT_ISSUES_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
-    { MERGE_REQUEST_SQUASH_FEATURE => 1 }
+    { MERGE_REQUEST_SQUASH_FEATURE => 1 },
+    { OBJECT_STORAGE_FEATURE => 1 },
+    { SERVICE_DESK_FEATURE => 1 }
   ].freeze
 
   FEATURES_BY_PLAN = {

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

 - milestone = local_assigns[:milestone]
 - project = local_assigns[:project]
 - burndown = local_assigns[:burndown]
-- can_generate_chart = burndown&.valid? && !burndown&.empty?
 - warning = data_warning_for(burndown)
 
 - content_for :page_specific_javascripts do
@@ -10,7 +9,7 @@
 
 = warning
 
-- if can_generate_chart
+- if can_generate_chart?(burndown)
   .burndown-header
     %h3
       Burndown chart
@@ -21,7 +20,7 @@
           Issue weight
   .burndown-chart{ data: { start_date: burndown.start_date.strftime("%Y-%m-%d"), due_date: burndown.due_date.strftime("%Y-%m-%d"), chart_data: burndown.to_json } }
 
-- elsif can?(current_user, :admin_milestone, @project) && cookies['hide_burndown_message'].nil? && warning.nil?
+- elsif show_burndown_placeholder?(warning)
   .burndown-hint.content-block.container-fluid
     = icon("times", class: "dismiss-icon")
     .row

changelogs/unreleased-ee/bvl-burndown-chart-license-check.yml

+---
+title: Add license checks for brundown charts
+merge_request: 2219
+author:

spec/features/milestones/show_spec.rb

@@ -3,7 +3,7 @@ require 'rails_helper'
 describe 'Milestone show', feature: true do
   let(:user) { create(:user) }
   let(:project) { create(:empty_project) }
-  let(:milestone) { create(:milestone, project: project, start_date: Date.today, due_date: 7.days.from_now) }
+  let(:milestone) { create(:milestone, project: project) }
   let(:labels) { create_list(:label, 2, project: project) }
   let(:issue_params) { { project: project, assignees: [user], author: user, milestone: milestone, labels: labels } }
 
@@ -23,47 +23,4 @@ describe 'Milestone show', feature: true do
 
     expect { visit_milestone }.not_to exceed_query_limit(control_count)
   end
-
-  context 'burndown' do
-    let(:issue_params) { { project: project, assignees: [user], author: user, milestone: milestone } }
-
-    context 'when any closed issues do not have closed_at value' do
-      it 'shows warning' do
-        create(:issue, issue_params)
-        create(:closed_issue, issue_params)
-        create(:closed_issue, issue_params.merge(closed_at: nil))
-
-        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_at value' do
-      it 'shows warning and hides burndown' do
-        create(:closed_issue, issue_params.merge(closed_at: nil))
-        create(:closed_issue, issue_params.merge(closed_at: nil))
-
-        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)
-        create(:closed_issue, issue_params)
-
-        visit_milestone
-
-        expect(page).not_to have_selector('#data-warning')
-        expect(page).to have_selector('.burndown-chart')
-      end
-    end
-  end
 end

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

+require 'spec_helper'
+
+describe 'Milestones on EE', feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project, name: 'test', namespace: user.namespace) }
+  let(:milestone) { create(:milestone, project: project, start_date: Date.today, due_date: 7.days.from_now) }
+
+  before do
+    login_as(user)
+  end
+
+  def visit_milestone
+    visit namespace_project_milestone_path(project.namespace, project, milestone)
+  end
+
+  context 'burndown charts' do
+    let(:milestone) do
+      create(:milestone,
+             project: project,
+             start_date: Date.yesterday,
+             due_date: Date.tomorrow)
+    end
+
+    context 'with the burndown chart feature available' do
+      let(:issue_params) { { project: project, assignees: [user], author: user, milestone: milestone } }
+
+      before do
+        stub_licensed_features(burndown_charts: true)
+      end
+
+      it 'shows a burndown chart' do
+        visit_milestone
+
+        within('#content-body') do
+          expect(page).to have_selector('.burndown-chart')
+        end
+      end
+
+      context 'when any closed issues do not have closed_at value' do
+        it 'shows warning' do
+          create(:issue, issue_params)
+          create(:closed_issue, issue_params)
+          create(:closed_issue, issue_params.merge(closed_at: nil))
+
+          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_at value' do
+        it 'shows warning and hides burndown' do
+          create(:closed_issue, issue_params.merge(closed_at: nil))
+          create(:closed_issue, issue_params.merge(closed_at: nil))
+
+          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)
+          create(:closed_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) }
+
+        it 'shows a mention to fill in dates' do
+          visit namespace_project_milestone_path(project.namespace, project, milestone_without_dates)
+
+          within('#content-body') do
+            expect(page).to have_link('Add start and due date')
+          end
+        end
+      end
+    end
+
+    context 'with the burndown chart feature disabled' do
+      before do
+        stub_licensed_features(burndown_charts: false)
+      end
+
+      it 'has a link to upgrade to Bronze when checking the namespace plan' do
+        # Not using `stub_application_setting` because the method is prepended in
+        # `EE::ApplicationSetting` which breaks when using `any_instance`
+        # https://gitlab.com/gitlab-org/gitlab-ce/issues/33587
+        allow(Gitlab::CurrentSettings.current_application_settings)
+          .to receive(:should_check_namespace_plan?) { true }
+
+        visit_milestone
+
+        within('#content-body') do
+          expect(page).not_to have_selector('.burndown-chart')
+        end
+      end
+
+      it 'has a link to upgrade to starter on premise' do
+        allow(Gitlab::CurrentSettings.current_application_settings)
+          .to receive(:should_check_namespace_plan?) { false }
+
+        visit_milestone
+
+        within('#content-body') do
+          expect(page).not_to have_selector('.burndown-chart')
+        end
+      end
+    end
+  end
+
+  context 'milestone summary' do
+    it 'shows the total weight when sum is greater than zero' do
+      create(:issue, project: project, milestone: milestone, weight: 3)
+      create(:issue, project: project, milestone: milestone, weight: 1)
+
+      visit_milestone
+
+      within '.milestone-sidebar' do
+        expect(page).to have_content 'Total issue weight 4'
+      end
+    end
+
+    it 'hides the total weight when sum is equal to zero' do
+      create(:issue, project: project, milestone: milestone, weight: nil)
+      create(:issue, project: project, milestone: milestone, weight: nil)
+
+      visit_milestone
+
+      within '.milestone-sidebar' do
+        expect(page).to have_content 'Total issue weight None'
+      end
+    end
+  end
+end

spec/features/projects/milestones/milestone_spec.rb

@@ -6,7 +6,7 @@ feature 'Project milestone', :feature do
   let(:milestone) { create(:milestone, project: project) }
 
   before do
-    gitlab_sign_in(user)
+    login_as(user)
   end
 
   context 'when project has enabled issues' do
@@ -64,32 +64,6 @@ feature 'Project milestone', :feature do
     end
   end
 
-  # EE-only
-  context 'milestone summary' do
-    it 'shows the total weight when sum is greater than zero' do
-      create(:issue, project: project, milestone: milestone, weight: 3)
-      create(:issue, project: project, milestone: milestone, weight: 1)
-
-      visit milestone_path
-
-      within '.milestone-sidebar' do
-        expect(page).to have_content 'Total issue weight 4'
-      end
-    end
-
-    it 'hides the total weight when sum is equal to zero' do
-      create(:issue, project: project, milestone: milestone, weight: nil)
-      create(:issue, project: project, milestone: milestone, weight: nil)
-
-      visit milestone_path
-
-      within '.milestone-sidebar' do
-        expect(page).to have_content 'Total issue weight None'
-      end
-    end
-  end
-  # EE-only
-
   def milestone_path
     namespace_project_milestone_path(project.namespace, project, milestone)
   end