kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Add endpoint to move multiple issues 

Add specs for new endpoint to move multiple issues.
Add changelog entry

Just check the first issue for the ability to move / update

Add specs for exceeding limits and malformed requests

Changed name of shared examples

Change title of changelog entry

Use %i instead of %w

Check permission to update issue on project instead of board

Use admin_issue permission to check for issue move ability

Changed variable name to avoid shadow issue_params method

Rename route to bulk_move

Change route definition

Check permissions for each issue

Combine methods for parameters permit check

Remove extra context

Change description of context

Check param for type Array

Add unit tests to MoveService

Use before_action for permission check

Use set instead of let!

Use let's instead of set
This commit is contained in:
Patrick Derichs 2019-07-11 15:29:16 +02:00
parent e6f8e12081
commit 69e02904fe
6 changed files with 354 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
app/controllers/boards/issues_controller.rb
View File

@ -2,16 +2,24 @@
module Boards
class IssuesController < Boards::ApplicationController
# This is the maximum amount of issues which can be moved by one request to
# bulk_move for now. This is temporary and might be removed in future by
# introducing an alternative (async?) approach.
# (related: https://gitlab.com/groups/gitlab-org/-/epics/382)
MAX_MOVE_ISSUES_COUNT = 50
include BoardsResponses
include ControllerWithCrossProjectAccessCheck
requires_cross_project_access if: -> { board&.group_board? }
before_action :whitelist_query_limiting, only: [:index, :update]
before_action :whitelist_query_limiting, only: [:index, :update, :bulk_move]
before_action :authorize_read_issue, only: [:index]
before_action :authorize_create_issue, only: [:create]
before_action :authorize_update_issue, only: [:update]
skip_before_action :authenticate_user!, only: [:index]
before_action :validate_id_list, only: [:bulk_move]
before_action :can_move_issues?, only: [:bulk_move]
# rubocop: disable CodeReuse/ActiveRecord
def index
@ -46,6 +54,17 @@ module Boards
end
end
def bulk_move
service = Boards::Issues::MoveService.new(board_parent, current_user, move_params(true))
issues = Issue.find(params[:ids])
if service.execute_multiple(issues)
head :ok
else
head :unprocessable_entity
end
end
def update
service = Boards::Issues::MoveService.new(board_parent, current_user, move_params)
@ -58,6 +77,10 @@ module Boards
private
def can_move_issues?
head(:forbidden) unless can?(current_user, :admin_issue, board)
end
def render_issues(issues, metadata)
data = { issues: serialize_as_json(issues) }
data.merge!(metadata)
@ -90,8 +113,9 @@ module Boards
end
end
def move_params
params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_id, :move_after_id)
def move_params(multiple = false)
id_param = multiple ? :ids : :id
params.permit(id_param, :board_id, :from_list_id, :to_list_id, :move_before_id, :move_after_id)
end
def issue_params
@ -112,5 +136,10 @@ module Boards
# Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42439
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42428')
end
def validate_id_list
head(:bad_request) unless params[:ids].is_a?(Array)
head(:unprocessable_entity) if params[:ids].size > MAX_MOVE_ISSUES_COUNT
end
end
end

38
app/services/boards/issues/move_service.rb
View File

