Merge branch '11403-max-issue-count-on-lists' into 'master'

Add max issue count to lists

See merge request gitlab-org/gitlab!15116

app/controllers/boards/lists_controller.rb

@@ -64,12 +64,16 @@ module Boards
       %i[label_id]
     end
 
+    def list_update_attrs
+      %i[collapsed position]
+    end
+
     def create_list_params
       params.require(:list).permit(list_creation_attrs)
     end
 
     def update_list_params
-      params.require(:list).permit(:collapsed, :position)
+      params.require(:list).permit(list_update_attrs)
     end
 
     def serialize_as_json(resource)

app/services/boards/lists/create_service.rb

@@ -43,7 +43,11 @@ module Boards
       end
 
       def create_list(board, type, target, position)
-        board.lists.create(type => target, list_type: type, position: position)
+        board.lists.create(create_list_attributes(type, target, position))
+      end
+
+      def create_list_attributes(type, target, position)
+        { type => target, list_type: type, position: position }
       end
     end
   end

app/services/boards/lists/update_service.rb

@@ -4,16 +4,22 @@ module Boards
   module Lists
     class UpdateService < Boards::BaseService
       def execute(list)
-        update_preferences_result = update_preferences(list) if can_read?(list)
-        update_position_result = update_position(list) if can_admin?(list)
-
-        if update_preferences_result || update_position_result
+        if execute_by_params(list)
           success(list: list)
         else
           error(list.errors.messages, 422)
         end
       end
 
+      private
+
+      def execute_by_params(list)
+        update_preferences_result = update_preferences(list) if can_read?(list)
+        update_position_result = update_position(list) if can_admin?(list)
+
+        update_preferences_result || update_position_result
+      end
+
       def update_preferences(list)
         return unless preferences?
 
@@ -50,3 +56,5 @@ module Boards
     end
   end
 end
+
+Boards::Lists::UpdateService.prepend_if_ee('EE::Boards::Lists::UpdateService')

db/migrate/20190901174200_add_max_issue_count_to_list.rb

+# frozen_string_literal: true
+
+class AddMaxIssueCountToList < ActiveRecord::Migration[4.2]
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    add_column_with_default :lists, :max_issue_count, :integer, default: 0
+  end
+
+  def down
+    remove_column :lists, :max_issue_count
+  end
+end

db/schema.rb

@@ -2014,6 +2014,7 @@ ActiveRecord::Schema.define(version: 2019_09_19_162036) do
     t.datetime "updated_at", null: false
     t.integer "user_id"
     t.integer "milestone_id"
+    t.integer "max_issue_count", default: 0, null: false
     t.index ["board_id", "label_id"], name: "index_lists_on_board_id_and_label_id", unique: true
     t.index ["label_id"], name: "index_lists_on_label_id"
     t.index ["list_type"], name: "index_lists_on_list_type"

doc/api/boards.md

@@ -48,7 +48,8 @@ Example response:
           "color" : "#F0AD4E",
           "description" : null
         },
-        "position" : 1
+        "position" : 1,
+        "max_issue_count": 0
       },
       {
         "id" : 2,
@@ -57,7 +58,8 @@ Example response:
           "color" : "#FF0000",
           "description" : null
         },
-        "position" : 2
+        "position" : 2,
+        "max_issue_count": 0
       },
       {
         "id" : 3,
@@ -66,7 +68,8 @@ Example response:
           "color" : "#FF5F00",
           "description" : null
         },
-        "position" : 3
+        "position" : 3,
+        "max_issue_count": 0
       }
     ]
   }
@@ -117,7 +120,8 @@ Example response:
           "color" : "#F0AD4E",
           "description" : null
         },
-        "position" : 1
+        "position" : 1,
+        "max_issue_count": 0
       },
       {
         "id" : 2,
@@ -126,7 +130,8 @@ Example response:
           "color" : "#FF0000",
           "description" : null
         },
-        "position" : 2
+        "position" : 2,
+        "max_issue_count": 0
       },
       {
         "id" : 3,
@@ -135,7 +140,8 @@ Example response:
           "color" : "#FF5F00",
           "description" : null
         },
-        "position" : 3
+        "position" : 3,
+        "max_issue_count": 0
       }
     ]
   }
@@ -185,7 +191,8 @@ Example response:
           "color" : "#F0AD4E",
           "description" : null
         },
-        "position" : 1
+        "position" : 1,
+        "max_issue_count": 0
       },
       {
         "id" : 2,
@@ -194,7 +201,8 @@ Example response:
           "color" : "#FF0000",
           "description" : null
         },
