diff --git a/CHANGELOG b/CHANGELOG index cb2f9d7210..7d672c9f87 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ [v#.#.#] ([month] [YYYY]) - [entity]: - [future tense verb] [feature] + - Cards: add option to move a task to a different list - Upgraded gems: - rails, rails-html-sanitizer, sqlite3, websocket-driver - Bugs fixes: diff --git a/app/assets/javascripts/hera/pages/boards/show.js.coffee b/app/assets/javascripts/hera/pages/boards/show.js.coffee index 51f3162af4..6ae87199d6 100644 --- a/app/assets/javascripts/hera/pages/boards/show.js.coffee +++ b/app/assets/javascripts/hera/pages/boards/show.js.coffee @@ -21,7 +21,7 @@ class SortableBoards url: url data: params dataType: 'json' - type: 'post' + type: 'patch' success: (data, status, xhr) -> if data.is_card $cardLink = $(".card[data-card-id='#{data.id}'] a") diff --git a/app/assets/stylesheets/hera/modules.scss b/app/assets/stylesheets/hera/modules.scss index 68229d7459..db9f188fae 100644 --- a/app/assets/stylesheets/hera/modules.scss +++ b/app/assets/stylesheets/hera/modules.scss @@ -199,10 +199,6 @@ margin-left: auto; .action { - &:not(:first-child)::before { - content: '-'; - } - &:first-child { margin-left: auto; } diff --git a/app/assets/stylesheets/hera/modules/_dropdown.scss b/app/assets/stylesheets/hera/modules/_dropdown.scss index bf5f93c486..4b34993dfe 100644 --- a/app/assets/stylesheets/hera/modules/_dropdown.scss +++ b/app/assets/stylesheets/hera/modules/_dropdown.scss @@ -36,6 +36,12 @@ font-weight: 600; } + &.disabled { + color: var(--text-muted); + cursor: not-allowed; + pointer-events: none; + } + i { margin-right: 0.25rem; } diff --git a/app/controllers/cards/position_controller.rb b/app/controllers/cards/position_controller.rb new file mode 100644 index 0000000000..0d24100cc1 --- /dev/null +++ b/app/controllers/cards/position_controller.rb @@ -0,0 +1,98 @@ +class Cards::PositionController < AuthenticatedController + include EventPublisher + include LinkedListMoveValidator + include ProjectScoped + + before_action :set_current_board_and_list + before_action :set_card + before_action :require_list_change, if: :append_to_list? + before_action :set_prev_item_and_next_item + before_action :validate_move_params + + def update + Card.transaction do + List.move(@card, prev_item: @prev_item, next_item: @next_item) + + if new_list + @card.list = new_list + @card.save! + end + end + + publish_event('card.updated', @card.to_event_payload) + @card.reload + + respond_to do |format| + format.html do + redirect_to project_board_list_card_path(current_project, @board, @card.list, @card), + notice: 'Task moved.' + end + + format.json do + render json: { + is_card: true, + id: @card.id, + link: polymorphic_path([current_project, @board, @card.list, @card]), + moveLink: project_board_list_card_position_path(current_project, @board, @card.list, @card) + } + end + end + end + + private + + # A request with a new_list_id but no explicit position appends the card to + # the end of the target list (e.g. the 'move to list' dropdown). + def append_to_list? + move_params[:new_list_id].present? && + move_params[:prev_id].blank? && + move_params[:next_id].blank? + end + + def move_params + params.permit(:card_id, :project_id, :board_id, :list_id, :next_id, :prev_id, :new_list_id) + end + + def moveable_item_name + 'card' + end + + def moveable_items + @board.cards + end + + def moveable_parent + new_list || @list + end + + def new_list + return if move_params[:new_list_id].blank? + + @new_list ||= @board.lists.find(move_params[:new_list_id]) + end + + def require_list_change + return if new_list.id != @card.list_id + + redirect_to project_board_list_card_path(current_project, @board, @list, @card), + alert: 'Task is already in that list.' + end + + def set_card + @card = @board.cards.find(params[:card_id]) + end + + def set_current_board_and_list + @board = current_project.boards.includes(:lists).find(params[:board_id]) + @list = @board.lists.includes(:cards).find(params[:list_id]) + end + + def set_prev_item_and_next_item + if append_to_list? + @prev_item = new_list.last_card + @next_item = nil + else + super + end + end +end diff --git a/app/controllers/cards_controller.rb b/app/controllers/cards_controller.rb index 8b5a381d8b..6fd958098d 100644 --- a/app/controllers/cards_controller.rb +++ b/app/controllers/cards_controller.rb @@ -11,12 +11,10 @@ class CardsController < AuthenticatedController before_action :initialize_sidebar, only: [:show, :new, :edit] before_action :set_auto_save_key, only: [:new, :create, :edit, :update] - # Not at top because we need board and list set first - include ValidateMove - layout 'cards' def show + @lists = @board.ordered_lists render layout: !request.xhr? end @@ -52,24 +50,6 @@ def update end end - def move - List.move(@card, prev_item: @prev_item, next_item: @next_item) - - if new_list - @card.list = new_list - @card.save - end - - track_updated(@card) - - render json: { - is_card: true, - id: @card.id, - link: polymorphic_path([current_project, @board, @card.reload.list, @card]), - moveLink: move_project_board_list_card_path(current_project, @board, @card.reload.list, @card) - } - end - def destroy if @card.destroy track_destroyed(@card) @@ -85,13 +65,6 @@ def card_params params.require(:card).permit(:name, :description, :due_date, assignee_ids: []) end - def move_params - params. - permit(:id, :project_id, :board_id, :list_id, - :next_id, :prev_id, :new_list_id - ) - end - def initialize_sidebar @sorted_cards = @list.ordered_cards.select(&:persisted?) end @@ -107,7 +80,7 @@ def set_or_initialize_card def set_current_board_and_list @board = current_project.boards.includes(:lists).find(params[:board_id]) - @list = @board.lists.includes(:cards).find(params[:list_id]) + @list = @board.lists.includes(:cards).find(params[:list_id]) end def set_auto_save_key @@ -119,8 +92,4 @@ def set_auto_save_key "#{@list.id}-card" end end - - def new_list - @board.lists.find(move_params[:new_list_id]) if move_params[:new_list_id] - end end diff --git a/app/controllers/concerns/linked_list_move_validator.rb b/app/controllers/concerns/linked_list_move_validator.rb new file mode 100644 index 0000000000..317a96f3fc --- /dev/null +++ b/app/controllers/concerns/linked_list_move_validator.rb @@ -0,0 +1,35 @@ +module LinkedListMoveValidator + extend ActiveSupport::Concern + + protected + + def set_prev_item_and_next_item + @prev_item = moveable_items.find_by(id: move_params[:prev_id]) + @next_item = moveable_items.find_by(id: move_params[:next_id]) + end + + def validate_move_params + unless valid_move_params? + redirect_to project_board_path(current_project, @board), alert: 'Something fishy is going on...' + end + end + + private + + def valid_move_params? + if @prev_item.present? + next_item_of_prev_item = @prev_item.send("next_#{moveable_item_name}") + if next_item_of_prev_item + @next_item == next_item_of_prev_item + else + @next_item.nil? + end + else + if moveable_parent.items.empty? + @next_item.nil? + else + @next_item == moveable_parent.first_item + end + end + end +end diff --git a/app/controllers/concerns/validate_move.rb b/app/controllers/concerns/validate_move.rb deleted file mode 100644 index 5e956107f6..0000000000 --- a/app/controllers/concerns/validate_move.rb +++ /dev/null @@ -1,45 +0,0 @@ -module ValidateMove - extend ActiveSupport::Concern - included do - before_action :set_prev_item_and_next_item, only: :move - before_action :validate_move_params, only: :move - end - - protected - - def set_prev_item_and_next_item - @prev_item = @board.send(controller_name).find_by(id: move_params[:prev_id]) - @next_item = @board.send(controller_name).find_by(id: move_params[:next_id]) - end - - def validate_move_params - unless valid_move_params? - redirect_to project_board_path(current_project, @board), alert: 'Something fishy is going on...' - end - end - - def parent - if controller_name == 'cards' - new_list || @list - elsif controller_name == 'lists' - @board - end - end - - def valid_move_params? - if @prev_item.present? - next_item_of_prev_item = @prev_item.send("next_#{controller_name.classify.downcase}") - if next_item_of_prev_item - @next_item == next_item_of_prev_item - else - @next_item.nil? - end - else - if parent.items.empty? - @next_item.nil? - else - @next_item == parent.first_item - end - end - end -end diff --git a/app/controllers/lists/position_controller.rb b/app/controllers/lists/position_controller.rb new file mode 100644 index 0000000000..b557a91ebc --- /dev/null +++ b/app/controllers/lists/position_controller.rb @@ -0,0 +1,44 @@ +class Lists::PositionController < AuthenticatedController + include ActivityTracking + include LinkedListMoveValidator + include ProjectScoped + + before_action :set_current_board + before_action :set_list + before_action :set_prev_item_and_next_item + before_action :validate_move_params + + def update + Board.move(@list, prev_item: @prev_item, next_item: @next_item) + + track_updated(@list) + + render json: @list + end + + private + + def move_params + params.permit(:list_id, :project_id, :board_id, :next_id, :prev_id) + end + + def moveable_item_name + 'list' + end + + def moveable_items + @board.lists + end + + def moveable_parent + @board + end + + def set_current_board + @board = current_project.boards.find(params[:board_id]) + end + + def set_list + @list = @board.lists.find(params[:list_id]) + end +end diff --git a/app/controllers/lists_controller.rb b/app/controllers/lists_controller.rb index 4f880e1ef5..541cd8e41f 100644 --- a/app/controllers/lists_controller.rb +++ b/app/controllers/lists_controller.rb @@ -3,10 +3,7 @@ class ListsController < AuthenticatedController include ProjectScoped before_action :set_current_board - before_action :set_list, only: [:edit, :update, :destroy, :move] - - # Not at top because we need board set first - include ValidateMove + before_action :set_list, only: [:edit, :update, :destroy] def new @list = @board.lists.new @@ -35,14 +32,6 @@ def update end end - def move - Board.move(@list, prev_item: @prev_item, next_item: @next_item) - - track_updated(@list) - - render json: @list - end - def destroy if @list.destroy track_destroyed(@list) @@ -58,11 +47,6 @@ def list_params params.require(:list).permit(:name) end - def move_params - params. - permit(:id, :project_id, :board_id, :next_id, :prev_id) - end - def set_current_board @board = current_project.boards.find(params[:board_id]) end diff --git a/app/models/card.rb b/app/models/card.rb index b0dd2ac557..4ba76baade 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -1,5 +1,6 @@ class Card < ApplicationRecord include Commentable + include Eventable include HasFields include RevisionTracking include Subscribable @@ -84,9 +85,27 @@ def to_xml(xml_builder, includes: [], version: 3) end end + def local_event_payload + { + board: { + id: board.id, + name: board.name + }, + list: { + id: list.id, + name: list.name + }, + name: name, + project: { + id: project.id, + name: project.name + } + } + end + def local_fields { - 'List' => list.name.parameterize(preserve_case: true, separator: '_'), + 'List' => list.name.parameterize(preserve_case: true, separator: '_'), 'Title' => name } end diff --git a/app/views/boards/_board.html.erb b/app/views/boards/_board.html.erb index e37800ea83..8635a9a2e4 100644 --- a/app/views/boards/_board.html.erb +++ b/app/views/boards/_board.html.erb @@ -4,7 +4,7 @@ <% board.ordered_lists.each do |list| %> <% cache ['board-list', list] do %> -
  • +

  • <%= list.name %> @@ -35,7 +35,7 @@
      <% list.ordered_cards.each do |card| %> <% cache ['board-list-card', card] do %> -
    • +
    • <%= link_to project_board_list_card_path(current_project, board, list, card) do %>
      <%= card.name %>
      diff --git a/app/views/cards/_actions.html.erb b/app/views/cards/_actions.html.erb index fc4a033fd9..df95d7a9c3 100644 --- a/app/views/cards/_actions.html.erb +++ b/app/views/cards/_actions.html.erb @@ -1,4 +1,22 @@
      + <% if @lists.count > 1 %> + + <%= @list.name %> +
      + <% @lists.each do |list| %> + <%= link_to_if( + list.id != @list.id, + list.name, + project_board_list_card_position_path(current_project, @board, @list, @card, new_list_id: list.id), + method: :patch, + class: 'dropdown-item' + ) do %> + <%= list.name %> + <% end %> + <% end %> +
      +       + <% end %> <%= link_to edit_project_board_list_card_path(current_project, @board, @list, @card) do %> Edit diff --git a/config/initializers/activity_service.rb b/config/initializers/activity_service.rb index 99a3f56bfe..383ef9b623 100644 --- a/config/initializers/activity_service.rb +++ b/config/initializers/activity_service.rb @@ -1,5 +1,6 @@ Rails.application.reloader.to_prepare do ActivityService.configure do |activity_service| + activity_service.subscribe_namespace 'card' activity_service.subscribe_namespace 'inline_thread' activity_service.subscribe_namespace 'issue' end diff --git a/config/routes.rb b/config/routes.rb index 6bbfe11a41..f0497d4a33 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -12,7 +12,7 @@ # ------------------------------------------------------------ Authentication # Sign in / sign out - get '/login' => 'sessions#new' + get '/login' => 'sessions#new' get '/logout' => 'sessions#destroy' resource :session @@ -56,9 +56,9 @@ resources :boards do resources :lists, except: [:index] do - member { post :move } + resource :position, only: [:update], controller: 'lists/position' resources :cards, except: [:index] do - member { post :move } + resource :position, only: [:update], controller: 'cards/position' resources :revisions, only: [:index, :show] end end @@ -132,12 +132,12 @@ get 'trash' => 'revisions#trash' # ------------------------------------------------------- Export Manager - get '/export' => 'export#index', as: :export_manager + get '/export' => 'export#index', as: :export_manager # ------------------------------------------------------- Upload Manager - get '/upload' => 'upload#index', as: :upload_manager - post '/upload' => 'upload#create' - post '/upload/parse' => 'upload#parse' + get '/upload' => 'upload#index', as: :upload_manager + post '/upload' => 'upload#create' + post '/upload/parse' => 'upload#parse' end resources :console, only: [] do diff --git a/spec/requests/cards/move_to_list_spec.rb b/spec/requests/cards/move_to_list_spec.rb new file mode 100644 index 0000000000..c897b1d13b --- /dev/null +++ b/spec/requests/cards/move_to_list_spec.rb @@ -0,0 +1,112 @@ +require 'rails_helper' + +describe 'Cards::PositionController#update (move to list)' do + before { login_to_project_as_user } + + let(:board) { create(:board, node: current_project.methodology_library, project: current_project) } + let(:source_list) { create(:list, board: board) } + let(:target_list) { create(:list, board: board, previous_id: source_list.id) } + + let(:card_a) { create(:card, list: source_list) } + let(:card_b) { create(:card, list: source_list, previous_id: card_a.id) } + let(:card_c) { create(:card, list: source_list, previous_id: card_b.id) } + + let(:last_card_in_target) { create(:card, list: target_list) } + + let(:submit) do + patch project_board_list_card_position_path(current_project, board, source_list, card_b), + params: { new_list_id: target_list.id } + end + + before do + card_a + card_b + card_c + last_card_in_target + end + + it 'moves the card to the target list' do + submit + expect(card_b.reload.list).to eq(target_list) + end + + it 'appends the card to the end of the target list' do + submit + expect(card_b.reload.previous_id).to eq(last_card_in_target.id) + end + + it 'repairs the source list chain' do + submit + expect(card_c.reload.previous_id).to eq(card_a.id) + end + + it 'redirects to the card in its new list with a notice' do + submit + expect(response).to redirect_to(project_board_list_card_path(current_project, board, target_list, card_b)) + expect(flash[:notice]).to eq('Task moved.') + end + + it 'creates an activity' do + expect { submit }.to have_enqueued_job(ActivityTrackingJob).with( + action: 'update', + project_id: current_project.id, + trackable_id: card_b.id, + trackable_type: 'Card', + user_id: @logged_in_as.id + ) + end + + context 'when moving the first card in the source list' do + let(:submit) do + patch project_board_list_card_position_path(current_project, board, source_list, card_a), + params: { new_list_id: target_list.id } + end + + it 'promotes the next card to list head' do + submit + expect(card_b.reload.previous_id).to be_nil + end + end + + context 'when moving the last card in the source list' do + let(:submit) do + patch project_board_list_card_position_path(current_project, board, source_list, card_c), + params: { new_list_id: target_list.id } + end + + it 'leaves the remaining chain intact' do + submit + expect(card_b.reload.previous_id).to eq(card_a.id) + end + end + + context 'when the target list is the same as the source list' do + let(:submit) do + patch project_board_list_card_position_path(current_project, board, source_list, card_c), + params: { new_list_id: source_list.id } + end + + it 'does not change the card' do + original_previous_id = card_c.previous_id + submit + expect(card_c.reload.previous_id).to eq(original_previous_id) + end + + it 'redirects with an alert' do + submit + expect(response).to redirect_to(project_board_list_card_path(current_project, board, source_list, card_c)) + expect(flash[:alert]).to eq('Task is already in that list.') + end + end + + context 'when the target list is empty' do + let(:last_card_in_target) { nil } + + before { target_list } + + it 'makes the card the first item in the target list' do + submit + expect(card_b.reload.previous_id).to be_nil + end + end +end diff --git a/spec/requests/move_position_spec.rb b/spec/requests/move_position_spec.rb index b251f66000..ea2c031d14 100644 --- a/spec/requests/move_position_spec.rb +++ b/spec/requests/move_position_spec.rb @@ -52,7 +52,7 @@ let(:invalid_prev_id) { another_list.id } let(:invalid_next_id) { another_list.id } subject do - post move_project_board_list_path(current_project, board, list_2), params: params + patch project_board_list_position_path(current_project, board, list_2), params: params end include_examples 'validates move params' @@ -66,7 +66,7 @@ let(:invalid_prev_id) { another_card.id } let(:invalid_next_id) { another_card.id } subject do - post move_project_board_list_card_path(current_project, board, list_1, card_1), params: params + patch project_board_list_card_position_path(current_project, board, list_1, card_1), params: params end include_examples 'validates move params'