From 8697cabbe2b319a0ecd8fef522a61b9c12fad9f5 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 7 May 2025 09:33:19 +0200 Subject: [PATCH] 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