-        "position" : 2
+        "position" : 2,
+        "max_issue_count": 0
       },
       {
         "id" : 3,
@@ -203,7 +211,8 @@ Example response:
           "color" : "#FF5F00",
           "description" : null
         },
-        "position" : 3
+        "position" : 3,
+        "max_issue_count": 0
       }
     ]
   }
@@ -336,7 +345,8 @@ Example response:
       "color" : "#F0AD4E",
       "description" : null
     },
-    "position" : 1
+    "position" : 1,
+    "max_issue_count": 0
   },
   {
     "id" : 2,
@@ -345,7 +355,8 @@ Example response:
       "color" : "#FF0000",
       "description" : null
     },
-    "position" : 2
+    "position" : 2,
+    "max_issue_count": 0
   },
   {
     "id" : 3,
@@ -354,7 +365,8 @@ Example response:
       "color" : "#FF5F00",
       "description" : null
     },
-    "position" : 3
+    "position" : 3,
+    "max_issue_count": 0
   }
 ]
 ```
@@ -387,7 +399,8 @@ Example response:
     "color" : "#F0AD4E",
     "description" : null
   },
-  "position" : 1
+  "position" : 1,
+  "max_issue_count": 0
 }
 ```
 
@@ -427,7 +440,8 @@ Example response:
     "color" : "#F0AD4E",
     "description" : null
   },
-  "position" : 1
+  "position" : 1,
+  "max_issue_count": 0
 }
 ```
 
@@ -460,7 +474,8 @@ Example response:
     "color" : "#F0AD4E",
     "description" : null
   },
-  "position" : 1
+  "position" : 1,
+  "max_issue_count": 0
 }
 ```
 

ee/app/controllers/ee/boards/lists_controller.rb

@@ -7,12 +7,30 @@ module EE
 
       override :list_creation_attrs
       def list_creation_attrs
-        super + %i[assignee_id milestone_id]
+        additional_attrs = %i[assignee_id milestone_id]
+        additional_attrs << :max_issue_count if wip_limits_available?
+
+        super + additional_attrs
+      end
+
+      override :list_update_attrs
+      def list_update_attrs
+        return super unless wip_limits_available?
+
+        super + %i[max_issue_count]
       end
 
       override :serialization_attrs
       def serialization_attrs
-        super.merge(user: true, milestone: true)
+        super.merge(user: true, milestone: true).tap do |attrs|
+          attrs[:only] << :max_issue_count if wip_limits_available?
+        end
+      end
+
+      private
+
+      def wip_limits_available?
+        board_parent.feature_available?(:wip_limits)
       end
     end
   end

ee/app/models/ee/list.rb

@@ -18,6 +18,7 @@ module EE
       base.validates :milestone, presence: true, if: :milestone?
       base.validates :user_id, uniqueness: { scope: :board_id }, if: :assignee?
       base.validates :milestone_id, uniqueness: { scope: :board_id }, if: :milestone?
+      base.validates :max_issue_count, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
       base.validates :list_type,
         exclusion: { in: %w[assignee], message: _('Assignee lists not available with your current license') },
         unless: -> { board&.parent&.feature_available?(:board_assignee_lists) }

ee/app/models/license.rb

@@ -36,6 +36,7 @@ class License < ApplicationRecord
     scoped_issue_board
     usage_quotas
     visual_review_app
