From a98242e5b4d55cf52740ead61f5d5ada7f2d0852 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 13:55:24 +1000
Subject: [PATCH 01/18] Capybara: enable_aria_label

---
 spec/system/support/capybara_setup.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/spec/system/support/capybara_setup.rb b/spec/system/support/capybara_setup.rb
index e253f192fe..6e46d77420 100644
--- a/spec/system/support/capybara_setup.rb
+++ b/spec/system/support/capybara_setup.rb
@@ -1,5 +1,9 @@
 # frozen_string_literal: true
 
+# Whether fields, links, and buttons will match against aria-label attribute.
+# This allows us to find <input aria-label="Name"> with `expect(page).to have_field "Name"`
+Capybara.enable_aria_label = true
+
 # Usually, especially when using Selenium, developers tend to increase the max wait time.
 # With Cuprite, there is no need for that.
 # We use a Capybara default value here explicitly.

From ae3cd6f7e046bb1eac5d8e8800bee3dba1f0f7f6 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 13 Jul 2023 16:51:40 +1000
Subject: [PATCH 02/18] Add bulk_update product form for product name

(For now at least,) we use one big standard Rails form, and ModelSet to update each record.
Submitting with Reflex allows us to manage the loading state along with the rest of the page (although I would rather use the built in HTTP POST standard).

Aria-label makes it a bit easier for testing (and accessibility software of course!). Technically it should have been aria-labelledby="id_of_column_header" but that would have resulted in more HTML and processing, which seemed silly.

Best viewed with whitespace ignored.
---
 app/reflexes/products_reflex.rb               |  51 +++++++
 app/views/admin/products_v3/_table.html.haml  | 138 +++++++++---------
 config/locales/en.yml                         |   5 +
 config/routes/admin.rb                        |   4 +-
 spec/reflexes/products_reflex_spec.rb         |  27 ++++
 .../system/admin/products_v3/products_spec.rb |  35 ++++-
 6 files changed, 190 insertions(+), 70 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index bc8bcd9081..b2c845d864 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -31,6 +31,24 @@ class ProductsReflex < ApplicationReflex
     fetch_and_render_products
   end
 
+  def bulk_update
+    product_set = product_set_from_params
+
+    product_set.collection.each { |p| authorize! :update, p }
+
+    if product_set.save
+      # flash[:success] = with_locale { I18n.t('.success') }
+      # morph_admin_flashes  # ERROR: selector morph type has already been set
+
+      fetch_and_render_products
+    elsif product_set.errors.present?
+      # todo: render form with error messages
+      render json: { errors: product_set.errors }, status: :bad_request
+    else
+      render body: nil, status: :internal_server_error
+    end
+  end
+
   private
 
   def init_filters_params
@@ -130,4 +148,37 @@ class ProductsReflex < ApplicationReflex
     url.query += "&_category_id=#{@category_id}" if @category_id.present?
     url.to_s
   end
+
+  # Similar to spree/admin/products_controller
+  def product_set_from_params
+    # Form field names:
+    #  '[products][<id>][name]'
+    #
+    # Resulting in params:
+    #     "products" => {
+    #       "<id>" =>  {
+    #         "name" => "Pommes",
+    #       }
+    #     }
+
+    # For ModelSet, we transform to:
+    # {
+    #   0=> {:id=>"7449", "name"=>"Pommes"}
+    # }
+    #
+    # TO Consider: We could actually rearrange the form to suit that format more directly. eg:
+    #   '[products][0][id]' (hidden field)
+    #   '[products][0][name]'
+    collection_hash = products_bulk_params[:products].map { |id, attributes|
+      { id:, **attributes }
+    }.each_with_index.to_h { |p, i|
+      [i, p]
+    }
+    Sets::ProductSet.new(collection_attributes: collection_hash)
+  end
+
+  def products_bulk_params
+    params.permit(products: ::PermittedAttributes::Product.attributes)
+      .to_h.with_indifferent_access
+  end
 end
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 779f9c5275..bd27a5417e 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -1,66 +1,74 @@
-%table.products
-  %col{ width:"15%" }
-  %col{ width:"5%", style: "max-width:5em" }
-  %col{ width:"8%" }
-  %col{ width:"5%", style: "max-width:5em"}
-  %col{ width:"5%", style: "max-width:5em"}
-  %col{ width:"10%" }= # producer
-  %col{ width:"10%" }
-  %col{ width:"5%" }
-  %col{ width:"5%", style: "max-width:5em" }
-  %thead
-    %tr
-      %th.align-left= t('admin.products_page.columns.name')
-      %th.align-right= t('admin.products_page.columns.sku')
-      %th.align-right= t('admin.products_page.columns.unit')
-      %th.align-right= t('admin.products_page.columns.price')
-      %th.align-right= t('admin.products_page.columns.on_hand')
-      %th.align-left= t('admin.products_page.columns.producer')
-      %th.align-left= t('admin.products_page.columns.category')
-      %th.align-left= t('admin.products_page.columns.tax_category')
-      %th.align-left= t('admin.products_page.columns.inherits_properties')
-  - products.each do |product|
-    %tbody.relaxed
+= form_with url: bulk_update_admin_products_v3_index_path, method: :patch,
+            html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form|
+  .container
+    .sixteen.columns.align-right
+      #fieldset
+        / = t('.products_modified', count: 'X')
+        = form.submit t('.reset'), type: :reset
+        = form.submit t('.save')
+  %table.products
+    %col{ width:"15%" }
+    %col{ width:"5%", style: "max-width:5em" }
+    %col{ width:"8%" }
+    %col{ width:"5%", style: "max-width:5em"}
+    %col{ width:"5%", style: "max-width:5em"}
+    %col{ width:"10%" }= # producer
+    %col{ width:"10%" }
+    %col{ width:"5%" }
+    %col{ width:"5%", style: "max-width:5em" }
+    %thead
       %tr
