From 836f5a1fb3f36bf51ab03a562a92c87b72f3aeac Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 10:16:05 +1000
Subject: [PATCH 1/8] Remove unused withError class

It added specificity but had no use.
I reviewed a couple of screens to make sure:
  - /admin/orders/Rx/customer
  - /admin/properties/new

I have to confess I don't know how Spree::Admin::BaseHelper is included, or where it's used.

Best viewed with whitespace ignored.
---
 app/helpers/spree/admin/base_helper.rb       |  3 ---
 app/webpacker/css/admin/shared/forms.scss    | 26 ++++++++++----------
 app/webpacker/css/admin_v3/shared/forms.scss | 26 ++++++++++----------
 3 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/app/helpers/spree/admin/base_helper.rb b/app/helpers/spree/admin/base_helper.rb
index 9109d082f0..5615d4d62a 100644
--- a/app/helpers/spree/admin/base_helper.rb
+++ b/app/helpers/spree/admin/base_helper.rb
@@ -6,9 +6,6 @@ module Spree
       def field_container(model, method, options = {}, &)
         css_classes = options[:class].to_a
         css_classes << 'field'
-        if error_message_on(model, method).present?
-          css_classes << 'withError'
-        end
         content_tag(:div,
                     capture(&),
                     class: css_classes.join(' '),
diff --git a/app/webpacker/css/admin/shared/forms.scss b/app/webpacker/css/admin/shared/forms.scss
index da593e3300..03bee63048 100644
--- a/app/webpacker/css/admin/shared/forms.scss
+++ b/app/webpacker/css/admin/shared/forms.scss
@@ -97,21 +97,21 @@ span.info {
     }
   }
 
-  &.withError {
-    .field_with_errors {
-      label {
-        color: $color-error;
-      }
-
-      input {
-        border-color: $color-error;
-      }
-    }
-    .formError {
+  // Errors described by default form builder
+  .field_with_errors {
+    label {
       color: $color-error;
-      font-style: italic;
-      font-size: 85%;
     }
+
+    input {
+      border-color: $color-error;
+    }
+  }
+  // Errors described by Spree::Admin::BaseHelper
+  .formError {
+    color: $color-error;
+    font-style: italic;
+    font-size: 85%;
   }
 }
 
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 6ee6f4271b..b269d6a3e2 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -108,21 +108,21 @@ span.info {
     }
   }
 
-  &.withError {
-    .field_with_errors {
-      label {
-        color: $color-error;
-      }
-
-      input {
-        border-color: $color-error;
-      }
-    }
-    .formError {
+  // Errors
+  .field_with_errors {
+    label {
       color: $color-error;
-      font-style: italic;
-      font-size: 85%;
     }
+
+    input {
+      border-color: $color-error;
+    }
+  }
+  // Inline error message
+  .formError {
+    color: $color-error;
+    font-style: italic;
+    font-size: 85%;
   }
 }
 

From 5e478b8a76d1233e16c385f59b0ed7d90053c369 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 12:15:12 +1000
Subject: [PATCH 2/8] Vertically align non-input content in table rows

---
 app/views/admin/products_v3/_table.html.haml | 18 +++++++++---------
 app/webpacker/css/admin/products_v3.scss     |  7 +++++++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index b64072a270..b0bf132b16 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -44,7 +44,7 @@
             %td.align-right
               = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
             %td.align-right
-              .line-clamp-1
+              .content
                 = product.variant_unit.upcase_first
                 / TODO: properly handle custom unit names
                 = WeightsAndMeasures::UNITS[product.variant_unit] && "(" + WeightsAndMeasures::UNITS[product.variant_unit][product.variant_unit_scale]["name"] + ")"
@@ -52,14 +52,14 @@
               -# empty
             %td.align-right
               -# TODO: new requirement "DISPLAY ON DEMAND IF ALL VARIANTS ARE ON DEMAND". And translate value
-              .line-clamp-1= if product.variants.all?(&:on_demand) then "On demand" else product.on_hand || 0 end
+              .content= if product.variants.all?(&:on_demand) then "On demand" else product.on_hand || 0 end
             %td.align-left
-              .line-clamp-1= product.supplier.name
+              .content= product.supplier.name
             %td.align-left
-              .line-clamp-1= product.primary_taxon.name
+              .content= product.primary_taxon.name
             %td.align-left
             %td.align-left
-              .line-clamp-1= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below)
+              .content= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below)
             %td.align-right
               = render partial: 'admin/products_v3/components/product_actions', locals: { product: product }
           - product.variants.each do |variant|
