Pre-fill new variant fields when adding a variant

When a hub manager adds a variant to a product, copy the producer,
category, tax category, unit, unit value and price from the most
recently created variant, and default the quantity to 0.

Previously these fields were blank, which caused confusing save errors
when the required producer, category and unit fields were left empty —
behaviour that regressed when those fields moved from the product to the
variant. Defaulting the quantity to 0 (and not copying the on-demand
flag) leaves new variants out of stock until the user sets a quantity,
preventing accidental sales.

Author: emilythericky

app/helpers/admin/products_helper.rb

@@ -10,10 +10,26 @@ def product_image_form_path(product)
       end
     end
 
+    NEW_VARIANT_TEMPLATE_FIELDS = %i[
+      tax_category_id
+      primary_taxon_id
+      supplier_id
+      variant_unit
+      variant_unit_scale
+      variant_unit_name
+      unit_value
+      price
+    ].freeze
+
     def prepare_new_variant(product, producer_id = nil)
       product.variants.build do |new_variant|
-        new_variant.supplier_id = producer_id
-        new_variant.tax_category_id = product.variants.first.tax_category_id
+        template = product.variants.reject(&:new_record?).last
+        copy_template_fields(template, new_variant) if template
+        new_variant.on_hand_desired = 0
+        # Integer producer_id explicitly overrides the template's supplier_id.
+        # The view passes an AR relation (allowed_producers), not an ID, so this
+        # guard ensures only a real ID overrides the copied value.
+        new_variant.supplier_id = producer_id if producer_id.is_a?(Integer)
       end
     end
 
@@ -57,6 +73,16 @@ def managed_product_enterprises
         .managed_product_enterprises
     end
 
+    private
+
+    def copy_template_fields(template, new_variant)
+      NEW_VARIANT_TEMPLATE_FIELDS.each do |field|
+        next unless template.respond_to?(field) && new_variant.respond_to?(:"#{field}=")
+
+        new_variant.public_send(:"#{field}=", template.public_send(field))
+      end
+    end
+
     # Query only name of the model to avoid loading the whole record
     def selected_option(id, model)
       return [] unless id

app/views/admin/products_v3/_product_variant_row.html.haml

@@ -2,7 +2,7 @@
 = form.fields_for("products", product, index: product_index) do |product_form|
   %tbody.relaxed{ id: dom_id(product), data: { 'record-id': product_form.object.id,
                           controller: "nested-form product",
-                          action: 'rails-nested-form:add->bulk-form#registerElements rails-nested-form:remove->bulk-form#toggleFormChanged' },
+                          action: 'rails-nested-form:add->bulk-form#registerElements rails-nested-form:add->bulk-form#toggleFormChanged rails-nested-form:remove->bulk-form#toggleFormChanged' },
                   class: (defined?(should_slide_in) && should_slide_in) ? 'slide-in' : '' }
     %tr
       = render partial: 'product_row', locals: { f: product_form, product:, product_index: }

app/webpacker/controllers/bulk_form_controller.js