-        %td.align-left.header
-          .line-clamp-1= product.name
-        %td.align-right
-          .line-clamp-1= product.sku
-        %td.align-right
-          .line-clamp-1
-            = 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"] + ")"
-        %td.align-right
-          -# 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
-        %td.align-left
-          .line-clamp-1= product.supplier.name
-        %td.align-left
-          .line-clamp-1= 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)
-      - product.variants.each do |variant|
-        %tr.condensed
-          %td.align-left
-            .line-clamp-1= variant.display_name
-          %td.align-right
-            .line-clamp-1= variant.sku
-          %td.align-right
-            .line-clamp-1= variant.unit_to_display
-          %td.align-right
-            .line-clamp-1= number_to_currency(variant.price)
-          %td.align-right
-            .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements.
-          %td.align-left
-            .line-clamp-1= 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.
-          %td.align-left
-            -# empty
-
+        %th.align-left= t('admin.products_page.columns.name')
+        %th.align-right= t('admin.products_page.columns.sku')
+        %th.align-right= t('admin.products_page.columns.unit')
+        %th.align-right= t('admin.products_page.columns.price')
+        %th.align-right= t('admin.products_page.columns.on_hand')
+        %th.align-left= t('admin.products_page.columns.producer')
+        %th.align-left= t('admin.products_page.columns.category')
+        %th.align-left= t('admin.products_page.columns.tax_category')
+        %th.align-left= t('admin.products_page.columns.inherits_properties')
+    - products.each do |product|
+      = form.fields_for(product) do |product_form|
+        %tbody.relaxed
+          %tr
+            %td.align-left.header
+              .line-clamp-1= product_form.text_field :name, name: "[products][#{product.id}][name]", id: "_product_name_#{product.id}", 'aria-label': t('admin.products_page.columns.name')
+            %td.align-right
+              .line-clamp-1= product.sku
+            %td.align-right
+              .line-clamp-1
+                = 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"] + ")"
+            %td.align-right
+              -# 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
+            %td.align-left
+              .line-clamp-1= product.supplier.name
+            %td.align-left
+              .line-clamp-1= 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)
+          - product.variants.each do |variant|
+            %tr.condensed
+              %td.align-left
+                .line-clamp-1= variant.display_name
+              %td.align-right
+                .line-clamp-1= variant.sku
+              %td.align-right
+                .line-clamp-1= variant.unit_to_display
+              %td.align-right
+                .line-clamp-1= number_to_currency(variant.price)
+              %td.align-right
+                .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements.
+              %td.align-left
+                .line-clamp-1= 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.
+              %td.align-left
+                -# empty
diff --git a/config/locales/en.yml b/config/locales/en.yml
index c9f57070de..7a3570d985 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -818,6 +818,11 @@ en:
         no_products_found: No products found
         import_products: Import multiple products
         no_products_found_for_search: No products found for your search criteria
+      table:
+        save: Save changes
+        reset: Discard changes
+      bulk_update:
+        success: "Products successfully updated"
     product_import:
       title: Product Import
       file_not_found: File not found or could not be opened
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index dea9207fb2..95da893872 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -70,7 +70,9 @@ Openfoodnetwork::Application.routes.draw do
     post '/product_import/reset_absent', to: 'product_import#reset_absent_products', as: 'product_import_reset_async'
 
     constraints FeatureToggleConstraint.new(:admin_style_v3) do
-      resources :products_v3, only: :index
+      resources :products_v3, as: :products_v3, only: :index do
+        patch :bulk_update, on: :collection
+      end
     end
 
     resources :variant_overrides do
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index 4b1b6a6b90..60fdcce81e 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -32,4 +32,31 @@ describe ProductsReflex, type: :reflex do
       end
     end
   end
+
+  describe '#bulk_update' do
+    let!(:product_z) { create(:simple_product, name: "Zucchini") }
+    let!(:product_a) { create(:simple_product, name: "Apples") }
+
+    it "saves valid changes" do
+      params = {
+        # '[products][<id>][name]'
+        "products" => {
+          product_a.id.to_s =>  {
+            "name" => "Pommes",
+          }
+        }
+      }
+
+      expect{
+        reflex(:bulk_update, params:)
+        product_a.reload
+      }.to change(product_a, :name).to("Pommes")
+    end
+  end
+end
+
+# Build and run a reflex using the context
+# Parameters can be added with params: option
+def reflex(method_name, opts = {})
+  build_reflex(method_name:, **context.merge(opts)).run(method_name)
 end
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index bddcfab8c1..b0e98484c4 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -24,7 +24,7 @@ describe 'As an admin, I can see the new product page' do
   end
 
   describe "sorting" do
-    let!(:product_z) { create(:simple_product, name: "Bananas") }
+    let!(:product_b) { create(:simple_product, name: "Bananas") }
     let!(:product_a) { create(:simple_product, name: "Apples") }
 
     before do
@@ -32,7 +32,13 @@ describe 'As an admin, I can see the new product page' do
     end
 
     it "Should sort products alphabetically by default" do
-      expect(page).to have_content /Apples.*Bananas/
+      within "table.products" do
+        # Gather input values, because page.content doesn't include them.
+        input_content = page.find_all('input[type=text]').map(&:value).join
+
+        # Products are in correct order.
+        expect(input_content).to match /Apples.*Bananas/
+      end
     end
   end
 
@@ -91,7 +97,7 @@ describe 'As an admin, I can see the new product page' do
         search_for "searchable product"
         expect(page).to have_field "search_term", with: "searchable product"
         expect_products_count_to_be 1
-        expect(page).to have_selector "table.products tbody tr td", text: product_by_name.name
+        expect(page).to have_field "Name", with: product_by_name.name
 
         click_link "Clear search"
         expect(page).to have_field "search_term", with: ""
@@ -130,11 +136,32 @@ describe 'As an admin, I can see the new product page' do
 
         expect(page).to have_select "category_id", selected: "Category 1"
         expect_products_count_to_be 1
-        expect(page).to have_selector "table.products tbody tr td", text: product_by_category.name
+        expect(page).to have_field "Name", with: product_by_category.name
       end
     end
   end
 
+  describe "updating" do
+    before do
+      visit admin_products_v3_index_url
+    end
+
+    it "can update product fields" do
+      fill_in id: "_product_name_#{product_1.id}", with: "An updated name"
+
+      expect {
+        click_button "Save changes"
+        product_1.reload
+      }.to(
+        change { product_1.name }.to("An updated name")
+      )
+
+      expect(page).to have_field id: "_product_name_#{product_1.id}", with: "An updated name"
+      pending
+      expect(page).to have_content "Changes saved"
+    end
+  end
+
   def expect_page_to_be(page_number)
     expect(page).to have_selector ".pagination span.page.current", text: page_number.to_s
   end

From 6ffe1ec1ad18fd73d189ee1aad7812c834b23200 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 28 Jul 2023 14:35:24 +1000
Subject: [PATCH 03/18] Retain the order of products in the collection

