From 3710aa2149e02701067ca35400079a50ce0f344e Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 16:35:12 +1000
Subject: [PATCH 01/12] Refactor spec

Better to explicitly test for the change, and specify expected values.
---
 spec/services/sets/product_set_spec.rb | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 72de1ab124..536e720c3e 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -138,12 +138,11 @@ describe Sets::ProductSet do
               it 'updates product and variant attributes' do
                 collection_hash[0][:sku] = "test_sku"
 
-                product_set.save
-
-                expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
-                expect(product.reload.attributes).to include(
-                  'sku' => "test_sku"
-                )
+                expect {
+                  product_set.save
+                  product.reload
+                }.to change { product.sku }.to("test_sku")
+                  .and change { product.variants.first.sku }.to("123")
               end
             end
           end

From be6481dac3ebfb65e8303671c5adf9fdc1bfab39 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 20 Sep 2023 16:42:16 +1000
Subject: [PATCH 02/12] Refactor spec: combine expectations

These expectations have the same conditions, so why set it up three times?
---
 spec/services/sets/product_set_spec.rb | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 536e720c3e..2a8b913730 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -56,22 +56,14 @@ describe Sets::ProductSet do
             }
           end
 
-          it 'updates the product' do
-            product_set.save
+          it 'updates the product without error' do
+            expect(product_set.save).to eq true
 
             expect(product.reload.attributes).to include(
               'variant_unit' => 'weight'
             )
-          end
 
-          it 'does not add an error' do
-            product_set.save
-            expect(product_set.errors)
-              .to be_empty
-          end
-
-          it 'returns true' do
-            expect(product_set.save).to eq(true)
+            expect(product_set.errors).to be_empty
           end
         end
 

From 224b6f514b2a77eedc5d658e9f4041466b6d149e Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 17 Oct 2023 16:46:14 +1100
Subject: [PATCH 03/12] Remove concept of master variant

from old bulk product screen. Hmm I just realised we're deleting that screen soon anyway.

But this helps clean up the spec before I refactor it further.
---
 .../admin/bulk_product_update.js.coffee       | 15 ------
 app/services/sets/product_set.rb              | 11 +----
 .../admin/bulk_product_update_spec.js.coffee  | 23 ---------
 spec/services/sets/product_set_spec.rb        | 49 -------------------
 4 files changed, 2 insertions(+), 96 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 9f0c5a7a81..910fb39f8b 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -248,7 +248,6 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     else
       product.variant_unit = product.variant_unit_scale = null
 
-    $scope.packVariant product, product.master if product.master
 
     if product.variants
       for id, variant of product.variants
@@ -299,7 +298,6 @@ filterSubmitProducts = (productsToFilter) ->
       if product.hasOwnProperty("id")
         filteredProduct = {id: product.id}
         filteredVariants = []
-        filteredMaster = null
         hasUpdatableProperty = false
 
         if product.hasOwnProperty("variants")
@@ -309,16 +307,6 @@ filterSubmitProducts = (productsToFilter) ->
             variantHasUpdatableProperty = result.hasUpdatableProperty
             filteredVariants.push filteredVariant  if variantHasUpdatableProperty
 
-        if product.master?.hasOwnProperty("unit_value")
-          filteredMaster ?= { id: product.master.id }
-          filteredMaster.unit_value = product.master.unit_value
-        if product.master?.hasOwnProperty("unit_description")
-          filteredMaster ?= { id: product.master.id }
-          filteredMaster.unit_description = product.master.unit_description
-        if product.master?.hasOwnProperty("display_as")
-          filteredMaster ?= { id: product.master.id }
-          filteredMaster.display_as = product.master.display_as
-
         if product.hasOwnProperty("sku")
           filteredProduct.sku = product.sku
           hasUpdatableProperty = true
@@ -350,9 +338,6 @@ filterSubmitProducts = (productsToFilter) ->
         if product.hasOwnProperty("inherits_properties")
           filteredProduct.inherits_properties = product.inherits_properties
           hasUpdatableProperty = true
-        if filteredMaster?
-          filteredProduct.master_attributes = filteredMaster
-          hasUpdatableProperty = true
         if filteredVariants.length > 0 # Note that the name of the property changes to enable mass assignment of variants attributes with rails
           filteredProduct.variants_attributes = filteredVariants
           hasUpdatableProperty = true
diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 7eb73e3bee..3e674717e5 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -59,12 +59,11 @@ module Sets
 
       ExchangeVariantDeleter.new.delete(product) if product.saved_change_to_supplier_id?
 
-      update_product_variants(product, attributes) &&
-        update_product_master(product, attributes)
+      update_product_variants(product, attributes)
     end
 
     def update_product_only_attributes(product, attributes)
-      variant_related_attrs = [:id, :variants_attributes, :master_attributes]
+      variant_related_attrs = [:id, :variants_attributes]
       product_related_attrs = attributes.except(*variant_related_attrs)
       return true if product_related_attrs.blank?
 
@@ -94,12 +93,6 @@ module Sets
       update_variants_attributes(product, attributes[:variants_attributes])
     end
 
-    def update_product_master(product, attributes)
-      return true unless attributes[:master_attributes]
-
-      create_or_update_variant(product, attributes[:master_attributes])
-    end
-
     def update_variants_attributes(product, variants_attributes)
       variants_attributes.each do |attributes|
         create_or_update_variant(product, attributes)