+    wip_limits
   ].freeze
 
   EEP_FEATURES = EES_FEATURES + %i[

ee/app/services/ee/boards/lists/create_service.rb

@@ -33,6 +33,19 @@ module EE
           end
         end
 
+        override :create_list_attributes
+        def create_list_attributes(type, target, position)
+          max_issue_count = if wip_limits_available?
+                              params[:max_issue_count] || 0
+                            else
+                              0
+                            end
+
+          super.merge(max_issue_count: max_issue_count)
+        end
+
+        private
+
         def find_milestone(board)
           milestones = milestone_finder(board).execute
           milestones.find(params['milestone_id'])
@@ -52,6 +65,10 @@ module EE
         def user_finder(board)
           @user_finder ||= ::Boards::UsersFinder.new(board, current_user)
         end
+
+        def wip_limits_available?
+          parent.feature_available?(:wip_limits)
+        end
       end
     end
   end

ee/app/services/ee/boards/lists/update_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Boards
+    module Lists
+      module UpdateService
+        extend ::Gitlab::Utils::Override
+
+        private
+
+        override :execute_by_params
+        def execute_by_params(list)
+          updated_max_issue_count = update_max_issue_count(list) if can_admin?(list)
+
+          super || updated_max_issue_count
+        end
+
+        def max_issue_count?(list)
+          params.has_key?(:max_issue_count) && list.board.parent.feature_available?(:wip_limits)
+        end
+
+        def update_max_issue_count(list)
+          return unless max_issue_count?(list)
+
+          max_issue_count = params[:max_issue_count] || 0
+
+          list.update(max_issue_count: max_issue_count)
+        end
+      end
+    end
+  end
+end

ee/changelogs/unreleased/11403-max-issue-count-on-lists.yml

+---
+title: Add max issue count to lists
+merge_request: 15116
+author:
+type: added

ee/lib/ee/api/entities.rb

@@ -168,6 +168,7 @@ module EE
         prepended do
           expose :milestone, using: ::API::Entities::Milestone, if: -> (entity, _) { entity.milestone? }
           expose :user, as: :assignee, using: ::API::Entities::UserSafe, if: -> (entity, _) { entity.assignee? }
+          expose :max_issue_count, if: -> (list, _) { list.board.parent.feature_available?(:wip_limits) }
         end
       end
 

ee/spec/controllers/boards/lists_controller_spec.rb

@@ -66,6 +66,36 @@ describe Boards::ListsController do
       end
     end
 
+    context 'with max issue count' do
+      let(:label) { create(:group_label, group: group, name: 'Development') }
+
+      context 'with licensed wip limits' do
+        before do
+          stub_licensed_features(wip_limits: true)
+        end
+
+        it 'returns the created list' do
+          create_board_list user: user, board: board, label_id: label.id, max_issue_count: 2
+
+          expect(response).to match_response_schema('list', dir: 'ee')
+          expect(json_response).to include('max_issue_count' => 2)
+        end
+      end
+
+      context 'without licensed wip limits' do
+        before do
+          stub_licensed_features(wip_limits: false)
+        end
+
+        it 'ignores max issue count' do
+          create_board_list user: user, board: board, label_id: label.id, max_issue_count: 2
+
+          expect(response).to match_response_schema('list', dir: 'ee')
+          expect(json_response).not_to include('max_issue_count')
+        end
+      end
+    end
+
     context 'with invalid params' do
       context 'when label is nil' do
         it 'returns a not found 404 response' do
@@ -96,12 +126,12 @@ describe Boards::ListsController do
       end
     end
 
-    def create_board_list(user:, board:, label_id:)
+    def create_board_list(user:, board:, label_id:, max_issue_count: 0)
       sign_in(user)
 
       post :create, params: {
                       board_id: board.to_param,
-                      list: { label_id: label_id }
+                      list: { label_id: label_id, max_issue_count: max_issue_count }
                     },
                     format: :json
     end
@@ -111,6 +141,132 @@ describe Boards::ListsController do
     let!(:planning)    { create(:list, board: board, position: 0) }
     let!(:development) { create(:list, board: board, position: 1) }
 
+    context 'when updating max issue count' do
+      before do
+        sign_in(user)
+        stub_licensed_features(wip_limits: true)
+      end
+
+      it 'returns a successful 200 response' do
+        params = update_params_with_max_issue_count_of(42)
+
+        patch :update, params: params, as: :json
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(development.reload.max_issue_count).to eq(42)
+      end
+
+      context 'multiple fields update behavior' do
+        shared_examples 'field updates' do
+          it 'updates fields as expected' do
+            params = update_params_with_list_params(update_params)
+
+            patch :update, params: params, as: :json
+
+            expect(response).to have_gitlab_http_status(200)
+
+            reloaded_list = development.reload
+            expect(reloaded_list.position).to eq(expected_position)
+            expect(reloaded_list.preferences_for(user).collapsed).to eq(expected_collapsed)
+            expect(reloaded_list.max_issue_count).to eq(expected_max_issue_count)
+          end
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { max_issue_count: 99, position: 0, collapsed: true } }
+
+          let(:expected_position) { 0 }
+          let(:expected_collapsed) { true }
+          let(:expected_max_issue_count) { 99 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { position: 0, collapsed: true } }
+
+          let(:expected_position) { 0 }
+          let(:expected_collapsed) { true }
+          let(:expected_max_issue_count) { 0 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { position: 0 } }
+
+          let(:expected_position) { 0 }
+          let(:expected_collapsed) { nil }
+          let(:expected_max_issue_count) { 0 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { max_issue_count: 42 } }
+
+          let(:expected_position) { 1 }
+          let(:expected_collapsed) { nil }
+          let(:expected_max_issue_count) { 42 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { collapsed: true } }
+
+          let(:expected_position) { 1 }
+          let(:expected_collapsed) { true }
+          let(:expected_max_issue_count) { 0 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { max_issue_count: 42, collapsed: true } }
+
+          let(:expected_position) { 1 }
+          let(:expected_collapsed) { true }
+          let(:expected_max_issue_count) { 42 }
+        end
+
+        it_behaves_like 'field updates' do
+          let(:update_params) { { max_issue_count: 42, position: 0 } }
+
+          let(:expected_position) { 0 }
+          let(:expected_collapsed) { nil }
+          let(:expected_max_issue_count) { 42 }
+        end
+      end
+
+      it 'fails if negative max_issue_count is provided' do
+        params = update_params_with_max_issue_count_of(-1)
+
+        patch :update, params: params, as: :json
+
+        expect(response).to have_gitlab_http_status(422)
+        expect(development.reload.max_issue_count).not_to eq(-1)
+      end
+
+      context 'when wip limits are not licensed' do
+        before do
+          stub_licensed_features(wip_limits: false)
+        end
+
+        it 'fails to update max issue count with expected status' do
+          params = update_params_with_max_issue_count_of(2)
+
+          patch :update, params: params, as: :json
+
+          expect(response).to have_gitlab_http_status(422)
+          expect(development.reload.max_issue_count).not_to eq(2)
+        end
+      end
+
+      def update_params_with_max_issue_count_of(count)
+        update_params_with_list_params(max_issue_count: count)
+      end
+
+      def update_params_with_list_params(list_update_params)
+        { namespace_id: group.to_param,
+          project_id: board.project,
+          board_id: board.to_param,
+          id: development.to_param,
+          list: list_update_params,
+          format: :json }
+      end
+    end
+
     context 'with valid position' do
       it 'returns a successful 200 response' do
         move user: user, board: board, list: planning, position: 1

