From 179ae45bf3cf7275f7218c978fb231ebbb2f6a96 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Mon, 30 Jun 2025 23:01:14 +0200 Subject: [PATCH 1/5] Show command errors I went with placing htis logic at the command level since it plays well with the composite structure, where you don't know whether you are handling one command or many commands, and you want to aggregate all the errors. I didn't leverage the existing support with rails' model errors because that customizing those message is cumbersome (either i18n or using custom validation logic for everything). --- app/controllers/commands_controller.rb | 3 ++- .../controllers/terminal_controller.js | 7 +++-- app/models/command.rb | 27 +++++++++++++++++++ app/models/command/composite.rb | 4 +++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/controllers/commands_controller.rb b/app/controllers/commands_controller.rb index ae5c2232e..adae98eb1 100644 --- a/app/controllers/commands_controller.rb +++ b/app/controllers/commands_controller.rb @@ -68,6 +68,7 @@ class CommandsController < ApplicationController end def respond_with_error(command) - render json: { error: command.errors.full_messages.join(", "), context_url: command.context.url }, status: :unprocessable_entity + error_message = command.error_messages.map { |msg| "- #{msg}\n" }.join + render json: { error: error_message, context_url: command.context.url }, status: :unprocessable_entity end end diff --git a/app/javascript/controllers/terminal_controller.js b/app/javascript/controllers/terminal_controller.js index b90be4cc5..009a14fdf 100644 --- a/app/javascript/controllers/terminal_controller.js +++ b/app/javascript/controllers/terminal_controller.js @@ -64,6 +64,7 @@ export default class extends Controller { } hideError() { + this.#hideOutput() this.element.classList.remove(this.errorClass) } @@ -113,13 +114,15 @@ export default class extends Controller { const status = response.status if (status === HttpStatus.UNPROCESSABLE) { - this.#showError() + this.#showError(response) } else if (status === HttpStatus.CONFLICT) { await this.#handleConflictResponse(response) } } - #showError() { + async #showError(response) { + const message = await response.json() + this.#showOutput(message.error) this.element.classList.add(this.errorClass) } diff --git a/app/models/command.rb b/app/models/command.rb index d5a3eacef..3f0640609 100644 --- a/app/models/command.rb +++ b/app/models/command.rb @@ -37,8 +37,35 @@ class Command < ApplicationRecord false end + def error_messages + errors.to_hash.flat_map do |attribute, message| + error_message_for(attribute, message) + end.uniq + end + private def redirect_to(...) Command::Result::Redirection.new(...) end + + def error_message_for(attribute, message) + case attribute.to_sym + when :cards, :card_ids + "No cards in this context" + when :card + "Can't find the card" + when :collection + "The collection is missing" + when :assignee_ids + "Assignees are missing" + when :user + "Can't find that user" + when :stage + "Can't find that workflow stage" + when :tag_title + "A tag is required" + else + message + end + end end diff --git a/app/models/command/composite.rb b/app/models/command/composite.rb index 59256b773..dd696e6bd 100644 --- a/app/models/command/composite.rb +++ b/app/models/command/composite.rb @@ -34,6 +34,10 @@ class Command::Composite < Command commands.any?(&:needs_confirmation?) end + def error_messages + commands.flat_map(&:error_messages).uniq + end + private def commands_excluding_redirections commands.reject { it.is_a?(Command::VisitUrl) } From ab5d6931de362075ffebb4ba8972692858ffa06d Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Mon, 30 Jun 2025 18:14:43 -0500 Subject: [PATCH 2/5] Color the output red, too --- app/assets/stylesheets/terminals.css | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/terminals.css b/app/assets/stylesheets/terminals.css index e0902fd49..58f1f3de6 100644 --- a/app/assets/stylesheets/terminals.css +++ b/app/assets/stylesheets/terminals.css @@ -170,6 +170,10 @@ color: var(--color-terminal-text-light); } + .terminal--error & { + color: var(--color-negative); + } + .terminal--showing-output & { display: block; } From fb1ec2ab3a88c5be8d668767d88bb3607384a239 Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Mon, 30 Jun 2025 18:14:48 -0500 Subject: [PATCH 3/5] Copy edits --- app/models/command.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/command.rb b/app/models/command.rb index 3f0640609..5cc96b6f0 100644 --- a/app/models/command.rb +++ b/app/models/command.rb @@ -51,19 +51,19 @@ class Command < ApplicationRecord def error_message_for(attribute, message) case attribute.to_sym when :cards, :card_ids - "No cards in this context" + "" when :card - "Can't find the card" + "Needs one or more cards to apply to (#123, #124)." when :collection - "The collection is missing" + "You need to specificy a Collection" when :assignee_ids - "Assignees are missing" + "Needs at leaset one assignee (@person)." when :user - "Can't find that user" + "Can’t find that person." when :stage - "Can't find that workflow stage" + "Can’t find that Workflow Stage." when :tag_title - "A tag is required" + "Needs at least one tag (#tag, #name)" else message end From 888923d32b43991fca5553e925de2cddb79d91eb Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 1 Jul 2025 09:50:10 +0200 Subject: [PATCH 4/5] Copy tweaks --- app/models/command.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/command.rb b/app/models/command.rb index 5cc96b6f0..83ccbef0f 100644 --- a/app/models/command.rb +++ b/app/models/command.rb @@ -51,13 +51,13 @@ class Command < ApplicationRecord def error_message_for(attribute, message) case attribute.to_sym when :cards, :card_ids - "" + "Needs one or more cards to apply to" when :card "Needs one or more cards to apply to (#123, #124)." when :collection - "You need to specificy a Collection" + "You need to specify a Collection" when :assignee_ids - "Needs at leaset one assignee (@person)." + "Needs at least one assignee (@person)." when :user "Can’t find that person." when :stage From 50b4f34f3422071359d6a776f60f55d983726161 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Tue, 1 Jul 2025 09:51:11 +0200 Subject: [PATCH 5/5] Copy tweaks --- app/models/command.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/command.rb b/app/models/command.rb index 83ccbef0f..5b5ecd943 100644 --- a/app/models/command.rb +++ b/app/models/command.rb @@ -51,9 +51,9 @@ class Command < ApplicationRecord def error_message_for(attribute, message) case attribute.to_sym when :cards, :card_ids - "Needs one or more cards to apply to" + "Needs one or more cards to apply to (#123, #124)" when :card - "Needs one or more cards to apply to (#123, #124)." + "Needs one card to apply to." when :collection "You need to specify a Collection" when :assignee_ids