diff --git a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
index 28ff35e1e8..aa56ccd595 100644
--- a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
@@ -195,10 +195,6 @@ describe "filtering products for submission to database", ->
       producer_id: 5
       group_buy: null
       group_buy_unit_size: null
-      master:
-        id: 2
-        unit_value: 250
-        unit_description: "foo"
       variants: [
         id: 1
         on_hand: 2
@@ -221,10 +217,6 @@ describe "filtering products for submission to database", ->
       variant_unit: 'volume'
       variant_unit_scale: 1
       variant_unit_name: 'loaf'
-      master_attributes:
-        id: 2
-        unit_value: 250
-        unit_description: "foo"
       variants_attributes: [
         id: 1
         on_hand: 2
@@ -558,17 +550,6 @@ describe "AdminProductEditCtrl", ->
           variant_unit_scale: null
           variant_unit_with_scale: 'items'
 
-      it "packs the master variant", ->
-        spyOn $scope, "packVariant"
-        testVariant = {id: 1}
-        testProduct =
-          id: 1
-          master: testVariant
-
-        $scope.packProduct(testProduct)
-
-        expect($scope.packVariant).toHaveBeenCalledWith(testProduct, testVariant)
-
       it "packs each variant", ->
         spyOn $scope, "packVariant"
         testVariant = {id: 1}
@@ -986,10 +967,6 @@ describe "AdminProductEditCtrl", ->
         producer_id: 5
         group_buy: null
         group_buy_unit_size: null
-        master:
-          id: 2
-          unit_value: 250
-          unit_description: "foo"
 
     describe 'product has variant', ->
       it 'should load the edit product variant page', ->
diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 2a8b913730..800b6209be 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -138,55 +138,6 @@ describe Sets::ProductSet do
               end
             end
           end
-
-          context 'when :master_attributes is passed' do
-            let(:master_attributes) { { sku: '123' } }
-
-            before do
-              collection_hash[0][:master_attributes] = master_attributes
-            end
-
-            context 'and the variant does exist' do
-              let!(:variant) { create(:variant, product:) }
-
-              before { master_attributes[:id] = variant.id }
-
-              it 'updates the attributes of the master variant' do
-                product_set.save
-                expect(variant.reload.sku).to eq('123')
-              end
-            end
-
-            context 'and the variant does not exist' do
-              context 'and attributes provided are valid' do
-                let(:master_attributes) do
-                  attributes_for(:variant).merge(sku: '123')
-                end
-
-                it 'creates it with the specified attributes' do
-                  number_of_variants = Spree::Variant.all.size
-                  product_set.save
-                  expect(Spree::Variant.last.sku).to eq('123')
-                  expect(Spree::Variant.all.size).to eq number_of_variants + 1
-                end
-              end
-
-              context 'and attributes provided are not valid' do
-                let(:master_attributes) do
-                  # unit_value nil makes the variant invalid
-                  # on_hand with a value would make on_hand be updated and fail with exception
-                  attributes_for(:variant).merge(unit_value: nil, on_hand: 1, sku: '321')
-                end
-
-                it 'does not create variant and notifies bugsnag still raising the exception' do
-                  number_of_variants = Spree::Variant.all.size
-                  expect(product_set.save).to eq(false)
-                  expect(Spree::Variant.all.size).to eq number_of_variants
-                  expect(Spree::Variant.last.sku).not_to eq('321')
-                end
-              end
-            end
-          end
         end
       end
 

From 8293f24ed3c93b59f9dc51c25e0e6c168ba1a805 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 20 Oct 2023 13:14:14 +1100
Subject: [PATCH 04/12] Remove redundant context

Best viewed with whitespace ignored.
---
 spec/services/sets/product_set_spec.rb | 302 ++++++++++++-------------
 1 file changed, 150 insertions(+), 152 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 800b6209be..4b9885493b 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -4,152 +4,179 @@ require 'spec_helper'
 
 describe Sets::ProductSet do
   describe '#save' do
-    context 'when passing :collection_attributes' do
-      let(:product_set) do
-        described_class.new(collection_attributes: collection_hash)
+    let(:product_set) do
+      described_class.new(collection_attributes: collection_hash)
+    end
+
+    context 'when the product does not exist yet' do
+      let!(:stock_location) { create(:stock_location, backorderable_default: false) }
+      let(:collection_hash) do
+        {
+          0 => {
+            name: 'a product',
+            price: 2.0,
+            supplier_id: create(:enterprise).id,
+            primary_taxon_id: create(:taxon).id,
+            unit_description: 'description',
+            variant_unit: 'items',
+            variant_unit_name: 'bunches',
+            shipping_category_id: create(:shipping_category).id
+          }
+        }
       end
 
-      context 'when the product does not exist yet' do
-        let!(:stock_location) { create(:stock_location, backorderable_default: false) }
+      it 'does not create a new product' do
+        product_set.save
+
+        expect(Spree::Product.last).to be nil
+      end
+    end
+
+    context 'when the product does exist' do
+      context 'when a different variant_unit is passed' do
+        let!(:product) do
+          create(
+            :simple_product,
+            variant_unit: 'items',
+            variant_unit_scale: nil,
+            variant_unit_name: 'bunches',
+            unit_value: nil,
+            unit_description: 'some description'
+          )
+        end
+
         let(:collection_hash) do
           {
             0 => {
-              name: 'a product',
-              price: 2.0,
-              supplier_id: create(:enterprise).id,
-              primary_taxon_id: create(:taxon).id,
-              unit_description: 'description',
-              variant_unit: 'items',
-              variant_unit_name: 'bunches',
-              shipping_category_id: create(:shipping_category).id
+              id: product.id,
+              variant_unit: 'weight',
+              variant_unit_scale: 1
             }
           }
         end
 
-        it 'does not create a new product' do
-          product_set.save
+        it 'updates the product without error' do
+          expect(product_set.save).to eq true
 
-          expect(Spree::Product.last).to be nil
+          expect(product.reload.attributes).to include(
+            'variant_unit' => 'weight'
+          )
+
+          expect(product_set.errors).to be_empty
         end
       end
 
-      context 'when the product does exist' do
-        context 'when a different variant_unit is passed' do
-          let!(:product) do
-            create(
-              :simple_product,
-              variant_unit: 'items',
-              variant_unit_scale: nil,
-              variant_unit_name: 'bunches',
-              unit_value: nil,
-              unit_description: 'some description'
-            )
-          end
+      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) {
+          create(:simple_order_cycle, variants: [product.variants.first],
+                                      coordinator: distributor,
+                                      distributors: [distributor])
+        }
+
+        context 'and only the name changes' do
           let(:collection_hash) do
-            {
-              0 => {
-                id: product.id,
-                variant_unit: 'weight',
-                variant_unit_scale: 1
-              }
-            }
+            { 0 => { id: product.id, name: "New season product" } }
           end
 
-          it 'updates the product without error' do
-            expect(product_set.save).to eq true
-
-            expect(product.reload.attributes).to include(
-              'variant_unit' => 'weight'
-            )
-
-            expect(product_set.errors).to be_empty
-          end
-        end
-
-        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) {
-            create(:simple_order_cycle, variants: [product.variants.first],
-                                        coordinator: distributor,
-                                        distributors: [distributor])
-          }
-
-          context 'and only the name changes' do
-            let(:collection_hash) do
-              { 0 => { id: product.id, name: "New season product" } }
-            end
-
-            it 'updates the product and keeps it in order cycles' do
-              expect {
-                product_set.save
-                product.reload
-              }.to change { product.name }.to("New season product").
-                and change { order_cycle.distributed_variants.count }.by(0)
-
-              expect(order_cycle.distributed_variants).to include product.variants.first
-            end
-          end
-
-          context 'and a different supplier is passed' do
-            let(:collection_hash) do
-              { 0 => { id: product.id, supplier_id: producer.id } }
-            end
-
-            it 'updates the product and removes the product from order cycles' do
-              expect {
-                product_set.save
-                product.reload
-              }.to change { product.supplier }.to(producer).
-                and change { order_cycle.distributed_variants.count }.by(-1)
-
-              expect(order_cycle.distributed_variants).to_not include product.variants.first
-            end
-          end
-        end
-
-        context 'when attributes of the variants are passed' do
-          let!(:product) { create(:simple_product) }
-          let(:collection_hash) { { 0 => { id: product.id } } }
-
-          context 'when :variants_attributes are passed' do
-            let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
-
-            before { collection_hash[0][:variants_attributes] = variants_attributes }
-
-            it 'updates the attributes of the variant' do
+          it 'updates the product and keeps it in order cycles' do
+            expect {
               product_set.save
+              product.reload
+            }.to change { product.name }.to("New season product").
+              and change { order_cycle.distributed_variants.count }.by(0)
 