@@ -71,17 +71,17 @@
                 %td.align-right
                   = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
                 %td.align-right
-                  .line-clamp-1= variant.unit_to_display
+                  .content= variant.unit_to_display
                 %td.align-right
                   = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                 %td.align-right
-                  .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements.
+                  .content= variant.on_hand || 0 #TODO: spec for this according to requirements.
                 %td.align-left
-                  .line-clamp-1= variant.product.supplier.name # same as product
+                  .content= variant.product.supplier.name # same as product
                 %td.align-left
                   -# empty
                 %td.align-left
-                  .line-clamp-1= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string.
+                  .content= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string.
                 %td.align-left
                   -# empty
                 %td.align-right
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index da512aecf7..ff21786e13 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -64,6 +64,13 @@
     td {
       padding: $padding-tbl-cell;
       border: none;
+
+      .content {
+        // Plain content fields need help to align with text in inputs (due to vertical-align)
+        margin: $vpadding-txt+1px 0;
+
+        @extend .line-clamp-1;
+      }
     }
 
     th {

From 3b19a1977643d7761e4e6570114cc8b43bcce003 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 14:54:09 +1000
Subject: [PATCH 3/8] Show inline errors for product fields

The form helper () doesn't work for this case, but it seems we can call it directly like this instead. I'd like to fix the helper, but got stuck this time.
---
 app/views/admin/products_v3/_table.html.haml  |  2 ++
 .../system/admin/products_v3/products_spec.rb | 26 +++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index b0bf132b16..d32927aee4 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -41,8 +41,10 @@
             %td.align-left.header
               = product_form.hidden_field :id
               = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
+              = error_message_on product, :name
             %td.align-right
               = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
+              = error_message_on product, :sku
             %td.align-right
               .content
                 = product.variant_unit.upcase_first
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 3630f727e3..85bf2e8c39 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -201,7 +201,7 @@ describe 'As an admin, I can see the new product page' do
       visit admin_products_v3_index_url
     end
 
-    it "can update product and variant fields" do
+    it "updates product and variant fields" do
       within row_containing_name("Apples") do
         fill_in "Name", with: "Pommes"
         fill_in "SKU", with: "POM-00"
@@ -236,7 +236,7 @@ describe 'As an admin, I can see the new product page' do
       expect(page).to have_content "Changes saved"
     end
 
-    it "can discard changes and reload latest data" do
+    it "discards changes and reloads latest data" do
       within row_containing_name("Apples") do
         fill_in "Name", with: "Pommes"
       end
@@ -262,6 +262,28 @@ describe 'As an admin, I can see the new product page' do
         expect(page).to have_field "SKU", with: "APL-10" # Updated value shown
       end
     end
+
+    it "shows errors for product fields" do
+      within row_containing_name("Apples") do
+        fill_in "Name", with: ""
+        fill_in "SKU", with: "A" * 256
+      end
+
+      expect {
+        click_button "Save changes"
+        product_a.reload
+      }.to_not change { product_a.name }
+
+      # (there's no identifier displayed, so the user must remember which product it is..)
+      within row_containing_name("") do
+        expect(page).to have_field "Name", with: ""
+        expect(page).to have_content "can't be blank"
+        expect(page).to have_field "SKU", with: "A" * 256
+        expect(page).to have_content "is too long"
+      end
+      pending
+      expect(page).to have_content "Please review the errors and try again"
+    end
   end
 
   def expect_page_to_be(page_number)

From fee126d6e1e115a1dffae16c41e199c86a81e341 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 12:29:16 +1000
Subject: [PATCH 4/8] Style form error messages

With an icon, and sentence case (upcase_first is similar to humanize, but simpler (https://dev.to/junko911/rails-helper-methods-to-change-the-form-of-strings-1h9c#upcase-first))
---
 app/views/admin/products_v3/_table.html.haml |  4 ++--
 app/webpacker/css/admin/products_v3.scss     |  1 +
 app/webpacker/css/admin_v3/shared/forms.scss | 13 ++++++++++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index d32927aee4..fe722ec40f 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -38,11 +38,11 @@
       = form.fields_for("products", product, index: nil) do |product_form|
         %tbody.relaxed{ 'data-record-id': product_form.object.id }
           %tr
-            %td.align-left.header
+            %td.field.align-left.header
               = product_form.hidden_field :id
               = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
               = error_message_on product, :name
-            %td.align-right
+            %td.field
               = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
               = error_message_on product, :sku
             %td.align-right
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index ff21786e13..4ea63f3a28 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -62,6 +62,7 @@
 
     th,
     td {
+      vertical-align: top;
       padding: $padding-tbl-cell;
       border: none;
 
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index b269d6a3e2..6b9cb9cf8e 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -120,9 +120,20 @@ span.info {
   }
   // Inline error message
   .formError {
+    display: inline-block;
     color: $color-error;
-    font-style: italic;
     font-size: 85%;
+
+    // Icon on left, with subsequent lines indented
+    text-indent: -0.6em;
+    margin-left: 1.2em;
+
+    @extend .icon-remove-sign;
+    @extend [class^="icon-"];
+
+    &:before {
+      margin-right: 0.3em;
+    }
   }
 }
 

From 3ec6386e1cb43210adfcf24d5e2b358efebb0ba4 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 14:31:45 +1000
Subject: [PATCH 5/8] Validate length of some product fields

We know if the values are too long, so let's provide a useful message rather than generating an unhandled database error.
This code seems rather repetetive, it would be good to use a shared module. I wonder if there's a gem for that.

Note that the existing /products/*/edit screen doesn't even handle validation errors yet, but that's something for another day..
---
 app/models/spree/product.rb       | 3 ++-
 spec/models/spree/product_spec.rb | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index be905c8ccc..88a6ef60c1 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -54,7 +54,8 @@ module Spree
     has_many :variant_images, -> { order(:position) }, source: :images,
                                                        through: :variants
 
-    validates :name, presence: true
+    validates :name, presence: true, length: { maximum: columns_hash['name'].limit }
+    validates :sku, length: { maximum: columns_hash['sku'].limit }
 
     validates :variant_unit, presence: true
     validates :unit_value, numericality: {
diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb
index 06ba1e2626..34d16b641f 100644
--- a/spec/models/spree/product_spec.rb
+++ b/spec/models/spree/product_spec.rb
@@ -163,6 +163,10 @@ module Spree
         expect(build(:product)).to be_valid
       end
 
+      it { is_expected.to validate_presence_of :name }
+      it { is_expected.to validate_length_of(:name).is_at_most(255) }
+      it { is_expected.to validate_length_of(:sku).is_at_most(255) }
+
       it "requires a primary taxon" do
         expect(build(:simple_product, primary_taxon: nil)).not_to be_valid
       end

From 875d083a1d0bdcb9f0a78b8f273857a187b2256b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 14:37:14 +1000
Subject: [PATCH 6/8] There's a gem for that [add gem]

---
 Gemfile                     | 1 +
 Gemfile.lock                | 3 +++
 app/models/spree/product.rb | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Gemfile b/Gemfile
index b0fd553864..b71f0ccdc6 100644
--- a/Gemfile
+++ b/Gemfile
@@ -32,6 +32,7 @@ gem "db2fog", github: "openfoodfoundation/db2fog", branch: "rails-7"
 gem "fog-aws", "~> 2.0" # db2fog does not support v3
 gem "mime-types" # required by fog
 
+gem "validates_lengths_from_database"
 gem "valid_email2"
 
 gem "catalog", path: "./engines/catalog"
diff --git a/Gemfile.lock b/Gemfile.lock
index b0d84fdc61..b4edeb19b5 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -736,6 +736,8 @@ GEM
     validate_url (1.0.15)
       activemodel (>= 3.0.0)
       public_suffix
+    validates_lengths_from_database (0.8.0)
+      activerecord (>= 4)
     vcr (6.2.0)
     view_component (3.6.0)
       activesupport (>= 5.2.0, < 8.0)
@@ -909,6 +911,7 @@ DEPENDENCIES
   stripe
   timecop
   valid_email2
+  validates_lengths_from_database
   vcr
   view_component
   view_component_reflex (= 3.1.14.pre9)
diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb
index 88a6ef60c1..7f13d46c0d 100755
--- a/app/models/spree/product.rb
+++ b/app/models/spree/product.rb
@@ -54,8 +54,8 @@ module Spree
     has_many :variant_images, -> { order(:position) }, source: :images,
                                                        through: :variants
 
-    validates :name, presence: true, length: { maximum: columns_hash['name'].limit }
-    validates :sku, length: { maximum: columns_hash['sku'].limit }
+    validates_lengths_from_database
+    validates :name, presence: true
 
     validates :variant_unit, presence: true
     validates :unit_value, numericality: {

From 9a9be8dacdf60f072258a7e0058b704c4d9a9777 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 14:59:54 +1000
Subject: [PATCH 7/8] Validate length of variant fields

---
 app/models/spree/variant.rb                    |  1 +
 app/views/admin/products_v3/_table.html.haml   |  9 ++++++---
 spec/system/admin/products_v3/products_spec.rb | 13 ++++++++++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 3ca3317aa8..039c1ebeca 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -61,6 +61,7 @@ module Spree
 
     localize_number :price, :weight
 
+    validates_lengths_from_database
     validate :check_currency
     validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true
     validates :tax_category, presence: true,
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index fe722ec40f..28d2a3ded3 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -67,15 +67,18 @@
           - product.variants.each do |variant|
             = form.fields_for("products][][variants_attributes][", variant, index: nil) do |variant_form|
               %tr.condensed
-                %td.align-left
+                %td.field
                   = variant_form.hidden_field :id
                   = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name
-                %td.align-right
+                  = error_message_on variant, :display_name
+                %td.field
                   = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
+                  = error_message_on variant, :sku
                 %td.align-right
                   .content= variant.unit_to_display
-                %td.align-right
+                %td.field
                   = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
+                  = error_message_on variant, :price
                 %td.align-right
                   .content= variant.on_hand || 0 #TODO: spec for this according to requirements.
                 %td.align-left
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 85bf2e8c39..a706beeb41 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -263,11 +263,15 @@ describe 'As an admin, I can see the new product page' do
       end
     end
 
-    it "shows errors for product fields" do
+    it "shows errors for both product and variant fields" do
       within row_containing_name("Apples") do
         fill_in "Name", with: ""
         fill_in "SKU", with: "A" * 256
       end
+      within row_containing_name("Medium box") do
+        fill_in "Name", with: "L" * 256
+        fill_in "SKU", with: "1" * 256
+      end
 
       expect {
         click_button "Save changes"
@@ -282,6 +286,13 @@ describe 'As an admin, I can see the new product page' do
         expect(page).to have_content "is too long"
       end
       pending
+      within row_containing_name("L" * 256) do
+        expect(page).to have_field "Name", with: "L" * 256
+        expect(page).to have_field "SKU", with: "1" * 256
+        expect(page).to have_content "is too long"
+        expect(page).to have_field "Price", with: "10.25"
+      end
+
       expect(page).to have_content "Please review the errors and try again"
     end
   end

From 117085aeba216d2d756eef695a8a876a22383c9f Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 21 Sep 2023 09:48:45 +1000
Subject: [PATCH 8/8] Transform weight before validation

I guess validates_length_from_database also validates numbers. That's not a bad thing.
So now it's being validated, we should validate the transformed value that will be saved to the database.
---
 app/models/spree/variant.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 039c1ebeca..9768f00ced 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -81,8 +81,8 @@ module Spree
     before_validation :ensure_shipping_category
     before_validation :ensure_unit_value
     before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? }
+    before_validation :convert_variant_weight_to_decimal
 
-    before_save :convert_variant_weight_to_decimal
     before_save :assign_units, if: ->(variant) {
       variant.new_record? || variant.changed_attributes.keys.intersection(NAME_FIELDS).any?
     }