From 1e883af9cd3102f2774910692542f0fdc9dbfcc4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 11 Mar 2020 18:37:28 +0100
Subject: [PATCH] Don't count errors for the same field twice

The number of errors in a form includes several errors for the same
field. For example, if a title is mandatory and has to have at least 5
characters, leaving the title blank will result in two errors. So users
will be invited to look for two errors, but they'll only find one field
with errors.

So it's a bit more intuitive to show as many errors as fields having
errors.

Note we're excluding errors on `:base`, which is a bit of a hack for
errors in association fields. For example, if the title of one
translation is not present, `resource.errors.messages` will contain two
elements: one for the translation's title, and one for the `base` field.
This resulted in the count of errors being 2 when there was only one.

Also note I haven't found a way to count errors on all `has_many`
relations. That is, if two translations have a missing title field, only
one error will be mentioned in the message (as it did before this
commit).
---
 app/views/shared/_errors.html.erb           |  4 ++-
 spec/system/admin/dashboard/actions_spec.rb |  3 +-
 spec/views/shared/errors_spec.rb            | 40 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 spec/views/shared/errors_spec.rb

diff --git a/app/views/shared/_errors.html.erb b/app/views/shared/_errors.html.erb
index 010cee74a..fff7801f8 100644
--- a/app/views/shared/_errors.html.erb
+++ b/app/views/shared/_errors.html.erb
@@ -5,7 +5,9 @@
     </button>
 
     <strong>
-      <%= pluralize resource.errors.count, t("form.error"), t("form.errors") %>
+      <% errors_count = resource.errors.messages.reject { |attribute| attribute == :base }.count %>
+
+      <%= pluralize errors_count, t("form.error"), t("form.errors") %>
 
       <% if local_assigns[:message].present? %>
         <%= message %>
diff --git a/spec/system/admin/dashboard/actions_spec.rb b/spec/system/admin/dashboard/actions_spec.rb
index 654c41ea3..bae5c6005 100644
--- a/spec/system/admin/dashboard/actions_spec.rb
+++ b/spec/system/admin/dashboard/actions_spec.rb
@@ -63,7 +63,8 @@ describe "Admin dashboard actions" do
 
     scenario "Renders create form in case data is invalid" do
       click_button "Save"
-      expect(page).to have_content("errors prevented this Dashboard/Action from being saved.")
+
+      expect(page).to have_content("error prevented this Dashboard/Action from being saved.")
     end
   end
 
diff --git a/spec/views/shared/errors_spec.rb b/spec/views/shared/errors_spec.rb
new file mode 100644
index 000000000..6e3c4ec49
--- /dev/null
+++ b/spec/views/shared/errors_spec.rb
@@ -0,0 +1,40 @@
+require "rails_helper"
+
+describe "shared errors" do
+  class DummyModel
+    include ActiveModel::Model
+    attr_accessor :title, :description, :days
+
+    validates :title, presence: true
+    validates :description, presence: true, length: { in: 10..100 }
+    validates :days, numericality: { greater_than: 10 }
+  end
+
+  it "counts the number of fields with errors" do
+    resource = DummyModel.new(title: "Present", description: "", days: 3)
+    resource.valid?
+
+    render "shared/errors", resource: resource
+
+    expect(rendered).to have_content "2 errors"
+  end
+
+  it "doesn't include `base` errors in new records" do
+    resource = build(:debate, title: "", description: "")
+    resource.valid?
+
+    render "shared/errors", resource: resource
+
+    expect(rendered).to have_content "2 errors"
+  end
+
+  it "doesn't include `base` errors in existing records" do
+    resource = create(:debate)
+    resource.translations << Debate::Translation.new(title: "Title", description: "", locale: "es")
+    resource.valid?
+
+    render "shared/errors", resource: resource
+
+    expect(rendered).to have_content "1 error"
+  end
+end