@ -4,14 +4,37 @@ module Boards
module Issues
class MoveService < Boards::BaseService
def execute(issue)
return false unless can?(current_user, :update_issue, issue)
return false if issue_params(issue).empty?
issue_modification_params = issue_params(issue)
return false if issue_modification_params.empty?
update(issue)
move_single_issue(issue, issue_modification_params)
end
def execute_multiple(issues)
return false if issues.empty?
last_inserted_issue_id = nil
issues.map do |issue|
issue_modification_params = issue_params(issue)
next if issue_modification_params.empty?
if last_inserted_issue_id
issue_modification_params[:move_between_ids] = move_between_ids({ move_after_id: nil, move_before_id: last_inserted_issue_id })
end
last_inserted_issue_id = issue.id
move_single_issue(issue, issue_modification_params)
end.all?
end
private
def move_single_issue(issue, issue_modification_params)
return false unless can?(current_user, :update_issue, issue)
update(issue, issue_modification_params)
end
def board
@board ||= parent.boards.find(params[:board_id])
end
@ -33,8 +56,8 @@ module Boards
end
# rubocop: enable CodeReuse/ActiveRecord
def update(issue)
::Issues::UpdateService.new(issue.project, current_user, issue_params(issue)).execute(issue)
def update(issue, issue_modification_params)
::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue)
end
def issue_params(issue)
@ -48,6 +71,7 @@ module Boards
)
end
move_between_ids = move_between_ids(params)
if move_between_ids
attrs[:move_between_ids] = move_between_ids
attrs[:board_group_id] = board.group&.id
@ -78,8 +102,8 @@ module Boards
end
# rubocop: enable CodeReuse/ActiveRecord
def move_between_ids
ids = [params[:move_after_id], params[:move_before_id]]
def move_between_ids(move_params)
ids = [move_params[:move_after_id], move_params[:move_before_id]]
.map(&:to_i)
.map { |m| m.positive? ? m : nil }

5
changelogs/unreleased/35757-move-issues-in-boards-pderichs.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Add endpoint to move multiple issues in boards
merge_request: 30216
author:
type: added

6
config/routes.rb
View File

@ -82,7 +82,11 @@ Rails.application.routes.draw do
resources :issues, only: [:index, :create, :update]
end
resources :issues, module: :boards, only: [:index, :update]
resources :issues, module: :boards, only: [:index, :update] do
collection do
put :bulk_move, format: :json
end
end
Gitlab.ee do
resources :users, module: :boards, only: [:index]

195
spec/controllers/boards/issues_controller_spec.rb
View File

@ -164,6 +164,201 @@ describe Boards::IssuesController do
end
end
describe 'PUT move_multiple' do
let(:todo) { create(:group_label, group: group, name: 'Todo') }
let(:development) { create(:group_label, group: group, name: 'Development') }
let(:user) { create(:group_member, :maintainer, user: create(:user), group: group ).user }
let(:guest) { create(:group_member, :guest, user: create(:user), group: group ).user }
let(:project) { create(:project, group: group) }
let(:group) { create(:group) }
let(:board) { create(:board, project: project) }
let(:list1) { create(:list, board: board, label: todo, position: 0) }
let(:list2) { create(:list, board: board, label: development, position: 1) }
let(:issue1) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 10) }
let(:issue2) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 20) }
let(:issue3) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 30) }
let(:issue4) { create(:labeled_issue, project: project, labels: [development], author: user, relative_position: 100) }
let(:move_params) do
{
board_id: board.id,
ids: [issue1.id, issue2.id, issue3.id],
from_list_id: list1.id,
to_list_id: list2.id,
move_before_id: issue4.id,
move_after_id: nil
}
end
before do
project.add_maintainer(user)
project.add_guest(guest)
end
shared_examples 'move issues endpoint provider' do
before do
sign_in(signed_in_user)
end
it 'moves issues as expected' do
put :bulk_move, params: move_issues_params
expect(response).to have_gitlab_http_status(expected_status)
list_issues user: requesting_user, board: board, list: list2
expect(response).to have_gitlab_http_status(200)
expect(response).to match_response_schema('entities/issue_boards')
responded_issues = json_response['issues']
expect(responded_issues.length).to eq expected_issue_count
ids_in_order = responded_issues.pluck('id')
expect(ids_in_order).to eq(expected_issue_ids_in_order)
end
end
context 'when items are moved to another list' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) { move_params }
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 4 }
let(:expected_issue_ids_in_order) { [issue4.id, issue1.id, issue2.id, issue3.id] }
end
end
context 'when moving just one issue' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:ids] = [issue2.id]
end
end
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 2 }
let(:expected_issue_ids_in_order) { [issue4.id, issue2.id] }
end
end
context 'when user is not allowed to move issue' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { guest }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:ids] = [issue2.id]
end
end
let(:requesting_user) { user }
let(:expected_status) { 403 }
let(:expected_issue_count) { 1 }
let(:expected_issue_ids_in_order) { [issue4.id] }
end
end
context 'when issues should be moved visually above existing issue in list' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:move_after_id] = issue4.id
hash[:move_before_id] = nil
end
end
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 4 }
let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id, issue4.id] }
end
end
context 'when destination list is empty' do
before do
# Remove issue from list
issue4.labels -= [development]
issue4.save!
end
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:move_before_id] = nil
end
end
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 3 }
let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id] }
end
end
context 'when no position arguments are given' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:move_before_id] = nil
end
end
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 4 }
let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id, issue4.id] }
end
end
context 'when move_before_id and move_after_id are given' do
let(:issue5) { create(:labeled_issue, project: project, labels: [development], author: user, relative_position: 90) }
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:move_before_id] = issue5.id
hash[:move_after_id] = issue4.id
end
end
let(:requesting_user) { user }
let(:expected_status) { 200 }
let(:expected_issue_count) { 5 }
let(:expected_issue_ids_in_order) { [issue5.id, issue1.id, issue2.id, issue3.id, issue4.id] }
end
end
context 'when request contains too many issues' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:ids] = (0..51).to_a
end
end
let(:requesting_user) { user }
let(:expected_status) { 422 }
let(:expected_issue_count) { 1 }
let(:expected_issue_ids_in_order) { [issue4.id] }
end
end
context 'when request is malformed' do
it_behaves_like 'move issues endpoint provider' do
let(:signed_in_user) { user }
let(:move_issues_params) do
move_params.dup.tap do |hash|
hash[:ids] = 'foobar'
end
end
let(:requesting_user) { user }
let(:expected_status) { 400 }
let(:expected_issue_count) { 1 }
let(:expected_issue_ids_in_order) { [issue4.id] }
end
end
end
def list_issues(user:, board:, list: nil)
sign_in(user)

