From 4d22123e02f426e4d203555a9a4380fbde6c8670 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Nov 2023 14:54:47 +1100
Subject: [PATCH 01/24] Remove stock level from product row

It creates the false impression we can handle agregated stock.
---
 app/views/admin/products_v3/_table.html.haml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 18009160d6..eadb11d1f2 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -57,8 +57,7 @@
             %td.align-right
               -# empty
             %td.align-right
-              -# TODO: new requirement "DISPLAY ON DEMAND IF ALL VARIANTS ARE ON DEMAND". And translate value
-              .content= if product.variants.all?(&:on_demand) then "On demand" else product.on_hand || 0 end
+              -# empty
             %td.align-left
               .content= product.supplier&.name
             %td.align-left

From 7aefa834bf44dbcc9168070d149a8b2e856ec4f5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 10 Nov 2023 16:21:13 +1100
Subject: [PATCH 02/24] Add on_hand input

---
 app/views/admin/products_v3/_table.html.haml   |  6 ++++--
 spec/reflexes/products_reflex_spec.rb          |  9 ++++-----
 spec/system/admin/products_v3/products_spec.rb | 10 +++++-----
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index eadb11d1f2..da8de1bf3b 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -85,8 +85,10 @@
                 %td.field
                   = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                   = error_message_on variant, :price
-                %td.align-right
-                  .content= variant.on_hand || 0 #TODO: spec for this according to requirements.
+                %td.field
+                  %div.on-hand__wrapper
+                    = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
+                    = error_message_on variant, :on_hand
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
                 %td.align-left
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index ba22d41b84..792d9667bd 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -30,11 +30,8 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
 
   describe '#bulk_update' do
     let!(:variant_a1) {
-      create(:variant,
-             product: product_a,
-             display_name: "Medium box",
-             sku: "APL-01",
-             price: 5.25)
+      create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25,
+                       on_hand: 5)
     }
     let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") }
     let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") }
@@ -73,6 +70,7 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
                 "display_name" => "Large box",
                 "sku" => "POM-01",
                 "price" => "10.25",
+                "on_hand" => "6",
               }
             ],
           }
@@ -87,6 +85,7 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
         .and change{ variant_a1.display_name }.to("Large box")
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
+        .and change{ variant_a1.on_hand }.to(6)
     end
 
     describe "sorting" do
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index c9f35240d2..e1560d3ee3 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -194,11 +194,8 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
 
   describe "updating" do
     let!(:variant_a1) {
-      create(:variant,
-             product: product_a,
-             display_name: "Medium box",
-             sku: "APL-01",
-             price: 5.25)
+      create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25,
+                       on_hand: 5)
     }
     let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
     before do
@@ -214,6 +211,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         fill_in "Name", with: "Large box"
         fill_in "SKU", with: "POM-01"
         fill_in "Price", with: "10.25"
+        fill_in "On Hand", with: "6"
       end
 
       expect {
@@ -225,6 +223,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         .and change{ variant_a1.display_name }.to("Large box")
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
+        .and change{ variant_a1.on_hand }.to(6)
 
       within row_containing_name("Pommes") do
         expect(page).to have_field "Name", with: "Pommes"
@@ -234,6 +233,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         expect(page).to have_field "Name", with: "Large box"
         expect(page).to have_field "SKU", with: "POM-01"
         expect(page).to have_field "Price", with: "10.25"
+        expect(page).to have_field "On Hand", with: "6"
       end
 
       pending

From 55cd8a6773247ea3e75bb096d4c8618858387dd0 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 10 Nov 2023 17:01:31 +1100
Subject: [PATCH 03/24] Add on_demand field

But 'Array parameters do not play well with the check_box helper.'

...
---
 app/views/admin/products_v3/_table.html.haml   | 3 +++
 spec/reflexes/products_reflex_spec.rb          | 2 +-
 spec/system/admin/products_v3/products_spec.rb | 6 +++++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index da8de1bf3b..77cc0656e0 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -89,6 +89,9 @@
                   %div.on-hand__wrapper
                     = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
                     = error_message_on variant, :on_hand
+                    = variant_form.label :on_demand do
+                      = variant_form.check_box :on_demand
+                      = t('admin.products_page.columns.on_demand')
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
                 %td.align-left
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index 792d9667bd..e45ff99e9a 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -31,7 +31,7 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
   describe '#bulk_update' do
     let!(:variant_a1) {
       create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25,
-                       on_hand: 5)
+                       on_hand: 5, on_demand: false)
     }
     let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") }
     let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") }
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index e1560d3ee3..4c8d1885af 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -195,7 +195,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
   describe "updating" do
     let!(:variant_a1) {
       create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25,
-                       on_hand: 5)
+                       on_hand: 5, on_demand: false)
     }
     let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
     before do
@@ -212,6 +212,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         fill_in "SKU", with: "POM-01"
         fill_in "Price", with: "10.25"
         fill_in "On Hand", with: "6"
