Merge branch '2575-namespace-license-issue-weights' into 'master'

Introduce namespace licensing for issue weights (EES)

Closes #2575

See merge request !2291

app/controllers/ee/projects/issues_controller.rb

@@ -3,7 +3,7 @@ module EE
     module IssuesController
       extend ActiveSupport::Concern
 
-      included do
+      prepended do
         before_action :check_export_issues_available!, only: [:export_csv]
       end
 
@@ -13,6 +13,20 @@ module EE
         index_path = namespace_project_issues_path(project.namespace, project)
         redirect_to(index_path, notice: "Your CSV export has started. It will be emailed to #{current_user.notification_email} when complete.")
       end
+
+      def issue_params_attributes
+        attrs = super
+        attrs.unshift(:weight) if project.feature_available?(:issue_weights)
+
+        attrs
+      end
+
+      def filter_params
+        params = super
+        params.reject! { |key| key == 'weight' } unless project.feature_available?(:issue_weights)
+
+        params
+      end
     end
   end
 end

app/controllers/projects/issues_controller.rb

@@ -6,7 +6,7 @@ class Projects::IssuesController < Projects::ApplicationController
   include IssuableCollections
   include SpammableActions
 
-  include ::EE::Projects::IssuesController
+  prepend ::EE::Projects::IssuesController
 
   prepend_before_action :authenticate_user!, only: [:new, :export_csv]
 
@@ -270,10 +270,22 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def issue_params
-    params.require(:issue).permit(
-      :title, :assignee_id, :position, :description, :confidential, :weight,
-      :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [], assignee_ids: []
-    )
+    params.require(:issue).permit(*issue_params_attributes)
+  end
+
+  def issue_params_attributes
+    %i[
+      title
+      assignee_id
+      position
+      description
+      confidential
+      milestone_id
+      due_date
+      state_event
+      task_num
+      lock_version
+    ] + [{ label_ids: [], assignee_ids: [] }]
   end
 
   def authenticate_user!

app/controllers/projects/milestones_controller.rb

@@ -45,7 +45,8 @@ class Projects::MilestonesController < Projects::ApplicationController
   end
 
   def show
-    if @project.feature_available?(:burndown_charts, current_user)
+    if @project.feature_available?(:burndown_charts, current_user) &&
+        @project.feature_available?(:issue_weights, current_user)
       @burndown = Burndown.new(@milestone)
     end
   end

app/helpers/milestones_helper.rb

@@ -134,14 +134,16 @@ module MilestonesHelper
   end
 
   def can_generate_chart?(burndown)
-    return unless @project.feature_available?(:burndown_charts, current_user)
+    return unless @project.feature_available?(:burndown_charts, current_user) &&
+        @project.feature_available?(:issue_weights, 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)
+    return false unless @project.feature_available?(:burndown_charts, current_user) &&
+        @project.feature_available?(:issue_weights, current_user)
 
     warning.nil? && can?(current_user, :admin_milestone, @project)
   end

app/models/burndown.rb

 class Burndown
+  Issue = Struct.new(:closed_at, :weight, :state)
+
   attr_reader :start_date, :due_date, :end_date, :issues_count, :issues_weight, :accurate, :legacy_data
   alias_method :accurate?, :accurate
   alias_method :empty?, :legacy_data
@@ -68,8 +70,10 @@ class Burndown
 
   def milestone_closed_issues
     @milestone_closed_issues ||=
-      @milestone.issues.select("closed_at, weight, state")
+      @milestone.issues
         .where("state IN ('reopened', 'closed')")
         .order("closed_at ASC")
+        .pluck("closed_at, weight, state")
+        .map {|attrs| ::Burndown::Issue.new(*attrs) }
   end
 end

app/models/ee/global_milestone.rb

+module EE
+  module GlobalMilestone
+    def supports_weight?
+      false
+    end
+  end
+end

app/models/ee/group_milestone.rb

+module EE
+  module GroupMilestone
+    def supports_weight?
+      group&.feature_available?(:issue_weights)
+    end
+  end
+end

app/models/ee/issue.rb

@@ -23,5 +23,14 @@ module EE
       # and doesn't actually show up in the participants list.
       user.support_bot? || super
     end
+
+    # override
+    def weight
+      super if supports_weight?
+    end
+
+    def supports_weight?
+      project&.feature_available?(:issue_weights)
+    end
   end
 end

app/models/ee/merge_request.rb

@@ -50,5 +50,9 @@ module EE
       super && project.feature_available?(:merge_request_squash)
     end
     alias_method :squash?, :squash
