From d0abbc5d2cedba0c92026a7b753a3bb5498f214a Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 27 Sep 2023 16:44:32 +1000 Subject: [PATCH] Ensure error summary always shows when error Best viewed with whitespace ignored. --- app/views/admin/products_v3/_table.html.haml | 4 +- .../controllers/bulk_form_controller.js | 6 +- .../stimulus/bulk_form_controller_test.js | 102 ++++++++++++------ .../system/admin/products_v3/products_spec.rb | 18 +++- 4 files changed, 93 insertions(+), 37 deletions(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 97122ee0e2..3c6d40fa74 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/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 diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 702d882d09..14680ff4bd 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/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 }); } diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index db0f990eef..d50dabbaf4 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/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 = ` -
-
-
- -
-
- - -
-
- -
- -
- `; - }); - 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 = ` +
+
+
+ +
+
+ + +
+
+ +
+ +
+ `; + 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 = ` +
+
+ An error occurred. + +
+
+ +
+
+ `; + + 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", () => { diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 5b403b0e18..5316a3118f 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/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