---
 app/services/sets/product_set.rb       |  6 +++--
 spec/services/sets/product_set_spec.rb | 32 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 101104ad38..c5abb3b267 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -13,8 +13,10 @@ module Sets
     end
 
     def collection_attributes=(attributes)
-      @collection = Spree::Product
-        .where(id: attributes.each_value.map { |product| product[:id] })
+      ids = attributes.each_value.map { |product| product[:id] }.compact
+      @collection = []
+      # Find and load existing products in the order they are provided
+      @collection = Spree::Product.find(ids) if ids.present?
       @collection_hash = attributes
     end
 
diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 07c17d2995..9b6fd5d2f8 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -198,6 +198,38 @@ describe Sets::ProductSet do
           end
         end
       end
+
+      context 'when there are multiple products' do
+        let!(:product_b) { create(:simple_product, name: "Bananas") }
+        let!(:product_a) { create(:simple_product, name: "Apples") }
+
+        let(:collection_hash) do
+          {
+            0 => {
+              id: product_a.id,
+              name: "Pommes",
+            },
+            1 => {
+              id: product_b.id,
+              name: "Bananes",
+            },
+          }
+        end
+
+        it 'updates the products' do
+          product_set.save
+
+          expect(product_a.reload.name).to eq "Pommes"
+          expect(product_b.reload.name).to eq "Bananes"
+        end
+
+        it 'retains the order of products' do
+          product_set.save
+
+          expect(product_set.collection[0]).to eq product_a.reload
+          expect(product_set.collection[1]).to eq product_b.reload
+        end
+      end
     end
   end
 end

From 71c36585bcb9457edd847bddc9098cde0b83cacc Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 28 Jul 2023 13:29:33 +1000
Subject: [PATCH 04/18] Retain order when saving.

Perhaps this should be tested in the system spec too ("I can rename a product and still see it after saving"). But I'd like to find the compromise to avoid bulking up system specs too much. I think it's covered well enough by the reflex spec?
---
 app/reflexes/products_reflex.rb              | 46 +++++++++++---------
 app/services/sets/product_set.rb             |  6 +++
 app/views/admin/products_v3/_table.html.haml |  5 ++-
 spec/reflexes/products_reflex_spec.rb        | 40 ++++++++++++++---
 4 files changed, 68 insertions(+), 29 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index b2c845d864..826aa5d0ab 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -35,18 +35,16 @@ class ProductsReflex < ApplicationReflex
     product_set = product_set_from_params
 
     product_set.collection.each { |p| authorize! :update, p }
+    @products = product_set.collection # use instance variable mainly for testing
 
     if product_set.save
       # flash[:success] = with_locale { I18n.t('.success') }
       # morph_admin_flashes  # ERROR: selector morph type has already been set
-
-      fetch_and_render_products
     elsif product_set.errors.present?
-      # todo: render form with error messages
-      render json: { errors: product_set.errors }, status: :bad_request
-    else
-      render body: nil, status: :internal_server_error
+      # flash there are errors..
     end
+
+    render_products_form(product_set)
   end
 
   private
@@ -87,6 +85,19 @@ class ProductsReflex < ApplicationReflex
     morph :nothing
   end
 
+  def render_products_form(product_set)
+    cable_ready.replace(
+      selector: "#products-form",
+      html: render(partial: "admin/products_v3/table",
+                   locals: { products: @products, errors: product_set.errors })
+    ).broadcast
+    morph :nothing
+
+    # dunno why this doesn't work.
+    # morph "#products-form", render(partial: "admin/products_v3/table",
+    #                locals: { products: products })
+  end
+
   def producers
     producers = OpenFoodNetwork::Permissions.new(current_user)
       .managed_product_enterprises.is_primary_producer.by_name
@@ -152,28 +163,21 @@ class ProductsReflex < ApplicationReflex
   # Similar to spree/admin/products_controller
   def product_set_from_params
     # Form field names:
-    #  '[products][<id>][name]'
+    #   '[products][0][id]' (hidden field)
+    #   '[products][0][name]'
     #
     # Resulting in params:
     #     "products" => {
-    #       "<id>" =>  {
+    #       "<i>" =>  {
+    #         "id" => "123"
     #         "name" => "Pommes",
     #       }
     #     }
 
-    # For ModelSet, we transform to:
-    # {
-    #   0=> {:id=>"7449", "name"=>"Pommes"}
-    # }
-    #
-    # TO Consider: We could actually rearrange the form to suit that format more directly. eg:
-    #   '[products][0][id]' (hidden field)
-    #   '[products][0][name]'
-    collection_hash = products_bulk_params[:products].map { |id, attributes|
-      { id:, **attributes }
-    }.each_with_index.to_h { |p, i|
-      [i, p]
-    }
+    collection_hash = products_bulk_params[:products].each_with_index
+      .to_h { |p, i|
+        [i, p]
+      }.with_indifferent_access
     Sets::ProductSet.new(collection_attributes: collection_hash)
   end
 
diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index c5abb3b267..6222a96c51 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -1,6 +1,12 @@
 # frozen_string_literal: true
 
 module Sets
+  # Accepts a collection_hash in format:
+  # {
+  #   0=> {id:"7449", name:"Pommes"},
+  #   1=> {...}
+  # }
+  #
   class ProductSet < ModelSet
     def initialize(attributes = {})
       super(Spree::Product, [], attributes)
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index bd27a5417e..d8b1b97665 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -27,12 +27,13 @@
         %th.align-left= t('admin.products_page.columns.category')
         %th.align-left= t('admin.products_page.columns.tax_category')
         %th.align-left= t('admin.products_page.columns.inherits_properties')
-    - products.each do |product|
+    - products.each do |product, i|
       = form.fields_for(product) do |product_form|
         %tbody.relaxed
           %tr
             %td.align-left.header
-              .line-clamp-1= product_form.text_field :name, name: "[products][#{product.id}][name]", id: "_product_name_#{product.id}", 'aria-label': t('admin.products_page.columns.name')
+              = product_form.hidden_field :id, name: "[products][#{i}][id]" #todo: can we remove #{i} and implicitly pop?
+              .line-clamp-1= product_form.text_field :name, name: "[products][#{i}][name]", id: "_product_name_#{product.id}", 'aria-label': t('admin.products_page.columns.name')
             %td.align-right
               .line-clamp-1= product.sku
             %td.align-right
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index 60fdcce81e..80255b3dd8 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -34,17 +34,18 @@ describe ProductsReflex, type: :reflex do
   end
 
   describe '#bulk_update' do