+        check "On Demand"
       end
 
       expect {
@@ -224,6 +225,8 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
         .and change{ variant_a1.on_hand }.to(6)
+      pending "Array parameters do not play well with the check_box helper."
+        # .and change{ variant_a1.on_demand }.to(true)
 
       within row_containing_name("Pommes") do
         expect(page).to have_field "Name", with: "Pommes"
@@ -234,6 +237,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         expect(page).to have_field "SKU", with: "POM-01"
         expect(page).to have_field "Price", with: "10.25"
         expect(page).to have_field "On Hand", with: "6"
+        expect(page).to have_field "On Demand", checked: true
       end
 
       pending

From eccfe96a5b1c4c96829b4abbaf8dc35d9db763ff Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Nov 2023 14:44:38 +1100
Subject: [PATCH 04/24] Use form hash structure

The array format is generally fine, but to properly support checkboxes, we need this format with hash keys.
https://guides.rubyonrails.org/form_helpers.html#understanding-parameter-naming-conventions
---
 app/reflexes/products_reflex.rb               | 18 +++++--
 app/views/admin/products_v3/_table.html.haml  |  8 +--
 spec/reflexes/products_reflex_spec.rb         | 51 ++++++++++---------
 .../system/admin/products_v3/products_spec.rb |  3 +-
 4 files changed, 45 insertions(+), 35 deletions(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 01736eedab..c75eddce24 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -167,18 +167,26 @@ class ProductsReflex < ApplicationReflex
     # Form field names:
     #   '[products][0][id]' (hidden field)
     #   '[products][0][name]'
+    #   '[products][0][variants_attributes][0][id]' (hidden field)
+    #   '[products][0][variants_attributes][0][display_name]'
     #
     # Resulting in params:
     #     "products" => {
-    #       "<i>" =>  {
+    #       "0" =>  {
     #         "id" => "123"
     #         "name" => "Pommes",
+    #         "variants_attributes" => {
+    #           "0" => {
+    #           "id" => "1234",
+    #           "display_name" => "Large box",
+    #         }
     #       }
     #     }
-
-    collection_hash = products_bulk_params[:products].each_with_index
-      .to_h { |p, i|
-        [i, p]
+    collection_hash = products_bulk_params[:products]
+      .transform_values { |product|
+        # Convert variants_attributes form hash to an array if present
+        product[:variants_attributes] &&= product[:variants_attributes].values
+        product
       }.with_indifferent_access
     Sets::ProductSet.new(collection_attributes: collection_hash)
   end
diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 77cc0656e0..0ec811f311 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -38,8 +38,8 @@
         %th.align-left= t('admin.products_page.columns.tax_category')
         %th.align-left= t('admin.products_page.columns.inherits_properties')
         %th.align-right= t('admin.products_page.columns.actions')
-    - products.each do |product|
-      = form.fields_for("products", product, index: nil) do |product_form|
+    - products.each_with_index do |product, product_index|
+      = form.fields_for("products", product, index: product_index) do |product_form|
         %tbody.relaxed{ 'data-record-id': product_form.object.id }
           %tr
             %td.field.align-left.header
@@ -70,8 +70,8 @@
                 = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product)
                 = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product)
 
-          - product.variants.each do |variant|
-            = form.fields_for("products][][variants_attributes][", variant, index: nil) do |variant_form|
+          - product.variants.each_with_index do |variant, variant_index|
+            = form.fields_for("products][#{product_index}][variants_attributes][", variant, variant_index:) do |variant_form|
               %tr.condensed
                 %td.field
                   = variant_form.hidden_field :id
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index e45ff99e9a..debfcd24c8 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -39,14 +39,14 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
 
     it "saves valid changes" do
       params = {
-        # '[products][][name]'
-        "products" => [
-          {
+        # '[products][0][name]'
+        "products" => {
+          "0" => {
             "id" => product_a.id.to_s,
             "name" => "Pommes",
             "sku" => "POM-00",
-          }
-        ]
+          },
+        },
       }
 
       expect{
@@ -57,24 +57,27 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
     end
 
     it "saves valid changes to products and nested variants" do
+      # Form field names:
+      #   '[products][0][id]' (hidden field)
+      #   '[products][0][name]'
+      #   '[products][0][variants_attributes][0][id]' (hidden field)
+      #   '[products][0][variants_attributes][0][display_name]'
       params = {
-        # '[products][][name]'
-        # '[products][][variants_attributes][][display_name]'
-        "products" => [
-          {
+        "products" => {
+          "0" => {
             "id" => product_a.id.to_s,
             "name" => "Pommes",
-            "variants_attributes" => [
-              {
+            "variants_attributes" => {
+              "0" => {
                 "id" => variant_a1.id.to_s,
                 "display_name" => "Large box",
                 "sku" => "POM-01",
                 "price" => "10.25",
                 "on_hand" => "6",
-              }
-            ],
-          }
-        ]
+              },
+            },
+          },
+        },
       }
 
       expect{
@@ -91,15 +94,15 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
     describe "sorting" do
       let(:params) {
         {
-          "products" => [
-            {
+          "products" => {
+            "0" => {
               "id" => product_a.id.to_s,
               "name" => "Pommes",
             },
-            {
+            "1" => {
               "id" => product_b.id.to_s,
             },
-          ]
+          },
         }
       }
       subject{ run_reflex(:bulk_update, params:) }
@@ -115,20 +118,20 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do
     describe "error messages" do
       it "summarises error messages" do
         params = {
-          "products" => [
-            {
+          "products" => {
+            "0" => {
               "id" => product_a.id.to_s,
               "name" => "Pommes",
             },
-            {
+            "1" => {
               "id" => product_b.id.to_s,
               "name" => "", # Name can't be blank
             },
-            {
+            "2" => {
               "id" => product_c.id.to_s,
               "name" => "", # Name can't be blank
             },
-          ]
+          },
         }
 
         reflex = run_reflex(:bulk_update, params:)
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 4c8d1885af..967c68b0aa 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -225,8 +225,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
         .and change{ variant_a1.on_hand }.to(6)
-      pending "Array parameters do not play well with the check_box helper."
-        # .and change{ variant_a1.on_demand }.to(true)
+        .and change{ variant_a1.on_demand }.to(true)
 
       within row_containing_name("Pommes") do
         expect(page).to have_field "Name", with: "Pommes"

From b7ac1f269657169c40e1ad1698af3f4152a3c81b Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Nov 2023 16:31:07 +1100
Subject: [PATCH 05/24] Move specific style rule to where it belongs

Also fixed it to line up properly. There's probably a better way to line it up but that's no my concern right now..
---
 app/webpacker/css/admin/orders.scss          | 7 +++++++
 app/webpacker/css/admin_v3/shared/forms.scss | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/webpacker/css/admin/orders.scss b/app/webpacker/css/admin/orders.scss
index 8738ce51c7..74531609f8 100644
--- a/app/webpacker/css/admin/orders.scss
+++ b/app/webpacker/css/admin/orders.scss
@@ -1,3 +1,10 @@
+.admin-orders-index-search {
+  .inline-checkbox {
+    // Ensure it lines up with other fields
+    min-height: 5.4em;
+  }
+}
+
 input,
 div {
   &.update-pending {
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 35f8697e04..9910b43246 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -76,8 +76,6 @@ span.info {
   padding: 10px 0;
 
   &.checkbox {
-    min-height: 70px;
-
     input[type="checkbox"] {
       display: inline-block;
       width: auto;

From d9570cdf32d17eb5212b2300126188737ce448c5 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Nov 2023 17:04:01 +1100
Subject: [PATCH 06/24] Update v3 checkbox style

This doesn't _quite_ match the design, but would require a big CSS hack to customise it further, so I thought let's start with this.
---
 app/views/admin/products_v3/_table.html.haml  | 12 +++++++-----
 .../css/admin_v3/globals/palette.scss         |  2 +-
 .../css/admin_v3/globals/variables.scss       |  4 ++++
 app/webpacker/css/admin_v3/shared/forms.scss  | 19 ++++++++++++++-----
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 0ec811f311..e9918389ea 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -87,11 +87,13 @@
                   = error_message_on variant, :price
                 %td.field
                   %div.on-hand__wrapper
-                    = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
-                    = error_message_on variant, :on_hand
-                    = variant_form.label :on_demand do
-                      = variant_form.check_box :on_demand
-                      = t('admin.products_page.columns.on_demand')
+                    .field
+                      = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
+                      = error_message_on variant, :on_hand
+                    .field.checkbox
+                      = variant_form.label :on_demand do
+                        = variant_form.check_box :on_demand
+                        = t('admin.products_page.columns.on_demand')
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
                 %td.align-left
diff --git a/app/webpacker/css/admin_v3/globals/palette.scss b/app/webpacker/css/admin_v3/globals/palette.scss
index 66e557b63a..59b3c88c3a 100644
--- a/app/webpacker/css/admin_v3/globals/palette.scss
+++ b/app/webpacker/css/admin_v3/globals/palette.scss
@@ -13,7 +13,7 @@ $lighter-grey: #f8f9fa !default; // Lighter grey
 $light-grey: #eff1f2 !default; // Light grey (Porcelain)
 $medium-grey: #919191 !default; // Medium grey
 $dark-grey: #2e3132 !default; // Dark Grey
-$near-black: #191c1d !default; // Near-black
+$near-black: #191c1d !default; // Near-black (Shark)
 $fair-pink: #ffefeb !default; // Fair Pink
 $roof-terracotta: #b83b1f !default; // Roof Terracotta
 
diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index a85da4fcc6..66bb3062dc 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -79,6 +79,10 @@ $color-txt-changed-brd:         $bright-orange !default;
 $vpadding-txt:                   5px;
 $hpadding-txt:                   8px;
 
+// Checkboxes
+$color-checkbox-brd:            $near-black !default;
+$color-checkbox-fill:           $teal !default;
+
 // Modal colors
 $color-modal-close-btn:          $color-5 !default;
 $color-modal-close-btn-hover:    darken($color-5, 5%) !default;
diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss
index 9910b43246..b2f4f553e7 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -36,6 +36,18 @@ fieldset {
   }
 }
 
+input[type="checkbox"] {
+  width: 1em;
+  height: 1em;
+  margin-right: 3px;
+}
+
+input[type="checkbox"],
+input[type="radio"],
+input[type="range"] {
+  accent-color: $color-checkbox-fill;
+}
+
 textarea {
   line-height: 19px;
 }
@@ -76,14 +88,11 @@ span.info {
   padding: 10px 0;
 
   &.checkbox {
-    input[type="checkbox"] {
-      display: inline-block;
-      width: auto;
-    }
-
     label {
       cursor: pointer;
       display: block;
+      font-size: inherit;
+      font-weight: normal;
     }
   }
 

From d218565834ff0e665f3c08d1a331011d5a91959e Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 15 Nov 2023 17:09:31 +1100
Subject: [PATCH 07/24] Style stock popout

---
 app/views/admin/products_v3/_table.html.haml |  4 +--
 app/webpacker/css/admin/products_v3.scss     | 35 +++++++++++++++++++-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index e9918389ea..54a7c0c691 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -85,8 +85,8 @@
                 %td.field
                   = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                   = error_message_on variant, :price
-                %td.field
-                  %div.on-hand__wrapper
+                %td.field.on-hand__wrapper
+                  %div.on-hand__popout
                     .field
                       = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
                       = error_message_on variant, :on_hand
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 2764f4b954..949575b7f5 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -114,8 +114,17 @@
       }
     }
 
+    .field {
+      padding: 0;
+      margin-bottom: 0.75em;
+    }
+
+    label {
+      margin: 0;
+    }
+
     // "Naked" inputs. Row hover helps reveal them.
-    input {
+    input:not([type="checkbox"]) {
       border-color: transparent;
       background-color: $color-tbl-cell-bg;
       height: auto;
@@ -265,4 +274,28 @@
       z-index: 1; // Ensure tom-select is covered
     }
   }
+
+  // Stock popout widget
+  .on-hand {
+    &__wrapper {
+      position: relative;
+    }
+    &__popout {
+      position: absolute;
+      top: -1em;
+      left: -1em;
+      z-index: 1; // Cover below row when hover
+      width: 9em;
+
+      padding: $padding-tbl-cell;
+
+      background: $color-tbl-cell-bg;
+      border-radius: 3px; //todo: check
+      box-shadow: 0px 0px 8px 0px rgba($near-black, 0.25);
+
+      .field:last-child {
+        margin-bottom: 0;
+      }
+    }
+  }
 }

From 735b5789cca20690e5e05015723f88fbfeec68fd Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 16 Nov 2023 11:29:58 +1100
Subject: [PATCH 08/24] [wip] Style on-hand button

Had to update the form controller a little bit to handle buttons.

But arrow not showwing on focus.
Getting some weird SCSS behaviour here.. maybe I'm trying to be too clever.
---
 app/views/admin/products_v3/_table.html.haml  |  2 +
 .../controllers/bulk_form_controller.js       |  2 +-
 app/webpacker/css/admin/products_v3.scss      | 47 +++++++++++++++++++
 .../stimulus/bulk_form_controller_test.js     |  1 +
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 54a7c0c691..31e8e27a24 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -86,6 +86,8 @@
                   = 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.field.on-hand__wrapper
+                  %button.on-hand__button
+                    = variant.on_demand ? t(:on_demand) : variant.on_hand
                   %div.on-hand__popout
                     .field
                       = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index ab93978584..9b10cd68c1 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -100,6 +100,6 @@ export default class BulkFormController extends Controller {
   }
 
   #isChanged(element) {
-    return element.value != element.defaultValue;
+    return element.defaultValue !== undefined && element.value != element.defaultValue;
   }
 }
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 949575b7f5..4dbbaf39b0 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -280,7 +280,54 @@
     &__wrapper {
       position: relative;
     }
+
+    &__button {
+      // override button styles
+      &.on-hand__button {
+        background: $color-tbl-cell-bg;
+        color: $color-txt-text;
+        white-space: nowrap;
+        border-color: transparent;
+        font-weight: normal;
+        padding-left: $border-radius; // Super compact
+        padding-right: 1rem; // Retain space for arrow
+        height: auto;
+
+        &:hover,
+        &:active,
+        &:focus {
+          background: $color-tbl-cell-bg;
+          color: $color-txt-text;
+          position: relative;
+        }
+      }
+
+      &:hover:not(:active):not(:focus) {
+        border-color: transparent;
+      }
+
+      &:hover,
+      &:active,
+      &:focus {
+        // for some reason, sass ignores &:active, &:focus here. we could make this a mixin and include it in multiple rules instead
+        &:before {
+          // for some reason, sass seems to extends the selector to include every other :before selector in the app! probably causing the above, and potentially breaking other styles.
+          // extending .icon-chevron-down causes infinite loop in compilation. does @include work for classes?
+          font-family: FontAwesome;
+          text-decoration: inherit;
+          display: inline-block;
+          speak: none;
+          content: "\f078";
+
+          position: absolute;
+          right: $border-radius;
+          font-size: 0.67em;
+        }
+      }
+    }
+
     &__popout {
+      display: none;
       position: absolute;
       top: -1em;
       left: -1em;
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index d558e4b37a..fdbfcde2be 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -36,6 +36,7 @@ describe("BulkFormController", () => {
           <div data-record-id="1">
             <input id="input1a" type="text" value="initial1a">
             <input id="input1b" type="text" value="initial1b">
+            <button>a button is an element, but never changed</button>
           </div>
           <div data-record-id="2">
             <input id="input2" type="text" value="initial2">

From 4560e3728cf8bb93f3fffb576ff65ac2492854d1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 13:20:47 +1100
Subject: [PATCH 09/24] Show popout on click or down key

It looks like a select drop-down, so it can behave like one too.
---
 app/views/admin/products_v3/_table.html.haml  |  6 +--
 .../controllers/popout_controller.js          | 26 ++++++++++
 app/webpacker/css/admin/products_v3.scss      |  1 -
 .../stimulus/popout_controller_test.js        | 49 +++++++++++++++++++
 4 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 app/webpacker/controllers/popout_controller.js
 create mode 100644 spec/javascripts/stimulus/popout_controller_test.js

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 31e8e27a24..0570f22e63 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -85,10 +85,10 @@
                 %td.field
                   = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs
                   = error_message_on variant, :price
-                %td.field.on-hand__wrapper
-                  %button.on-hand__button
+                %td.field.on-hand__wrapper{'data-controller': "popout"}
+                  %button.on-hand__button{'data-popout-target': "button"}
                     = variant.on_demand ? t(:on_demand) : variant.on_hand
-                  %div.on-hand__popout
+                  %div.on-hand__popout{ style: 'display: none;', 'data-popout-target': "dialog"}
                     .field
                       = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
                       = error_message_on variant, :on_hand
diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
new file mode 100644
index 0000000000..06c5156ce4
--- /dev/null
+++ b/app/webpacker/controllers/popout_controller.js
@@ -0,0 +1,26 @@
+import { Controller } from "stimulus";
+
+// Allows a form section to "pop out" and show additional options
+export default class PopoutController extends Controller {
+  static targets = ["button", "dialog"];
+
+  connect() {
+    this.first_input = this.dialogTarget.querySelector("input");
+
+    // Show when click or down-arrow on button
+    this.buttonTarget.addEventListener("click", this.show.bind(this));
+    this.buttonTarget.addEventListener("keydown", this.showIfDownArrow.bind(this));
+  }
+
+  show(e) {
+    this.dialogTarget.style.display = "block";
+    this.first_input.focus();
+    e.preventDefault();
+  }
+
+  showIfDownArrow(e) {
+    if (e.keyCode == 40) {
+      this.show(e);
+    }
+  }
+}
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 4dbbaf39b0..99f9612c20 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -327,7 +327,6 @@
     }
 
     &__popout {
-      display: none;
       position: absolute;
       top: -1em;
       left: -1em;
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
new file mode 100644
index 0000000000..f3f332143d
--- /dev/null
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -0,0 +1,49 @@
+/**
+ * @jest-environment jsdom
+ */
+
+import { Application } from "stimulus";
+import popout_controller from "../../../app/webpacker/controllers/popout_controller";
+
+describe("PopoutController", () => {
+  beforeAll(() => {
+    const application = Application.start();
+    application.register("popout", popout_controller);
+  });
+
+  beforeEach(() => {
+    document.body.innerHTML = `
+      <div data-controller="popout">
+        <button id="button" data-popout-target="button">On demand</button>
+        <div id="dialog" data-popout-target="dialog" style="display: none;">
+          <input id="input1">
+          <input id="input2">
+        </div>
+      </div>
+    `;
+
+    const button = document.getElementById("button");
+    const input1 = document.getElementById("input1");
+    const input2 = document.getElementById("input2");
+  });
+
+  describe("Show", () => {
+    it("shows the dialog on click", () => {
+      button.click();
+
+      expect(dialog.style.display).toBe("block"); // visible
+    });
+
+    it("shows the dialog on keyboard down arrow", () => {
+      button.dispatchEvent(new KeyboardEvent("keydown", { keyCode: 40 }));
+
+      expect(dialog.style.display).toBe("block"); // visible
+    });
+
+    it("doesn't show the dialog on other key press (tab)", () => {
+      button.dispatchEvent(new KeyboardEvent("keydown", { keyCode: 9 }));
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+  });
+});

From 78d2ddb9b771d1110ba78a02a4e3cd14325230fd Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 14:59:18 +1100
Subject: [PATCH 10/24] Close popout when focus outside

I'm starting to think that these stimulus tests are worthless. The environment is not the same as a browser, which creates extra work to deal with the inconsistencies. And it means we're not testing real world behaviour.

So these are just unit tests, but they take extra effort to put together due to the inter-relatedness with the DOM. Hmm.
---
 .../controllers/popout_controller.js          | 20 +++++++++++
 .../stimulus/popout_controller_test.js        | 33 ++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 06c5156ce4..a2fac4e58e 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -10,6 +10,20 @@ export default class PopoutController extends Controller {
     // Show when click or down-arrow on button
     this.buttonTarget.addEventListener("click", this.show.bind(this));
     this.buttonTarget.addEventListener("keydown", this.showIfDownArrow.bind(this));
+
+    // Close when click or tab outside of dialog. Run async (don't block primary event handlers).
+    this.closeIfOutsideBound = this.closeIfOutside.bind(this); // Store reference for removing listeners later.
+    document.addEventListener("click", this.closeIfOutsideBound, { passive: true });
+    document.addEventListener("focusin", this.closeIfOutsideBound, { passive: true });
+  }
+
+  disconnect() {
+    // Clean up handlers registered outside the controller element.
+    // (jest cleans up document too early)
+    if (document) {
+      document.removeEventListener("click", this.closeIfOutsideBound);
+      document.removeEventListener("focusin", this.closeIfOutsideBound);
+    }
   }
 
   show(e) {
@@ -23,4 +37,10 @@ export default class PopoutController extends Controller {
       this.show(e);
     }
   }
+
+  closeIfOutside(e) {
+    if (!this.dialogTarget.contains(e.target)) {
+      this.dialogTarget.style.display = "none";
+    }
+  }
 }
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index f3f332143d..59708a8e46 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -20,16 +20,19 @@ describe("PopoutController", () => {
           <input id="input2">
         </div>
       </div>
+      <input id="input3">
     `;
 
     const button = document.getElementById("button");
     const input1 = document.getElementById("input1");
     const input2 = document.getElementById("input2");
+    const input3 = document.getElementById("input3");
   });
 
   describe("Show", () => {
     it("shows the dialog on click", () => {
-      button.click();
+      // button.click(); // For some reason this fails due to passive: true, but works in real life.
+      button.dispatchEvent(new Event("click"));
 
       expect(dialog.style.display).toBe("block"); // visible
     });
@@ -46,4 +49,32 @@ describe("PopoutController", () => {
       expect(dialog.style.display).toBe("none"); // not visible
     });
   });
+
+  describe("Close", () => {
+    beforeEach(() => {
+      button.dispatchEvent(new Event("click")); // Dialog is open
+    })
+
+    it("closes the dialog when click outside", () => {
+      input3.click();
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+
+    it("closes the dialog when focusing another field (eg with tab)", () => {
+      input3.focus();
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+
+    it("doesn't close the dialog when focusing internal field", () => {
+      input2.focus();
+
+      expect(dialog.style.display).toBe("block"); // visible
+    });
+  });
+
+  describe("Cleaning up", () => {
+    // unable to test disconnect
+  });
 });

From b6045655ee4d6098121ac13f4879fb72b4e611c9 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 15:34:40 +1100
Subject: [PATCH 11/24] Hide popout when checkbox is checked

---
 app/views/admin/products_v3/_table.html.haml    |  2 +-
 app/webpacker/controllers/popout_controller.js  | 17 ++++++++++++++++-
 .../stimulus/popout_controller_test.js          | 17 ++++++++++++++++-
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 0570f22e63..454bb2ed7f 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -94,7 +94,7 @@
                       = error_message_on variant, :on_hand
                     .field.checkbox
                       = variant_form.label :on_demand do
-                        = variant_form.check_box :on_demand
+                        = variant_form.check_box :on_demand, 'data-action': 'change->popout#closeIfChecked'
                         = t('admin.products_page.columns.on_demand')
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index a2fac4e58e..89a7d79791 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -38,9 +38,24 @@ export default class PopoutController extends Controller {
     }
   }
 
+  close() {
+    this.dialogTarget.style.display = "none";
+  }
+
   closeIfOutside(e) {
     if (!this.dialogTarget.contains(e.target)) {
-      this.dialogTarget.style.display = "none";
+      this.close();
+    }
+  }
+
+  // Close if checked
+  // But the `change` or `input` events are fired before the mouseup, therefore the user never sees the item has been successfully checked, making it feel like it wasn't
+  // We could try listening to the mouseup on the label and check for e.target.controls.checked, but that doesn't support use of keybaord, and the value is inverted for some reason..
+  // but maybe we don't need to. User will get enough feedback when the button text is updated..
+  closeIfChecked(e) {
+    if (e.target.checked) {
+      this.close();
+      this.buttonTarget.focus();
     }
   }
 }
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index 59708a8e46..8b32fbf70b 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -17,7 +17,7 @@ describe("PopoutController", () => {
         <button id="button" data-popout-target="button">On demand</button>
         <div id="dialog" data-popout-target="dialog" style="display: none;">
           <input id="input1">
-          <input id="input2">
+          <input id="input2" type="checkbox" data-action="change->popout#closeIfChecked">
         </div>
       </div>
       <input id="input3">
@@ -72,6 +72,21 @@ describe("PopoutController", () => {
 
       expect(dialog.style.display).toBe("block"); // visible
     });
+
+    it("closes the dialog when checkbox is checked", () => {
+      input2.click();
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+
+    it("doesn't close the dialog when checkbox is unchecked", () => {
+      input2.click();
+      button.dispatchEvent(new Event("click")); // Dialog is opened again
+      input2.click();
+
+      expect(input2.checked).toBe(false);
+      expect(dialog.style.display).toBe("block"); // visible
+    });
   });
 
   describe("Cleaning up", () => {

From 888e0b976b1f662d554327d1ba9acad1383f8523 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 16:48:12 +1100
Subject: [PATCH 12/24] Refactor spec

Shoulda done this at the start.
---
 .../stimulus/popout_controller_test.js        | 23 ++++++++++++-------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index 8b32fbf70b..4d413ab2cd 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -34,19 +34,19 @@ describe("PopoutController", () => {
       // button.click(); // For some reason this fails due to passive: true, but works in real life.
       button.dispatchEvent(new Event("click"));
 
-      expect(dialog.style.display).toBe("block"); // visible
+      expectToBeShown(dialog);
     });
 
     it("shows the dialog on keyboard down arrow", () => {
       button.dispatchEvent(new KeyboardEvent("keydown", { keyCode: 40 }));
 
-      expect(dialog.style.display).toBe("block"); // visible
+      expectToBeShown(dialog);
     });
 
     it("doesn't show the dialog on other key press (tab)", () => {
       button.dispatchEvent(new KeyboardEvent("keydown", { keyCode: 9 }));
 
-      expect(dialog.style.display).toBe("none"); // not visible
+      expectToBeClosed(dialog);
     });
   });
 
@@ -58,25 +58,25 @@ describe("PopoutController", () => {
     it("closes the dialog when click outside", () => {
       input3.click();
 
-      expect(dialog.style.display).toBe("none"); // not visible
+      expectToBeClosed(dialog);
     });
 
     it("closes the dialog when focusing another field (eg with tab)", () => {
       input3.focus();
 
-      expect(dialog.style.display).toBe("none"); // not visible
+      expectToBeClosed(dialog);
     });
 
     it("doesn't close the dialog when focusing internal field", () => {
       input2.focus();
 
-      expect(dialog.style.display).toBe("block"); // visible
+      expectToBeShown(dialog);
     });
 
     it("closes the dialog when checkbox is checked", () => {
       input2.click();
 
-      expect(dialog.style.display).toBe("none"); // not visible
+      expectToBeClosed(dialog);
     });
 
     it("doesn't close the dialog when checkbox is unchecked", () => {
@@ -85,7 +85,7 @@ describe("PopoutController", () => {
       input2.click();
 
       expect(input2.checked).toBe(false);
-      expect(dialog.style.display).toBe("block"); // visible
+      expectToBeShown(dialog);
     });
   });
 
@@ -93,3 +93,10 @@ describe("PopoutController", () => {
     // unable to test disconnect
   });
 });
+
+function expectToBeShown(element) {
+  expect(element.style.display).toBe("block");
+}
+function expectToBeClosed(element) {
+  expect(element.style.display).toBe("none");
+}

From 9bc1e873d38141b50542a2037a72eb71a92299ca Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 17:10:22 +1100
Subject: [PATCH 13/24] Display summary of the popout values

I couldn't think of a simpler way to hardcode it, so now we have a clever generic method :)

We can assume that hidden elements will stay hidden, but we need to check each time if an element is disabled or not.
---
 .../controllers/popout_controller.js          | 31 ++++++++++++++++---
 .../stimulus/popout_controller_test.js        | 18 ++++++++---
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 89a7d79791..71ad3c0dfc 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -6,6 +6,7 @@ export default class PopoutController extends Controller {
 
   connect() {
     this.first_input = this.dialogTarget.querySelector("input");
+    this.displayElements = Array.from(this.element.querySelectorAll('input:not([type="hidden"]'));
 
     // Show when click or down-arrow on button
     this.buttonTarget.addEventListener("click", this.show.bind(this));
@@ -39,7 +40,11 @@ export default class PopoutController extends Controller {
   }
 
   close() {
-    this.dialogTarget.style.display = "none";
+    // Close if not already closed
+    if (this.dialogTarget.style.display != "none") {
+      this.buttonTarget.innerText = this.#displayValue();
+      this.dialogTarget.style.display = "none";
+    }
   }
 
   closeIfOutside(e) {
@@ -49,13 +54,31 @@ export default class PopoutController extends Controller {
   }
 
   // Close if checked
-  // But the `change` or `input` events are fired before the mouseup, therefore the user never sees the item has been successfully checked, making it feel like it wasn't
-  // We could try listening to the mouseup on the label and check for e.target.controls.checked, but that doesn't support use of keybaord, and the value is inverted for some reason..
-  // but maybe we don't need to. User will get enough feedback when the button text is updated..
   closeIfChecked(e) {
     if (e.target.checked) {
       this.close();
       this.buttonTarget.focus();
     }
   }
+
+  // private
+
+  // Summarise the active field(s)
+  #displayValue() {
+    let values = this.#enabledDisplayElements().map((element) => {
+      if (element.type == "checkbox") {
+        if (element.checked && element.labels[0]) {
+          return element.labels[0].innerText;
+        }
+      } else {
+        return element.value;
+      }
+    });
+    // Filter empty values and convert to string
+    return values.filter(Boolean).join();
+  }
+
+  #enabledDisplayElements() {
+    return this.displayElements.filter((element) => !element.disabled);
+  }
 }
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index 4d413ab2cd..91f62e7ec5 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -16,17 +16,22 @@ describe("PopoutController", () => {
       <div data-controller="popout">
         <button id="button" data-popout-target="button">On demand</button>
         <div id="dialog" data-popout-target="dialog" style="display: none;">
-          <input id="input1">
-          <input id="input2" type="checkbox" data-action="change->popout#closeIfChecked">
+          <input id="input1" value="value1">
+          <label>
+            <input id="input2" type="checkbox" value="value2" data-action="change->popout#closeIfChecked">
+            label2
+          </label>
+          <input id="input3" type="hidden" value="value3">
         </div>
       </div>
-      <input id="input3">
+      <input id="input4">
     `;
 
     const button = document.getElementById("button");
     const input1 = document.getElementById("input1");
     const input2 = document.getElementById("input2");
     const input3 = document.getElementById("input3");
+    const input4 = document.getElementById("input4");
   });
 
   describe("Show", () => {
@@ -56,15 +61,17 @@ describe("PopoutController", () => {
     })
 
     it("closes the dialog when click outside", () => {
-      input3.click();
+      input4.click();
 
       expectToBeClosed(dialog);
+      expect(button.innerText).toBe("value1");
     });
 
     it("closes the dialog when focusing another field (eg with tab)", () => {
-      input3.focus();
+      input4.focus();
 
       expectToBeClosed(dialog);
+      expect(button.innerText).toBe("value1");
     });
 
     it("doesn't close the dialog when focusing internal field", () => {
@@ -77,6 +84,7 @@ describe("PopoutController", () => {
       input2.click();
 
       expectToBeClosed(dialog);
+      expect(button.innerText).toBe("value1");// The checkbox label should be here, but I just couldn't get the test to work with labels. Don't worry, it works in the browser.
     });
 
     it("doesn't close the dialog when checkbox is unchecked", () => {

From 69f160ff955802a70c983ebf5e823bd78d877b6e Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 23 Nov 2023 12:06:08 +1100
Subject: [PATCH 14/24] Toggle input disabled when on demand checked

This introduces a new 'toggle' controller, and we already had three\! So I created a generic interface that could be extended to potentially support all of them. I propose we try to reduce them all into the one controller, but won't go down the rabbit-hole just yet..

I have an idea on how to re-arrange and make it more contained, by assigning the controller only to the checkbox, and defining targets with aria-controls="", but chose to stick with Stimulus conventions for now.
---
 app/views/admin/products_v3/_table.html.haml  |  6 +-
 .../controllers/toggle_control_controller.js  | 33 +++++++++
 .../toggle_control_controller_test.js         | 70 +++++++++++++++++++
 3 files changed, 106 insertions(+), 3 deletions(-)
 create mode 100644 app/webpacker/controllers/toggle_control_controller.js
 create mode 100644 spec/javascripts/stimulus/toggle_control_controller_test.js

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 454bb2ed7f..9b7adebe2a 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -88,13 +88,13 @@
                 %td.field.on-hand__wrapper{'data-controller': "popout"}
                   %button.on-hand__button{'data-popout-target': "button"}
                     = variant.on_demand ? t(:on_demand) : variant.on_hand
-                  %div.on-hand__popout{ style: 'display: none;', 'data-popout-target': "dialog"}
+                  %div.on-hand__popout{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" }
                     .field
-                      = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand')
+                      = variant_form.number_field :on_hand, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: variant_form.object.on_demand
                       = error_message_on variant, :on_hand
                     .field.checkbox
                       = variant_form.label :on_demand do
-                        = variant_form.check_box :on_demand, 'data-action': 'change->popout#closeIfChecked'
+                        = variant_form.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked'
                         = t('admin.products_page.columns.on_demand')
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
diff --git a/app/webpacker/controllers/toggle_control_controller.js b/app/webpacker/controllers/toggle_control_controller.js
new file mode 100644
index 0000000000..c9a64d9b29
--- /dev/null
+++ b/app/webpacker/controllers/toggle_control_controller.js
@@ -0,0 +1,33 @@
+import { Controller } from "stimulus";
+
+export default class extends Controller {
+  static targets = ["control"];
+
+  disableIfPresent(event) {
+    const input = event.currentTarget;
+    const disable = !!this.#inputValue(input); // Coerce value to boolean
+
+    this.controlTargets.forEach((target) => {
+      target.disabled = disable;
+    });
+
+    // Focus when enabled
+    if (!disable) {
+      this.controlTargets[0].focus();
+    }
+  }
+
+  //todo: can a new method disableIfBlank replace ButtonDisabledController?
+  //todo: can a new method toggleDisplay replace ToggleController?
+  //todo: can toggleDisplay with optional chevron-target replace RemoteToggleController?
+
+  // private
+
+  // Return input's value, but only if it would be submitted by a form
+  // Radio buttons not supported (yet)
+  #inputValue(input) {
+    if (input.type != "checkbox" || input.checked) {
+      return input.value;
+    }
+  }
+}
diff --git a/spec/javascripts/stimulus/toggle_control_controller_test.js b/spec/javascripts/stimulus/toggle_control_controller_test.js
new file mode 100644
index 0000000000..eeffc17ae6
--- /dev/null
+++ b/spec/javascripts/stimulus/toggle_control_controller_test.js
@@ -0,0 +1,70 @@
+/**
+ * @jest-environment jsdom
+ */
+
+import { Application } from "stimulus";
+import toggle_controller from "../../../app/webpacker/controllers/toggle_control_controller";
+
+describe("ToggleControlController", () => {
+  beforeAll(() => {
+    const application = Application.start();
+    application.register("toggle-control", toggle_controller);
+  });
+
+  describe("#disableIfPresent", () => {
+    describe("with checkbox", () => {
+      beforeEach(() => {
+        document.body.innerHTML = `<div data-controller="toggle-control">
+          <input id="checkbox" type="checkbox" value="1" data-action="change->toggle-control#disableIfPresent" />
+          <input id="control" data-toggle-control-target="control">
+        </div>`;
+
+        const checkbox = document.getElementById("checkbox");
+        const control = document.getElementById("control");
+      });
+
+      it("Disables when checkbox is checked", () => {
+        checkbox.click();
+        expect(checkbox.checked).toBe(true);
+
+        expect(control.disabled).toBe(true);
+      });
+
+      it("Enables when checkbox is un-checked", () => {
+        checkbox.click();
+        checkbox.click();
+        expect(checkbox.checked).toBe(false);
+
+        expect(control.disabled).toBe(false);
+      });
+    });
+    describe("with input", () => {
+      beforeEach(() => {
+        document.body.innerHTML = `<div data-controller="toggle-control">
+          <input id="input" value="" data-action="input->toggle-control#disableIfPresent" />
+          <input id="control" data-toggle-control-target="control">
+        </div>`;
+
+        const input = document.getElementById("input");
+        const control = document.getElementById("control");
+      });
+
+      it("Disables when input is filled", () => {
+        input.value = "test"
+        input.dispatchEvent(new Event("input"));
+
+        expect(control.disabled).toBe(true);
+      });
+
+      it("Enables when input is emptied", () => {
+        input.value = "test"
+        input.dispatchEvent(new Event("input"));
+
+        input.value = ""
+        input.dispatchEvent(new Event("input"));
+
+        expect(control.disabled).toBe(false);
+      });
+    });
+  });
+});

From 5e96368c857b6f53d800c79d2b72284d1b8ed35f Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 23 Nov 2023 13:07:31 +1100
Subject: [PATCH 15/24] Count changed checkboxes

---
 app/webpacker/controllers/bulk_form_controller.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 9b10cd68c1..cb0cd64159 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -100,6 +100,10 @@ export default class BulkFormController extends Controller {
   }
 
   #isChanged(element) {
-    return element.defaultValue !== undefined && element.value != element.defaultValue;
+    if (element.type == "checkbox") {
+      return element.defaultChecked !== undefined && element.checked != element.defaultChecked;
+    } else {
+      return element.defaultValue !== undefined && element.value != element.defaultValue;
+    }
   }
 }

From 88fe8dcbe00ae5e54c44a0f124f849f870f45a54 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 23 Nov 2023 13:32:44 +1100
Subject: [PATCH 16/24] Show changes on popout button

---
 app/webpacker/controllers/popout_controller.js | 7 +++++++
 app/webpacker/css/admin/products_v3.scss       | 6 +++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 71ad3c0dfc..0e430ce1a9 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -42,7 +42,10 @@ export default class PopoutController extends Controller {
   close() {
     // Close if not already closed
     if (this.dialogTarget.style.display != "none") {
+      // Update button to represent any changes
       this.buttonTarget.innerText = this.#displayValue();
+      this.buttonTarget.classList.toggle("changed", this.#isChanged());
+
       this.dialogTarget.style.display = "none";
     }
   }
@@ -78,6 +81,10 @@ export default class PopoutController extends Controller {
     return values.filter(Boolean).join();
   }
 
+  #isChanged() {
+    return this.#enabledDisplayElements().some((element) => element.classList.contains("changed"));
+  }
+
   #enabledDisplayElements() {
     return this.displayElements.filter((element) => !element.disabled);
   }
diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 99f9612c20..937dcd2c11 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -300,9 +300,13 @@
           color: $color-txt-text;
           position: relative;
         }
+
+        &.changed {
+          border-color: $color-txt-changed-brd;
+        }
       }
 
-      &:hover:not(:active):not(:focus) {
+      &:hover:not(:active):not(:focus):not(.changed) {
         border-color: transparent;
       }
 

From 373743f96da8749c2cf4547feb488661712e94ae Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 23 Nov 2023 13:35:15 +1100
Subject: [PATCH 17/24] Simplify event handlers

The new 'input' event is for this exact use case.
---
 .../controllers/bulk_form_controller.js       |  3 +-
 .../stimulus/bulk_form_controller_test.js     | 42 +++++++------------
 2 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index cb0cd64159..91af33e9e0 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -15,8 +15,7 @@ export default class BulkFormController extends Controller {
     // Start listening for any changes within the form
     // 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.toggleChanged.bind(this)); // instant response
-      element.addEventListener("change", this.toggleChanged.bind(this)); // just in case (eg right-click paste)
+      element.addEventListener("input", this.toggleChanged.bind(this)); // immediately respond to any change
 
       // 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.
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index fdbfcde2be..a1f0ee6250 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -57,9 +57,9 @@ describe("BulkFormController", () => {
     });
 
     describe("marking changed fields", () => {
-      it("onChange", () => {
+      it("onInput", () => {
         input1a.value = 'updated1a';
-        input1a.dispatchEvent(new Event("change"));
+        input1a.dispatchEvent(new Event("input"));
         // Expect only first field to show changed
         expect(input1a.classList).toContain('changed');
         expect(input1b.classList).not.toContain('changed');
@@ -67,30 +67,16 @@ describe("BulkFormController", () => {
 
         // Change back to original value
         input1a.value = 'initial1a';
-        input1a.dispatchEvent(new Event("change"));
+        input1a.dispatchEvent(new Event("input"));
         expect(input1a.classList).not.toContain('changed');
 
       });
 
-      it("onKeyup", () => {
-        input1a.value = 'u1a';
-        input1a.dispatchEvent(new Event("keyup"));
-        // 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('changed');
-      });
-
       it("multiple fields", () => {
         input1a.value = 'updated1a';
-        input1a.dispatchEvent(new Event("change"));
+        input1a.dispatchEvent(new Event("input"));
         input2.value = 'updated2';
-        input2.dispatchEvent(new Event("change"));
+        input2.dispatchEvent(new Event("input"));
         // Expect only first field to show changed
         expect(input1a.classList).toContain('changed');
         expect(input1b.classList).not.toContain('changed');
@@ -98,7 +84,7 @@ describe("BulkFormController", () => {
 
         // Change only one back to original value
         input1a.value = 'initial1a';
-        input1a.dispatchEvent(new Event("change"));
+        input1a.dispatchEvent(new Event("input"));
         expect(input1a.classList).not.toContain('changed');
         expect(input1b.classList).not.toContain('changed');
         expect(input2.classList).toContain('changed');
@@ -110,7 +96,7 @@ describe("BulkFormController", () => {
       it("counts changed records ", () => {
         // Record 1: First field changed
         input1a.value = 'updated1a';
-        input1a.dispatchEvent(new Event("change"));
+        input1a.dispatchEvent(new Event("input"));
         // Actions and changed summary are shown, with other sections disabled
         expect(actions.classList).not.toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
@@ -121,21 +107,21 @@ describe("BulkFormController", () => {
 
         // Record 1: Second field changed
         input1b.value = 'updated1b';
-        input1b.dispatchEvent(new Event("change"));
+        input1b.dispatchEvent(new Event("input"));
         // Expect to show same summary translation
         expect(actions.classList).not.toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
 
         // Record 2: has been changed
         input2.value = 'updated2';
-        input2.dispatchEvent(new Event("change"));
+        input2.dispatchEvent(new Event("input"));
         // Expect summary to count both records
         expect(actions.classList).not.toContain('hidden');
         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"));
+        input1a.dispatchEvent(new Event("input"));
         // Both records are still changed.
         expect(input1a.classList).not.toContain('changed');
         expect(input1b.classList).toContain('changed');
@@ -145,7 +131,7 @@ describe("BulkFormController", () => {
 
         // Record 1: Change second field back to original value
         input1b.value = 'initial1b';
-        input1b.dispatchEvent(new Event("change"));
+        input1b.dispatchEvent(new Event("input"));
         // Both fields for record 1 show unchanged, but second record is still changed
         expect(actions.classList).not.toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
@@ -156,7 +142,7 @@ describe("BulkFormController", () => {
 
         // Record 2: Change back to original value
         input2.value = 'initial2';
-        input2.dispatchEvent(new Event("change"));
+        input2.dispatchEvent(new Event("input"));
         // Actions are hidden and other sections are now re-enabled
         expect(actions.classList).toContain('hidden');
         expect(changed_summary.textContent).toBe('changed_summary, {"count":0}');
@@ -193,13 +179,13 @@ describe("BulkFormController", () => {
 
       // Record 1: First field changed
       input1a.value = 'updated1a';
-      input1a.dispatchEvent(new Event("change"));
+      input1a.dispatchEvent(new Event("input"));
       // Expect actions to remain visible
       expect(actions.classList).not.toContain('hidden');
 
       // Change back to original value
       input1a.value = 'initial1a';
-      input1a.dispatchEvent(new Event("change"));
+      input1a.dispatchEvent(new Event("input"));
       // Expect actions to remain visible
       expect(actions.classList).not.toContain('hidden');
     });

From bc6a83017b6c661ab385fcb70b491082a9fb3e95 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 23 Nov 2023 14:22:51 +1100
Subject: [PATCH 18/24] Update spec

Ok that was an afterthought, but better late than never.
---
 app/views/admin/products_v3/_table.html.haml  |  2 +-
 .../system/admin/products_v3/products_spec.rb | 28 ++++++++++++++++---
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 9b7adebe2a..5794133f23 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -86,7 +86,7 @@
                   = 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.field.on-hand__wrapper{'data-controller': "popout"}
-                  %button.on-hand__button{'data-popout-target': "button"}
+                  %button.on-hand__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')}
                     = variant.on_demand ? t(:on_demand) : variant.on_hand
                   %div.on-hand__popout{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" }
                     .field
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 967c68b0aa..218d63ac9d 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -211,8 +211,8 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         fill_in "Name", with: "Large box"
         fill_in "SKU", with: "POM-01"
         fill_in "Price", with: "10.25"
+        click_on "On Hand" # activate stock popout
         fill_in "On Hand", with: "6"
-        check "On Demand"
       end
 
       expect {
@@ -225,7 +225,6 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         .and change{ variant_a1.sku }.to("POM-01")
         .and change{ variant_a1.price }.to(10.25)
         .and change{ variant_a1.on_hand }.to(6)
-        .and change{ variant_a1.on_demand }.to(true)
 
       within row_containing_name("Pommes") do
         expect(page).to have_field "Name", with: "Pommes"
@@ -235,8 +234,29 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
         expect(page).to have_field "Name", with: "Large box"
         expect(page).to have_field "SKU", with: "POM-01"
         expect(page).to have_field "Price", with: "10.25"
-        expect(page).to have_field "On Hand", with: "6"
-        expect(page).to have_field "On Demand", checked: true
+        expect(page).to have_css "button[aria-label='On Hand']", text: "6"
+      end
+
+      pending
+      expect(page).to have_content "Changes saved"
+    end
+
+    it "switches stock to on-demand" do
+      within row_containing_name("Medium box") do
+        click_on "On Hand" # activate stock popout
+        check "On Demand"
+
+        expect(page).to have_css "button[aria-label='On Hand']", text: "On Demand"
+      end
+
+      expect {
+        click_button "Save changes"
+        product_a.reload
+        variant_a1.reload
+      }.to change{ variant_a1.on_demand }.to(true)
+
+      within row_containing_name("Medium box") do
+        expect(page).to have_css "button[aria-label='On Hand']", text: "On Demand"
       end
 
       pending

From 994dd606b9773df9c5a9f23a1907025875dd3c3f Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 24 Nov 2023 14:08:51 +1100
Subject: [PATCH 19/24] Refactor: remove dead code

I was mistakenly thinking that you can define variables in beforeEach, but it turns out these are not used.

Rather, Jest was automatically creating variables for the elements according to their IDs. How convenient!
---
 .../stimulus/bulk_form_controller_test.js            | 12 +-----------
 spec/javascripts/stimulus/popout_controller_test.js  |  6 ------
 .../stimulus/toggle_control_controller_test.js       |  6 ------
 3 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index a1f0ee6250..b0a3386ce9 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -36,7 +36,7 @@ describe("BulkFormController", () => {
           <div data-record-id="1">
             <input id="input1a" type="text" value="initial1a">
             <input id="input1b" type="text" value="initial1b">
-            <button>a button is an element, but never changed</button>
+            <button>a button is counted as a form element, but value is undefined</button>
           </div>
           <div data-record-id="2">
             <input id="input2" type="text" value="initial2">
@@ -44,16 +44,6 @@ describe("BulkFormController", () => {
           <input type="submit">
         </form>
       `;
-
-      const disable1 = document.getElementById("disable1");
-      const disable1_element = document.getElementById("disable1_element");
-      const disable2 = document.getElementById("disable2");
-      const disable2_element = document.getElementById("disable2_element");
-      const actions = document.getElementById("actions");
-      const changed_summary = document.getElementById("changed_summary");
-      const input1a = document.getElementById("input1a");
-      const input1b = document.getElementById("input1b");
-      const input2 = document.getElementById("input2");
     });
 
     describe("marking changed fields", () => {
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index 91f62e7ec5..60b6f8ce89 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -26,12 +26,6 @@ describe("PopoutController", () => {
       </div>
       <input id="input4">
     `;
-
-    const button = document.getElementById("button");
-    const input1 = document.getElementById("input1");
-    const input2 = document.getElementById("input2");
-    const input3 = document.getElementById("input3");
-    const input4 = document.getElementById("input4");
   });
 
   describe("Show", () => {
diff --git a/spec/javascripts/stimulus/toggle_control_controller_test.js b/spec/javascripts/stimulus/toggle_control_controller_test.js
index eeffc17ae6..f56686fe9d 100644
--- a/spec/javascripts/stimulus/toggle_control_controller_test.js
+++ b/spec/javascripts/stimulus/toggle_control_controller_test.js
@@ -18,9 +18,6 @@ describe("ToggleControlController", () => {
           <input id="checkbox" type="checkbox" value="1" data-action="change->toggle-control#disableIfPresent" />
           <input id="control" data-toggle-control-target="control">
         </div>`;
-
-        const checkbox = document.getElementById("checkbox");
-        const control = document.getElementById("control");
       });
 
       it("Disables when checkbox is checked", () => {
@@ -44,9 +41,6 @@ describe("ToggleControlController", () => {
           <input id="input" value="" data-action="input->toggle-control#disableIfPresent" />
           <input id="control" data-toggle-control-target="control">
         </div>`;
-
-        const input = document.getElementById("input");
-        const control = document.getElementById("control");
       });
 
       it("Disables when input is filled", () => {

From f034e46aedde6589a40cbc23076e66c5deeae4da Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 24 Nov 2023 14:21:26 +1100
Subject: [PATCH 20/24] Fix label

Strangely, the spec passed when run locally, but not in CI. It's a curious case of the wrong letter case.
---
 app/views/admin/products_v3/_table.html.haml   | 2 +-
 spec/system/admin/products_v3/products_spec.rb | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 5794133f23..8deccd2d84 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -95,7 +95,7 @@
                     .field.checkbox
                       = variant_form.label :on_demand do
                         = variant_form.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked'
-                        = t('admin.products_page.columns.on_demand')
+                        = t(:on_demand)
                 %td.align-left
                   .content= variant.product.supplier&.name # same as product
                 %td.align-left
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 218d63ac9d..b4294bdc4d 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -244,9 +244,9 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
     it "switches stock to on-demand" do
       within row_containing_name("Medium box") do
         click_on "On Hand" # activate stock popout
-        check "On Demand"
+        check "On demand"
 
-        expect(page).to have_css "button[aria-label='On Hand']", text: "On Demand"
+        expect(page).to have_css "button[aria-label='On Hand']", text: "On demand"
       end
 
       expect {
@@ -256,7 +256,7 @@ describe 'As an admin, I can see the new product page', feature: :admin_style_v3
       }.to change{ variant_a1.on_demand }.to(true)
 
       within row_containing_name("Medium box") do
-        expect(page).to have_css "button[aria-label='On Hand']", text: "On Demand"
+        expect(page).to have_css "button[aria-label='On Hand']", text: "On demand"
       end
 
       pending

From aa792346fc5f5098a7384158ff4778dabf5b05c3 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 29 Nov 2023 15:18:23 +1100
Subject: [PATCH 21/24] Refactor

---
 app/webpacker/css/admin/products_v3.scss | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 937dcd2c11..3eba3b7179 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -125,18 +125,13 @@
 
     // "Naked" inputs. Row hover helps reveal them.
     input:not([type="checkbox"]) {
-      border-color: transparent;
       background-color: $color-tbl-cell-bg;
       height: auto;
       font-size: inherit;
       font-weight: inherit;
 
-      &:focus {
-        border-color: $color-txt-hover-brd;
-      }
-
-      &.changed {
-        border-color: $color-txt-changed-brd;
+      &:not(:focus):not(.changed) {
+        border-color: transparent;
       }
     }
 

From 9ce511f69d10e031c11e328648d0f6811c28cc90 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 29 Nov 2023 15:20:48 +1100
Subject: [PATCH 22/24] Update disabled field style

---
 app/webpacker/css/admin_v3/globals/variables.scss | 4 +++-
 app/webpacker/css/admin_v3/shared/forms.scss      | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss
index 66bb3062dc..7eecf26ea8 100644
--- a/app/webpacker/css/admin_v3/globals/variables.scss
+++ b/app/webpacker/css/admin_v3/globals/variables.scss
@@ -75,7 +75,9 @@ $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-changed-brd:         $bright-orange !default;
+$color-txt-disabled-text:        $medium-grey !default;
+$color-txt-disabled-brd:         $light-grey !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 b2f4f553e7..8a37545a29 100644
--- a/app/webpacker/css/admin_v3/shared/forms.scss
+++ b/app/webpacker/css/admin_v3/shared/forms.scss
@@ -28,7 +28,8 @@ fieldset {
   }
 
   &[disabled] {
-    opacity: 0.7;
+    color: $color-txt-disabled-text;
+    border-color: $color-txt-disabled-brd;
   }
 
   &.changed {

From ccc0c17e04ad9f1a76d3f7cfd794400d1f173bb3 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 29 Nov 2023 15:22:06 +1100
Subject: [PATCH 23/24] Hide value of disabled input in popout

I'm not sure if we want to do this on all fields, so just scoped it here for now.
---
 app/webpacker/css/admin/products_v3.scss | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss
index 3eba3b7179..6416645b3d 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -130,7 +130,7 @@
       font-size: inherit;
       font-weight: inherit;
 
-      &:not(:focus):not(.changed) {
+      &:not(:focus):not(.changed):not([disabled]) {
         border-color: transparent;
       }
     }
@@ -341,6 +341,10 @@
       .field:last-child {
         margin-bottom: 0;
       }
+
+      input[disabled] {
+        color: transparent; // hide value completely
+      }
     }
   }
 }

From 6bf418c4ebb755bd502aeafda06c974e4737bf18 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 29 Nov 2023 15:33:14 +1100
Subject: [PATCH 24/24] Fixup

---
 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 6416645b3d..f8d9a8272f 100644
--- a/app/webpacker/css/admin/products_v3.scss
+++ b/app/webpacker/css/admin/products_v3.scss
@@ -335,7 +335,7 @@
       padding: $padding-tbl-cell;
 
       background: $color-tbl-cell-bg;
-      border-radius: 3px; //todo: check
+      border-radius: $border-radius;
       box-shadow: 0px 0px 8px 0px rgba($near-black, 0.25);
 
       .field:last-child {