From 39eeb0e917befec59f3ba514fc6491f8830da1f0 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 26 Oct 2023 10:26:31 +1100
Subject: [PATCH 01/10] Add spec for truthiness

It's generally expected that a #save method will return true on succes, and false on failure.
---
 spec/services/sets/product_set_spec.rb | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 18964d7f21..076ea25c0c 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -7,6 +7,7 @@ describe Sets::ProductSet do
     let(:product_set) do
       described_class.new(collection_attributes: collection_hash)
     end
+    subject{ product_set.save }
 
     context 'when the product does not exist yet' do
       let!(:stock_location) { create(:stock_location, backorderable_default: false) }
@@ -33,6 +34,24 @@ describe Sets::ProductSet do
     end
 
     context 'when the product does exist' do
+      let(:product) { create(:simple_product, name: "product name") }
+
+      context "with valid name" do
+        let(:collection_hash) {
+          { 0 => { id: product.id, name: "New season product" } }
+        }
+
+        it { is_expected.to eq true }
+      end
+
+      context "with invalid name" do
+        let(:collection_hash) {
+          { 0 => { id: product.id, name: "" } } # Product Name can't be blank
+        }
+
+        it { is_expected.to eq false }
+      end
+
       context 'when a different variant_unit is passed' do
         let!(:product) do
           create(
@@ -68,7 +87,6 @@ describe Sets::ProductSet do
 
       context "when the product is in an order cycle" do
         let(:producer) { create(:enterprise) }
-        let(:product) { create(:simple_product) }
 
         let(:distributor) { create(:distributor_enterprise) }
         let!(:order_cycle) {

From e651e3cd5e99901712b18742e65d939527a60e48 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 27 Oct 2023 16:21:46 +1100
Subject: [PATCH 02/10] Tiny spec refactor

I found this in my stashes
---
 spec/services/sets/product_set_spec.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 076ea25c0c..bfdc8db97f 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -139,9 +139,11 @@ describe Sets::ProductSet do
         before { collection_hash[0][:variants_attributes] = variants_attributes }
 
         it 'updates the attributes of the variant' do
-          product_set.save
+          expect {
+            expect(product_set.save).to eq true
+          }.to change { product.variants.first.sku }.to("123")
 
-          expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
+          expect(product_set.invalid.count).to eq 0
         end
 
         context 'and when product attributes are also passed' do

From 1d5ec6b8d2b5697d875c022fe24c74c9949c14a2 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 20 Oct 2023 13:41:21 +1100
Subject: [PATCH 03/10] Spec for updating products with variants

Trying to cover it more comprehensively, and revealing we have a lot of behaviour to update.
Products and their variants should always get saved (or not saved) together. This is considered the most intuitive behaviour.

There's still duplication with the "variant has error" context, but I try to avoid nesting shared_examples, it starts to get ugly. Happy to discuss though.
---
 spec/services/sets/product_set_spec.rb | 110 ++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 20 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index bfdc8db97f..cbb71f6271 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -129,32 +129,102 @@ describe Sets::ProductSet do
       end
     end
 
-    describe "updating variants" do
-      let!(:product) { create(:simple_product) }
-      let(:collection_hash) { { 0 => { id: product.id } } }
+    describe "updating a product's variants" do
+      let(:product) { create(:simple_product) }
+      let(:variant) { product.variants.first }
+      let(:product_attributes) { {} }
+      let(:variant_attributes) { { sku: "var_sku" } }
+      let(:variants_attributes) { [{ **variant_attributes, id: product.variants.first.id.to_s }] }
+      let(:collection_hash) {
+        {
+          0 => { id: product.id, **product_attributes, variants_attributes: }
+        }
+      }
 
-      context 'when :variants_attributes are passed' do
-        let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
+      it "updates the variant" do
+        expect {
+          product_set.save
+          variant.reload
+        }.to change { variant.sku }.to("var_sku")
+      end
 
-        before { collection_hash[0][:variants_attributes] = variants_attributes }
-
-        it 'updates the attributes of the variant' do
+      shared_examples "nothing saved" do
+        it "doesn't update product" do
           expect {
-            expect(product_set.save).to eq true
-          }.to change { product.variants.first.sku }.to("123")
-
-          expect(product_set.invalid.count).to eq 0
+            product_set.save
+            product.reload
+          }.to_not change { product.sku }
         end
 
-        context 'and when product attributes are also passed' do
-          it 'updates product and variant attributes' do
-            collection_hash[0][:sku] = "test_sku"
+        it "doesn't update variant" do
+          expect {
+            product_set.save
+            variant.reload
+          }.to_not change { variant.sku }
+        end
 
-            expect {
-              product_set.save
-              product.reload
-            }.to change { product.sku }.to("test_sku")
-              .and change { product.variants.first.sku }.to("123")
+        it 'assigns the in-memory attributes of the variant' do
+          pending
+          expect {
+            product_set.save
+          }.to change { variant.sku }.to("123")
+        end
+      end
+
+      xcontext "variant has error" do
+        let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
+
+        include_examples "nothing saved"
+      end
+
+      context "when products attributes are also updated" do
+        let(:product_attributes) {
+          { sku: "prod_sku" }
+        }
+
+        it "updates product and variant" do
+          expect {
+            product_set.save
+            product.reload
+          }.to change { product.sku }.to("prod_sku")
+            .and change { product.variants.first.sku }.to("var_sku")
+        end
+
+        xcontext "variant has error" do
+          let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
+
+          include_examples "nothing saved"
+        end
+
+        context "product has error" do
+          before { collection_hash[0][:name] = "" } # product.name can't be blank
+
+          include_examples "nothing saved"
+        end
+      end
+
+      context "when multiple variants are updated" do
+        let(:variant2) { create(:variant, product:) }
+        let(:variants_attributes) {
+          [
+            { **variant_attributes, id: product.variants.first.id.to_s },
+            { sku: "var_sku2", id: variant2.id.to_s },
+          ]
+        }
+
+        it "updates each variant" do
+          expect {
+            product_set.save
+            variant2.reload
+          }.to change { product.variants.first.sku }.to("var_sku")
+            .and change { variant2.sku }.to("var_sku2")
+        end
+
+        xcontext "variant has error" do
+          let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
+
+          include_examples "nothing saved" do
+            after { expect(variant2.reload.sku).to_not eq "var_sku2" }
           end
         end
       end

From de915e8bd7accf340157dd145a5730a7eb394f66 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 2 Nov 2023 15:51:51 +1100
Subject: [PATCH 04/10] Capture variant errors when updating

---
 app/services/sets/product_set.rb              | 17 +++++++++++------
 spec/services/sets/product_set_spec.rb        |  4 +++-
 spec/system/admin/bulk_product_update_spec.rb |  4 ++--
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 23be044239..5c3298e473 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -105,8 +105,15 @@ module Sets
       if variant.present?
         variant.update(variant_attributes.except(:id))
       else
-        create_variant(product, variant_attributes)
+        variant = create_variant(product, variant_attributes)
       end
+
+      # Copy any variant errors to product
+      variant&.errors&.each do |error|
+        # The name is namespaced to avoid confusion with product attrs of same name.
+        product.errors.add("variant_#{error.attribute}".to_sym, error.message)
+      end
+      variant&.errors.blank?
     end
 
     def create_variant(product, variant_attributes)
@@ -117,11 +124,7 @@ module Sets
       on_demand = variant_attributes.delete(:on_demand)
 
       variant = product.variants.create(variant_attributes)
-
-      if variant.errors.present?
-        product.errors.merge!(variant.errors)
-        return false
-      end
+      return variant if variant.errors.present?
 
       begin
         variant.on_demand = on_demand if on_demand.present?
@@ -130,6 +133,8 @@ module Sets
         notify_bugsnag(e, product, variant, variant_attributes)
         raise e
       end
+
+      variant
     end
 
     def notify_bugsnag(error, product, variant, variant_attributes)
diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index cbb71f6271..5a57b8e472 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -154,6 +154,8 @@ describe Sets::ProductSet do
             product_set.save
             product.reload
           }.to_not change { product.sku }
