From 8697cabbe2b319a0ecd8fef522a61b9c12fad9f5 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 09:33:19 +0200 Subject: [PATCH 01/21] Show confirmation when submitting certain commands https://3.basecamp.com/2914079/buckets/37331921/todos/8617430347 --- app/controllers/commands_controller.rb | 23 ++++-- .../controllers/terminal_controller.js | 70 ++++++++++++++++--- app/javascript/helpers/http_helpers.js | 4 ++ app/models/command.rb | 4 ++ app/models/command/assign.rb | 4 ++ app/views/commands/_form.html.erb | 5 +- app/views/commands/_terminal.html.erb | 1 + test/controllers/commands_controller_test.rb | 10 ++- 8 files changed, 105 insertions(+), 16 deletions(-) create mode 100644 app/javascript/helpers/http_helpers.js diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index c70750bfc..f39deb7a7 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -7,13 +7,11 @@ class CommandsController < ApplicationController command = parse_command(params[:command]) if command&.valid? - result = command.execute - - case result - when Command::Result::Redirection - redirect_to result.url + if confirmed?(command) + result = command.execute + respond_with_execution_result(result) else - redirect_back_or_to root_path + render plain: command.title, status: :conflict end else head :unprocessable_entity @@ -30,4 +28,17 @@ class CommandsController < ApplicationController def parsing_context Command::Parser::Context.new(Current.user, url: request.referrer) end + + def confirmed?(command) + !command.needs_confirmation? || params[:confirmed].present? + end + + def respond_with_execution_result(result) + case result + when Command::Result::Redirection + redirect_to result.url + else + redirect_back_or_to root_path + end + end end diff --git a/app/javascript/controllers/terminal_controller.js b/app/javascript/controllers/terminal_controller.js index 177aa5a23..e951695f0 100644 --- a/app/javascript/controllers/terminal_controller.js +++ b/app/javascript/controllers/terminal_controller.js @@ -1,8 +1,9 @@ import { Controller } from "@hotwired/stimulus" +import { HttpStatus } from "helpers/http_helpers" export default class extends Controller { - static targets = [ "input" ] - static classes = [ "error" ] + static targets = [ "input", "form", "confirmation" ] + static classes = [ "error", "confirmation" ] // Actions @@ -10,17 +11,70 @@ export default class extends Controller { this.inputTarget.focus() } - handleSubmit(event) { + executeCommand(event) { if (event.detail.success) { - event.target.reset() + this.#reset() } else { - this.#handleErrorResponse(event.detail.fetchResponse.response.status) + const response = event.detail.fetchResponse.response + this.#handleErrorResponse(response) } } - #handleErrorResponse(code) { - if (code == 422) { - this.element.classList.add(this.errorClass) + async #handleErrorResponse(response) { + const status = response.status + const message = await response.text() + + if (status === HttpStatus.UNPROCESSABLE) { + this.#showError() + } else if (status === HttpStatus.CONFLICT) { + this.#requestConfirmation(message) } } + + #reset(inputValue = "") { + this.formTarget.reset() + this.inputTarget.value = inputValue + this.confirmationTarget.value = "" + + this.element.classList.remove(this.errorClass) + this.element.classList.remove(this.confirmationClass) + } + + #showError() { + this.element.classList.add(this.errorClass) + } + + async #requestConfirmation(message) { + const originalInputValue = this.inputTarget.value + this.element.classList.add(this.confirmationClass) + this.inputTarget.value = `${message}? [Y/n] ` + + try { + await this.#waitForConfirmation() + this.#submitWithConfirmation(originalInputValue) + } catch { + this.#reset(originalInputValue) + } + } + + #waitForConfirmation() { + return new Promise((resolve, reject) => { + this.inputTarget.addEventListener("keydown", (event) => { + event.preventDefault() + const key = event.key.toLowerCase() + + if (key === "enter" || key === "y") { + resolve() + } else { + reject() + } + }, { once: true }) + }) + } + + #submitWithConfirmation(inputValue) { + this.inputTarget.value = inputValue + this.confirmationTarget.value = "confirmed" + this.formTarget.requestSubmit() + } } diff --git a/app/javascript/helpers/http_helpers.js b/app/javascript/helpers/http_helpers.js new file mode 100644 index 000000000..1fb986f70 --- /dev/null +++ b/app/javascript/helpers/http_helpers.js @@ -0,0 +1,4 @@ +export const HttpStatus = { + CONFLICT: 409, + UNPROCESSABLE: 422 +} diff --git a/app/models/command.rb b/app/models/command.rb index dc924dd82..1b38f82b9 100644 --- a/app/models/command.rb +++ b/app/models/command.rb @@ -24,6 +24,10 @@ class Command < ApplicationRecord false end + def needs_confirmation? + false + end + private def redirect_to(...) Command::Result::Redirection.new(...) diff --git a/app/models/command/assign.rb b/app/models/command/assign.rb index 082b54fe1..439f01f12 100644 --- a/app/models/command/assign.rb +++ b/app/models/command/assign.rb @@ -43,6 +43,10 @@ class Command::Assign < Command true end + def needs_confirmation? + cards.many? + end + private def assignees User.where(id: assignee_ids) diff --git a/app/views/commands/_form.html.erb b/app/views/commands/_form.html.erb index f8e530cd1..be669b82d 100644 --- a/app/views/commands/_form.html.erb +++ b/app/views/commands/_form.html.erb @@ -3,7 +3,8 @@ class: [ "flex align-center gap-half" ], data: { controller: "form", - action: "turbo:submit-end->terminal#focus keydown.meta+k@document->terminal#focus turbo:submit-end->terminal#handleSubmit" + terminal_target: "form", + action: "turbo:submit-end->terminal#focus keydown.meta+k@document->terminal#focus turbo:submit-end->terminal#executeCommand" } do %> @@ -18,5 +19,7 @@ turbo_permanent: true }, placeholder: "Press ⌘+K to search or type commands…" %> + + <%= hidden_field_tag "confirmed", nil, data: { terminal_target: "confirmation" } %> <% end %> diff --git a/app/views/commands/_terminal.html.erb b/app/views/commands/_terminal.html.erb index 06a153c75..0361692e0 100644 --- a/app/views/commands/_terminal.html.erb +++ b/app/views/commands/_terminal.html.erb @@ -1,6 +1,7 @@ <%= tag.div class: "terminal full-width flex flex-column gap justify-end", data: { controller: "toggle-class terminal", terminal_error_class: "terminal--error", + terminal_confirmation_class: "terminal--confirmation", toggle_class_toggle_class: "terminal--open" } do %> <%= turbo_frame_tag :recent_commands, src: commands_path, refresh: "morph" %> diff --git a/test/controllers/commands_controller_test.rb b/test/controllers/commands_controller_test.rb index 2dfdd20f5..2b5f027ba 100644 --- a/test/controllers/commands_controller_test.rb +++ b/test/controllers/commands_controller_test.rb @@ -15,12 +15,20 @@ class CommandsControllerTest < ActionDispatch::IntegrationTest test "command that triggers a redirect back" do assert_difference -> { users(:kevin).commands.count }, +1 do - post commands_path, params: { command: "/assign @kevin" }, headers: { "HTTP_REFERER" => cards_path } + post commands_path, params: { command: "/assign @kevin", confirmed: "confirmed" }, headers: { "HTTP_REFERER" => cards_path } end assert_redirected_to cards_path end + test "commands requiring confirmation return a 409 conflict response" do + assert_difference -> { users(:kevin).commands.count }, +1 do + post commands_path, params: { command: "/assign @kevin" }, headers: { "HTTP_REFERER" => cards_path } + end + + assert_response :conflict + end + test "get a 422 on errors" do post commands_path, params: { command: "/assign @some_missing_user" }, headers: { "HTTP_REFERER" => cards_path } assert_response :unprocessable_entity From dc28f4c9c52b42c26970d2ae75bfeac772fe5e4f Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 10:46:39 +0200 Subject: [PATCH 02/21] Add command to close cards https://3.basecamp.com/2914079/buckets/37331921/todos/8620236700 --- .../controllers/local_time_controller.js | 10 ++-- app/models/card/closeable.rb | 2 +- app/models/closure/reason.rb | 4 ++ app/models/command/assign.rb | 30 +++-------- app/models/command/cards.rb | 30 +++++++++++ app/models/command/close.rb | 35 +++++++++++++ app/models/command/parser.rb | 2 + test/models/command/assign_test.rb | 2 +- test/models/command/close_test.rb | 51 +++++++++++++++++++ 9 files changed, 136 insertions(+), 30 deletions(-) create mode 100644 app/models/command/cards.rb create mode 100644 app/models/command/close.rb create mode 100644 test/models/command/close_test.rb diff --git a/app/javascript/controllers/local_time_controller.js b/app/javascript/controllers/local_time_controller.js index 7fe903c85..e539d9eb5 100644 --- a/app/javascript/controllers/local_time_controller.js +++ b/app/javascript/controllers/local_time_controller.js @@ -11,8 +11,8 @@ export default class extends Controller { initialize() { this.timeFormatter = new Intl.DateTimeFormat(undefined, { timeStyle: "short" }) this.dateFormatter = new Intl.DateTimeFormat(undefined, { dateStyle: "long" }) - this.shortDateFormatter = new Intl.DateTimeFormat(undefined, { month: "short", day: "numeric" }) - this.dateTimeFormatter = new Intl.DateTimeFormat(undefined, { timeStyle: "short", dateStyle: "short" }) + this.shortdateFormatter = new Intl.DateTimeFormat(undefined, { month: "short", day: "numeric" }) + this.datetimeFormatter = new Intl.DateTimeFormat(undefined, { timeStyle: "short", dateStyle: "short" }) this.agoFormatter = new AgoFormatter() this.daysagoFormatter = new DaysAgoFormatter() this.datewithweekdayFormatter = new Intl.DateTimeFormat(undefined, { weekday: "long", month: "long", day: "numeric" }) @@ -46,11 +46,11 @@ export default class extends Controller { } datetimeTargetConnected(target) { - this.#formatTime(this.dateTimeFormatter, target) + this.#formatTime(this.datetimeFormatter, target) } shortdateTargetConnected(target) { - this.#formatTime(this.shortDateFormatter, target) + this.#formatTime(this.shortdateFormatter, target) } agoTargetConnected(target) { @@ -78,7 +78,7 @@ export default class extends Controller { #formatTime(formatter, target) { const dt = new Date(target.getAttribute("datetime")) target.innerHTML = formatter.format(dt) - target.title = this.dateTimeFormatter.format(dt) + target.title = this.datetimeFormatter.format(dt) } } diff --git a/app/models/card/closeable.rb b/app/models/card/closeable.rb index b599b6a03..e0d15831b 100644 --- a/app/models/card/closeable.rb +++ b/app/models/card/closeable.rb @@ -42,7 +42,7 @@ module Card::Closeable closure&.created_at end - def close(user: Current.user, reason: Closure::Reason::FALLBACK_LABEL) + def close(user: Current.user, reason: Closure::Reason.default) unless closed? transaction do create_closure! user: user, reason: reason diff --git a/app/models/closure/reason.rb b/app/models/closure/reason.rb index 1472d09d1..9dca74fc3 100644 --- a/app/models/closure/reason.rb +++ b/app/models/closure/reason.rb @@ -13,6 +13,10 @@ class Closure::Reason < ApplicationRecord pluck(:label).presence || [ FALLBACK_LABEL ] end + def default + labels.first + end + def create_defaults DEFAULT_LABELS.each do |label| create! label: label diff --git a/app/models/command/assign.rb b/app/models/command/assign.rb index 439f01f12..c149b936c 100644 --- a/app/models/command/assign.rb +++ b/app/models/command/assign.rb @@ -1,27 +1,23 @@ class Command::Assign < Command - store_accessor :data, :card_ids, :assignee_ids, :toggled_assignees_by_card + include Command::Cards - validates_presence_of :card_ids, :assignee_ids + store_accessor :data, :assignee_ids, :toggled_assignees_by_card + + validates_presence_of :assignee_ids def title - card_description = if cards.one? - "card '#{cards.first.title}'" - else - "#{cards.count} cards" - end - assignee_description = assignees.collect(&:first_name).join(", ") - "Assign #{assignee_description} to #{card_description}" + "Assign #{assignee_description} to #{cards_description}" end def execute toggled_assignees_by_card = {} transaction do - cards.each do |card| + cards.find_each do |card| toggled_assignees_by_card[card.id] = [] - assignees.each do |assignee| + assignees.find_each do |assignee| assign(assignee, card, toggled_assignees_by_card) end end @@ -39,23 +35,11 @@ class Command::Assign < Command end end - def undoable? - true - end - - def needs_confirmation? - cards.many? - end - private def assignees User.where(id: assignee_ids) end - def cards - user.accessible_cards.where(id: card_ids) - end - def assign(assignee, card, toggled_assignees_by_card) unless card.assigned_to?(assignee) toggled_assignees_by_card[card.id] << assignee.id diff --git a/app/models/command/cards.rb b/app/models/command/cards.rb new file mode 100644 index 000000000..8ba9e4c74 --- /dev/null +++ b/app/models/command/cards.rb @@ -0,0 +1,30 @@ +module Command::Cards + extend ActiveSupport::Concern + + included do + store_accessor :data, :card_ids + + validates_presence_of :card_ids + end + + def undoable? + true + end + + def needs_confirmation? + cards.many? + end + + private + def cards + user.accessible_cards.where(id: card_ids) + end + + def cards_description + if cards.one? + "card '#{cards.first.title}'" + else + "#{cards.count} cards" + end + end +end diff --git a/app/models/command/close.rb b/app/models/command/close.rb new file mode 100644 index 000000000..ac1051c07 --- /dev/null +++ b/app/models/command/close.rb @@ -0,0 +1,35 @@ +class Command::Close < Command + include Command::Cards + + store_accessor :data, :reason, :closed_card_ids + + def title + "Close #{cards_description}" + end + + def execute + closed_card_ids = [] + + transaction do + cards.find_each do |card| + closed_card_ids << card.id + card.close(user: user, reason: reason.presence || Closure::Reason.default) + end + + update! closed_card_ids: closed_card_ids + end + end + + def undo + transaction do + closed_cards.find_each do |card| + card.reopen + end + end + end + + private + def closed_cards + user.accessible_cards.where(id: closed_card_ids) + end +end diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index df5af978a..c56032d91 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -13,6 +13,8 @@ class Command::Parser case command_name when "/assign", "/assignto" Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) + when "/close" + Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) when /^@/ Command::GoToUser.new(user_id: assignee_from(command_name)&.id) else diff --git a/test/models/command/assign_test.rb b/test/models/command/assign_test.rb index c551da602..705dad10e 100644 --- a/test/models/command/assign_test.rb +++ b/test/models/command/assign_test.rb @@ -17,7 +17,7 @@ class Command::AssignTest < ActionDispatch::IntegrationTest assert_includes @card.assignees, users(:kevin) end - test "assigns card on cards' index page" do + test "assigns cards on cards' index page" do execute_command "/assign @kevin @david", context_url: collection_cards_url(@card.collection) cards(:logo, :text, :layout).each do |card| diff --git a/test/models/command/close_test.rb b/test/models/command/close_test.rb new file mode 100644 index 000000000..abdb821b3 --- /dev/null +++ b/test/models/command/close_test.rb @@ -0,0 +1,51 @@ +require "test_helper" + +class Command::CloseTest < ActionDispatch::IntegrationTest + include CommandTestHelper + + setup do + Current.session = sessions(:david) + @card = cards(:text) + end + + test "closes card on perma" do + assert_changes -> { @card.reload.closed? }, from: false, to: true do + execute_command "/close", context_url: collection_card_url(@card.collection, @card) + end + + assert_equal Closure::Reason.default, @card.closure.reason + assert_equal users(:david), @card.closed_by + end + + test "closes card on perma with reason" do + assert_changes -> { @card.reload.closed? }, from: false, to: true do + execute_command "/close Not now", context_url: collection_card_url(@card.collection, @card) + end + + assert_equal users(:david), @card.closed_by + assert_equal "Not now", @card.closure.reason + end + + test "closes cards on cards' index page" do + cards_to_check = cards(:logo, :text, :layout) + cards_to_check.each(&:reopen) + + execute_command "/close", context_url: collection_cards_url(@card.collection) + + cards_to_check.each { it.reload.closed? } + end + + test "undo closing" do + cards_to_check = cards(:logo, :text, :layout) + cards_to_check.each(&:reopen) + + command = parse_command "/close", context_url: collection_cards_url(@card.collection) + command.execute + + cards_to_check.each { it.reload.closed? } + + command.undo + + cards_to_check.each { it.reload.open? } + end +end From 561e9fad0c06ea777e7dd3fd2c85cf6ef664c3ea Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 11:30:44 +0200 Subject: [PATCH 03/21] Support selecting multiple cards by using a list of numbers (separated by comma, or space) https://3.basecamp.com/2914079/buckets/37331921/todos/8620236700#__recording_8620273944 --- app/models/command/filter_cards.rb | 11 +++++++++ app/models/command/parser.rb | 29 ++++++++++++++++-------- app/models/filter.rb | 1 + app/models/filter/fields.rb | 2 +- app/models/filter/params.rb | 4 +++- test/models/command/filter_cards_test.rb | 21 +++++++++++++++++ test/models/filter_test.rb | 3 +++ 7 files changed, 60 insertions(+), 11 deletions(-) create mode 100644 app/models/command/filter_cards.rb create mode 100644 test/models/command/filter_cards_test.rb diff --git a/app/models/command/filter_cards.rb b/app/models/command/filter_cards.rb new file mode 100644 index 000000000..65d87c36f --- /dev/null +++ b/app/models/command/filter_cards.rb @@ -0,0 +1,11 @@ +class Command::FilterCards < Command + store_accessor :data, :card_ids, :params + + def title + "Filter cards #{card_ids.join(",")}" + end + + def execute + redirect_to cards_path(**params.without("card_ids").merge(card_ids: card_ids)) + end +end diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index c56032d91..097c92e56 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -11,14 +11,14 @@ class Command::Parser command_name, *command_arguments = string.strip.split(" ") case command_name - when "/assign", "/assignto" - Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) - when "/close" - Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) - when /^@/ - Command::GoToUser.new(user_id: assignee_from(command_name)&.id) - else - search(string) + when "/assign", "/assignto" + Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) + when "/close" + Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) + when /^@/ + Command::GoToUser.new(user_id: assignee_from(command_name)&.id) + else + search(string) end end @@ -37,10 +37,21 @@ class Command::Parser end def search(string) - if card = user.accessible_cards.find_by_id(string) + if cards = multiple_cards_from(string) + Command::FilterCards.new(card_ids: cards.ids, params: filter.as_params) + elsif card = user.accessible_cards.find_by_id(string) Command::GoToCard.new(card_id: card.id) else Command::Search.new(query: string, params: filter.as_params) end end + + def multiple_cards_from(string) + if tokens = string.split(/[\s,]+/).filter { it =~ /\A\d+\z/ }.presence + if tokens.many? + cards = user.accessible_cards.where(id: tokens) + cards.any? ? cards : nil + end + end + end end diff --git a/app/models/filter.rb b/app/models/filter.rb index e5ce39383..772e80c8b 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -19,6 +19,7 @@ class Filter < ApplicationRecord def cards @cards ||= begin result = creator.accessible_cards.indexed_by(indexed_by) + result = result.where(id: card_ids) if card_ids.present? && !indexed_by.closed? result = result.open unless indexed_by.closed? result = result.by_engagement_status(engagement_status) if engagement_status.present? result = result.unassigned if assignment_status.unassigned? diff --git a/app/models/filter/fields.rb b/app/models/filter/fields.rb index 9ff9764d4..8c0996649 100644 --- a/app/models/filter/fields.rb +++ b/app/models/filter/fields.rb @@ -16,7 +16,7 @@ module Filter::Fields end included do - store_accessor :fields, :assignment_status, :indexed_by, :terms, :engagement_status + store_accessor :fields, :assignment_status, :indexed_by, :terms, :engagement_status, :card_ids def assignment_status super.to_s.inquiry diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index c6f0df13e..a7bed80bb 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -5,6 +5,7 @@ module Filter::Params :assignment_status, :indexed_by, :engagement_status, + card_ids: [], assignee_ids: [], creator_ids: [], collection_ids: [], @@ -40,8 +41,9 @@ module Filter::Params params[:assignment_status] = assignment_status params[:terms] = terms params[:tag_ids] = tags.ids - params[:collection_ids] = collections.ids + params[:collection_ids] = collections.ids params[:stage_ids] = stages.ids + params[:card_ids] = card_ids params[:assignee_ids] = assignees.ids params[:creator_ids] = creators.ids end.compact_blank.reject(&method(:default_value?)) diff --git a/test/models/command/filter_cards_test.rb b/test/models/command/filter_cards_test.rb new file mode 100644 index 000000000..0280157fe --- /dev/null +++ b/test/models/command/filter_cards_test.rb @@ -0,0 +1,21 @@ +require "test_helper" + +class Command::FilterCardsTest < ActionDispatch::IntegrationTest + include CommandTestHelper + + setup do + @card_ids = cards(:logo, :layout).collect(&:id) + end + + test "redirects to the cards index filtering by cards" do + result = execute_command "#{@card_ids.join(" ")}" + + assert_equal cards_path(indexed_by: "newest", card_ids: @card_ids), result.url + end + + test "respects existing filters" do + result = execute_command "#{@card_ids.join(",")}", context_url: "http://37signals.fizzy.localhost:3006/cards?collection_ids%5B%5D=#{collections(:writebook).id}" + + assert_equal cards_path(indexed_by: "newest", collection_ids: [ collections(:writebook).id ], card_ids: @card_ids), result.url + end +end diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 586fc6b0e..ecc690798 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -31,6 +31,9 @@ class FilterTest < ActiveSupport::TestCase filter = users(:david).filters.new indexed_by: "closed" assert_equal [ cards(:shipping) ], filter.cards + + filter = users(:david).filters.new card_ids: [ cards(:logo, :layout).collect(&:id) ] + assert_equal [ cards(:logo), cards(:layout) ], filter.cards end test "can't see cards in collections that aren't accessible" do From 3e5c811bea233595cd684cc8d2f5311dec8ef702 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 11:37:47 +0200 Subject: [PATCH 04/21] Format and rename method --- app/models/command/parser.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index 097c92e56..ea134214b 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -11,14 +11,14 @@ class Command::Parser command_name, *command_arguments = string.strip.split(" ") case command_name - when "/assign", "/assignto" - Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) - when "/close" - Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) - when /^@/ - Command::GoToUser.new(user_id: assignee_from(command_name)&.id) - else - search(string) + when "/assign", "/assignto" + Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) + when "/close" + Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) + when /^@/ + Command::GoToUser.new(user_id: assignee_from(command_name)&.id) + else + parse_free_string(string) end end @@ -36,7 +36,7 @@ class Command::Parser User.all.find { |user| user.mentionable_handles.include?(string_without_at) } end - def search(string) + def parse_free_string(string) if cards = multiple_cards_from(string) Command::FilterCards.new(card_ids: cards.ids, params: filter.as_params) elsif card = user.accessible_cards.find_by_id(string) From c9813f2fb142f0b2f0dd4e666a6398195aadbffc Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 11:38:17 +0200 Subject: [PATCH 05/21] Extract method --- app/models/command/parser.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index ea134214b..e389b2d64 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -39,7 +39,7 @@ class Command::Parser def parse_free_string(string) if cards = multiple_cards_from(string) Command::FilterCards.new(card_ids: cards.ids, params: filter.as_params) - elsif card = user.accessible_cards.find_by_id(string) + elsif card = single_card_from(string) Command::GoToCard.new(card_id: card.id) else Command::Search.new(query: string, params: filter.as_params) @@ -54,4 +54,8 @@ class Command::Parser end end end + + def single_card_from(string) + user.accessible_cards.find_by_id(string) + end end From 181e35b5b2def1ba9590ab69c8345946eb24ba13 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 11:58:17 +0200 Subject: [PATCH 06/21] Restore command when clicking on it --- .../controllers/terminal_controller.js | 5 ++++ app/models/command/parser.rb | 29 ++++++++++++------- app/views/commands/_command.html.erb | 2 +- .../20250507095113_add_line_to_commands.rb | 5 ++++ db/schema.rb | 3 +- db/schema_cache.yml | 12 +++++++- test/models/command/parser_test.rb | 11 +++++++ 7 files changed, 53 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20250507095113_add_line_to_commands.rb diff --git a/app/javascript/controllers/terminal_controller.js b/app/javascript/controllers/terminal_controller.js index e951695f0..26cbde90b 100644 --- a/app/javascript/controllers/terminal_controller.js +++ b/app/javascript/controllers/terminal_controller.js @@ -20,6 +20,11 @@ export default class extends Controller { } } + restoreCommand(event) { + this.#reset(event.target.dataset.line) + this.focus() + } + async #handleErrorResponse(response) { const status = response.status const message = await response.text() diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index e389b2d64..fa758154b 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -8,20 +8,27 @@ class Command::Parser end def parse(string) - command_name, *command_arguments = string.strip.split(" ") - - case command_name - when "/assign", "/assignto" - Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) - when "/close" - Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) - when /^@/ - Command::GoToUser.new(user_id: assignee_from(command_name)&.id) - else - parse_free_string(string) + parse_command(string).tap do |command| + command&.line = string end end + private + def parse_command(string) + command_name, *command_arguments = string.strip.split(" ") + + case command_name + when "/assign", "/assignto" + Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) + when "/close" + Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) + when /^@/ + Command::GoToUser.new(user_id: assignee_from(command_name)&.id) + else + parse_free_string(string) + end + end + private def assignees_from(strings) Array(strings).filter_map do |string| diff --git a/app/views/commands/_command.html.erb b/app/views/commands/_command.html.erb index c10b933e3..f034130b9 100644 --- a/app/views/commands/_command.html.erb +++ b/app/views/commands/_command.html.erb @@ -1,5 +1,5 @@
  • - + <% if command.undoable? %> <%= button_to "Undo", command_undo_path(command), class: "btn btn--plain terminal__button flex-item-justify-end", data: { turbo_confirm: "Are you sure you want to undo '#{command.title}'?", turbo_frame: "_top" } %> diff --git a/db/migrate/20250507095113_add_line_to_commands.rb b/db/migrate/20250507095113_add_line_to_commands.rb new file mode 100644 index 000000000..2978f9869 --- /dev/null +++ b/db/migrate/20250507095113_add_line_to_commands.rb @@ -0,0 +1,5 @@ +class AddLineToCommands < ActiveRecord::Migration[8.1] + def change + add_column :commands, :line, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index a483fa57b..ddcbf6956 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2025_05_05_123008) do +ActiveRecord::Schema[8.1].define(version: 2025_05_07_095113) do create_table "accesses", force: :cascade do |t| t.integer "collection_id", null: false t.datetime "created_at", null: false @@ -162,6 +162,7 @@ ActiveRecord::Schema[8.1].define(version: 2025_05_05_123008) do create_table "commands", force: :cascade do |t| t.datetime "created_at", null: false t.json "data", default: {} + t.text "line" t.string "type" t.datetime "updated_at", null: false t.integer "user_id", null: false diff --git a/db/schema_cache.yml b/db/schema_cache.yml index 80a9473e8..1ae033f77 100644 --- a/db/schema_cache.yml +++ b/db/schema_cache.yml @@ -553,6 +553,16 @@ columns: collation: comment: - *6 + - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column + auto_increment: + name: line + cast_type: *14 + sql_type_metadata: *15 + 'null': true + default: + default_function: + collation: + comment: - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column auto_increment: name: type @@ -2051,4 +2061,4 @@ indexes: comment: valid: true workflows: [] -version: 20250505123008 +version: 20250507095113 diff --git a/test/models/command/parser_test.rb b/test/models/command/parser_test.rb index c4d442dee..a1d648a1f 100644 --- a/test/models/command/parser_test.rb +++ b/test/models/command/parser_test.rb @@ -1 +1,12 @@ +require "test_helper" + # The parser is tested through the tests of specific commands. See +Command::AssignTests+, etc. +class Command::ParserTest < ActionDispatch::IntegrationTest + include CommandTestHelper + + test "the parsed command contains the raw line" do + result = parse_command "assign @kevin" + assert_equal "assign @kevin", result.line + end +end + From 189c7e4a879624079823319b32b84ebc552a6ca9 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 12:07:13 +0200 Subject: [PATCH 07/21] Serve the last commands in reverse order so that the most recent one is just above --- app/controllers/commands_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index f39deb7a7..3fd3ac3a6 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -1,6 +1,6 @@ class CommandsController < ApplicationController def index - @commands = Current.user.commands.order(created_at: :desc).limit(20) + @commands = Current.user.commands.order(created_at: :desc).limit(20).reverse end def create From ba1aa60423581bd0c867590db8ede66e7a7ad7b5 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 12:09:12 +0200 Subject: [PATCH 08/21] Add separation space --- app/models/command/filter_cards.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/command/filter_cards.rb b/app/models/command/filter_cards.rb index 65d87c36f..32d7ce426 100644 --- a/app/models/command/filter_cards.rb +++ b/app/models/command/filter_cards.rb @@ -2,7 +2,7 @@ class Command::FilterCards < Command store_accessor :data, :card_ids, :params def title - "Filter cards #{card_ids.join(",")}" + "Filter cards #{card_ids.join(", ")}" end def execute From 4d2f2067ed7f21de1896c987f06d99bbec98389e Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 12:13:45 +0200 Subject: [PATCH 09/21] Format --- app/views/commands/_command.html.erb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/views/commands/_command.html.erb b/app/views/commands/_command.html.erb index f034130b9..6cfb53375 100644 --- a/app/views/commands/_command.html.erb +++ b/app/views/commands/_command.html.erb @@ -1,7 +1,9 @@
  • - + <%= button_tag command.title, type: "button", class: "btn btn--plain overflow-ellipsis terminal__command flex-item-grow justify-start", + data: { action: "toggle-class#remove terminal#restoreCommand", line: command.line } %> <% if command.undoable? %> - <%= button_to "Undo", command_undo_path(command), class: "btn btn--plain terminal__button flex-item-justify-end", data: { turbo_confirm: "Are you sure you want to undo '#{command.title}'?", turbo_frame: "_top" } %> + <%= button_to "Undo", command_undo_path(command), class: "btn btn--plain terminal__button flex-item-justify-end", + data: { turbo_confirm: "Are you sure you want to undo '#{command.title}'?", turbo_frame: "_top" } %> <% end %>
  • From 0dcc6959da4c48fef4a19c2922cb9b3f3e9e2795 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:31:52 +0200 Subject: [PATCH 10/21] Extract constant to clarify regexp --- .../controllers/navigable_list_controller.js | 74 +++++++++++++++++++ app/views/commands/_command.html.erb | 6 +- 2 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 app/javascript/controllers/navigable_list_controller.js diff --git a/app/javascript/controllers/navigable_list_controller.js b/app/javascript/controllers/navigable_list_controller.js new file mode 100644 index 000000000..d6bf52a0f --- /dev/null +++ b/app/javascript/controllers/navigable_list_controller.js @@ -0,0 +1,74 @@ +import { Controller } from "@hotwired/stimulus" +import { nextFrame } from "helpers/timing_helpers" + +export default class extends Controller { + static targets = [ "item" ] + static values = { selectionAttribute: { type: String, default: "aria-current" } } + + // Actions + + navigate(event) { + if (this.itemTargets.includes(event.target)) { + this.#keyHandlers[event.key]?.call(this, event) + } + } + + selectLast(event) { + this.#setCurrentFrom(this.itemTargets[this.itemTargets.length - 1]) + } + + #handleArrowKey(event, fn) { + if (event.shiftKey || event.metaKey || event.ctrlKey) { return } + fn.call() + event.preventDefault() + } + + #selectPrevious() { + if (this.currentItem.previousElementSibling) { + this.#setCurrentFrom(this.currentItem.previousElementSibling) + } + } + + #selectNext() { + if (this.currentItem.nextElementSibling) { + this.#setCurrentFrom(this.currentItem.nextElementSibling) + } + } + + async #setCurrentFrom(element) { + const selectedItem = this.itemTargets.find(item => item.contains(element)) + + if (selectedItem) { + this.#clearSelection() + selectedItem.setAttribute(this.selectionAttributeValue, "true") + this.currentItem = selectedItem + await nextFrame() + this.currentItem.focus() + } + } + + #clearSelection() { + for (const item of this.itemTargets) { + item.removeAttribute(this.selectionAttributeValue) + } + } + + #keyHandlers = { + ArrowDown(event) { + this.#handleArrowKey(event, this.#selectNext.bind(this)) + }, + ArrowUp(event) { + this.#handleArrowKey(event, this.#selectPrevious.bind(this)) + }, + ArrowRight(event) { + this.#handleArrowKey(event, this.#selectNext.bind(this)) + }, + ArrowLeft(event) { + this.#handleArrowKey(event, this.#selectPrevious.bind(this)) + }, + Enter(event) { + // this.#currentFirstFocusableControl?.focus() + // event.preventDefault() + } + } +} diff --git a/app/views/commands/_command.html.erb b/app/views/commands/_command.html.erb index 6cfb53375..7261472ba 100644 --- a/app/views/commands/_command.html.erb +++ b/app/views/commands/_command.html.erb @@ -1,4 +1,6 @@ -
  • +<%= tag.li class: "min-width full-width flex gap-half terminal__item justify-start", tabindex: 0, data: { + action: "keydown.enter->terminal#restoreCommand:π", + navigable_list_target: "item" } do %> <%= button_tag command.title, type: "button", class: "btn btn--plain overflow-ellipsis terminal__command flex-item-grow justify-start", data: { action: "toggle-class#remove terminal#restoreCommand", line: command.line } %> @@ -6,4 +8,4 @@ <%= button_to "Undo", command_undo_path(command), class: "btn btn--plain terminal__button flex-item-justify-end", data: { turbo_confirm: "Are you sure you want to undo '#{command.title}'?", turbo_frame: "_top" } %> <% end %> -
  • +<% end %> From 87484dbca051cf13d29836c463ac5dd77f7cea29 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:33:54 +0200 Subject: [PATCH 11/21] Add basic keyboard navigation support for the Fizzy do menu --- .../controllers/navigable_list_controller.js | 10 +++++++++- app/javascript/controllers/terminal_controller.js | 7 +++++-- app/views/commands/_command.html.erb | 2 +- app/views/commands/_form.html.erb | 2 +- app/views/commands/_terminal.html.erb | 5 +++-- 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/javascript/controllers/navigable_list_controller.js b/app/javascript/controllers/navigable_list_controller.js index d6bf52a0f..cc6c9aa91 100644 --- a/app/javascript/controllers/navigable_list_controller.js +++ b/app/javascript/controllers/navigable_list_controller.js @@ -13,7 +13,15 @@ export default class extends Controller { } } - selectLast(event) { + selectCurrentOrLast(event) { + if (this.currentItem) { + this.#setCurrentFrom(this.currentItem) + } else { + this.selectLast() + } + } + + selectLast() { this.#setCurrentFrom(this.itemTargets[this.itemTargets.length - 1]) } diff --git a/app/javascript/controllers/terminal_controller.js b/app/javascript/controllers/terminal_controller.js index 26cbde90b..a4148623c 100644 --- a/app/javascript/controllers/terminal_controller.js +++ b/app/javascript/controllers/terminal_controller.js @@ -21,8 +21,11 @@ export default class extends Controller { } restoreCommand(event) { - this.#reset(event.target.dataset.line) - this.focus() + const target = event.target.querySelector("[data-line]") || event.target + if (target.dataset.line) { + this.#reset(target.dataset.line) + this.focus() + } } async #handleErrorResponse(response) { diff --git a/app/views/commands/_command.html.erb b/app/views/commands/_command.html.erb index 7261472ba..9fcecfac9 100644 --- a/app/views/commands/_command.html.erb +++ b/app/views/commands/_command.html.erb @@ -1,5 +1,5 @@ <%= tag.li class: "min-width full-width flex gap-half terminal__item justify-start", tabindex: 0, data: { - action: "keydown.enter->terminal#restoreCommand:π", + action: "keydown.enter->terminal#restoreCommand:prevent keydown.enter->toggle-class#remove:prevent", navigable_list_target: "item" } do %> <%= button_tag command.title, type: "button", class: "btn btn--plain overflow-ellipsis terminal__command flex-item-grow justify-start", data: { action: "toggle-class#remove terminal#restoreCommand", line: command.line } %> diff --git a/app/views/commands/_form.html.erb b/app/views/commands/_form.html.erb index be669b82d..5a82423ad 100644 --- a/app/views/commands/_form.html.erb +++ b/app/views/commands/_form.html.erb @@ -15,7 +15,7 @@ class: "terminal__input input fill-transparent unpad", data: { terminal_target: "input", - action: "keydown.up->toggle-class#add:prevent keydown.down->toggle-class#remove:prevent", + action: "keydown.up->toggle-class#add:prevent keydown.up->navigable-list#selectCurrentOrLast", turbo_permanent: true }, placeholder: "Press ⌘+K to search or type commands…" %> diff --git a/app/views/commands/_terminal.html.erb b/app/views/commands/_terminal.html.erb index 0361692e0..8cb5172ca 100644 --- a/app/views/commands/_terminal.html.erb +++ b/app/views/commands/_terminal.html.erb @@ -1,8 +1,9 @@ <%= tag.div class: "terminal full-width flex flex-column gap justify-end", data: { - controller: "toggle-class terminal", + controller: "toggle-class terminal navigable-list", terminal_error_class: "terminal--error", terminal_confirmation_class: "terminal--confirmation", - toggle_class_toggle_class: "terminal--open" } do %> + toggle_class_toggle_class: "terminal--open", + action: "keydown->navigable-list#navigate keydown.esc->toggle-class#remove keydown.esc->terminal#focus keydown.esc->navigable-list#selectLast" } do %> <%= turbo_frame_tag :recent_commands, src: commands_path, refresh: "morph" %> <%= render "commands/form" %> From 695383da7a46a3e4dfe7adc41e47536c7e20f112 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:49:45 +0200 Subject: [PATCH 12/21] Don't fail when no cards This will prevent nil errors when parsing the command. Instead, the regular command validation will trigger and the error will be handled as expected. --- app/models/command/cards.rb | 2 +- app/models/command/parser/context.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/command/cards.rb b/app/models/command/cards.rb index 8ba9e4c74..d3e47792e 100644 --- a/app/models/command/cards.rb +++ b/app/models/command/cards.rb @@ -4,7 +4,7 @@ module Command::Cards included do store_accessor :data, :card_ids - validates_presence_of :card_ids + validates_presence_of :card_ids, :cards end def undoable? diff --git a/app/models/command/parser/context.rb b/app/models/command/parser/context.rb index 770477963..09591ce72 100644 --- a/app/models/command/parser/context.rb +++ b/app/models/command/parser/context.rb @@ -11,6 +11,8 @@ class Command::Parser::Context user.accessible_cards.where id: params[:id] elsif controller == "cards" && action == "index" filter.cards + else + Card.none end end From 6d0855da287e089d009a865ac696d52996bfd1be Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:50:41 +0200 Subject: [PATCH 13/21] Format --- app/controllers/commands_controller.rb | 4 ++-- app/models/command/parser.rb | 16 ++++++++-------- test/models/command/parser_test.rb | 1 - 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index 3fd3ac3a6..27a33adf0 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -35,9 +35,9 @@ class CommandsController < ApplicationController def respond_with_execution_result(result) case result - when Command::Result::Redirection + when Command::Result::Redirection redirect_to result.url - else + else redirect_back_or_to root_path end end diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index fa758154b..6b3f83bfc 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -18,14 +18,14 @@ class Command::Parser command_name, *command_arguments = string.strip.split(" ") case command_name - when "/assign", "/assignto" - Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) - when "/close" - Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) - when /^@/ - Command::GoToUser.new(user_id: assignee_from(command_name)&.id) - else - parse_free_string(string) + when "/assign", "/assignto" + Command::Assign.new(assignee_ids: assignees_from(command_arguments).collect(&:id), card_ids: cards.ids) + when "/close" + Command::Close.new(card_ids: cards.ids, reason: command_arguments.join(" ")) + when /^@/ + Command::GoToUser.new(user_id: assignee_from(command_name)&.id) + else + parse_free_string(string) end end diff --git a/test/models/command/parser_test.rb b/test/models/command/parser_test.rb index a1d648a1f..21d6e92e9 100644 --- a/test/models/command/parser_test.rb +++ b/test/models/command/parser_test.rb @@ -9,4 +9,3 @@ class Command::ParserTest < ActionDispatch::IntegrationTest assert_equal "assign @kevin", result.line end end - From 837de05baa886d72ef733e7ae5e3477557c4929a Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:54:22 +0200 Subject: [PATCH 14/21] Format --- app/controllers/commands_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index 27a33adf0..12631b481 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -36,9 +36,9 @@ class CommandsController < ApplicationController def respond_with_execution_result(result) case result when Command::Result::Redirection - redirect_to result.url + redirect_to result.url else - redirect_back_or_to root_path + redirect_back_or_to root_path end end end From b9875033bc337eafc1ee9e399c73c8232392b388 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:54:47 +0200 Subject: [PATCH 15/21] Remove unused --- app/javascript/controllers/navigable_list_controller.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/javascript/controllers/navigable_list_controller.js b/app/javascript/controllers/navigable_list_controller.js index cc6c9aa91..ea518de34 100644 --- a/app/javascript/controllers/navigable_list_controller.js +++ b/app/javascript/controllers/navigable_list_controller.js @@ -73,10 +73,6 @@ export default class extends Controller { }, ArrowLeft(event) { this.#handleArrowKey(event, this.#selectPrevious.bind(this)) - }, - Enter(event) { - // this.#currentFirstFocusableControl?.focus() - // event.preventDefault() } } } From bfc89124cf6565ed1c1e3cdbf3e9b3a0cf787a31 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 13:55:28 +0200 Subject: [PATCH 16/21] Reorder --- .../controllers/navigable_list_controller.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/javascript/controllers/navigable_list_controller.js b/app/javascript/controllers/navigable_list_controller.js index ea518de34..4951517d9 100644 --- a/app/javascript/controllers/navigable_list_controller.js +++ b/app/javascript/controllers/navigable_list_controller.js @@ -25,12 +25,6 @@ export default class extends Controller { this.#setCurrentFrom(this.itemTargets[this.itemTargets.length - 1]) } - #handleArrowKey(event, fn) { - if (event.shiftKey || event.metaKey || event.ctrlKey) { return } - fn.call() - event.preventDefault() - } - #selectPrevious() { if (this.currentItem.previousElementSibling) { this.#setCurrentFrom(this.currentItem.previousElementSibling) @@ -61,6 +55,12 @@ export default class extends Controller { } } + #handleArrowKey(event, fn) { + if (event.shiftKey || event.metaKey || event.ctrlKey) { return } + fn.call() + event.preventDefault() + } + #keyHandlers = { ArrowDown(event) { this.#handleArrowKey(event, this.#selectNext.bind(this)) From b017a3fd67a92111a17d123cf4879f6c03376ed3 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 14:29:46 +0200 Subject: [PATCH 17/21] Destroy unconfirmed commands so that they don't end up in the user history --- app/controllers/commands_controller.rb | 1 + test/controllers/commands_controller_test.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index 12631b481..d1247a668 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -11,6 +11,7 @@ class CommandsController < ApplicationController result = command.execute respond_with_execution_result(result) else + command.destroy render plain: command.title, status: :conflict end else diff --git a/test/controllers/commands_controller_test.rb b/test/controllers/commands_controller_test.rb index 2b5f027ba..96a1dd578 100644 --- a/test/controllers/commands_controller_test.rb +++ b/test/controllers/commands_controller_test.rb @@ -22,7 +22,7 @@ class CommandsControllerTest < ActionDispatch::IntegrationTest end test "commands requiring confirmation return a 409 conflict response" do - assert_difference -> { users(:kevin).commands.count }, +1 do + assert_no_difference -> { users(:kevin).commands.count } do post commands_path, params: { command: "/assign @kevin" }, headers: { "HTTP_REFERER" => cards_path } end From 6776e69ed8274c9c83eccc3e3e6738a3530479d0 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 14:42:19 +0200 Subject: [PATCH 18/21] Stop propagation so that it doesn't navigate back on ESC --- app/views/commands/_terminal.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/commands/_terminal.html.erb b/app/views/commands/_terminal.html.erb index 8cb5172ca..31395cb47 100644 --- a/app/views/commands/_terminal.html.erb +++ b/app/views/commands/_terminal.html.erb @@ -3,7 +3,7 @@ terminal_error_class: "terminal--error", terminal_confirmation_class: "terminal--confirmation", toggle_class_toggle_class: "terminal--open", - action: "keydown->navigable-list#navigate keydown.esc->toggle-class#remove keydown.esc->terminal#focus keydown.esc->navigable-list#selectLast" } do %> + action: "keydown->navigable-list#navigate keydown.esc->toggle-class#remove:stop keydown.esc->terminal#focus keydown.esc->navigable-list#selectLast" } do %> <%= turbo_frame_tag :recent_commands, src: commands_path, refresh: "morph" %> <%= render "commands/form" %> From 9c1185055ecfe04947676aeb267b05ed1cd335cc Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 14:47:15 +0200 Subject: [PATCH 19/21] Instead of removing uncofirmed commands don't create them in the first place --- app/controllers/commands_controller.rb | 8 +++----- app/models/command/parser.rb | 3 ++- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index d1247a668..1f835408f 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -6,12 +6,12 @@ class CommandsController < ApplicationController def create command = parse_command(params[:command]) - if command&.valid? + if command.valid? if confirmed?(command) + command.save! result = command.execute respond_with_execution_result(result) else - command.destroy render plain: command.title, status: :conflict end else @@ -21,9 +21,7 @@ class CommandsController < ApplicationController private def parse_command(string) - Command::Parser.new(parsing_context).parse(string).tap do |command| - Current.user.commands << command - end + Command::Parser.new(parsing_context).parse(string) end def parsing_context diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index 6b3f83bfc..dc20bd369 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -9,7 +9,8 @@ class Command::Parser def parse(string) parse_command(string).tap do |command| - command&.line = string + command.user = user + command.line = string end end From 5e812b15ba40a27535bf5e7deff66c78d592c0fe Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 14:52:49 +0200 Subject: [PATCH 20/21] Make undo transactional too --- app/models/command/assign.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/command/assign.rb b/app/models/command/assign.rb index c149b936c..69c4a06dd 100644 --- a/app/models/command/assign.rb +++ b/app/models/command/assign.rb @@ -27,11 +27,13 @@ class Command::Assign < Command end def undo - toggled_assignees_by_card.each do |card_id, assignee_ids| - card = user.accessible_cards.find_by_id(card_id) - assignees = User.where(id: assignee_ids) + transaction do + toggled_assignees_by_card.each do |card_id, assignee_ids| + card = user.accessible_cards.find_by_id(card_id) + assignees = User.where(id: assignee_ids) - undo_assignment(assignees, card) + undo_assignment(assignees, card) + end end end From cfdf19db9dc4eaee1e0b815df0cdd8d708a97cc7 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 14:56:57 +0200 Subject: [PATCH 21/21] Replace conditional with presence --- app/models/command/parser.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/command/parser.rb b/app/models/command/parser.rb index dc20bd369..830b6c93d 100644 --- a/app/models/command/parser.rb +++ b/app/models/command/parser.rb @@ -56,10 +56,7 @@ class Command::Parser def multiple_cards_from(string) if tokens = string.split(/[\s,]+/).filter { it =~ /\A\d+\z/ }.presence - if tokens.many? - cards = user.accessible_cards.where(id: tokens) - cards.any? ? cards : nil - end + user.accessible_cards.where(id: tokens).presence if tokens.many? end end