-    let!(:product_z) { create(:simple_product, name: "Zucchini") }
+    let!(:product_b) { create(:simple_product, name: "Bananas") }
     let!(:product_a) { create(:simple_product, name: "Apples") }
 
     it "saves valid changes" do
       params = {
-        # '[products][<id>][name]'
-        "products" => {
-          product_a.id.to_s =>  {
+        # '[products][<i>][name]'
+        "products" => [
+          {
+            "id" => product_a.id.to_s,
             "name" => "Pommes",
           }
-        }
+        ]
       }
 
       expect{
@@ -52,11 +53,38 @@ describe ProductsReflex, type: :reflex do
         product_a.reload
       }.to change(product_a, :name).to("Pommes")
     end
+
+    describe "sorting" do
+      let(:params) {
+        {
+          "products" => [
+            {
+              "id" => product_a.id.to_s,
+              "name" => "Pommes",
+            },
+            {
+              "id" => product_b.id.to_s,
+            },
+          ]
+        }
+      }
+      subject{ reflex(:bulk_update, params:) }
+
+      it "Should retain sort order, even when names change" do
+        expect(subject.get(:products).map(&:id)).to eq [
+          product_a.id,
+          product_b.id,
+        ]
+      end
+    end
   end
 end
 
 # Build and run a reflex using the context
 # Parameters can be added with params: option
+# For more options see https://github.com/podia/stimulus_reflex_testing#usage
 def reflex(method_name, opts = {})
-  build_reflex(method_name:, **context.merge(opts)).run(method_name)
+  build_reflex(method_name:, **context.merge(opts)).tap{ |reflex|
+    reflex.run(method_name)
+  }
 end

From a0dba001bcd45ba50acf4ff8b332faa0d8a46b46 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 2 Aug 2023 10:25:44 +1000
Subject: [PATCH 05/18] Attempt to save all records in bulk update

Before, it would abort after the first invalid record, and it doesn't tell you about the others. This way you find out about all at once.

This affects the existing Bulk Edit Products screen, and can result in longer error messages than before. But I would argue that's a good thing.

I think this is technically optional for BUU at this point, but a helpful improvement.
---
 app/services/sets/product_set.rb       |  5 +++--
 spec/services/sets/product_set_spec.rb | 27 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 6222a96c51..06f3ac4f4f 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -13,9 +13,10 @@ module Sets
     end
 
     def save
-      @collection_hash.each_value.all? do |product_attributes|
+      # Attempt to save all records, collecting model errors.
+      @collection_hash.each_value.map do |product_attributes|
         update_product_attributes(product_attributes)
-      end
+      end.all?
     end
 
     def collection_attributes=(attributes)
diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 9b6fd5d2f8..e68601f887 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -229,6 +229,33 @@ describe Sets::ProductSet do
           expect(product_set.collection[0]).to eq product_a.reload
           expect(product_set.collection[1]).to eq product_b.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
+
+          it 'continues to update subsequent products' do
+            product_set.save
+
+            # Errors are logged on the model
+            first_item = product_set.collection[0]
+            expect(first_item.errors.full_messages.to_sentence).to eq "Product Name can't be blank"
+            expect(first_item.name).to eq ""
+
+            # Subsequent product was updated
+            expect(product_b.reload.name).to eq "Bananes"
+          end
+        end
       end
     end
   end

From a70f392654802f2c6aa7f394cd1e6de60e4678b1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 26 Jul 2023 17:17:55 +1000
Subject: [PATCH 06/18] Show error messages

It's kinda hard to test reflexes..
---
 app/reflexes/products_reflex.rb              |  8 ++---
 app/views/admin/products_v3/_table.html.haml |  6 +++-
 spec/reflexes/products_reflex_spec.rb        | 35 ++++++++++++++++++--
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 826aa5d0ab..9e9370aacc 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -41,10 +41,10 @@ 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?
-      # flash there are errors..
+      @error_msg = product_set.errors.full_messages.to_sentence
     end
 
-    render_products_form(product_set)
+    render_products_form
   end
 
   private
@@ -85,11 +85,11 @@ class ProductsReflex < ApplicationReflex
     morph :nothing
   end
 
-  def render_products_form(product_set)
+  def render_products_form
     cable_ready.replace(
       selector: "#products-form",
       html: render(partial: "admin/products_v3/table",
-                   locals: { products: @products, errors: product_set.errors })
+                   locals: { products: @products, error_msg: @error_msg })
     ).broadcast
     morph :nothing
 
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index d8b1b97665..a6ffbf0b82 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -1,4 +1,5 @@
-= form_with url: bulk_update_admin_products_v3_index_path, method: :patch,
+= form_with url: bulk_update_admin_products_v3_index_path, method: :patch, id: "products-form",
+
             html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form|
   .container
     .sixteen.columns.align-right
@@ -6,6 +7,9 @@
         / = t('.products_modified', count: 'X')
         = form.submit t('.reset'), type: :reset
         = form.submit t('.save')
+        - if defined?(error_msg) && error_msg.present?
+          .error
+            = error_msg
   %table.products
     %col{ width:"15%" }
     %col{ width:"5%", style: "max-width:5em" }
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index 80255b3dd8..0a7eccfc1f 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -49,7 +49,7 @@ describe ProductsReflex, type: :reflex do
       }
 
       expect{
-        reflex(:bulk_update, params:)
+        run_reflex(:bulk_update, params:)
         product_a.reload
       }.to change(product_a, :name).to("Pommes")
     end
@@ -68,7 +68,7 @@ describe ProductsReflex, type: :reflex do
           ]
         }
       }
-      subject{ reflex(:bulk_update, params:) }
+      subject{ run_reflex(:bulk_update, params:) }
 
       it "Should retain sort order, even when names change" do
         expect(subject.get(:products).map(&:id)).to eq [
@@ -77,13 +77,42 @@ describe ProductsReflex, type: :reflex do
         ]
       end
     end
+
+    describe "error messages" do
+      it "summarises duplicate error messages" do
+        params = {
+          "products" => [
+            {
+              "id" => product_a.id.to_s,
+              "name" => "",
+            },
+            {
+              "id" => product_b.id.to_s,
+              "name" => "",
+            },
+          ]
+        }
+
+        reflex = run_reflex(:bulk_update, params:)
+        expect(reflex.get(:error_msg)).to eq "Product Name can't be blank"
+
+        # # WTF
+        # expect{ reflex(:bulk_update, params:) }.to broadcast(
+        #   replace: {
+        #     selector: "#products-form",
+        #     html: /Product Name can't be blank/,
+        #   },
+        #   broadcast: nil
+        # )
+      end
+    end
   end
 end
 
 # Build and run a reflex using the context
 # Parameters can be added with params: option
 # For more options see https://github.com/podia/stimulus_reflex_testing#usage