@@ -72,7 +72,15 @@ export default class BulkFormController extends Controller {
     const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
       elements.some(this.#checkIsChanged.bind(this)),
     ).length;
-    this.formChanged = changedRecordCount > 0 || this.errorValue;
+
+    // New (unsaved) variants have an empty hidden id field (no value attribute, or value="").
+    // Their pre-filled values look "unchanged" to #isChanged because Rails bakes the defaults
+    // into the HTML, so we detect them separately and treat their presence as a pending change.
+    const hasNewVariants = Array.from(
+      this.form.querySelectorAll('input[type="hidden"][name*="variants_attributes"][name$="[id]"]'),
+    ).some((el) => !el.value);
+
+    this.formChanged = changedRecordCount > 0 || this.errorValue || hasNewVariants;
 
     // Show actions
     this.hasActionsTarget && this.actionsTarget.classList.toggle("hidden", !this.formChanged);
@@ -103,6 +111,12 @@ export default class BulkFormController extends Controller {
     this.variantUnits = this.element.querySelectorAll("button.popout__button");
     this.variantUnits.forEach((element) => {
       if (element.textContent == "") {
+        // If the row already has a unit type set (e.g. from pre-filled defaults or a prior
+        // TomSelect choice), skip the popout — the unit type is known, and the unit value
+        // will be caught by server-side validation with a clear error message.
+        const row = element.closest("tr");
+        const variantUnitField = row?.querySelector('input[type="hidden"][name$="[variant_unit]"]');
+        if (variantUnitField?.value) return;
         element.click();
       }
     });

app/webpacker/controllers/tom_select_controller.js

@@ -10,6 +10,12 @@ export default class extends Controller {
   };
 
   connect(options = {}) {
+    // Capture the select's current value before TomSelect replaces the element.
+    // For non-remote selects, passing this as `items` tells TomSelect to restore
+    // the pre-selected option during its silent constructor phase (isSetup = false),
+    // so no input/change events fire on the underlying select.
+    const initialValue = !this.remoteUrlValue ? this.element.value : null;
+
     let tomSelectOptions = {
       maxItems: 1,
       maxOptions: null,
@@ -19,6 +25,7 @@ export default class extends Controller {
       onItemAdd: function () {
         this.setTextboxValue("");
       },
+      ...(initialValue ? { items: [initialValue] } : {}),
       ...this.optionsValue,
       ...options,
     };

spec/helpers/admin/products_helper_spec.rb

@@ -47,15 +47,74 @@
 
   describe '#prepare_new_variant' do
     let(:zone) { create(:zone_with_member) }
+    let(:taxon) { create(:taxon) }
+    let(:supplier) { create(:supplier_enterprise) }
     let(:product) {
       create(:taxed_product, zone:, price: 12.54, tax_rate_amount: 0,
                              included_in_price: true)
     }
 
-    context 'when tax category is present for first varient' do
-      it 'sets tax category for new variant' do
-        first_variant_tax_id = product.variants.first.tax_category_id
-        expect(helper.prepare_new_variant(product, []).tax_category_id).to eq(first_variant_tax_id)
+    before do
+      product.variants.last.update!(
+        primary_taxon: taxon,
+        supplier:,
+        variant_unit: "weight",
+        variant_unit_scale: 1000.0,
+        unit_value: 1000.0,
+        price: 9.99,
+      )
+    end
+
+    it 'copies tax category from the last variant' do
+      expect(helper.prepare_new_variant(product).tax_category_id)
+        .to eq(product.variants.last.tax_category_id)
+    end
+
+    it 'copies category (primary taxon) from the last variant' do
+      expect(helper.prepare_new_variant(product).primary_taxon_id).to eq(taxon.id)
+    end
+
+    it 'copies unit type from the last variant' do
+      new_variant = helper.prepare_new_variant(product)
+      expect(new_variant.variant_unit).to eq("weight")
+      expect(new_variant.variant_unit_scale).to eq(1000.0)
+    end
+
+    it 'copies unit value from the last variant so the unit field renders non-empty' do
+      expect(helper.prepare_new_variant(product).unit_value).to eq(1000.0)
+    end
+
+    it 'copies price from the last variant' do
+      expect(helper.prepare_new_variant(product).price).to eq(9.99)
+    end
+
+    it 'copies producer (supplier) from the last variant' do
+      expect(helper.prepare_new_variant(product).supplier_id).to eq(supplier.id)
+    end
+
+    it 'sets on_hand_desired to 0' do
+      expect(helper.prepare_new_variant(product).on_hand_desired).to eq(0)
+    end
+
+    it 'does not copy on_demand, so new variants default to out of stock' do
+      expect(helper.prepare_new_variant(product).on_demand_desired).to be_falsey
+    end
+
+    it 'overrides producer with an explicit integer producer_id' do
+      other_supplier = create(:supplier_enterprise)
+      expect(helper.prepare_new_variant(product, other_supplier.id).supplier_id)
+        .to eq(other_supplier.id)
+    end
+
+    context 'when the product has no existing variants' do
+      let(:product) { create(:product) }
+
+      before { product.variants.destroy_all }
+
+      it 'returns a variant with only supplier_id set' do
+        new_variant = helper.prepare_new_variant(product, supplier.id)
+        expect(new_variant.supplier_id).to eq(supplier.id)
+        expect(new_variant.primary_taxon_id).to be_nil
       end
     end
   end

spec/javascripts/stimulus/bulk_form_controller_test.js

@@ -306,6 +306,91 @@ describe("BulkFormController", () => {
     });
   });
 
+  describe("New unsaved variant detection", () => {
+    beforeEach(() => {
+      document.body.innerHTML = `
+        <form id="form" data-controller="bulk-form"
+              data-action="custom-event->bulk-form#toggleFormChanged">
+          <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
+          <table class="products">
+            <div data-record-id="1">
+              <input id="input1" type="text" value="initial">
+            </div>
+          </table>
+        </form>
+      `;
+    });
+
+    it("shows save button when a new (unsaved) variant id field is present (no value attribute)", () => {
+      // Rails renders new-record hidden id fields with no value attribute
+      input1.insertAdjacentHTML(
+        "afterend",
+        '<input id="new_variant_id" type="hidden" name="form[products][0][variants_attributes][1][id]">',
+      );
+
+      form.dispatchEvent(new Event("custom-event"));
+
+      expect(actions.classList).not.toContain("hidden");
+    });
+
+    it("does not show save button when all variant ids are present (existing variants)", () => {
+      input1.insertAdjacentHTML(
+        "afterend",
+        '<input id="existing_variant_id" type="hidden" name="form[products][0][variants_attributes][0][id]" value="42">',
+      );
+
+      form.dispatchEvent(new Event("custom-event"));
+
+      expect(actions.classList).toContain("hidden");
+    });
+  });
+
+  describe("popoutEmptyVariantUnit", () => {
+    // Trigger the action via a custom event wired to popoutEmptyVariantUnit, the same
+    // pattern used by the rest of this test file to invoke controller actions.
+    const triggerPopout = () => form.dispatchEvent(new Event("trigger-popout"));
+
+    beforeEach(() => {
+      document.body.innerHTML = `
+        <form id="form" data-controller="bulk-form"
+              data-action="trigger-popout->bulk-form#popoutEmptyVariantUnit">
+          <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
+          <table class="products">
+            <tr>
+              <td class="col-unit_scale">
+                <input id="variant_unit_field" type="hidden"
+                       name="form[products][0][variants_attributes][0][variant_unit]" value="">
+              </td>
+              <td class="col-unit">
+                <button id="unit_popout_button" class="popout__button"></button>
+              </td>
+            </tr>
+          </table>
+        </form>
+      `;
+    });
+
+    it("clicks the button when its text is empty and variant_unit is not set", () => {
+      const clickSpy = jest.spyOn(unit_popout_button, "click");
+      triggerPopout();
+      expect(clickSpy).toHaveBeenCalled();
+    });
+
+    it("does not click the button when variant_unit is already set (pre-filled variant)", () => {
+      variant_unit_field.value = "weight";
+      const clickSpy = jest.spyOn(unit_popout_button, "click");
+      triggerPopout();
+      expect(clickSpy).not.toHaveBeenCalled();
+    });
+
+    it("does not click the button when its text is not empty", () => {
+      unit_popout_button.textContent = "100 g";
+      const clickSpy = jest.spyOn(unit_popout_button, "click");
+      triggerPopout();
+      expect(clickSpy).not.toHaveBeenCalled();
+    });
+  });
+
   // unable to test disconnect at this stage
   // describe("disconnect()", () => {
   //   it("resets other elements", () => {

spec/system/admin/products_v3/create_spec.rb

@@ -48,15 +48,19 @@
       expect(page).to have_content "New variant"
     end
 
-    it "has the empty unit value for the new variant display_as by default" do
+    it "pre-fills the unit from the last variant on a new variant row" do
       new_variant_button.click
 
       within new_variant_row do
         unit_button = find('button[aria-label="Unit"]')
-        expect(unit_button.text.strip).to eq('')
-
         unit_button.click
-        expect(page).to have_field "Display unit as", placeholder: ""
+
+        # The new variant inherits the unit from the existing variant (1g):
+        # the unit value is pre-filled so the user doesn't get a "can't be blank"
+        # error on save. The display_as field itself stays empty (the inherited
+        # unit shows as its placeholder) so the user can still override it.
+        expect(page).to have_field "Unit value", with: "1"
+        expect(page).to have_field "Display unit as", with: "", placeholder: "1g"
       end
     end
 

spec/system/admin/products_v3/update_spec.rb

@@ -505,7 +505,8 @@
           within new_variant_row do
             fill_in "Name", with: "N" * 256 # too long
             fill_in "SKU", with: "n" * 256
-            # didn't fill_in "Unit", can't be blank
+            # Unit, producer and category are pre-filled from the existing variant,
+            # so the new row is valid apart from the too-long Name and SKU above.
             fill_in "Price", with: "10.25" # valid
           end
         end
@@ -518,30 +519,6 @@
             fill_in "Price", with: "10.25"
           end
 
-          # Client side validation
-          click_button "Save changes"
-          within new_variant_row do
-            expect_browser_validation('select[aria-label="Unit scale"]')
-          end
-
-          # Fix error
-          within new_variant_row do
-            tomselect_select("Weight (kg)", from: "Unit scale")
-          end
-
-          # Client side validation
-          click_button "Save changes"
-          within new_variant_row do
-            # In CI we get "Please fill out this field." and locally we get
-            # "Please fill in this field."
-            expect_browser_validation('input[aria-label="Unit value"]')
-          end
-
-          # Fix error
-          within new_variant_row do
-            fill_in "Unit value", with: "200"
-          end
-
           expect {
             click_button "Save changes"
 
@@ -550,13 +527,12 @@
             variant_a1.reload
           }.not_to change { variant_a1.display_name }
 
-          # New variant
+          # New variant: unit, producer and category are pre-filled, so the only
+          # errors are the too-long Name and SKU.
           within row_containing_name("N" * 256) do
             expect(page).to have_field "Name", with: "N" * 256
             expect(page).to have_field "SKU", with: "n" * 256
             expect(page).to have_content "is too long"
-            expect(page.find('.col-producer')).to have_content('must exist')
-            expect(page.find('.col-category')).to have_content('must exist')
             expect(page).to have_field "Price", with: "10.25" # other updated value is retained
           end