ee/spec/models/ee/list_spec.rb

@@ -10,6 +10,10 @@ describe List do
     it { is_expected.to belong_to(:milestone) }
   end
 
+  describe 'validations' do
+    it { is_expected.to validate_numericality_of(:max_issue_count).only_integer.is_greater_than_or_equal_to(0) }
+  end
+
   context 'when it is an assignee type' do
     subject { described_class.new(list_type: :assignee, board: board) }
 

ee/spec/requests/api/boards_spec.rb

@@ -34,4 +34,32 @@ describe API::Boards do
       expect(json_response["milestone"]["title"]).to eq(Milestone::Started.title)
     end
   end
+
+  describe 'GET /projects/:id/boards/:board_id/lists with max_issue_count' do
+    let(:url) { "/projects/#{board_parent.id}/boards/#{board.id}/lists" }
+
+    let!(:list) { create(:list, board: board) }
+
+    context 'with WIP limits license' do
+      it 'includes max_issue_count' do
+        stub_licensed_features(wip_limits: true)
+
+        get api(url, user)
+
+        expect(json_response).not_to be_empty
+        expect(json_response.all? { |list_response| list_response.include?('max_issue_count') }).to be_truthy
+      end
+    end
+
+    context 'without WIP limits license' do
+      it 'does not include max_issue_count' do
+        stub_licensed_features(wip_limits: false)
+
+        get api(url, user)
+
+        expect(json_response).not_to be_empty
+        expect(json_response.none? { |list_response| list_response.include?('max_issue_count') }).to be_truthy
+      end
+    end
+  end
 end