+
+          expect(product_set.invalid.count).to be_positive
         end
 
         it "doesn't update variant" do
@@ -171,7 +173,7 @@ describe Sets::ProductSet do
         end
       end
 
-      xcontext "variant has error" do
+      context "variant has error" do
         let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
 
         include_examples "nothing saved"
diff --git a/spec/system/admin/bulk_product_update_spec.rb b/spec/system/admin/bulk_product_update_spec.rb
index 9444cc87ab..199c8e218f 100644
--- a/spec/system/admin/bulk_product_update_spec.rb
+++ b/spec/system/admin/bulk_product_update_spec.rb
@@ -307,7 +307,7 @@ describe '
 
         click_button 'Save Changes', match: :first
         expect(page.find("#status-message"))
-          .to have_content "Unit value can't be blank Unit value is not a number"
+          .to have_content "Variant unit value can't be blank Variant unit value is not a number"
       end
 
       it "creating a variant with unit value is: '120g' and 'on_demand' checked" do
@@ -323,7 +323,7 @@ describe '
 
         click_button 'Save Changes', match: :first
         expect(page.find("#status-message"))
-          .to have_content "Unit value can't be blank Unit value is not a number"
+          .to have_content "Variant unit value can't be blank Variant unit value is not a number"
       end
     end
   end