-              expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
-            end
+            expect(order_cycle.distributed_variants).to include product.variants.first
+          end
+        end
 
-            context 'and when product attributes are also passed' do
-              it 'updates product and variant attributes' do
-                collection_hash[0][:sku] = "test_sku"
+        context 'and a different supplier is passed' do
+          let(:collection_hash) do
+            { 0 => { id: product.id, supplier_id: producer.id } }
+          end
 
-                expect {
-                  product_set.save
-                  product.reload
-                }.to change { product.sku }.to("test_sku")
-                  .and change { product.variants.first.sku }.to("123")
-              end
-            end
+          it 'updates the product and removes the product from order cycles' do
+            expect {
+              product_set.save
+              product.reload
+            }.to change { product.supplier }.to(producer).
+              and change { order_cycle.distributed_variants.count }.by(-1)
+
+            expect(order_cycle.distributed_variants).to_not include product.variants.first
           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") }
+      context 'when attributes of the variants are passed' do
+        let!(:product) { create(:simple_product) }
+        let(:collection_hash) { { 0 => { id: product.id } } }
 
+        context 'when :variants_attributes are passed' do
+          let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
+
+          before { collection_hash[0][:variants_attributes] = variants_attributes }
+
+          it 'updates the attributes of the variant' do
+            product_set.save
+
+            expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
+          end
+
+          context 'and when product attributes are also passed' do
+            it 'updates product and variant attributes' do
+              collection_hash[0][:sku] = "test_sku"
+
+              expect {
+                product_set.save
+                product.reload
+              }.to change { product.sku }.to("test_sku")
+                .and change { product.variants.first.sku }.to("123")
+            end
+          end
+        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
+
+      context 'first product has an error' do
         let(:collection_hash) do
           {
             0 => {
               id: product_a.id,
-              name: "Pommes",
+              name: "", # Product Name can't be blank
             },
             1 => {
               id: product_b.id,
@@ -158,46 +185,17 @@ describe Sets::ProductSet do
           }
         end
 
-        it 'updates the products' do
+        it 'continues to update subsequent products' do
           product_set.save
 
-          expect(product_a.reload.name).to eq "Pommes"
+          # 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
-
-        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
-
-        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 89e53c88f314b93760638aec2f748653adb66f67 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 20 Oct 2023 13:17:27 +1100
Subject: [PATCH 05/12] Move context up a level

Best viewed with whitespace ignored.
---
 spec/services/sets/product_set_spec.rb | 38 +++++++++++++-------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb
index 4b9885493b..18964d7f21 100644
--- a/spec/services/sets/product_set_spec.rb
+++ b/spec/services/sets/product_set_spec.rb
@@ -109,32 +109,32 @@ describe Sets::ProductSet do
           end
         end
       end
+    end
 
-      context 'when attributes of the variants are passed' do
-        let!(:product) { create(:simple_product) }
-        let(:collection_hash) { { 0 => { id: product.id } } }
+    describe "updating variants" do
+      let!(:product) { create(:simple_product) }
+      let(:collection_hash) { { 0 => { id: product.id } } }
 
-        context 'when :variants_attributes are passed' do
-          let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
+      context 'when :variants_attributes are passed' do
+        let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
 
-          before { collection_hash[0][:variants_attributes] = variants_attributes }
+        before { collection_hash[0][:variants_attributes] = variants_attributes }
 
-          it 'updates the attributes of the variant' do
-            product_set.save
+        it 'updates the attributes of the variant' do
+          product_set.save
 
-            expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
-          end
+          expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
+        end
 
-          context 'and when product attributes are also passed' do
-            it 'updates product and variant attributes' do
-              collection_hash[0][:sku] = "test_sku"
+        context 'and when product attributes are also passed' do
+          it 'updates product and variant attributes' do
+            collection_hash[0][:sku] = "test_sku"
 
-              expect {
-                product_set.save
-                product.reload
-              }.to change { product.sku }.to("test_sku")
-                .and change { product.variants.first.sku }.to("123")
-            end
+            expect {
+              product_set.save
+              product.reload
+            }.to change { product.sku }.to("test_sku")
+              .and change { product.variants.first.sku }.to("123")
           end
         end
       end

From 153490889503d90bd41151d490f94ea92e5a614d Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 28 Sep 2023 11:09:13 +1000
Subject: [PATCH 06/12] Move generic method to base class

Because rubocop complained about the size of ProductSet.
---
 app/services/sets/model_set.rb   | 6 ++++++
 app/services/sets/product_set.rb | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb
index 7347e2d9c9..1db02abf92 100644
--- a/app/services/sets/model_set.rb
+++ b/app/services/sets/model_set.rb
@@ -79,5 +79,11 @@ module Sets
     def persisted?
       false
     end
+
+    def find_model(collection, model_id)
+      collection.find do |model|
+        model.id.to_s == model_id.to_s && model.persisted?
+      end
+    end
   end
 end
diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 3e674717e5..0068a6b66a 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -140,11 +140,5 @@ module Sets
         report.add_metadata(:variant_error, variant.errors.first) unless variant.valid?
       end
     end
-
-    def find_model(collection, model_id)
-      collection.find do |model|
-        model.id.to_s == model_id.to_s && model.persisted?
-      end
-    end
   end
 end

From ceb9d9af92ee83800d6cc96af2cc6f6e5505844b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 20 Oct 2023 16:40:08 +1100
Subject: [PATCH 07/12] Add comment

I decided to look into why this is so complicated, in case some of the complexity can be removed. It can't :(
---
 app/services/sets/product_set.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb
index 0068a6b66a..23be044239 100644
--- a/app/services/sets/product_set.rb
+++ b/app/services/sets/product_set.rb
@@ -112,6 +112,7 @@ module Sets
     def create_variant(product, variant_attributes)
       return if variant_attributes.blank?
 
+      # 'You need to save the variant to create a stock item before you can set stock levels.'
       on_hand = variant_attributes.delete(:on_hand)
       on_demand = variant_attributes.delete(:on_demand)
 

From fdad45bb461d2be6c6c7b748a2f998e7f5ffff6b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 24 Oct 2023 14:49:04 +1100
Subject: [PATCH 08/12] Rename 'modified' state to 'changed'

This conveniently matches the terminology used in both JavaScript and ActiveModel::Dirty.
---
 app/views/admin/products_v3/_table.html.haml  |  2 +-
 .../controllers/bulk_form_controller.js       | 40 +++++------
 app/webpacker/css/admin/products_v3.scss      |  4 +-
 .../css/admin_v3/globals/variables.scss       |  2 +-
 app/webpacker/css/admin_v3/shared/forms.scss  |  4 +-
 config/locales/en.yml                         |  2 +-
 .../stimulus/bulk_form_controller_test.js     | 66 +++++++++----------
 7 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 7417f676cd..6be9db6c90 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -4,7 +4,7 @@
   %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" }
     .container
       .status.ten.columns
-        .modified_summary{ 'data-bulk-form-target': "modifiedSummary", 'data-translation-key': 'admin.products_v3.table.modified_summary'}
+        .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
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 526aa1d5b5..842a1aaa89 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -1,8 +1,8 @@
 import { Controller } from "stimulus";
 
-// Manages "modified" state for a form with multiple records
+// Manages "changed" state for a form with multiple records
 export default class BulkFormController extends Controller {
-  static targets = ["actions", "modifiedSummary"];
+  static targets = ["actions", "changedSummary"];
   static values = {
     disableSelector: String,
   };
@@ -12,10 +12,10 @@ export default class BulkFormController extends Controller {
     this.form = this.element;
 
     // Start listening for any changes within the form
-    // this.element.addEventListener('change', this.toggleModified.bind(this)); // dunno why this doesn't work
+    // this.element.addEventListener('change', this.toggleChanged.bind(this)); // dunno why this doesn't work
     for (const element of this.form.elements) {
-      element.addEventListener("keyup", this.toggleModified.bind(this)); // instant response
-      element.addEventListener("change", this.toggleModified.bind(this)); // just in case (eg right-click paste)
+      element.addEventListener("keyup", this.toggleChanged.bind(this)); // instant response
+      element.addEventListener("change", this.toggleChanged.bind(this)); // just in case (eg right-click paste)
 
       // Set up a tree of fields according to their associated record
       const recordContainer = element.closest("[data-record-id]"); // The JS could be more efficient if this data was added to each element. But I didn't want to pollute the HTML too much.
@@ -33,32 +33,32 @@ export default class BulkFormController extends Controller {
     window.removeEventListener("beforeunload", this.preventLeavingBulkForm);
   }
 
-  toggleModified(e) {
+  toggleChanged(e) {
     const element = e.target;
-    element.classList.toggle("modified", this.#isModified(element));
+    element.classList.toggle("changed", this.#isChanged(element));
 
-    this.toggleFormModified();
+    this.toggleFormChanged();
   }
 
-  toggleFormModified() {
-    // For each record, check if any fields are modified
-    const modifiedRecordCount = Object.values(this.recordElements).filter((elements) =>
-      elements.some(this.#isModified)
+  toggleFormChanged() {
+    // For each record, check if any fields are changed
+    const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
+      elements.some(this.#isChanged)
     ).length;
-    const formModified = modifiedRecordCount > 0;
+    const formChanged = changedRecordCount > 0;
 
     // Show actions
-    this.actionsTarget.classList.toggle("hidden", !formModified);
-    this.#disableOtherElements(formModified); // like filters and sorting
+    this.actionsTarget.classList.toggle("hidden", !formChanged);
+    this.#disableOtherElements(formChanged); // like filters and sorting
 
-    // Display number of records modified
-    const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey;
+    // Display number of records changed
+    const key = this.changedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
     if (key) {
-      this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount });
+      this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount });
     }
 
     // Prevent accidental data loss
-    if (formModified) {
+    if (formChanged) {
       window.addEventListener("beforeunload", this.preventLeavingBulkForm);
     } else {
       window.removeEventListener("beforeunload", this.preventLeavingBulkForm);
@@ -85,7 +85,7 @@ export default class BulkFormController extends Controller {
     }
   }
 
-  #isModified(element) {
+  #isChanged(element) {
     return element.value != element.defaultValue;
   }
 }
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 4ea63f3a28..6c4d8b4ebd 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -126,8 +126,8 @@
         border-color: $color-txt-hover-brd;
       }
 
-      &.modified {
-        border-color: $color-txt-modified-brd;
+      &.changed {
+        border-color: $color-txt-changed-brd;
       }
     }
 
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index aa45159e59..73a32c877b 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -74,7 +74,7 @@ $color-sel-hover-bg:             $lighter-grey !default;
 $color-txt-brd:                  $color-border !default;
 $color-txt-text:                 $near-black !default;
 $color-txt-hover-brd:            $teal !default;
-$color-txt-modified-brd:         $bright-orange !default;
+$color-txt-changed-brd:         $bright-orange !default;
 $vpadding-txt:                   5px;
 $hpadding-txt:                   8px;
 
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 6b9cb9cf8e..96cef47615 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -31,8 +31,8 @@ fieldset {
     opacity: 0.7;
   }
 
-  &.modified {
-    border-color: $color-txt-modified-brd;
+  &.changed {
+    border-color: $color-txt-changed-brd;
   }
 }
 
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 2e9842a73f..68cd6ef091 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -833,7 +833,7 @@ en:
         import_products: Import multiple products
         no_products_found_for_search: No products found for your search criteria
       table:
-        modified_summary:
+        changed_summary:
           zero: ""
           one: "%{count} product modified."
           other: "%{count} products modified."
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index cc31531950..db0f990eef 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -32,7 +32,7 @@ describe("BulkFormController", () => {
       <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="modified_summary" data-bulk-form-target="modifiedSummary" data-translation-key="modified_summary"></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">
@@ -50,7 +50,7 @@ describe("BulkFormController", () => {
       const disable1 = document.getElementById("disable1");
       const disable2 = document.getElementById("disable2");
       const actions = document.getElementById("actions");
-      const modified_summary = document.getElementById("modified_summary");
+      const changed_summary = document.getElementById("changed_summary");
       const input1a = document.getElementById("input1a");
       const input1b = document.getElementById("input1b");
       const input2 = document.getElementById("input2");
@@ -60,30 +60,30 @@ describe("BulkFormController", () => {
       it("onChange", () => {
         input1a.value = 'updated1a';
         input1a.dispatchEvent(new Event("change"));
-        // Expect only first field to show modified
-        expect(input1a.classList).toContain('modified');
-        expect(input1b.classList).not.toContain('modified');
-        expect(input2.classList).not.toContain('modified');
+        // Expect only first field to show changed
+        expect(input1a.classList).toContain('changed');
+        expect(input1b.classList).not.toContain('changed');
+        expect(input2.classList).not.toContain('changed');
 
         // Change back to original value
         input1a.value = 'initial1a';
         input1a.dispatchEvent(new Event("change"));
-        expect(input1a.classList).not.toContain('modified');
+        expect(input1a.classList).not.toContain('changed');
 
       });
 
       it("onKeyup", () => {
         input1a.value = 'u1a';
         input1a.dispatchEvent(new Event("keyup"));
-        // Expect only first field to show modified
-        expect(input1a.classList).toContain('modified');
-        expect(input1b.classList).not.toContain('modified');
-        expect(input2.classList).not.toContain('modified');
+        // Expect only first field to show changed
+        expect(input1a.classList).toContain('changed');
+        expect(input1b.classList).not.toContain('changed');
+        expect(input2.classList).not.toContain('changed');
 
         // Change back to original value
         input1a.value = 'initial1a';
         input1a.dispatchEvent(new Event("keyup"));
-        expect(input1a.classList).not.toContain('modified');
+        expect(input1a.classList).not.toContain('changed');
       });
 
       it("multiple fields", () => {
@@ -91,29 +91,29 @@ describe("BulkFormController", () => {
         input1a.dispatchEvent(new Event("change"));
         input2.value = 'updated2';
         input2.dispatchEvent(new Event("change"));
-        // Expect only first field to show modified
-        expect(input1a.classList).toContain('modified');
-        expect(input1b.classList).not.toContain('modified');
-        expect(input2.classList).toContain('modified');
+        // Expect only first field to show changed
+        expect(input1a.classList).toContain('changed');
+        expect(input1b.classList).not.toContain('changed');
+        expect(input2.classList).toContain('changed');
 
         // Change only one back to original value
         input1a.value = 'initial1a';
         input1a.dispatchEvent(new Event("change"));
-        expect(input1a.classList).not.toContain('modified');
-        expect(input1b.classList).not.toContain('modified');
-        expect(input2.classList).toContain('modified');
+        expect(input1a.classList).not.toContain('changed');
+        expect(input1b.classList).not.toContain('changed');
+        expect(input2.classList).toContain('changed');
       });
     })
 
     describe("activating sections, and showing a summary", () => {
       // This scenario should probably be broken up into smaller units.
-      it("counts modified records ", () => {
+      it("counts changed records ", () => {
         // Record 1: First field changed
         input1a.value = 'updated1a';
         input1a.dispatchEvent(new Event("change"));
-        // Actions and modified summary are shown, with other sections disabled
+        // Actions and changed summary are shown, with other sections disabled
         expect(actions.classList).not.toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":1}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
         expect(disable1.classList).toContain('disabled-section');
         expect(disable2.classList).toContain('disabled-section');
 
@@ -122,31 +122,31 @@ describe("BulkFormController", () => {
         input1b.dispatchEvent(new Event("change"));
         // Expect to show same summary translation
         expect(actions.classList).not.toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":1}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
 
         // Record 2: has been changed
         input2.value = 'updated2';
         input2.dispatchEvent(new Event("change"));
         // Expect summary to count both records
         expect(actions.classList).not.toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":2}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":2}');
 
         // Record 1: Change first field back to original value
         input1a.value = 'initial1a';
         input1a.dispatchEvent(new Event("change"));
-        // Both records are still modified.
-        expect(input1a.classList).not.toContain('modified');
-        expect(input1b.classList).toContain('modified');
-        expect(input2.classList).toContain('modified');
+        // Both records are still changed.
+        expect(input1a.classList).not.toContain('changed');
+        expect(input1b.classList).toContain('changed');
+        expect(input2.classList).toContain('changed');
         expect(actions.classList).not.toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":2}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":2}');
 
         // Record 1: Change second field back to original value
         input1b.value = 'initial1b';
         input1b.dispatchEvent(new Event("change"));
-        // Both fields for record 1 show unmodified, but second record is still modified
+        // Both fields for record 1 show unchanged, but second record is still changed
         expect(actions.classList).not.toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":1}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
         expect(disable1.classList).toContain('disabled-section');
         expect(disable2.classList).toContain('disabled-section');
 
@@ -155,7 +155,7 @@ describe("BulkFormController", () => {
         input2.dispatchEvent(new Event("change"));
         // Actions are hidden and other sections are now re-enabled
         expect(actions.classList).toContain('hidden');
-        expect(modified_summary.textContent).toBe('modified_summary, {"count":0}');
+        expect(changed_summary.textContent).toBe('changed_summary, {"count":0}');
         expect(disable1.classList).not.toContain('disabled-section');
         expect(disable2.classList).not.toContain('disabled-section');
       });
@@ -170,7 +170,7 @@ describe("BulkFormController", () => {
   //     const controller = document.querySelector('[data-controller="bulk-form"]');
   //     const form = document.querySelector('[data-controller="bulk-form"]');
 
-  //     // Form is modified and other sections are disabled
+  //     // Form is changed and other sections are disabled
   //     input1a.value = 'updated1a';
   //     input1a.dispatchEvent(new Event("change"));
   //     expect(disable1.classList).toContain('disabled-section');

From 04032e61e2164242de7586f591c387cf1a94f01b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 24 Oct 2023 14:49:27 +1100
Subject: [PATCH 09/12] Prettier

---
 app/webpacker/css/admin/products_v3.scss | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 6c4d8b4ebd..2764f4b954 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -68,7 +68,7 @@
 
       .content {
         // Plain content fields need help to align with text in inputs (due to vertical-align)
-        margin: $vpadding-txt+1px 0;
+        margin: $vpadding-txt + 1px 0;
 
         @extend .line-clamp-1;
       }

From aa4630d74c8dfcbba6901b14dcfa39ef11b043b8 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 24 Oct 2023 11:50:16 +1100
Subject: [PATCH 10/12] Mark fields as changed if they contain unsaved values.

This can happen when there's a validation error. The field with error will also be marked changed, but the error style will override it.

I'd like to move this into a FormBuilder. Existing formbuilder gems don't seem to support it (though I didn't look very hard).
---
 app/models/spree/price.rb                    |  4 ++++
 app/models/spree/variant.rb                  |  3 ++-
 app/views/admin/products_v3/_table.html.haml | 10 +++++-----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb
index 68d5caf306..710e4ef5c3 100644
--- a/app/models/spree/price.rb
+++ b/app/models/spree/price.rb
@@ -24,6 +24,10 @@ module Spree
       amount
     end
 
+    def price_changed?
+      amount_changed?
+    end
+
     def price=(price)
       self[:amount] = parse_price(price)
     end
diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index 528790313b..ea05767a63 100644
--- a/app/models/spree/variant.rb
+++ b/app/models/spree/variant.rb
@@ -50,7 +50,8 @@ module Spree
     has_many :prices,
              class_name: 'Spree::Price',
              dependent: :destroy
-    delegate :display_price, :display_amount, :price, :price=, :currency, :currency=,
+    delegate :display_price, :display_amount, :price, :price_changed?, :price=,
+             :currency, :currency=,
              to: :find_or_build_default_price
 
     has_many :exchange_variants
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 6be9db6c90..1c24161239 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -40,10 +40,10 @@
           %tr
             %td.field.align-left.header
               = product_form.hidden_field :id
-              = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
+              = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name'), class: (product_form.object.name_changed? ? 'changed' : nil)
               = error_message_on product, :name
             %td.field
-              = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
+              = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku'), class: (product_form.object.sku_changed? ? 'changed' : nil)
               = error_message_on product, :sku
             %td.align-right
               .content
@@ -69,15 +69,15 @@
               %tr.condensed
                 %td.field
                   = variant_form.hidden_field :id
-                  = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name
+                  = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name, class: (variant_form.object.display_name_changed? ? 'changed' : nil)
                   = error_message_on variant, :display_name
                 %td.field
-                  = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
+                  = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku'), class: (variant_form.object.sku_changed? ? 'changed' : nil)
                   = error_message_on variant, :sku
                 %td.align-right
                   .content= variant.unit_to_display
                 %td.field
-                  = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
+                  = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'),  class: (variant_form.object.price_changed? ? 'changed' : nil), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                   = error_message_on variant, :price
                 %td.align-right
                   .content= variant.on_hand || 0 #TODO: spec for this according to requirements.

From 671dc570ecf88ef554bdc9f3523add39a699b037 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 24 Oct 2023 15:11:28 +1100
Subject: [PATCH 11/12] Refactor: with form builder

---
 app/helpers/bulk_form_builder.rb             | 13 ++++++++++
 app/views/admin/products_v3/_table.html.haml | 11 +++++----
 spec/helpers/bulk_form_builder_spec.rb       | 26 ++++++++++++++++++++
 3 files changed, 45 insertions(+), 5 deletions(-)
 create mode 100644 app/helpers/bulk_form_builder.rb
 create mode 100644 spec/helpers/bulk_form_builder_spec.rb

diff --git a/app/helpers/bulk_form_builder.rb b/app/helpers/bulk_form_builder.rb
new file mode 100644
index 0000000000..dcb45ae3bc
--- /dev/null
+++ b/app/helpers/bulk_form_builder.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class BulkFormBuilder < ActionView::Helpers::FormBuilder
+  def text_field(field, **opts)
+    # Mark field if it is changed (unsaved)
+    changed_method = "#{field}_changed?"
+    if object.respond_to?(changed_method) && object.public_send(changed_method)
+      opts[:class] = "#{opts[:class]} changed".strip
+    end
+
+    super(field, **opts)
+  end
+end
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 1c24161239..33806fc195 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_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" }
@@ -40,10 +41,10 @@
           %tr
             %td.field.align-left.header
               = product_form.hidden_field :id
-              = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name'), class: (product_form.object.name_changed? ? 'changed' : nil)
+              = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name')
               = error_message_on product, :name
             %td.field
-              = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku'), class: (product_form.object.sku_changed? ? 'changed' : nil)
+              = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
               = error_message_on product, :sku
             %td.align-right
               .content
@@ -69,15 +70,15 @@
               %tr.condensed
                 %td.field
                   = variant_form.hidden_field :id
-                  = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name, class: (variant_form.object.display_name_changed? ? 'changed' : nil)
+                  = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name
                   = error_message_on variant, :display_name
                 %td.field
-                  = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku'), class: (variant_form.object.sku_changed? ? 'changed' : nil)
+                  = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku')
                   = error_message_on variant, :sku
                 %td.align-right
                   .content= variant.unit_to_display
                 %td.field
-                  = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'),  class: (variant_form.object.price_changed? ? 'changed' : nil), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
+                  = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                   = error_message_on variant, :price
                 %td.align-right
                   .content= variant.on_hand || 0 #TODO: spec for this according to requirements.
diff --git a/spec/helpers/bulk_form_builder_spec.rb b/spec/helpers/bulk_form_builder_spec.rb
new file mode 100644
index 0000000000..00ab150c2c
--- /dev/null
+++ b/spec/helpers/bulk_form_builder_spec.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+class TestHelper < ActionView::Base; end
+
+describe BulkFormBuilder do
+  describe '#text_field' do
+    let(:product) { create(:product) }
+    let(:form) { BulkFormBuilder.new(:product, product, self, {}) }
+
+    it { expect(form.text_field(:name)).to_not include "changed" }
+
+    context "attribute has been changed" do
+      before { product.assign_attributes name: "updated name" }
+
+      it { expect(form.text_field(:name)).to include "changed" }
+
+      context "and saved" do
+        before { product.save }
+
+        it { expect(form.text_field(:name)).to_not include "changed" }
+      end
+    end
+  end
+end

From 08ac46fd9145c18f3876890515dc20c907e45d26 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 24 Oct 2023 16:44:34 +1100
Subject: [PATCH 12/12] Refactor spec: Only create necessary records

This was creating a lot of records for every single test. Now it's much more efficient and spec conditions are clearer.
---
 .../system/admin/products_v3/products_spec.rb | 57 ++++++++++++-------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index c6ddc8e7f3..8ea6f57c48 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -7,11 +7,6 @@ describe 'As an admin, I can see the new product page' do
   include AuthenticationHelper
   include FileHelper
 
-  # create lot of products
-  70.times do |i|
-    let!("product_#{i}".to_sym) { create(:simple_product, name: "product #{i}") }
-  end
-
   before do
     # activate feature toggle admin_style_v3 to use new admin interface
     Flipper.enable(:admin_style_v3)
@@ -43,24 +38,26 @@ describe 'As an admin, I can see the new product page' do
   end
 
   describe "pagination" do
-    before do
-      visit admin_products_url
-    end
-
     it "has a pagination, has 15 products per page by default and can change the page" do
+      create_products 16
+      visit admin_products_url
+
       expect(page).to have_selector ".pagination"
       expect_products_count_to_be 15
       within ".pagination" do
         click_link "2"
       end
 
-      expect(page).to have_content "Showing 16 to 30"
+      expect(page).to have_content "Showing 16 to 16" # todo: remove unnecessary duplication
       expect_page_to_be 2
       expect_per_page_to_be 15
-      expect_products_count_to_be 15
+      expect_products_count_to_be 1
     end
 
     it "can change the number of products per page" do
+      create_products 51
+      visit admin_products_url
+
       select "50", from: "per_page"
 
       expect(page).to have_content "Showing 1 to 50"
@@ -75,11 +72,10 @@ describe 'As an admin, I can see the new product page' do
       # create a product with a name that can be searched
       let!(:product_by_name) { create(:simple_product, name: "searchable product") }
 
-      before do
-        visit admin_products_url
-      end
-
       it "can search for a product" do
+        create_products 1
+        visit admin_products_url
+
         search_for "searchable product"
 
         expect(page).to have_field "search_term", with: "searchable product"
@@ -88,20 +84,26 @@ describe 'As an admin, I can see the new product page' do
       end
 
       it "reset the page when searching" do
+        create_products 15
+        visit admin_products_url
+
         within ".pagination" do
           click_link "2"
         end
 
-        expect(page).to have_content "Showing 16 to 30"
+        expect(page).to have_content "Showing 16 to 16"
         expect_page_to_be 2
         expect_per_page_to_be 15
-        expect_products_count_to_be 15
+        expect_products_count_to_be 1
         search_for "searchable product"
         # expect(page).to have_content "1 product found for your search criteria."
         expect_products_count_to_be 1
       end
 
       it "can clear filters" do
+        create_products 1
+        visit admin_products_url
+
         search_for "searchable product"
         expect(page).to have_field "search_term", with: "searchable product"
         # expect(page).to have_content "1 product found for your search criteria."
@@ -110,12 +112,13 @@ describe 'As an admin, I can see the new product page' do
 
         click_link "Clear search"
         expect(page).to have_field "search_term", with: ""
-        expect(page).to have_content "Showing 1 to 15"
-        expect_page_to_be 1
-        expect_products_count_to_be 15
+        expect(page).to have_content "Showing 1 to 2"
+        expect_products_count_to_be 2
       end
 
       it "shows a message when there are no results" do
+        visit admin_products_url
+
         search_for "no results"
         expect(page).to have_content "No products found for your search criteria"
         expect(page).to have_link "Clear search"
@@ -123,7 +126,9 @@ describe 'As an admin, I can see the new product page' do
     end
 
     context "product has producer" do
-      # create a product with a supplier that can be searched
+      before { create_products 1 }
+
+      # create a product with a different supplier
       let!(:producer) { create(:supplier_enterprise, name: "Producer 1") }
       let!(:product_by_supplier) { create(:simple_product, supplier: producer) }
 
@@ -139,7 +144,9 @@ describe 'As an admin, I can see the new product page' do
     end
 
     context "product has category" do
-      # create a product with a category that can be searched
+      before { create_products 1 }
+
+      # create a product with a different category
       let!(:product_by_category) {
         create(:simple_product, primary_taxon: create(:taxon, name: "Category 1"))
       }
@@ -297,6 +304,12 @@ describe 'As an admin, I can see the new product page' do
     end
   end
 
+  def create_products(amount)
+    amount.times do |i|
+      create(:simple_product, name: "product #{i}")
+    end
+  end
+
   def expect_page_to_be(page_number)
     expect(page).to have_selector ".pagination span.page.current", text: page_number.to_s
   end