Ensure error summary always shows when error

Best viewed with whitespace ignored.

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

@@ -1,7 +1,9 @@
 = form_with url: bulk_update_admin_products_path, method: :patch, id: "products-form",
             builder: BulkFormBuilder,
             html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update',
-                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters" } do |form|
+                   'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters",
+                   'data-bulk-form-error-value': defined?(error_counts),
+                  } do |form|
   %fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" }
     .container
       .status.eleven.columns

app/webpacker/controllers/bulk_form_controller.js

@@ -5,6 +5,7 @@ export default class BulkFormController extends Controller {
   static targets = ["actions", "changedSummary"];
   static values = {
     disableSelector: String,
+    error: Boolean,
   };
   recordElements = {};
 
@@ -25,6 +26,8 @@ export default class BulkFormController extends Controller {
         this.recordElements[recordId].push(element);
       }
     }
+
+    this.toggleFormChanged();
   }
 
   disconnect() {
@@ -45,7 +48,7 @@ export default class BulkFormController extends Controller {
     const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
       elements.some(this.#isChanged)
     ).length;
-    const formChanged = changedRecordCount > 0;
+    const formChanged = changedRecordCount > 0 || this.errorValue;
 
     // Show actions
     this.actionsTarget.classList.toggle("hidden", !formChanged);
@@ -54,6 +57,7 @@ export default class BulkFormController extends Controller {
     // Display number of records changed
     const key = this.hasChangedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
     if (key) {
+      // TODO: save processing and only run if changedRecordCount has changed.
       this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount });
     }
 

spec/javascripts/stimulus/bulk_form_controller_test.js

@@ -11,42 +11,39 @@ describe("BulkFormController", () => {
     application.register("bulk-form", bulk_form_controller);
   });
 
-  // Mock I18n. TODO: moved to a shared helper
-  beforeAll(() => {
-    const mockedT = jest.fn();
-    mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
-
-    global.I18n =  {
-      t: mockedT
-    };
-  })
-
-  // (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
-  afterAll(() => {
-    delete global.I18n;
-  })
-
-  beforeEach(() => {
-    document.body.innerHTML = `
-      <div id="disable1"></div>
-      <div id="disable2"></div>
-      <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
-        <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
-        <div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
-        <div data-record-id="1">
-          <input id="input1a" type="text" value="initial1a">
-          <input id="input1b" type="text" value="initial1b">
-        </div>
-        <div data-record-id="2">
-          <input id="input2" type="text" value="initial2">
-        </div>
-        <input type="submit">
-      </form>
-    `;
-  });
-
   describe("Modifying input values", () => {
+    // Mock I18n. TODO: moved to a shared helper
+    beforeAll(() => {
+      const mockedT = jest.fn();
+      mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
+
+      global.I18n =  {
+        t: mockedT
+      };
+    })
+    // (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
+    afterAll(() => {
+      delete global.I18n;
+    })
+
     beforeEach(() => {
+      document.body.innerHTML = `
+        <div id="disable1"></div>
+        <div id="disable2"></div>
+        <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
+          <div id="actions" data-bulk-form-target="actions" class="hidden"></div>
+          <div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
+          <div data-record-id="1">
+            <input id="input1a" type="text" value="initial1a">
+            <input id="input1b" type="text" value="initial1b">
+          </div>
+          <div data-record-id="2">
+            <input id="input2" type="text" value="initial2">
+          </div>
+          <input type="submit">
+        </form>
+      `;
+
       const disable1 = document.getElementById("disable1");
       const disable2 = document.getElementById("disable2");
       const actions = document.getElementById("actions");
@@ -162,6 +159,43 @@ describe("BulkFormController", () => {
     });
   });
 
+  describe("When there are errors", () => {
+    beforeEach(() => {
+      document.body.innerHTML = `
+        <form data-controller="bulk-form" data-bulk-form-error-value="true">
+          <div id="actions" data-bulk-form-target="actions">
+            An error occurred.
+            <input type="submit">
+          </div>
+          <div data-record-id="1">
+            <input id="input1a" type="text" value="initial1a">
+          </div>
+        </form>
+      `;
+
+      const actions = document.getElementById("actions");
+      const changed_summary = document.getElementById("changed_summary");
+      const input1a = document.getElementById("input1a");
+    });
+
+    it("form actions section remains visible", () => {
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+
+      // Record 1: First field changed
+      input1a.value = 'updated1a';
+      input1a.dispatchEvent(new Event("change"));
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+
+      // Change back to original value
+      input1a.value = 'initial1a';
+      input1a.dispatchEvent(new Event("change"));
+      // Expect actions to remain visible
+      expect(actions.classList).not.toContain('hidden');
+    });
+  });
+
   // unable to test disconnect at this stage
   // describe("disconnect()", () => {
   //   it("resets other elements", () => {

spec/system/admin/products_v3/products_spec.rb

@@ -203,7 +203,6 @@ describe 'As an admin, I can see the new product page' do
              price: 5.25)
     }
     let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
-
     before do
       visit admin_products_url
     end
@@ -318,6 +317,23 @@ describe 'As an admin, I can see the new product page' do
           expect(page).to have_field "Price", with: "10.25" # other updated value is retained
         end
       end
+
+      it "saves changes after fixing errors" do
+        within row_containing_name("Apples") do
+          fill_in "Name", with: "Pommes"
+          fill_in "SKU", with: "POM-00"
+        end
+
+        expect {
+          click_button "Save changes"
+          product_a.reload
+          variant_a1.reload
+        }.to change { product_a.name }.to("Pommes")
+          .and change{ product_a.sku }.to("POM-00")
+
+        pending
+        expect(page).to have_content "Changes saved"
+      end
     end
   end