-def reflex(method_name, opts = {})
+def run_reflex(method_name, opts = {})
   build_reflex(method_name:, **context.merge(opts)).tap{ |reflex|
     reflex.run(method_name)
   }

From ef63c520c03f58695a22d630cfb93928dd527b32 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 2 Aug 2023 16:00:15 +1000
Subject: [PATCH 07/18] Aggregate errors

---
 app/reflexes/products_reflex.rb       |  3 ++-
 app/services/sets/model_set.rb        |  4 ++++
 config/locales/en.yml                 |  7 +++++--
 spec/reflexes/products_reflex_spec.rb |  6 +++---
 spec/services/sets/model_set_spec.rb  | 27 +++++++++++++++++++++++++++
 5 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 9e9370aacc..8e18198a40 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -41,7 +41,8 @@ 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 = product_set.errors.full_messages.to_sentence
+      # @error_msg = with_locale{ I18n.t('.products_have_error', count: product_set.invalid.count) }
+      @error_msg = "#{product_set.invalid.count} products have errors."
     end
 
     render_products_form
diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb
index d5ff4e2ccb..7347e2d9c9 100644
--- a/app/services/sets/model_set.rb
+++ b/app/services/sets/model_set.rb
@@ -54,6 +54,10 @@ module Sets
       errors
     end
 
+    def invalid
+      @collection.select { |model| model.errors.any? }
+    end
+
     def save
       collection_to_delete.each(&:destroy)
       collection_to_keep.all?(&:save)
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 7a3570d985..9020961d32 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -821,8 +821,11 @@ en:
       table:
         save: Save changes
         reset: Discard changes
-      bulk_update:
-        success: "Products successfully updated"
+      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 0a7eccfc1f..c927930efc 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -79,7 +79,7 @@ describe ProductsReflex, type: :reflex do
     end
 
     describe "error messages" do
