Add move to list option to Methodology cards - #1580
Conversation
etdsoft
left a comment
There was a problem hiding this comment.
Automated peer review — PR #1580
Reviewers: Claude (sub-agent 1), Codex (sub-agent 2)
Verdict: request-changes
Findings: 2 High, 2 Medium, 0 Low
What I checked
This is an automated peer code review by two independent LLM sub-agents reading the same rubric. Findings flagged by both agents are higher confidence; findings flagged by only one are still worth your attention. The verdict is computed from severity counts — there is no editorial judgment in the merge step.
Both reviewers agreed
(No findings flagged by both sub-agents.)
Claude only
F1 [High|correctness] app/controllers/cards_controller.rb:66 — save without bang inside transaction allows silent chain corruption
What: @card.save (line 66) returns false on validation failure but does not raise, so the wrapping Card.transaction block commits regardless. By that point, next_card.update_attribute (line 61) has already rewritten the source list's linked-list chain. If save fails, the source list is permanently broken: card_c.previous_id points to card_a, but card_b never left the source list.
Why: save returning false is not an exception — ActiveRecord transactions only roll back on raised exceptions. The chain-repair write and the move write must be atomic; right now they are not. Although current model validations make a failure unlikely (only list_id and previous_id change, both to valid values), any future validation, callback, or DB constraint failure would silently corrupt the linked list. Every other transaction in ActsAsLinkedList.move uses update_attribute for all writes, sidestepping this — the new code mixes update_attribute with save, introducing the asymmetry.
How to fix: Change @card.save to @card.save!. This makes the transaction atomic — any failure rolls back the chain repair too. Optionally rescue ActiveRecord::RecordInvalid to redirect with an error instead of a 500:
Card.transaction do
if (next_card = @card.next_card)
next_card.update_attribute(:previous_id, @card.previous_id)
end
@card.list_id = target_list.id
@card.previous_id = target_list.last_card&.id
@card.save!
endF2 [Medium|consistency] app/controllers/cards_controller.rb:70 — new code uses deprecated ActivityTracking instead of EventPublisher
What: The new move_to_list action calls track_updated(@card) (line 70) from the ActivityTracking concern.
Why: The project CLAUDE.md explicitly states: "Deprecated: ActivityTracking (track_created, track_destroyed, track_activity, track_state_change) — do not use in new code. Use EventPublisher + publish_event instead." The existing controller methods (create, update, destroy) also use ActivityTracking, so there's a tension between file-level consistency and the project directive. However, the directive is unambiguous: new methods should use EventPublisher. This is the right time to introduce publish_event for this action — mixing patterns in a single file is a smaller cost than extending a deprecated API.
How to fix: Replace track_updated(@card) with publish_event('card.moved', card: @card) (or 'card.updated' if a new event name isn't warranted). Add include EventPublisher alongside the existing include ActivityTracking. The existing methods can be migrated in a separate PR.
F3 [Medium|tests] spec/requests/cards/move_to_list_spec.rb:0 — missing edge-case coverage for first-card and last-card moves
What: The spec moves the middle card (card_b) from a three-card list. Two distinct boundary cases are not covered: (1) moving the first card in the source list (previous_id: nil), where the next card must become the new head; (2) moving the last card, where next_card is nil and no chain repair is needed.
Why: Both paths exercise different branches in the transaction block. The first-card case verifies that next_card.update_attribute(:previous_id, nil) correctly promotes the next card to list head. The last-card case verifies the if guard around next_card correctly skips the repair. Regressions in either would silently break list ordering with no test failure.
How to fix: Add two contexts:
context 'when moving the first card in the source list' do
let(:submit) do
post move_to_list_project_board_list_card_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
post move_to_list_project_board_list_card_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
endCodex only
F4 [High|correctness] app/controllers/cards_controller.rb:55 — same-list move can corrupt card order
What: move_to_list accepts any new_list_id from params, including the card's current list. If the card is already the last card in that list, target_list.last_card&.id is the card itself, so line 66 saves previous_id to its own id.
Why: This creates a self-referential card link and removes the card from the normal ordered_cards chain. The UI hides the current list (lines 22–35 in _actions.html.erb), but a crafted POST can still trigger this and corrupt task ordering data.
How to fix: Add a server-side guard before mutating the chain:
if target_list.id == @card.list_id
redirect_to [current_project, @board, @list, @card], alert: 'Task is already in that list.'
return
endAlso add a request spec that posts new_list_id: source_list.id for the last card and asserts the card's previous_id remains unchanged.
Notes
- Authorization is handled correctly: the card is scoped via
@board.cards.find(params[:id]), the board viacurrent_project.boards.find(params[:board_id]), and the target list via@board.lists.find(params[:new_list_id]). No IDOR risk. - The cache key change in
show.html.erb(adding@board.lists.maximum(:updated_at)) is correct — it invalidates when lists are renamed or added, ensuring the dropdown stays current. - The
link_towithmethod: :postis consistent with the existing delete link in the same partial. CSRF protection is inherited fromApplicationController. list.namein the view uses<%= %>(auto-escaped), so no XSS concern.
Reviewed automatically. Raw outputs in agents/product-context/reviews/dradis-ce/1580/review/.
etdsoft
left a comment
There was a problem hiding this comment.
Automated peer review — PR #1580
Reviewers: Claude (sub-agent 1), Codex (sub-agent 2)
Verdict: request-changes
Findings: 2 High, 2 Medium, 1 Low
What I checked
This is an automated peer code review by two independent LLM sub-agents reading the same rubric. Findings flagged by both agents are higher confidence; findings flagged by only one are still worth your attention. The verdict is computed from severity counts — there is no editorial judgment in the merge step.
Both reviewers agreed
No overlapping findings
(The two High findings differ: Claude flags a save failure issue, Codex flags a same-list edge case that creates a self-link.)
Claude only
F1 [High|correctness] app/controllers/cards_controller.rb:64 — save without ! inside transaction allows partial commit and list chain corruption
What: @card.save on line 64 does not raise on validation failure. The update_attribute on line 59 (which repairs the source list chain) has already been executed and will be committed even if @card.save returns false. The user then sees "Task moved." despite the card not actually moving. The source list chain is left broken — next_card now skips the unmoved card.
Why: ActiveRecord::Base.transaction only rolls back on an uncaught exception. save returns false on failure; save! raises ActiveRecord::RecordInvalid. With save, the transaction commits the chain repair but not the card's list change — a data corruption scenario. While current Card validations make this unlikely for a valid card, a card that drifted into an invalid state (e.g., blank name from an import or migration) would silently break.
How to fix: Use save! instead of save:
Card.transaction do
if (next_card = @card.next_card)
next_card.update_attribute(:previous_id, @card.previous_id)
end
@card.list_id = target_list.id
@card.previous_id = target_list.last_card&.id
@card.save!
endThis ensures the transaction rolls back atomically if the card can't be saved. You'll also want a rescue ActiveRecord::RecordInvalid to redirect with an error flash instead of a 500.
F2 [Medium|consistency] app/controllers/cards_controller.rb:53 — new action uses deprecated ActivityTracking instead of EventPublisher
What: move_to_list calls track_updated(@card) from the ActivityTracking concern. This is new code added in this PR.
Why: The project CLAUDE.md explicitly states: "ActivityTracking (track_created, track_destroyed, track_activity, track_state_change) — do not use in new code. Use EventPublisher + publish_event instead." While the rest of the controller still uses the deprecated pattern, the project convention is clear about new code. This new action should set the right precedent.
How to fix: Include EventPublisher and replace track_updated(@card) with publish_event('card.moved', card: @card) (or 'card.updated' if a moved event doesn't exist yet). The rest of the controller can be migrated in a separate PR.
F3 [Medium|tests] spec/requests/cards/move_to_list_spec.rb:0 — no test for moving the last card in a list (false branch of next_card conditional)
What: All tests move card_b, which is the middle card in the source list. The if (next_card = @card.next_card) conditional on line 58 always evaluates to true. The false branch — moving the last card in a list (no next_card to repair) — is never exercised.
Why: This is a meaningful code path: when the last card in a list is moved, no chain repair should happen. Testing it verifies the conditional guard works and that no nil.update_attribute error is raised. Similarly, moving the first card (where @card.previous_id is nil) would verify that next_card.update_attribute(:previous_id, nil) correctly promotes it to list head.
How to fix: Add two contexts:
context 'when moving the last card in the source list' do
let(:submit) do
post move_to_list_project_board_list_card_path(current_project, board, source_list, card_c),
params: { new_list_id: target_list.id }
end
it 'moves the card without breaking the source chain' do
submit
expect(card_c.reload.list).to eq(target_list)
expect(card_b.reload.previous_id).to eq(card_a.id) # unchanged
end
end
context 'when moving the first card in the source list' do
let(:submit) do
post move_to_list_project_board_list_card_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
endF4 [Low|consistency] config/routes.rb:55 — move_to_list as member action rather than extracted sub-resource
What: The new post :move_to_list is added as a member route alongside the existing post :move.
Why: The project CLAUDE.md says to "identify 'hidden' sub-resources" and gives inline_threads/resolutions as the pattern. A card's list placement is a sub-resource: resource :list_placement, only: [:update], controller: 'cards/list_placements'. However, the existing move action also uses a member route, so this follows precedent within the file.
How to fix: If the team wants to follow the sub-resource convention strictly, extract to Cards::ListPlacementsController with PATCH /cards/:card_id/list_placement. Otherwise, the current approach is fine given move sets the precedent — just flag for future cleanup.
Codex only
F1 [High|correctness] app/controllers/cards_controller.rb:66 — same-list move can create a self-link
What: move_to_list accepts any list on the board as new_list_id. If a user posts the card's current list, and the card is already the last or only card in that list, target_list.last_card&.id resolves to the card itself, so line 66 saves previous_id as the card's own id.
Why: The linked-list implementation follows previous_id; a self-referential previous_id corrupts the task ordering and can make the card disappear from normal ordered_cards traversal. The UI excludes the current list, but the server endpoint is directly callable.
How to fix: Add a server-side guard before the transaction, for example redirect/422 when target_list.id == @card.list_id, and add a request spec that posts new_list_id: source_list.id for the last/only card and asserts the list chain is unchanged.
Notes
Both sub-agents reviewed the code and pulled their own data. Codex noted that it could not fetch PR metadata via gh pr view/gh pr diff because the sandbox cannot connect to api.github.com, and could not run the request spec because Ruby is unavailable on PATH in that environment.
Reviewed automatically. Raw outputs in agents/product-context/reviews/dradis-ce/1580/review/.
Acceptance Test Results — PR #1580Branch: SummaryMove-to-list dropdown works as described. Shows before Edit on multi-list boards, absent on single-list boards, moves card inline and flashes correctly. Results
Notes
|
Activities were silently dropped because 'move_to_list' was not in Activity::VALID_ACTIONS, causing ActivityTrackingJob to fail validation.
Cards::PositionController#create replaces #move (drag-and-drop JSON endpoint) and Cards::TransferController#create replaces #move_to_list (dropdown form POST), eliminating the naming ambiguity between the two actions in CardsController. ValidateMove is refactored to use moveable_items, moveable_item_name, and moveable_parent template methods instead of controller_name, so it works correctly from namespaced controllers.
etdsoft
left a comment
There was a problem hiding this comment.
Automated peer review -- PR #1580
Reviewers: Claude (sub-agent 1), Codex (sub-agent 2)
Verdict: request-changes
Findings: 0 High, 3 Medium, 1 Low
What I checked
This is an automated peer code review by two independent LLM sub-agents reading the same rubric. Findings flagged by both agents are higher confidence; findings flagged by only one are still worth your attention. The verdict is computed from severity counts — there is no editorial judgment in the merge step.
Both reviewers agreed
(None — only one sub-agent produced output.)
Claude only
F1 [Medium|correctness] app/views/cards/show.html.erb:25 -- Cache key misses board-level list changes
What: The cache key ['card-information-tab', @card, @card.list] covers only the card and its current list. The _actions.html.erb partial rendered inside this cache block iterates @lists (all board lists) to build the transfer dropdown.
Why: When a list is added to, removed from, or renamed on the board, neither @card.updated_at nor @card.list.updated_at changes. The cached fragment continues serving stale dropdown options — missing new lists, showing deleted ones, or displaying old names — until some unrelated action touches the card or its list. Since belongs_to :list, touch: true only flows card→list (not list→card), renames of other lists on the board never bust this cache.
How to fix: Include the board in the cache key so any list change (which touches the board's updated_at) invalidates the fragment:
<% cache ['card-information-tab', @card, @card.list, @board] do %>Alternatively, move the transfer dropdown outside the cache block entirely (it's dynamic content that depends on board-wide state).
F2 [Medium|consistency] app/controllers/cards/transfer_controller.rb:2 -- New code uses deprecated ActivityTracking
What: Cards::TransferController (entirely new code) and Cards::PositionController (new file, mostly moved code) both include ActivityTracking and call track_updated. Meanwhile, this PR adds include Eventable to Card and registers subscribe_namespace 'card' in the activity service initializer — the model-side and infrastructure-side halves of the new event system.
Why: CLAUDE.md explicitly says: "Deprecated: ActivityTracking (track_created, track_destroyed, track_activity, track_state_change) — do not use in new code. Use EventPublisher + publish_event instead." The PR sets up Eventable on the model but doesn't connect the controller side. The subscribe_namespace 'card' subscription is currently inert (nothing publishes card.* events). If a future PR adds EventPublisher to a card controller without removing ActivityTracking, activities would be duplicated.
How to fix: In Cards::TransferController and Cards::PositionController, replace include ActivityTracking with include EventPublisher, and replace track_updated(@card) with publish_event('card.updated', card: @card) — check how inline_thread and issue controllers publish their events for the exact signature.
F3 [Medium|sad-path] app/controllers/cards/position_controller.rb:16 -- Unguarded save after committed position change
What: PositionController#create calls List.move(@card, ...) (which commits in its own transaction), then calls @card.save (no !) to update the list assignment. If the save fails (e.g. a validation error), the card's linked-list position is already committed but its list_id is not updated. The action still renders a 200 JSON response.
Why: The card ends up in an inconsistent state: repositioned in the linked list of the new list but still belonging to the old list by list_id. The JSON response uses @card.reload.list which would reflect the old list, so the JS would also get stale URLs.
How to fix: Wrap both operations in a single transaction and use save!:
def create
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
track_updated(@card)
# render json...
endThis ensures the position change and list assignment either both commit or both roll back.
F4 [Low|hygiene] config/routes.rb:- -- Unrelated whitespace changes in routes
What: The diff includes whitespace-only alignment fixes on lines unrelated to the card transfer feature: get '/login', get '/export', get '/upload', post '/upload', post '/upload/parse'.
Why: These are separate from the feature and make the diff noisier. Project convention: keep commits focused on a single logical change.
How to fix: Move the whitespace cleanup to a separate commit or drop it.
Codex only
(None — Codex sub-agent failed; see Notes.)
Notes
- Codex sub-agent FAILED:
gpt-5.3-codexmodel not supported on this ChatGPT account (API error 400, exit code 1). All findings above are from Claude only. Consider re-running after the Codex model configuration is fixed. - Authorization: both new controllers inherit from
AuthenticatedControllerand scope all records throughcurrent_project.boards → @board.lists → @board.cards. This matches the existingCardsControllerpattern. No IDOR risk found. - CSRF:
protect_from_forgery with: :exceptionis inherited fromApplicationController. Themethod: :postlink uses rails-ujs to submit a form with the CSRF token. Correct. - Minor:
new_listinPositionControlleris not memoized — each call re-queries. Impact negligible (≤2 extra queries per drag-and-drop), but@new_list ||= ...would be cleaner.
Reviewed automatically. Raw outputs in the shared product reviews directory.
Add @board to the card show cache key so the transfer dropdown busts when any list on the board is added, renamed, or deleted. Wrap List.move and the list assignment save in a single transaction in Cards::PositionController so a save failure rolls back the linked-list position change atomically.
| end | ||
| end | ||
|
|
||
| def local_event_payload |
There was a problem hiding this comment.
We're adding this for cards but are we actually using it? Either we keep this and use eventpublisher for cards, or leave this out for now.
There was a problem hiding this comment.
@aapomm please go ahead and add it.
We probably need to include list information in the payload (and even a before/after in the particular Position one).
There was a problem hiding this comment.
why was this resolved @aapomm ? I think that someone paying attention to this event would like List / Board?
Activity tracking for cards already works via ActivityTracking; the Eventable concern and local_event_payload were dead code.
Acceptance Test Results -- PR #1580Branch: Test Setup
Results
SkippedStep 5 — Click the list dropdown (JS interaction) Sad Path Results
Code Notes
Cache key expansion: Board partial SummaryAll 30 test plan steps pass. The feature works correctly: cards can be moved between lists from the card show page, the linked-list chain is properly repaired on both source and target lists, flash messaging is correct, activity tracking fires, and the dropdown is correctly suppressed on single-list boards. Drag-and-drop reordering via the position endpoint continues to function correctly after the controller refactoring. |
| end | ||
| end | ||
|
|
||
| def local_event_payload |
There was a problem hiding this comment.
@aapomm please go ahead and add it.
We probably need to include list information in the payload (and even a before/after in the particular Position one).
Summary
Adds the ability to move a card to a different list directly from the card's show page. A new inline dropdown — showing the card's current list name with a fa-table-columns icon — appears before the Edit action. Selecting a list from the dropdown moves the card there immediately. The option only appears when other lists exist on the board.
The two card move actions are extracted into dedicated sub-resource controllers to remove naming ambiguity:
Cards::PositionController#createhandles drag-and-drop reordering (JSON), andCards::TransferController#createhandles the dropdown list transfer (HTML redirect).ValidateMoveis refactored to use template methods (moveable_items,moveable_item_name,moveable_parent) instead ofcontroller_nameso it works correctly from namespaced controllers.How to test
Copyright assignment
Collaboration is difficult with commercial closed source but we want
to keep as much of the OSS ethos as possible available to users
who want to fix it themselves.
In order to unambiguously own and sell Dradis Framework commercial
products, we must have the copyright associated with the entire
codebase. Any code you create which is merged must be owned by us.
That's not us trying to be a jerks, that's just the way it works.
Please review the CONTRIBUTING.md
file for the details.
You can delete this section, but the following sentence needs to
remain in the PR's description:
Check List