From 179ae45bf3cf7275f7218c978fb231ebbb2f6a96 Mon Sep 17 00:00:00 2001
From: Jorge Manrubia <jorge@37signals.com>
Date: Mon, 30 Jun 2025 23:01:14 +0200
Subject: [PATCH] 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) }