From a94c50f0c1ab7ece5b196351110f38eb800a5353 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 2 Nov 2023 14:46:05 +1100
Subject: [PATCH 05/10] Count updated products

Ignoring variants for now.
---
 app/services/sets/product_set.rb       | 13 +++++++-
 spec/services/sets/product_set_spec.rb | 42 +++++++++++++++++---------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 5c3298e473..362acae3f1 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -8,11 +8,15 @@ module Sets
   # }
   #
   class ProductSet < ModelSet
+    attr_reader :saved_count
+
     def initialize(attributes = {})
       super(Spree::Product, [], attributes)
     end
 
     def save
+      @saved_count = 0
+
       # Attempt to save all records, collecting model errors.
       @collection_hash.each_value.map do |product_attributes|
         update_product_attributes(product_attributes)
@@ -71,7 +75,10 @@ module Sets
 
       validate_presence_of_unit_value_in_product(product)
 
-      product.errors.empty? && product.save
+      changed = product.changed?
+      success = product.errors.empty? && product.save
+      count_result(success && changed)
+      success
     end
 
     def validate_presence_of_unit_value_in_product(product)
@@ -137,6 +144,10 @@ module Sets
       variant
     end
 
+    def count_result(saved)
+      @saved_count += 1 if saved
+    end
+
     def notify_bugsnag(error, product, variant, variant_attributes)
       Bugsnag.notify(error) do |report|
         report.add_metadata(:product, product.attributes)
diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 5a57b8e472..4dd6b94315 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -41,7 +41,10 @@ describe Sets::ProductSet do
           { 0 => { id: product.id, name: "New season product" } }
         }
 
-        it { is_expected.to eq true }
+        it "returns true and counts results" do
+          is_expected.to eq true
+          expect(product_set.saved_count).to eq 1
+        end
       end
 
       context "with invalid name" do
@@ -49,7 +52,10 @@ describe Sets::ProductSet do
           { 0 => { id: product.id, name: "" } } # Product Name can't be blank
         }
 
-        it { is_expected.to eq false }
+        it "returns false and counts results" do
+          is_expected.to eq false
+          expect(product_set.saved_count).to eq 0
+        end
       end
 
       context 'when a different variant_unit is passed' do
@@ -76,6 +82,7 @@ describe Sets::ProductSet do
 
         it 'updates the product without error' do
           expect(product_set.save).to eq true
+          expect(product_set.saved_count).to eq 1
 
           expect(product.reload.attributes).to include(
             'variant_unit' => 'weight'
@@ -146,6 +153,9 @@ describe Sets::ProductSet do
           product_set.save
           variant.reload
         }.to change { variant.sku }.to("var_sku")
+
+        pending
+        expect(product_set.saved_count).to eq 1
       end
 
       shared_examples "nothing saved" do