86
spec/services/boards/issues/move_service_spec.rb
View File

@ -52,5 +52,91 @@ describe Boards::Issues::MoveService do
it_behaves_like 'issues move service', true
end
describe '#execute_multiple' do
set(:group) { create(:group) }
set(:user) { create(:user) }
set(:project) { create(:project, namespace: group) }
set(:board1) { create(:board, group: group) }
set(:development) { create(:group_label, group: group, name: 'Development') }
set(:testing) { create(:group_label, group: group, name: 'Testing') }
set(:list1) { create(:list, board: board1, label: development, position: 0) }
set(:list2) { create(:list, board: board1, label: testing, position: 1) }
let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } }
before do
project.add_developer(user)
end
it 'returns false if list of issues is empty' do
expect(described_class.new(group, user, params).execute_multiple([])).to eq(false)
end
context 'moving multiple issues' do
let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
it 'moves multiple issues from one list to another' do
expect(described_class.new(group, user, params).execute_multiple([issue1, issue2])).to be_truthy
expect(issue1.labels).to eq([testing])
expect(issue2.labels).to eq([testing])
end
end
context 'moving a single issue' do
let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
it 'moves one issue' do
expect(described_class.new(group, user, params).execute_multiple([issue1])).to be_truthy
expect(issue1.labels).to eq([testing])
end
end
context 'moving issues visually after an existing issue' do
let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) }
let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
let(:move_params) do
params.dup.tap do |hash|
hash[:move_before_id] = existing_issue.id
end
end
it 'moves one issue' do
expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy
expect(issue1.labels).to eq([testing])
expect(issue2.labels).to eq([testing])
expect(issue1.relative_position > existing_issue.relative_position).to eq(true)
expect(issue2.relative_position > issue1.relative_position).to eq(true)
end
end
context 'moving issues visually before an existing issue' do
let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) }
let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
let(:move_params) do
params.dup.tap do |hash|
hash[:move_after_id] = existing_issue.id
end
end
it 'moves one issue' do
expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy
expect(issue1.labels).to eq([testing])
expect(issue2.labels).to eq([testing])
expect(issue2.relative_position < existing_issue.relative_position).to eq(true)
expect(issue1.relative_position < issue2.relative_position).to eq(true)
end
end
end
end
end