-      it "summarises duplicate error messages" do
+      it "summarises error messages" do
         params = {
           "products" => [
             {
@@ -94,13 +94,13 @@ describe ProductsReflex, type: :reflex do
         }
 
         reflex = run_reflex(:bulk_update, params:)
-        expect(reflex.get(:error_msg)).to eq "Product Name can't be blank"
+        expect(reflex.get(:error_msg)).to include "2 products have errors"
 
         # # WTF
         # expect{ reflex(:bulk_update, params:) }.to broadcast(
         #   replace: {
         #     selector: "#products-form",
-        #     html: /Product Name can't be blank/,
+        #     html: /2 products have errors/,
         #   },
         #   broadcast: nil
         # )
diff --git a/spec/services/sets/model_set_spec.rb b/spec/services/sets/model_set_spec.rb
index 416aa63fb4..8008771343 100644
--- a/spec/services/sets/model_set_spec.rb
+++ b/spec/services/sets/model_set_spec.rb
@@ -55,5 +55,32 @@ describe Sets::ModelSet do
 
       expect { ms.save }.to change(Enterprise, :count).by(0)
     end
+
+    describe "errors" do
+      let(:product_b) { create(:simple_product, name: "Bananas") }
+      let(:product_a) { create(:simple_product, name: "Apples") }
+      let(:collection_attributes) do
+        {
+          0 => {
+            id: product_a.id,
+            name: "", # Product Name can't be blank
+          },
+          1 => {
+            id: product_b.id,
+            name: "Bananes",
+          },
+        }
+      end
+      subject{ Sets::ModelSet.new(Spree::Product, [product_a, product_b], collection_attributes:) }
+
+      it 'errors are aggregated' do
+        subject.save
+
+        expect(subject.errors.full_messages).to eq ["Product Name can't be blank"]
+
+        expect(subject.invalid).to     include product_a
+        expect(subject.invalid).to_not include product_b
+      end
+    end
   end
 end

From 565ea231753dd2236d618fa711c72dea31645193 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 8 Aug 2023 16:26:26 +1000
Subject: [PATCH 08/18] Hide borders from inputs until hover

---
 app/webpacker/css/admin/products_v3.scss | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index ba34c089d4..0eee044a47 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -23,6 +23,7 @@
     padding: 4px;
     border-collapse: separate; // This is needed for the outer padding. Also should be helpful to give more flexibility of borders between rows.
 
+    // Row hover
     tr:hover {
       th,
       td {
@@ -75,6 +76,21 @@
         padding: $padding-tbl-cell-condensed;
       }
     }
+
+    // "Naked" inputs. Row hover helps reveal them.
+    input {
+      border-color: transparent;
+
+      &:focus {
+        border-color: $color-txt-hover-brd;
+      }
+    }
+
+    .field_with_errors {
+      input {
+        border-color: $color-error;
+      }
+    }
   }
 
   #no-products {

From be24247df2b058af9d930f1498e12a84fa7999d8 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 8 Aug 2023 16:38:43 +1000
Subject: [PATCH 09/18] [wip] Left line for row hover

But it's not perfect. Can we use a pseudo element instead?
---
 app/webpacker/css/admin/products_v3.scss | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 0eee044a47..8168fd1c3e 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -20,14 +20,17 @@
     table-layout: fixed; // Column widths are based solely on col definitions (not content). This allows more efficient rendering.
 
     background-color: $color-tbl-bg;
-    padding: 4px;
+    padding: 4px 4px 4px 0; // Left border is implemented in td:first-child
     border-collapse: separate; // This is needed for the outer padding. Also should be helpful to give more flexibility of borders between rows.
 
     // Row hover
     tr:hover {
-      th,
       td {
         background-color: $light-grey;
+
+        &:first-child {
+          border-left-color: $teal;
+        }
       }
     }
 
@@ -35,6 +38,11 @@
     td {
       padding: $padding-tbl-cell;
       border: none;
+
+      &:first-child {
+        // Implement the
+        border-left: 4px solid $color-tbl-bg;
+      }
     }
 
     th {

From 2b09ec7c21d42fa5acf00d2f0d19b78a22f75aab Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 8 Aug 2023 17:18:32 +1000
Subject: [PATCH 10/18] [fixup] Left line for row hover

---
 app/webpacker/css/admin/products_v3.scss | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 8168fd1c3e..688015bffe 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -20,16 +20,23 @@
     table-layout: fixed; // Column widths are based solely on col definitions (not content). This allows more efficient rendering.
 
     background-color: $color-tbl-bg;
-    padding: 4px 4px 4px 0; // Left border is implemented in td:first-child
+    padding: 4px;
     border-collapse: separate; // This is needed for the outer padding. Also should be helpful to give more flexibility of borders between rows.
 
     // Row hover
     tr:hover {
       td {
         background-color: $light-grey;
+        position: relative;
 
-        &:first-child {
-          border-left-color: $teal;
+        // Left border
+        &:first-child:before {
+          content: "";
+          position: absolute;
+          top: 0;
+          left: -4px;
+          border-left: 4px solid $teal;
+          height: 100%;
         }
       }
     }
@@ -38,11 +45,6 @@
     td {
       padding: $padding-tbl-cell;
       border: none;
-
-      &:first-child {
-        // Implement the
-        border-left: 4px solid $color-tbl-bg;
-      }
     }
 
     th {

From 8440c44a6f57f8daefd7d5b3633b9ced70135a5b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 9 Aug 2023 11:10:06 +1000
Subject: [PATCH 11/18] Remove unused style

---
 app/webpacker/css/admin/shared/forms.scss | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/webpacker/css/admin/shared/forms.scss b/app/webpacker/css/admin/shared/forms.scss
index 6e956df095..4f11d677fc 100644
--- a/app/webpacker/css/admin/shared/forms.scss
+++ b/app/webpacker/css/admin/shared/forms.scss
@@ -236,9 +236,6 @@ fieldset {
   }
 }
 
-.form-actions {
-  margin-top: 18px;
-}
 .form-buttons {
   text-align: center;
 }

From 37d1113e4c337ebcdbc1debac20cb81a42962501 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 8 Aug 2023 17:20:30 +1000
Subject: [PATCH 12/18] Align row headers with input contents

Inputs add extra padding, so we add the same padding to the header. Using an opt-in class, because I think we won't want this on all columns.
---
 app/views/admin/products_v3/_table.html.haml      | 2 +-
 app/webpacker/css/admin/globals/variables.scss    | 2 ++
 app/webpacker/css/admin/products_v3.scss          | 6 ++++++
 app/webpacker/css/admin/shared/forms.scss         | 2 +-
 app/webpacker/css/admin_v3/globals/variables.scss | 8 +++++---
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index a6ffbf0b82..e2c57f8b30 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -22,7 +22,7 @@
     %col{ width:"5%", style: "max-width:5em" }
     %thead
       %tr
-        %th.align-left= t('admin.products_page.columns.name')
+        %th.align-left.with-input= t('admin.products_page.columns.name')
         %th.align-right= t('admin.products_page.columns.sku')
         %th.align-right= t('admin.products_page.columns.unit')
         %th.align-right= t('admin.products_page.columns.price')
diff --git a/app/webpacker/css/admin/globals/variables.scss b/app/webpacker/css/admin/globals/variables.scss
index 86fb56b7af..7ea08bbc7d 100644
--- a/app/webpacker/css/admin/globals/variables.scss
+++ b/app/webpacker/css/admin/globals/variables.scss
@@ -67,6 +67,8 @@ $color-sel-hover-text:           $color-1 !default;
 $color-txt-brd:                  $color-border !default;
 $color-txt-text:                 $color-3 !default;
 $color-txt-hover-brd:            $color-2 !default;
+$vpadding-txt:                   7px;
+$hpadding-txt:                   10px;
 
 // Modal colors
 $color-modal-close-btn:          $color-5 !default;
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 688015bffe..5b5cb2fe9c 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -23,6 +23,12 @@
     padding: 4px;
     border-collapse: separate; // This is needed for the outer padding. Also should be helpful to give more flexibility of borders between rows.
 
+    // Additional horizontal padding to align with input contents
+    thead th.with-input {
+      padding-left: $padding-tbl-cell + $hpadding-txt;
+      padding-right: $padding-tbl-cell + $hpadding-txt;
+    }
+
     // Row hover
     tr:hover {
       td {
diff --git a/app/webpacker/css/admin/shared/forms.scss b/app/webpacker/css/admin/shared/forms.scss
index 4f11d677fc..da593e3300 100644
--- a/app/webpacker/css/admin/shared/forms.scss
+++ b/app/webpacker/css/admin/shared/forms.scss
@@ -8,7 +8,7 @@ input[type="number"],
 textarea,
 fieldset {
   @include border-radius($border-radius);
-  padding: 7px 10px;
+  padding: $vpadding-txt $hpadding-txt;
   border: 1px solid $color-txt-brd;
   color: $color-txt-text;
   font-size: 90%;
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index cf8fa70498..89a15caee4 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -33,9 +33,9 @@ $color-tbl-cell-shadow:          rgb(0, 0, 0, 0.15) !default;
 $color-tbl-thead-txt:            $color-headers !default;
 $color-tbl-thead-bg:             $light-grey !default;
 $color-tbl-border:               $pale-blue !default;
-$padding-tbl-cell:               12px 12px;
+$padding-tbl-cell:               12px;
 $padding-tbl-cell-condensed:     10px 12px;
-$padding-tbl-cell-relaxed:       16px 12px;
+$padding-tbl-cell-relaxed:       12px 12px;
 
 // Button colors
 $color-btn-bg:                   $teal !default;
@@ -70,10 +70,12 @@ $color-sel-text:                 $white !default;
 $color-sel-hover-bg:             lighten($color-sel-bg, 2) !default;
 $color-sel-hover-text:           $white !default;
 
-// Text inputs colors
+// Text inputs styles
 $color-txt-brd:                  $color-border !default;
 $color-txt-text:                 $near-black !default;
 $color-txt-hover-brd:            $teal !default;
+$vpadding-txt:                   5px;
+$hpadding-txt:                   8px;
 
 // Modal colors
 $color-modal-close-btn:          $color-5 !default;

From 3cc5d7ba1de7aee2c5c1684fdccc2b1ce12bf241 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 9 Aug 2023 11:22:52 +1000
Subject: [PATCH 13/18] Copy form styles to v3

---
 app/webpacker/css/admin_v3/all.scss          |   2 +-
 app/webpacker/css/admin_v3/shared/forms.scss | 261 +++++++++++++++++++
 2 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 app/webpacker/css/admin_v3/shared/forms.scss

diff --git a/app/webpacker/css/admin_v3/all.scss b/app/webpacker/css/admin_v3/all.scss
index 7f93f602ad..0f48247044 100644
--- a/app/webpacker/css/admin_v3/all.scss
+++ b/app/webpacker/css/admin_v3/all.scss
@@ -31,7 +31,7 @@
 @import "shared/typography"; // admin_v3
 @import "shared/tables"; // admin_v3
 @import "shared/icons"; // admin_v3
-@import "../admin/shared/forms";
+@import "shared/forms"; // admin_v3
 @import "shared/layout"; // admin_v3
 @import "../admin/shared/scroll_bar";
 
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
new file mode 100644
index 0000000000..da593e3300
--- /dev/null
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -0,0 +1,261 @@
+$text-inputs: "input[type=text], input[type=password], input[type=email], input[type=url], input[type=tel]";
+
+#{$text-inputs},
+input[type="date"],
+input[type="datetime"],
+input[type="time"],
+input[type="number"],
+textarea,
+fieldset {
+  @include border-radius($border-radius);
+  padding: $vpadding-txt $hpadding-txt;
+  border: 1px solid $color-txt-brd;
+  color: $color-txt-text;
+  font-size: 90%;
+
+  &:focus {
+    outline: none;
+    border-color: $color-txt-hover-brd;
+  }
+
+  &[disabled] {
+    opacity: 0.7;
+  }
+}
+
+textarea {
+  line-height: 19px;
+}
+
+.fullwidth {
+  width: 100%;
+}
+
+label {
+  font-weight: 600;
+  text-transform: uppercase;
+  font-size: 85%;
+  display: inline;
+  margin-bottom: 5px;
+  color: $color-4;
+
+  &.inline {
+    display: inline-block !important;
+  }
+
+  &.block {
+    display: block !important;
+  }
+}
+
+.label-block label {
+  display: block;
+}
+
+span.info {
+  font-style: italic;
+  font-size: 85%;
+  color: lighten($color-body-text, 15);
+  display: block;
+  line-height: 20px;
+  margin: 5px 0;
+}
+
+.field {
+  padding: 10px 0;
+
+  &.checkbox {
+    min-height: 70px;
+
+    input[type="checkbox"] {
+      display: inline-block;
+      width: auto;
+    }
+
+    label {
+      cursor: pointer;
+      display: block;
+    }
+  }
+
+  ul {
+    border-top: 1px solid $color-border;
+    list-style: none;
+    padding-top: 5px;
+
+    li {
+      display: inline-block;
+      padding-right: 10px;
+
+      label {
+        font-weight: normal;
+        text-transform: none;
+      }
+      &.white-space-nowrap {
+        white-space: nowrap;
+      }
+    }
+  }
+
+  &.withError {
+    .field_with_errors {
+      label {
+        color: $color-error;
+      }
+
+      input {
+        border-color: $color-error;
+      }
+    }
+    .formError {
+      color: $color-error;
+      font-style: italic;
+      font-size: 85%;
+    }
+  }
+}
+
+fieldset {
+  box-shadow: none;
+  box-sizing: border-box;
+  border-color: $color-border;
+  -webkit-box-sizing: border-box;
+  -moz-box-sizing: border-box;
+  margin-left: 0;
+  margin-right: 0;
+  position: relative;
+  margin-bottom: 35px;
+  padding: 10px 0 15px 0;
+  background-color: transparent;
+  border-left: none;
+  border-right: none;
+  border-radius: 0;
+
+  &.no-border-bottom {
+    border-bottom: none;
+    margin-bottom: 0;
+  }
+
+  &.no-border-top {
+    border-top: none;
+    padding-top: 0;
+  }
+
+  legend {
+    background-color: $color-1;
+    color: $color-2;
+    font-size: 14px;
+    font-weight: 600;
+    text-transform: uppercase;
+    text-align: center;
+    padding: 8px 15px;
+    margin: 0 auto;
+
+    -webkit-font-smoothing: antialiased;
+    -moz-osx-font-smoothing: grayscale;
+
+    i {
+      color: $color-link;
+    }
+  }
+
+  label {
+    color: lighten($color-body-text, 8);
+  }
+
+  .filter-actions {
+    margin-bottom: -32px;
+    margin-top: 15px;
+    text-align: center;
+
+    form {
+      display: inline-block;
+    }
+
+    button,
+    .button,
+    input[type="submit"],
+    input[type="button"],
+    span.or {
+      @include border-radius($border-radius);
+
+      -webkit-box-shadow: 0 0 0 15px $color-1;
+      -moz-box-shadow: 0 0 0 15px $color-1;
+      -ms-box-shadow: 0 0 0 15px $color-1;
+      -o-box-shadow: 0 0 0 15px $color-1;
+      box-shadow: 0 0 0 15px $color-1;
+
+      &:hover {
+        border-color: $color-1;
+      }
+
+      &:first-of-type {
+        margin-right: 1.25em;
+      }
+    }
+
+    span.or {
+      background-color: $color-1;
+      border-width: 5px;
+      margin-left: 5px;
+      margin-right: 5px;
+      position: relative;
+
+      -webkit-box-shadow: 0 0 0 5px $color-1;
+      -moz-box-shadow: 0 0 0 5px $color-1;
+      -ms-box-shadow: 0 0 0 5px $color-1;
+      -o-box-shadow: 0 0 0 5px $color-1;
+      box-shadow: 0 0 0 5px $color-1;
+    }
+  }
+
+  &.labels-inline {
+    .field {
+      margin-bottom: 0;
+      display: table;
+      width: 100%;
+
+      label,
+      input {
+        display: table-cell !important;
+      }
+      input {
+        width: 100%;
+      }
+
+      &.checkbox {
+        input {
+          width: auto !important;
+        }
+      }
+    }
+    .actions {
+      padding: 0;
+      text-align: right;
+    }
+  }
+}
+
+.form-buttons {
+  text-align: center;
+}
+
+select {
+  @extend input, [type="text"];
+  background-color: white;
+}
+
+.inline-checkbox {
+  display: inline-flex;
+  align-items: center;
+  margin-top: 3px;
+
+  input,
+  label {
+    cursor: pointer;
+  }
+  label {
+    margin: 0;
+    padding-left: 0.4rem;
+  }
+}

From 7bf79441c89ee04dadc9d9cf7eba96809b85c451 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 9 Aug 2023 11:53:53 +1000
Subject: [PATCH 14/18] Update link colours to match new design

---
 app/webpacker/css/admin_v3/globals/variables.scss | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index 89a15caee4..7706a4073d 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -14,9 +14,9 @@ $color-body-bg:                  $white !default;
 $color-body-text:                $near-black !default;
 $color-headers:                  $dark-blue !default;
 $color-link:                     $teal !default;
-$color-link-hover:               lighten($color-link, 2) !default;
-$color-link-active:              $green !default;
-$color-link-focus:               $green !default;
+$color-link-hover:               $orient !default;
+$color-link-active:              $dark-blue !default;
+$color-link-focus:               $orient !default;
 $color-link-visited:             $teal !default;
 $color-border:                   $light-grey !default;
 

From fccde964bf373ff1107170f4aeb05b061baeeefe Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 9 Aug 2023 11:55:49 +1000
Subject: [PATCH 15/18] Enable 'medium' style for reset buttons

---
 app/webpacker/css/admin_v3/components/buttons.scss | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/webpacker/css/admin_v3/components/buttons.scss b/app/webpacker/css/admin_v3/components/buttons.scss
index f233e23a48..8291e04147 100644
--- a/app/webpacker/css/admin_v3/components/buttons.scss
+++ b/app/webpacker/css/admin_v3/components/buttons.scss
@@ -1,4 +1,5 @@
 input[type="submit"],
+input[type="reset"],
 input[type="button"]:not(.trix-button),
 button:not(.plain):not(.trix-button),
 .button {

From b0e77df2262b7d06d30a10a256e0018036c0f1c8 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 9 Aug 2023 13:05:29 +1000
Subject: [PATCH 16/18] Style form actions area

---
 app/views/admin/products_v3/_table.html.haml  | 11 +++++-----
 .../css/admin_v3/components/buttons.scss      | 20 +++++++++++++++++-
 app/webpacker/css/admin_v3/shared/forms.scss  | 21 +++++++++++++++++++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index e2c57f8b30..ff9c2e19d9 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_v3_index_path, method: :patch, id: "products-form",
 
             html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form|
-  .container
-    .sixteen.columns.align-right
-      #fieldset
+  %fieldset.form-actions
+    .container
+      .status.ten.columns
         / = t('.products_modified', count: 'X')
-        = form.submit t('.reset'), type: :reset
-        = form.submit t('.save')
         - if defined?(error_msg) && error_msg.present?
           .error
             = error_msg
+      .form-buttons.six.columns
+        = form.submit t('.reset'), type: :reset, class: "medium"
+        = form.submit t('.save'), class: "medium"
   %table.products
     %col{ width:"15%" }
     %col{ width:"5%", style: "max-width:5em" }
diff --git a/app/webpacker/css/admin_v3/components/buttons.scss b/app/webpacker/css/admin_v3/components/buttons.scss
index 8291e04147..9b77e318d2 100644
--- a/app/webpacker/css/admin_v3/components/buttons.scss
+++ b/app/webpacker/css/admin_v3/components/buttons.scss
@@ -12,7 +12,6 @@ button:not(.plain):not(.trix-button),
   background-color: $color-btn-bg;
   border: 1px solid $color-btn-bg;
   color: $color-btn-text;
-  text-transform: uppercase;
   line-height: $btn-regular-height - 2px; // remove 2px to compensate for border
   height: $btn-regular-height;
   font-weight: bold;
@@ -113,6 +112,25 @@ button:not(.plain):not(.trix-button),
   }
 }
 
+input[type="reset"] {
+  // Reset button looks like a link, but has a border the same as buttons when active.
+  background: none;
+  border: 1px solid transparent;
+  color: $color-link;
+
+  &:active {
+    color: $color-link-active;
+  }
+  &:focus {
+    color: $color-link-focus;
+  }
+  &:hover {
+    color: $color-link-hover;
+    background: none;
+    border-color: transparent;
+  }
+}
+
 a.button {
   text-decoration: none;
 }
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index da593e3300..9f5fe2ac58 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -236,6 +236,27 @@ fieldset {
   }
 }
 
+.form-actions {
+  @include defaultBoxShadow;
+  background-color: $fair-pink;
+  border: none;
+  border-left: 4px solid $red;
+  border-radius: 4px;
+  margin: 0.5em 0;
+  padding: 0;
+
+  .status {
+    font-size: 1rem;
+    font-weight: bold;
+    padding: 0.75em 1em;
+  }
+
+  .form-buttons {
+    padding: 0.5em 1em;
+    text-align: right;
+  }
+}
+
 .form-buttons {
   text-align: center;
 }

From 2edf504d6537aba01dcee65afa315ca54b270d71 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 18 Aug 2023 16:36:46 +1000
Subject: [PATCH 17/18] Ignore long classes for now

As much as I hate to add to this list, this is still a work in progress so it's not worth refactoring at this point.
---
 .rubocop_todo.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 2a88db9bfe..5d8f529e8e 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -220,11 +220,13 @@ Metrics/ClassLength:
     - 'app/models/spree/user.rb'
     - 'app/models/spree/variant.rb'
     - 'app/models/spree/zone.rb'
+    - 'app/reflexes/products_reflex.rb'
     - 'app/serializers/api/cached_enterprise_serializer.rb'
     - 'app/serializers/api/enterprise_shopfront_serializer.rb'
     - 'app/services/cart_service.rb'
     - 'app/services/order_cycle_form.rb'
     - 'app/services/order_syncer.rb'
+    - 'app/services/sets/product_set.rb'
     - 'engines/order_management/app/services/order_management/order/updater.rb'
     - 'lib/open_food_network/enterprise_fee_calculator.rb'
     - 'lib/open_food_network/order_cycle_form_applicator.rb'

From a8d37d0899093d57bb63ac3a30b30f3d1a45ba9a Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 24 Aug 2023 08:48:27 +1000
Subject: [PATCH 18/18] Apply suggestions from code review

Rails is clever enough to not query the database without ids

Co-authored-by: Maikel <maikel@email.org.au>
---
 app/services/sets/product_set.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 06f3ac4f4f..7eb73e3bee 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -20,10 +20,9 @@ module Sets
     end
 
     def collection_attributes=(attributes)
-      ids = attributes.each_value.map { |product| product[:id] }.compact
-      @collection = []
+      ids = attributes.values.pluck(:id).compact
       # Find and load existing products in the order they are provided
-      @collection = Spree::Product.find(ids) if ids.present?
+      @collection = Spree::Product.find(ids)
       @collection_hash = attributes
     end