+
+    def supports_weight?
+      false
+    end
   end
 end

app/models/ee/milestone.rb

+module EE
+  module Milestone
+    def supports_weight?
+      project&.feature_available?(:issue_weights)
+    end
+  end
+end

app/models/global_milestone.rb

 class GlobalMilestone
   include Milestoneish
 
+  include ::EE::GlobalMilestone
+
   EPOCH = DateTime.parse('1970-01-01')
 
   attr_accessor :title, :milestones

app/models/group_milestone.rb

 class GroupMilestone < GlobalMilestone
+  include ::EE::GroupMilestone
+
   attr_accessor :group
 
   def self.build_collection(group, projects, params)

app/models/license.rb

@@ -6,28 +6,31 @@ class License < ActiveRecord::Base
   DEPLOY_BOARD_FEATURE = 'GitLab_DeployBoard'.freeze
   ELASTIC_SEARCH_FEATURE = 'GitLab_ElasticSearch'.freeze
   EXPORT_ISSUES_FEATURE  = 'GitLab_ExportIssues'.freeze
+  FAST_FORWARD_MERGE_FEATURE = 'GitLab_FastForwardMerge'.freeze
   FILE_LOCK_FEATURE = 'GitLab_FileLocks'.freeze
   GEO_FEATURE = 'GitLab_Geo'.freeze
+  ISSUE_WEIGHTS_FEATURE = 'GitLab_IssueWeights'.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,
     auditor_user: AUDITOR_USER_FEATURE,
-    service_desk: SERVICE_DESK_FEATURE,
-    object_storage: OBJECT_STORAGE_FEATURE,
     elastic_search: ELASTIC_SEARCH_FEATURE,
+    geo: GEO_FEATURE,
+    object_storage: OBJECT_STORAGE_FEATURE,
     related_issues: RELATED_ISSUES_FEATURE,
+    service_desk: SERVICE_DESK_FEATURE,
+
     # Features that make sense to Namespace:
     burndown_charts: BURNDOWN_CHARTS_FEATURE,
     deploy_board: DEPLOY_BOARD_FEATURE,
     export_issues: EXPORT_ISSUES_FEATURE,
     fast_forward_merge: FAST_FORWARD_MERGE_FEATURE,
     file_lock: FILE_LOCK_FEATURE,
+    issue_weights: ISSUE_WEIGHTS_FEATURE,
     merge_request_rebase: MERGE_REQUEST_REBASE_FEATURE,
     merge_request_squash: MERGE_REQUEST_SQUASH_FEATURE
   }.freeze
@@ -42,6 +45,7 @@ class License < ActiveRecord::Base
     { ELASTIC_SEARCH_FEATURE => 1 },
     { EXPORT_ISSUES_FEATURE => 1 },
     { FAST_FORWARD_MERGE_FEATURE => 1 },
