Add max issue count to lists

Also updating schema and specs.

Add model validation for max_issue_count

Add license check for max_issue_count

Add spec to check license handling

Move validation of max_issue_count and add creation list params

Move validation of max_issue_count

Add MR to changelog

Remove unnecessary comment

Remove unnecessary allow_null: false

Move changelog entry

Remove possible null value for max_issue_count from schema
definitions

Add max_issue_count response to documentation

Make methods private

Add condition to check max_issue_count was not changed in that situation

Replace stub_feature_flags with stub_licensed_features

Apply suggestion to ee/spec/controllers/boards/lists_controller_spec.rb
Add license check

Remove some new lines

Call super even if max issue count should be updated

Revert "Call super even if max issue count should be updated"

This reverts commit 7156ffa1464e69528757c06902db321f0cc28122.

Remove unnecessary validation

Move can_admin?(list) one level up

Apply new base request behavior

update of multiple fields is possible now
Add specs for create_list_attributes

Remove list_properties override

Add specs for UpdateService

Fix rubocop issue

Made execute_by_params private

Use parent to check for licensed feature

Use board_parent to check for available feature

Change order of checks

Move check into update method

Specs are now calling UpdateService

instead of just testing the module behavior

Use list board parent to check for feature availability

Add specs for conditional expose of max_issue_count

Use board parent licensed feature check

Apply pleitzens suggestion

Replace each / expect with all? / none? (rubocop)

Check result of service.execute

Apply suggested spec changes

Apply suggested changes to specs

Add check for empty response

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