ee/spec/services/boards/lists/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'EE::Boards::Lists::UpdateService' do
+  let(:group) { create(:group) }
+  let(:user) { create(:group_member, :owner, group: group, user: create(:user)).user }
+  let(:unpriviledged_user) { create(:group_member, :guest, group: group, user: create(:user)).user }
+
+  shared_examples 'board list update' do
+    context 'with licensed wip limits' do
+      before do
+        stub_licensed_features(wip_limits: true)
+      end
+
+      it 'updates the list if max_issue_count is given' do
+        service = Boards::Lists::UpdateService.new(board, user, max_issue_count: 42)
+        expect(service.execute(list)).to be_truthy
+
+        reloaded_list = list.reload
+        expect(reloaded_list.max_issue_count).to eq(42)
+      end
+
+      it 'updates the list with max_issue_count of 0 if max_issue_count is nil' do
+        service = Boards::Lists::UpdateService.new(board, user, max_issue_count: nil)
+        expect(service.execute(list)).to be_truthy
+
+        reloaded_list = list.reload
+        expect(reloaded_list.max_issue_count).to eq(0)
+      end
+
+      it 'does not update the list if can_admin returns false' do
+        service = Boards::Lists::UpdateService.new(board, unpriviledged_user, max_issue_count: 42)
+        expect(service.execute(list)).to be_truthy
+
+        reloaded_list = list.reload
+        expect(reloaded_list.max_issue_count).to eq(0)
+      end
+    end
+
+    context 'without licensed wip limits' do
+      before do
+        stub_licensed_features(wip_limits: false)
+      end
+
+      it 'does not update the list even if max_issue_count is given' do
+        service = Boards::Lists::UpdateService.new(board, user, max_issue_count: 42)
+        expect(service.execute(list)).to be_truthy
+
+        reloaded_list = list.reload
+        expect(reloaded_list.max_issue_count).to eq(0)
+      end
+
+      it 'does not update the list if can_admin returns false' do
+        service = Boards::Lists::UpdateService.new(board, unpriviledged_user, max_issue_count: 42)
+        expect(service.execute(list)).to be_truthy
+
+        reloaded_list = list.reload
+        expect(reloaded_list.max_issue_count).to eq(0)
+      end
+    end
+  end
+
+  context 'with project' do
+    let(:project_board) { create(:board, project: project) }
+    let(:project) { create(:project, group: group) }
+    let(:project_board_list) { create(:list, board: project_board) }
+    let(:board) { project_board }
+    let(:list) { project_board_list }
+
+    before do
+      project.add_maintainer(user)
+    end
+
+    it_behaves_like 'board list update'
+  end
+
+  context 'with group' do
+    let(:group) { create(:group) }
+    let(:group_board) { create(:board, group: group) }
+    let(:group_board_list) { create(:list, board: group_board) }
+    let(:board) { group_board }
+    let(:list) { group_board_list }
+
+    before do
+      group.add_owner(user)
+    end
+
+    it_behaves_like 'board list update'
+  end
+end

ee/spec/services/ee/boards/lists/create_service_spec.rb

@@ -4,9 +4,9 @@ describe Boards::Lists::CreateService do
   describe '#execute' do
     let(:project) { create(:project) }
     let(:board) { create(:board, project: project) }
+    let(:user) { create(:user) }
 
     context 'when assignee_id param is sent' do
-      let(:user) { create(:user) }
       let(:other_user) { create(:user) }
       subject(:service) { described_class.new(project, user, 'assignee_id' => other_user.id) }
 
@@ -43,5 +43,31 @@ describe Boards::Lists::CreateService do
         expect(list).to be_valid
       end
     end
+
+    context 'wip limits' do
+      describe '#create_list_attributes' do
+        subject(:service) { described_class.new(project, user, max_issue_count: 42) }
+
+        context 'license unavailable' do
+          before do
+            stub_licensed_features(wip_limits: false)
+          end
+
+          it 'contains a max_issue_count of 0' do
+            expect(service.create_list_attributes(nil, nil, nil)).to include(max_issue_count: 0)
+          end
+        end
+
+        context 'license available' do
+          before do
+            stub_licensed_features(wip_limits: true)
+          end
+
+          it 'contains the params provided max issue count' do
+            expect(service.create_list_attributes(nil, nil, nil)).to include(max_issue_count: 42)
+          end
+        end
+      end
+    end
   end
 end

spec/fixtures/api/schemas/list.json

@@ -35,7 +35,8 @@
       }
     },
     "title": { "type": "string" },
-    "position": { "type": ["integer", "null"] }
+    "position": { "type": ["integer", "null"] },
+    "max_issue_count": { "type": "integer" }
   },
   "additionalProperties": true
 }

spec/fixtures/api/schemas/public_api/v4/board.json

@@ -76,7 +76,8 @@
               "name": { "type": "string" }
             }
           },
-          "position": { "type": ["integer", "null"] }
+          "position": { "type": ["integer", "null"] },
+          "max_issue_count": { "type": "integer" }
         },
         "additionalProperties": false
       }

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -717,6 +717,7 @@ List:
 - updated_at
 - milestone_id
 - user_id
+- max_issue_count
 ExternalPullRequest:
 - id
 - created_at