+    { ISSUE_WEIGHTS_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
     { MERGE_REQUEST_SQUASH_FEATURE => 1 },
     { RELATED_ISSUES_FEATURE => 1 }
@@ -49,12 +53,12 @@ class License < ActiveRecord::Base
 
   EEP_FEATURES = [
     *EES_FEATURES,
+    { AUDITOR_USER_FEATURE => 1 },
     { DEPLOY_BOARD_FEATURE => 1 },
     { FILE_LOCK_FEATURE => 1 },
     { GEO_FEATURE => 1 },
-    { AUDITOR_USER_FEATURE => 1 },
-    { SERVICE_DESK_FEATURE => 1 },
-    { OBJECT_STORAGE_FEATURE => 1 }
+    { OBJECT_STORAGE_FEATURE => 1 },
+    { SERVICE_DESK_FEATURE => 1 }
   ].freeze
 
   EEU_FEATURES = [
@@ -78,6 +82,7 @@ class License < ActiveRecord::Base
     { FAST_FORWARD_MERGE_FEATURE => 1 },
     { FILE_LOCK_FEATURE => 1 },
     { GEO_FEATURE => 1 },
+    { ISSUE_WEIGHTS_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
     { MERGE_REQUEST_SQUASH_FEATURE => 1 },
     { OBJECT_STORAGE_FEATURE => 1 },

app/models/milestone.rb

@@ -15,6 +15,8 @@ class Milestone < ActiveRecord::Base
   include Elastic::MilestonesSearch
   include Milestoneish
 
+  include ::EE::Milestone
+
   cache_markdown_field :title, pipeline: :single_line
   cache_markdown_field :description
 

app/serializers/issue_entity.rb

@@ -7,7 +7,7 @@ class IssueEntity < IssuableEntity
   expose :due_date
   expose :moved_to_id
   expose :project_id
-  expose :weight
+  expose :weight, if: ->(issue, _) { issue.supports_weight? }
   expose :milestone, using: API::Entities::Milestone
   expose :labels, using: LabelEntity
 

app/services/ee/issuable_base_service.rb

+module EE
+  module IssuableBaseService
+    private
+
+    def filter_params(issuable)
+      params.delete(:weight) unless issuable.supports_weight?
+
+      super
+    end
+  end
+end

app/services/issuable_base_service.rb

 class IssuableBaseService < BaseService
+  prepend ::EE::IssuableBaseService
+
   private
 
   def create_milestone_note(issuable)

app/services/quick_actions/interpret_service.rb

@@ -470,7 +470,7 @@ module QuickActions
     end
     params Issue::WEIGHT_RANGE.to_s.squeeze('.').tr('.', '-')
     condition do
-      issuable.respond_to?(:weight) &&
+      issuable.supports_weight? &&
         current_user.can?(:"admin_#{issuable.to_ability_name}", issuable)
     end
     parse_params do |weight|
@@ -484,7 +484,7 @@ module QuickActions
     explanation 'Clears weight.'
     condition do
       issuable.persisted? &&
-        issuable.respond_to?(:weight) &&
+        issuable.supports_weight? &&
         issuable.weight? &&
         current_user.can?(:"admin_#{issuable.to_ability_name}", issuable)
     end

app/views/shared/_sort_dropdown.html.haml

+- viewing_issues = controller.controller_name == 'issues' || controller.action_name == 'issues'
+
 .dropdown.inline.prepend-left-10
   %button.dropdown-toggle{ type: 'button', data: {toggle: 'dropdown' } }
     - if @sort.present?
@@ -19,16 +21,18 @@
         = sort_title_recently_updated
       = link_to page_filter_path(sort: sort_value_oldest_updated, label: true) do
         = sort_title_oldest_updated
-      - if local_assigns[:type] == :issues
+
+      - if viewing_issues && (@project || @group)&.feature_available?(:issue_weights)
         = link_to page_filter_path(sort: sort_value_more_weight, label: true) do
           = sort_title_more_weight
         = link_to page_filter_path(sort: sort_value_less_weight, label: true) do
           = sort_title_less_weight
+
       = link_to page_filter_path(sort: sort_value_milestone_soon, label: true) do
         = sort_title_milestone_soon
       = link_to page_filter_path(sort: sort_value_milestone_later, label: true) do
         = sort_title_milestone_later
-      - if controller.controller_name == 'issues' || controller.action_name == 'issues'
+      - if viewing_issues
         = link_to page_filter_path(sort: sort_value_due_date_soon, label: true) do
           = sort_title_due_date_soon
         = link_to page_filter_path(sort: sort_value_due_date_later, label: true) do

app/views/shared/issuable/_search_bar.html.haml

@@ -127,7 +127,7 @@
                   = dropdown_loading
               #js-add-issues-btn.prepend-left-10
           - elsif type != :boards_modal
-            = render 'shared/sort_dropdown', type: local_assigns[:type]
+            = render 'shared/sort_dropdown'
 
 - unless type === :boards_modal
   :javascript

app/views/shared/issuable/_sidebar.html.haml

@@ -115,7 +115,7 @@
                 - if can? current_user, :admin_label, @project and @project
                   = render partial: "shared/issuable/label_page_create"
 
-      - if issuable.respond_to?(:weight)
+      - if issuable.supports_weight?
         .block.weight
           .sidebar-collapsed-icon
             = icon('balance-scale')

app/views/shared/issuable/form/_weight.html.haml

 - issuable = local_assigns.fetch(:issuable)
 
-- return unless issuable.respond_to?(:weight)
+- return unless issuable.supports_weight?
 
 - has_due_date = issuable.has_attribute?(:due_date)
 - form = local_assigns.fetch(:form)

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

@@ -85,22 +85,7 @@
               Closed:
               = milestone.issues_visible_to_user(current_user).closed.count
 
-    - total_weight = milestone.issues_visible_to_user(current_user).sum(:weight)
-    .block.weight
-      .sidebar-collapsed-icon
-        = icon('balance-scale')
-        %span
-          - unless total_weight.zero?
-            = total_weight
-          - else
-            None
-      .title.hide-collapsed
-        Total issue weight
-      .value.hide-collapsed
-        - unless total_weight.zero?
-          %strong.bold= total_weight
-        - else
-          .no-value None
+    = render 'shared/milestones/weight', milestone: milestone
 
     .block.merge-requests
       .sidebar-collapsed-icon

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

+- milestone = local_assigns.fetch(:milestone)
+
+- return unless milestone.supports_weight?
+
+- total_weight = milestone.issues_visible_to_user(current_user).sum(:weight)
+.block.weight
+  .sidebar-collapsed-icon
+    = icon('balance-scale')
+    %span
+      - unless total_weight.zero?
+        = total_weight
+      - else
+        None
+  .title.hide-collapsed
+    Total issue weight
+  .value.hide-collapsed
+    - unless total_weight.zero?
+      %strong.bold= total_weight
+    - else
+      .no-value None
+

changelogs/unreleased-ee/2575-namespace-license-issue-weights.yml

+---
+title: Introduce namespace licensing for issue weights (EES)
+merge_request: 2291
+author:

lib/api/entities.rb

@@ -313,7 +313,7 @@ module API
       expose :upvotes, :downvotes
       expose :due_date
       expose :confidential
-      expose :weight
+      expose :weight, if: ->(issue, _) { issue.supports_weight? }
 
       expose :web_url do |issue, options|
         Gitlab::UrlBuilder.build(issue)

spec/controllers/projects/issues_controller_ee_spec.rb

@@ -76,4 +76,120 @@ describe Projects::IssuesController do
       end
     end
   end
+
+  describe 'issue weights' do
+    let(:project) { create(:project) }
+    let(:user) { create(:user) }
+    let(:issue) { create(:issue, project: project, weight: 5) }
+    let(:issue2) { create(:issue, project: project, weight: 1) }
+    let(:new_issue) { build(:issue, project: project, weight: 5) }
+
+    before do
+      project.add_developer(user)
+      sign_in(user)
+    end
+
+    def perform(method, action, opts = {})
+      send(method, action, opts.merge(namespace_id: project.namespace.to_param, project_id: project.to_param))
+    end
+
+    context 'licensed' do
+      before do
+        stub_licensed_features(issue_weights: true)
+      end
+
+      describe '#index' do
+        it 'allows sorting by weight (ascending)' do
+          expected = [issue, issue2].sort_by(&:weight)
+
+          perform :get, :index, sort: 'weight_asc'
+
+          expect(response).to have_http_status(200)
+          expect(assigns(:issues)).to eq(expected)
+        end
+
+        it 'allows sorting by weight (descending)' do
+          expected = [issue, issue2].sort { |a, b| b.weight <=> a.weight }
+
+          perform :get, :index, sort: 'weight_desc'
+
+          expect(response).to have_http_status(200)
+          expect(assigns(:issues)).to eq(expected)
+        end
+
+        it 'allows filtering by weight' do
+          _ = issue
+          _ = issue2
+
+          perform :get, :index, weight: 1
+          
+          expect(response).to have_http_status(200)
+          expect(assigns(:issues)).to eq([issue2])
+        end
+      end
+
+      describe '#update' do
+        it 'sets issue weight' do
+          perform :put, :update, id: issue.to_param, issue: { weight: 6 }, format: :json
+
+          expect(response).to have_http_status(200)
+          expect(issue.reload.weight).to eq(6)
+        end
+      end
+
+      describe '#create' do
+        it 'sets issue weight' do
+          perform :post, :create, issue: new_issue.attributes
+
+          expect(response).to have_http_status(302)
+          expect(Issue.count).to eq(1)
+
+          issue = Issue.first
+          expect(issue.weight).to eq(new_issue.weight)
+        end
+      end
+    end
+
+    context 'unlicensed' do
+      before do
+        stub_licensed_features(issue_weights: false)
+      end
+
+      describe '#index' do
+        it 'ignores sorting by weight (ascending)'
+        it 'ignores sorting by weight (descending)'
+
+        it 'ignores filtering by weight' do
+          expected = [issue, issue2]
+
+          perform :get, :index, weight: 1
+
+          expect(response).to have_http_status(200)
+          expect(assigns(:issues)).to match_array(expected)
+        end
+      end
+
+      describe '#update' do
+        it 'does not set issue weight' do
+          perform :put, :update, id: issue.to_param, issue: { weight: 6 }, format: :json
+
+          expect(response).to have_http_status(200)
+          expect(issue.reload.weight).to be_nil
+          expect(issue.reload.read_attribute(:weight)).to eq(5) # pre-existing data is not overwritten
+        end
+      end
+
+      describe '#create' do
+        it 'does not set issue weight' do
+          perform :post, :create, issue: new_issue.attributes
+
+          expect(response).to have_http_status(302)
+          expect(Issue.count).to eq(1)
+
+          issue = Issue.first
+          expect(issue.read_attribute(:weight)).to be_nil
+        end
+      end
+    end
+  end
 end

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

@@ -88,11 +88,7 @@ describe 'Milestones on EE', feature: true do
       end
     end
 
-    context 'with the burndown chart feature disabled' do
-      before do
-        stub_licensed_features(burndown_charts: false)
-      end
-
+    shared_examples 'burndown charts disabled' do
       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`
@@ -118,6 +114,22 @@ describe 'Milestones on EE', feature: true do
         end
       end
     end
+
+    context 'with the burndown chart feature disabled' do
+      before do
+        stub_licensed_features(burndown_charts: false)
+      end
+
+      include_examples 'burndown charts disabled'
+    end
+
+    context 'with the issuable weights feature disabled' do
+      before do
+        stub_licensed_features(issue_weights: false)
+      end
+
+      include_examples 'burndown charts disabled'
+    end
   end
 
   context 'milestone summary' do

spec/models/ee/issue_spec.rb

+require 'spec_helper'
+
+describe Issue do
+  describe '#weight' do
+    [
+      { license: true,  database: 5,    expected: 5 },
+      { license: true,  database: nil,  expected: nil },
+      { license: false, database: 5,    expected: nil },
+      { license: false, database: nil,  expected: nil }
+    ].each do |spec|
+      context spec.inspect do
+        let(:spec) { spec }
+        let(:issue) { build(:issue, weight: spec[:database]) }
+
+        subject { issue.weight }
+
+        before do
+          stub_licensed_features(issue_weights: spec[:license])
+        end
+
+        it { is_expected.to eq(spec[:expected]) }
+      end
+    end
+  end
+end

spec/requests/api/issues_spec.rb

@@ -1248,6 +1248,20 @@ describe API::Issues do
       expect(response).to have_http_status(400)
       expect(json_response['error']).to eq('weight does not have a valid value')
     end
+
+    context 'issuable weights unlicensed' do
+      before do
+        stub_licensed_features(issue_weights: false)
+      end
+
+      it 'ignores the update' do
+        put api("/projects/#{project.id}/issues/#{issue.iid}", user), weight: 5
+
+        expect(response).to have_http_status(200)
+        expect(json_response['weight']).to be_nil
+        expect(issue.reload.read_attribute(:weight)).to be_nil
+      end
+    end
   end
 
   describe "DELETE /projects/:id/issues/:issue_iid" do

spec/requests/api/v3/issues_spec.rb

@@ -1189,6 +1189,20 @@ describe API::V3::Issues do
       expect(response).to have_http_status(400)
       expect(json_response['error']).to eq('weight does not have a valid value')
     end
+
+    context 'issuable weights unlicensed' do
+      before do
+        stub_licensed_features(issue_weights: false)
+      end
+
+      it 'ignores the update' do
+        put v3_api("/projects/#{project.id}/issues/#{issue.id}", user), weight: 5
+
+        expect(response).to have_http_status(200)
+        expect(json_response['weight']).to be_nil
+        expect(issue.reload.read_attribute(:weight)).to be_nil
+      end
+    end
   end
 
   describe "DELETE /projects/:id/issues/:issue_id" do

spec/services/quick_actions/interpret_service_spec.rb

@@ -725,6 +725,11 @@ describe QuickActions::InterpretService, services: true do
       let(:issuable) { issue }
     end
 
+    context 'issuable weights licensed' do
+      before do
+        stub_licensed_features(issue_weights: true)
+      end
+
       it_behaves_like 'weight command' do
         let(:content) { '/weight 5'}
         let(:issuable) { issue }
@@ -734,6 +739,25 @@ describe QuickActions::InterpretService, services: true do
         let(:content) { '/clear_weight' }
         let(:issuable) { issue }
       end
+    end
+
+    context 'issuable weights unlicensed' do
+      before do
+        stub_licensed_features(issue_weights: false)
+      end
+
+      it 'does not recognise /weight X' do
+        _, updates = service.execute('/weight 5', issue)
+
+        expect(updates).to be_empty
+      end
+
+      it 'does not recognise /clear_weight' do
+        _, updates = service.execute('/clear_weight', issue)
+
+        expect(updates).to be_empty
+      end
+    end
 
     context 'when current_user cannot :admin_issue' do
       let(:visitor) { create(:user) }