@@ -155,6 +165,7 @@ describe Sets::ProductSet do
             product.reload
           }.to_not change { product.sku }
 
+          expect(product_set.saved_count).to be_zero
           expect(product_set.invalid.count).to be_positive
         end
 
@@ -190,6 +201,8 @@ describe Sets::ProductSet do
             product.reload
           }.to change { product.sku }.to("prod_sku")
             .and change { product.variants.first.sku }.to("var_sku")
+
+          expect(product_set.saved_count).to eq 1
         end
 
         xcontext "variant has error" do
@@ -233,6 +246,7 @@ describe Sets::ProductSet do
     end
 
     context 'when there are multiple products' do
+      let(:product_c) { create(:simple_product, name: "Carrots") }
       let!(:product_b) { create(:simple_product, name: "Bananas") }
       let!(:product_a) { create(:simple_product, name: "Apples") }
 
@@ -246,39 +260,37 @@ describe Sets::ProductSet do
             id: product_b.id,
             name: "Bananes",
           },
+          2 => {
+            id: product_c.id,
+            name: "Carrots",
+          },
         }
       end
 
       it 'updates the products' do
-        product_set.save
+        expect(product_set.save).to eq true
+        expect(product_set.saved_count).to eq 2 # only two were changed
 
         expect(product_a.reload.name).to eq "Pommes"
         expect(product_b.reload.name).to eq "Bananes"
+        expect(product_c.reload.name).to eq "Carrots" # no change
       end
 
       it 'retains the order of products' do
         product_set.save
 
+        # even though the first product is now alphabetically last
         expect(product_set.collection[0]).to eq product_a.reload
         expect(product_set.collection[1]).to eq product_b.reload
+        expect(product_set.collection[2]).to eq product_c.reload
       end
 
       context 'first product has an error' do
-        let(:collection_hash) do
-          {
-            0 => {
-              id: product_a.id,
-              name: "", # Product Name can't be blank
-            },
-            1 => {
-              id: product_b.id,
-              name: "Bananes",
-            },
-          }
-        end
+        before { collection_hash[0][:name] = "" } # product.name can't be blank
 
         it 'continues to update subsequent products' do
           product_set.save
+          expect(product_set.saved_count).to eq 1
 
           # Errors are logged on the model
           first_item = product_set.collection[0]

From f05d27b58b4222620904362871eed7d7278c4f8c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 27 Sep 2023 12:15:00 +1000
Subject: [PATCH 06/10] Show error message summary at top of form

---
 app/reflexes/products_reflex.rb               |  9 +--
 app/views/admin/products_v3/_table.html.haml  | 17 ++---
 app/webpacker/css/admin_v3/shared/forms.scss  | 14 ++++
 config/locales/en.yml                         | 11 ++-
 spec/reflexes/products_reflex_spec.rb         | 13 ++--
 .../system/admin/products_v3/products_spec.rb | 71 ++++++++++++-------
 6 files changed, 89 insertions(+), 46 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 8e18198a40..01736eedab 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -41,8 +41,7 @@ class ProductsReflex < ApplicationReflex
       # flash[:success] = with_locale { I18n.t('.success') }
       # morph_admin_flashes  # ERROR: selector morph type has already been set
     elsif product_set.errors.present?
-      # @error_msg = with_locale{ I18n.t('.products_have_error', count: product_set.invalid.count) }
-      @error_msg = "#{product_set.invalid.count} products have errors."
+      @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count }
     end
 
     render_products_form
@@ -87,10 +86,12 @@ class ProductsReflex < ApplicationReflex
   end
 
   def render_products_form
+    locals = { products: @products }
+    locals[:error_counts] = @error_counts if @error_counts.present?
+
     cable_ready.replace(
       selector: "#products-form",
-      html: render(partial: "admin/products_v3/table",
-                   locals: { products: @products, error_msg: @error_msg })
+      html: render(partial: "admin/products_v3/table", locals:)
     ).broadcast
     morph :nothing
 
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 33806fc195..97122ee0e2 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -1,15 +1,16 @@
 = form_with url: bulk_update_admin_products_path, method: :patch, id: "products-form",
             builder: BulkFormBuilder,
             html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update',
-                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters"} do |form|
-  %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" }
+                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters" } do |form|
+  %fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" }
     .container
-      .status.ten.columns
-        .changed_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'}
-        - if defined?(error_msg) && error_msg.present?
-          .error
-            = error_msg
-      .form-buttons.six.columns
+      .status.eleven.columns
+        .modified_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'}
+        - if defined?(error_counts)
+          .error_summary
+            -# X products were saved correctly, but Y products could not be saved correctly. Please review errors and try again
+            = t('.error_summary.saved', count: error_counts[:saved]) + t('.error_summary.invalid', count: error_counts[:invalid])
+      .form-buttons.five.columns
         = form.submit t('.reset'), type: :reset, class: "medium", 'data-reflex': 'click->products#fetch'
         = form.submit t('.save'), class: "medium"
   %table.products
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 96cef47615..35f8697e04 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -282,6 +282,20 @@ fieldset {
   text-align: center;
 }
 
+.modified_summary {
+  font-size: inherit;
+}
+
+.error_summary {
+  font-size: inherit;
+
+  @extend .icon-remove-sign;
+  &:before {
+    font-family: FontAwesome;
+    color: $color-error;
+  }
+}
+
 select {
   @extend input, [type="text"];
   background-color: white;
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 68cd6ef091..edbc5b677f 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -837,13 +837,18 @@ en:
           zero: ""
           one: "%{count} product modified."
           other: "%{count} products modified."
+        error_summary:
+          saved:
+            zero: ""
+            one: "%{count} product was saved correctly, but "
+            other: "%{count} products were saved correctly, but "
+          invalid:
+            one: "%{count} product could not be saved. Please review the errors and try again."
+            other: "%{count} products could not be saved. Please review the errors and try again."
         save: Save changes
         reset: Discard changes
       bulk_update: # TODO: fix these
         success: "Products successfully updated" #TODO: add count
-        products_have_error:
-          one: "%{count} product has an error."
-          other: "%{count} products have an error."
     product_import:
       title: Product Import
       file_not_found: File not found or could not be opened
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index ae95f16b8c..0672c46863 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -13,7 +13,7 @@ describe ProductsReflex, type: :reflex do
     Flipper.enable(:admin_style_v3)
   end
 
-  describe 'fetch' do
+  describe '#fetch' do
     subject{ build_reflex(method_name: :fetch, **context) }
 
     describe "sorting" do
@@ -41,6 +41,7 @@ describe ProductsReflex, type: :reflex do
              sku: "APL-01",
              price: 5.25)
     }
+    let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") }
     let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") }
     let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
 
@@ -123,17 +124,21 @@ describe ProductsReflex, type: :reflex do
           "products" => [
             {
               "id" => product_a.id.to_s,
-              "name" => "",
+              "name" => "Pommes",
             },
             {
               "id" => product_b.id.to_s,
-              "name" => "",
+              "name" => "", # Name can't be blank
+            },
+            {
+              "id" => product_c.id.to_s,
+              "name" => "", # Name can't be blank
             },
           ]
         }
 
         reflex = run_reflex(:bulk_update, params:)
-        expect(reflex.get(:error_msg)).to include "2 products have errors"
+        expect(reflex.get(:error_counts)).to eq({ saved: 1, invalid: 2 })
 
         # # WTF
         # expect{ reflex(:bulk_update, params:) }.to broadcast(
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 8ea6f57c48..5b403b0e18 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -270,37 +270,54 @@ describe 'As an admin, I can see the new product page' do
       end
     end
 
-    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
+    context "with invalid data" do
+      let!(:product_b) { create(:simple_product, name: "Bananas") }
+
+      before 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
+          fill_in "Price", with: "10.25"
+        end
       end
 
-      expect {
-        click_button "Save changes"
-        product_a.reload
-      }.to_not change { product_a.name }
+      it "shows errors for both product and variant fields" do
+        # Also update another product with valid data
+        within row_containing_name("Bananas") do
+          fill_in "Name", with: "Bananes"
+        end
 
-      # (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
-      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 {
+          click_button "Save changes"
+          product_a.reload
+        }.to_not change { product_a.name }
 
-      expect(page).to have_content "Please review the errors and try again"
+        # pending("unchanged rows are being saved") # TODO: don't report unchanged rows
+        # expect(page).to_not have_content("rows were saved correctly")
+        # Both the product and variant couldn't be saved.
+        expect(page).to have_content("1 product could not be saved")
+        expect(page).to have_content "Please review the errors and try again"
+
+        # (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 "bug #11748"
+        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" # other updated value is retained
+        end
+      end
     end
   end
 

From 41cf0bedfcccfdb96a0d2950901ac10d737c47e6 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 27 Sep 2023 16:24:12 +1000
Subject: [PATCH 07/10] Fix: Handle missing attributes

---
 app/webpacker/controllers/bulk_form_controller.js | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 842a1aaa89..702d882d09 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -52,7 +52,7 @@ export default class BulkFormController extends Controller {
     this.#disableOtherElements(formChanged); // like filters and sorting
 
     // Display number of records changed
-    const key = this.changedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
+    const key = this.hasChangedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
     if (key) {
       this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount });
     }
@@ -76,13 +76,15 @@ export default class BulkFormController extends Controller {
   // private
 
   #disableOtherElements(disable) {
+    if (!this.hasDisableSelectorValue) return;
+
     this.disableElements ||= document.querySelectorAll(this.disableSelectorValue);
 
-    if (this.disableElements) {
-      this.disableElements.forEach((element) => {
-        element.classList.toggle("disabled-section", disable);
-      });
-    }
+    if (!this.disableElements) return;
+
+    this.disableElements.forEach((element) => {
+      element.classList.toggle("disabled-section", disable);
+    });
   }
 
   #isChanged(element) {

From d0abbc5d2cedba0c92026a7b753a3bb5498f214a Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 27 Sep 2023 16:44:32 +1000
Subject: [PATCH 08/10] Ensure error summary always shows when error

Best viewed with whitespace ignored.
---
 app/views/admin/products_v3/_table.html.haml  |   4 +-
 .../controllers/bulk_form_controller.js       |   6 +-
 .../stimulus/bulk_form_controller_test.js     | 102 ++++++++++++------
 .../system/admin/products_v3/products_spec.rb |  18 +++-
 4 files changed, 93 insertions(+), 37 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 97122ee0e2..3c6d40fa74 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -1,7 +1,9 @@
 = form_with url: bulk_update_admin_products_path, method: :patch, id: "products-form",
             builder: BulkFormBuilder,
             html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update',
-                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters" } do |form|
+                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters",
+                   'data-bulk-form-error-value': defined?(error_counts),
+                  } do |form|
   %fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" }
     .container
       .status.eleven.columns
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 702d882d09..14680ff4bd 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -5,6 +5,7 @@ export default class BulkFormController extends Controller {
   static targets = ["actions", "changedSummary"];
   static values = {
     disableSelector: String,
+    error: Boolean,
   };
   recordElements = {};
 
@@ -25,6 +26,8 @@ export default class BulkFormController extends Controller {
         this.recordElements[recordId].push(element);
       }
     }
+
+    this.toggleFormChanged();
   }
 
   disconnect() {
@@ -45,7 +48,7 @@ export default class BulkFormController extends Controller {
     const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
       elements.some(this.#isChanged)
     ).length;
-    const formChanged = changedRecordCount > 0;
+    const formChanged = changedRecordCount > 0 || this.errorValue;
 
     // Show actions
     this.actionsTarget.classList.toggle("hidden", !formChanged);
@@ -54,6 +57,7 @@ export default class BulkFormController extends Controller {
     // Display number of records changed
     const key = this.hasChangedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
     if (key) {
+      // TODO: save processing and only run if changedRecordCount has changed.
       this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount });
     }
 
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index db0f990eef..d50dabbaf4 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -11,42 +11,39 @@ describe("BulkFormController", () => {
     application.register("bulk-form", bulk_form_controller);
   });
 
-  // Mock I18n. TODO: moved to a shared helper
-  beforeAll(() => {
-    const mockedT = jest.fn();
-    mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
-
-    global.I18n =  {
-      t: mockedT
-    };
-  })
-
-  // (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
-  afterAll(() => {
-    delete global.I18n;
-  })
-
-  beforeEach(() => {
-    document.body.innerHTML = `
-      <div id="disable1"></div>
-      <div id="disable2"></div>
-      <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
-        <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
-        <div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
-        <div data-record-id="1">
-          <input id="input1a" type="text" value="initial1a">
-          <input id="input1b" type="text" value="initial1b">
-        </div>
-        <div data-record-id="2">
-          <input id="input2" type="text" value="initial2">
-        </div>
-        <input type="submit">
-      </form>
-    `;
-  });
-
   describe("Modifying input values", () => {
+    // Mock I18n. TODO: moved to a shared helper
+    beforeAll(() => {
+      const mockedT = jest.fn();
+      mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
+
+      global.I18n =  {
+        t: mockedT
+      };
+    })
+    // (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
+    afterAll(() => {
+      delete global.I18n;
+    })
+
     beforeEach(() => {
+      document.body.innerHTML = `
+        <div id="disable1"></div>
+        <div id="disable2"></div>
+        <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
+          <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
+          <div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
+          <div data-record-id="1">
+            <input id="input1a" type="text" value="initial1a">
+            <input id="input1b" type="text" value="initial1b">
+          </div>
+          <div data-record-id="2">
+            <input id="input2" type="text" value="initial2">
+          </div>
+          <input type="submit">
+        </form>
+      `;
+
       const disable1 = document.getElementById("disable1");
       const disable2 = document.getElementById("disable2");
       const actions = document.getElementById("actions");
@@ -162,6 +159,43 @@ describe("BulkFormController", () => {
     });
   });
 
+  describe("When there are errors", () => {
+    beforeEach(() => {
+      document.body.innerHTML = `
+        <form data-controller="bulk-form" data-bulk-form-error-value="true">
+          <div id="actions" data-bulk-form-target="actions">
+            An error occurred.
+            <input type="submit">
+          </div>
+          <div data-record-id="1">
+            <input id="input1a" type="text" value="initial1a">
+          </div>
+        </form>
+      `;
+
+      const actions = document.getElementById("actions");
+      const changed_summary = document.getElementById("changed_summary");
+      const input1a = document.getElementById("input1a");
+    });
+
+    it("form actions section remains visible", () => {
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+
+      // Record 1: First field changed
+      input1a.value = 'updated1a';
+      input1a.dispatchEvent(new Event("change"));
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+
+      // Change back to original value
+      input1a.value = 'initial1a';
+      input1a.dispatchEvent(new Event("change"));
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+    });
+  });
+
   // unable to test disconnect at this stage
   // describe("disconnect()", () => {
   //   it("resets other elements", () => {
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 5b403b0e18..5316a3118f 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -203,7 +203,6 @@ describe 'As an admin, I can see the new product page' do
              price: 5.25)
     }
     let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
-
     before do
       visit admin_products_url
     end
@@ -318,6 +317,23 @@ describe 'As an admin, I can see the new product page' do
           expect(page).to have_field "Price", with: "10.25" # other updated value is retained
         end
       end
+
+      it "saves changes after fixing errors" do
+        within row_containing_name("Apples") do
+          fill_in "Name", with: "Pommes"
+          fill_in "SKU", with: "POM-00"
+        end
+
+        expect {
+          click_button "Save changes"
+          product_a.reload
+          variant_a1.reload
+        }.to change { product_a.name }.to("Pommes")
+          .and change{ product_a.sku }.to("POM-00")
+
+        pending
+        expect(page).to have_content "Changes saved"
+      end
     end
   end
 

From f63f37fd3bac6446d3f7f6f194d6ff3dd8d9fffd Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 17 Oct 2023 14:48:30 +1100
Subject: [PATCH 09/10] Wrap form element in a form

This makes it easier to control in the next commit.
---
 app/views/admin/products_v3/_sort.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_sort.html.haml b/app/views/admin/products_v3/_sort.html.haml
index 20c67e083b..d3b4829f84 100644
--- a/app/views/admin/products_v3/_sort.html.haml
+++ b/app/views/admin/products_v3/_sort.html.haml
@@ -4,6 +4,6 @@
     - if search_term.present? || producer_id.present? || category_id.present?
       %a{ href: "#", class: "button disruptive", data: { reflex: "click->products#clear_search" } }
         = t(".pagination.clear_search")  
-  %div.with-dropdown
+  %form.with-dropdown
     = t(".pagination.per_page.show")
     = select_tag :per_page, options_for_select([15, 25, 50, 100].collect{|i| [t('.pagination.per_page.per_page', num: i), i]}, pagy.items), data: { reflex: "change->products#change_per_page" }

From 7fe6f3fe89e49aa47e0367a272f672b98284d6dc Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 17 Oct 2023 15:18:58 +1100
Subject: [PATCH 10/10] Disable form elements in a disabled-section

I chose to use the 'elements' collection rather than choosing which elements to include (ie this supports inputs, textareas, buttons and anything else I didn't think of). It could be a bit simpler if we assume the element is a form. Even simpler if it's a fieldset (that has a disabled property). But I didn't want to limit it too much.

Unfortunately JS is quite ugly compared to Ruby. And 'prettier' made it uglier in my opinion.
---
 app/webpacker/controllers/bulk_form_controller.js    |  8 ++++++++
 .../stimulus/bulk_form_controller_test.js            | 12 ++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 14680ff4bd..ab93978584 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -88,6 +88,14 @@ export default class BulkFormController extends Controller {
 
     this.disableElements.forEach((element) => {
       element.classList.toggle("disabled-section", disable);
+
+      // Also disable any form elements
+      let forms = element.tagName == "FORM" ? [element] : element.querySelectorAll("form");
+
+      forms &&
+        forms.forEach((form) =>
+          Array.from(form.elements).forEach((formElement) => (formElement.disabled = disable))
+        );
     });
   }
 
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index d50dabbaf4..d558e4b37a 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -28,8 +28,8 @@ describe("BulkFormController", () => {
 
     beforeEach(() => {
       document.body.innerHTML = `
-        <div id="disable1"></div>
-        <div id="disable2"></div>
+        <form id="disable1"><input id="disable1_element"></form>
+        <div id="disable2"><form><input id="disable2_element"></form></div>
         <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
           <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
           <div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
@@ -45,7 +45,9 @@ describe("BulkFormController", () => {
       `;
 
       const disable1 = document.getElementById("disable1");
+      const disable1_element = document.getElementById("disable1_element");
       const disable2 = document.getElementById("disable2");
+      const disable2_element = document.getElementById("disable2_element");
       const actions = document.getElementById("actions");
       const changed_summary = document.getElementById("changed_summary");
       const input1a = document.getElementById("input1a");
@@ -112,7 +114,9 @@ describe("BulkFormController", () => {
         expect(actions.classList).not.toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
         expect(disable1.classList).toContain('disabled-section');
+        expect(disable1_element.disabled).toBe(true);
         expect(disable2.classList).toContain('disabled-section');
+        expect(disable2_element.disabled).toBe(true);
 
         // Record 1: Second field changed
         input1b.value = 'updated1b';
@@ -145,7 +149,9 @@ describe("BulkFormController", () => {
         expect(actions.classList).not.toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
         expect(disable1.classList).toContain('disabled-section');
+        expect(disable1_element.disabled).toBe(true);
         expect(disable2.classList).toContain('disabled-section');
+        expect(disable2_element.disabled).toBe(true);
 
         // Record 2: Change back to original value
         input2.value = 'initial2';
@@ -154,7 +160,9 @@ describe("BulkFormController", () => {
         expect(actions.classList).toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":0}');
         expect(disable1.classList).not.toContain('disabled-section');
+        expect(disable1_element.disabled).toBe(false);
         expect(disable2.classList).not.toContain('disabled-section');
+        expect(disable2_element.disabled).toBe(false);
       });
     });
   });