diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index c9f33c55ef..0cda71ff9a 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -7,10 +7,10 @@ category_options: category_options, category_id: category_id } - if products.any? - .container + .container.results .sixteen.columns = render partial: 'sort', locals: { pagy: pagy, search_term: search_term, producer_id: producer_id, category_id: category_id } - = render partial: 'table', locals: { products: products } + = render partial: 'table', locals: { products: products } - if pagy.pages > 1 = render partial: 'admin/shared/v3/pagy', locals: { pagy: pagy, reflex: "click->Products#fetch" } - else diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 7c3aa22d5a..95dbf74398 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,15 +1,15 @@ = form_with url: bulk_update_admin_products_v3_index_path, method: :patch, id: "products-form", - - html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form| - %fieldset.form-actions + html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', + 'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters"} do |form| + %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" } .container .status.ten.columns - / = t('.products_modified', count: 'X') + .modified_summary{ 'data-bulk-form-target': "modifiedSummary", 'data-translation-key': 'admin.products_v3.table.modified_summary'} - if defined?(error_msg) && error_msg.present? .error = error_msg .form-buttons.six.columns - = form.submit t('.reset'), type: :reset, class: "medium" + = form.submit t('.reset'), type: :reset, class: "medium", 'data-reflex': 'click->products#fetch' = form.submit t('.save'), class: "medium" %table.products %col{ width:"15%" } @@ -34,7 +34,7 @@ %th.align-left= t('admin.products_page.columns.inherits_properties') - products.each do |product| = form.fields_for("products", product, index: nil) do |product_form| - %tbody.relaxed + %tbody.relaxed{ 'data-record-id': product_form.object.id } %tr %td.align-left.header = product_form.hidden_field :id diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js new file mode 100644 index 0000000000..3c98a5ce65 --- /dev/null +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -0,0 +1,90 @@ +import { Controller } from "stimulus"; + +// Manages "modified" state for a form with multiple records +export default class BulkFormController extends Controller { + static targets = ["actions", "modifiedSummary"]; + static values = { + disableSelector: String, + }; + recordElements = {}; + + connect() { + this.form = this.element; + + // Start listening for any changes within the form + // this.element.addEventListener('change', this.toggleModified.bind(this)); // dunno why this doesn't work + for (const element of this.form.elements) { + element.addEventListener("keyup", this.toggleModified.bind(this)); // instant response + element.addEventListener("change", this.toggleModified.bind(this)); // just in case (eg right-click paste) + + // 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. + const recordId = recordContainer && recordContainer.dataset.recordId; + if (recordId) { + this.recordElements[recordId] ||= []; + this.recordElements[recordId].push(element); + } + } + } + + disconnect() { + // Make sure to clean up anything that happened outside + this.#disableOtherElements(false); + window.removeEventListener("beforeunload", this.preventLeavingBulkForm); + } + + toggleModified(e) { + const element = e.target; + const modified = element.value != element.defaultValue; + element.classList.toggle("modified", modified); + + this.toggleFormModified(); + } + + toggleFormModified() { + // For each record, check if any fields are modified + const modifiedRecordCount = Object.keys(this.recordElements).filter((recordId) => { + return this.recordElements[recordId].some((element) => { + return element.value != element.defaultValue; + }); + }).length; + const formModified = modifiedRecordCount > 0; + + // Show actions + this.actionsTarget.classList.toggle("hidden", !formModified); + this.#disableOtherElements(formModified); // like filters and sorting + + // Display number of records modified + const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; + if (key) { + this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); + } + + // Prevent accidental data loss + if (formModified) { + window.addEventListener("beforeunload", this.preventLeavingBulkForm); + } else { + window.removeEventListener("beforeunload", this.preventLeavingBulkForm); + } + } + + preventLeavingBulkForm(e) { + // Cancel the event + e.preventDefault(); + // Chrome requires returnValue to be set. Other browsers may display this if provided, but let's + // not create a new translation key, and keep the behaviour consistent. + e.returnValue = ""; + } + + // private + + #disableOtherElements(disable) { + this.disableElements ||= document.querySelectorAll(this.disableSelectorValue); + + if (this.disableElements) { + this.disableElements.forEach((element) => { + element.classList.toggle("disabled-section", disable); + }); + } + } +} diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index c673cbe139..434c20fce3 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -15,6 +15,19 @@ padding: 0; } + .results { + position: relative; + } + + // Form actions floats over other controls when active + .form-actions { + position: absolute; + top: -1em; + left: 0; + right: 0; + z-index: 1; // Ensure tom-select and .disabled-section are covered + } + // Hopefully these rules will be moved to component(s). table.products { table-layout: fixed; // Column widths are based solely on col definitions (not content). This allows more efficient rendering. @@ -100,6 +113,10 @@ &:focus { border-color: $color-txt-hover-brd; } + + &.modified { + border-color: $color-txt-modified-brd; + } } .field_with_errors { @@ -218,4 +235,22 @@ } } } + + // Blurred and non-clickable + $disabled-blur: 1.5px; + .disabled-section { + position: relative; + + &::after { + content: ""; + position: absolute; + backdrop-filter: blur($disabled-blur); + // Stretch outside for a soft blur edge: + left: -$disabled-blur; + top: -$disabled-blur; + bottom: -$disabled-blur; + right: -$disabled-blur; + z-index: 1; // Ensure tom-select is covered + } + } } diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss index d584e67db9..aa45159e59 100644 --- a/app/webpacker/css/admin_v3/globals/variables.scss +++ b/app/webpacker/css/admin_v3/globals/variables.scss @@ -74,6 +74,7 @@ $color-sel-hover-bg: $lighter-grey !default; $color-txt-brd: $color-border !default; $color-txt-text: $near-black !default; $color-txt-hover-brd: $teal !default; +$color-txt-modified-brd: $bright-orange !default; $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 87cbe94a82..6ee6f4271b 100644 --- a/app/webpacker/css/admin_v3/shared/forms.scss +++ b/app/webpacker/css/admin_v3/shared/forms.scss @@ -30,6 +30,10 @@ fieldset { &[disabled] { opacity: 0.7; } + + &.modified { + border-color: $color-txt-modified-brd; + } } textarea { diff --git a/config/locales/en.yml b/config/locales/en.yml index 245f23ea60..d966cd21c1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -818,6 +818,10 @@ en: import_products: Import multiple products no_products_found_for_search: No products found for your search criteria table: + modified_summary: + zero: "" + one: "%{count} product modified." + other: "%{count} products modified." save: Save changes reset: Discard changes bulk_update: # TODO: fix these diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js new file mode 100644 index 0000000000..cc31531950 --- /dev/null +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -0,0 +1,188 @@ +/** + * @jest-environment jsdom + */ + +import { Application } from "stimulus"; +import bulk_form_controller from "../../../app/webpacker/controllers/bulk_form_controller"; + +describe("BulkFormController", () => { + beforeAll(() => { + const application = Application.start(); + 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", () => { + beforeEach(() => { + const disable1 = document.getElementById("disable1"); + const disable2 = document.getElementById("disable2"); + const actions = document.getElementById("actions"); + const modified_summary = document.getElementById("modified_summary"); + const input1a = document.getElementById("input1a"); + const input1b = document.getElementById("input1b"); + const input2 = document.getElementById("input2"); + }); + + describe("marking changed fields", () => { + it("onChange", () => { + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); + + // Change back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + expect(input1a.classList).not.toContain('modified'); + + }); + + it("onKeyup", () => { + input1a.value = 'u1a'; + input1a.dispatchEvent(new Event("keyup")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).not.toContain('modified'); + + // Change back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("keyup")); + expect(input1a.classList).not.toContain('modified'); + }); + + it("multiple fields", () => { + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect only first field to show modified + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); + + // Change only one back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); + }); + }) + + describe("activating sections, and showing a summary", () => { + // This scenario should probably be broken up into smaller units. + it("counts modified records ", () => { + // Record 1: First field changed + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Actions and modified summary are shown, with other sections disabled + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); + + // Record 1: Second field changed + input1b.value = 'updated1b'; + input1b.dispatchEvent(new Event("change")); + // Expect to show same summary translation + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + + // Record 2: has been changed + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect summary to count both records + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + + // Record 1: Change first field back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + // Both records are still modified. + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + + // Record 1: Change second field back to original value + input1b.value = 'initial1b'; + input1b.dispatchEvent(new Event("change")); + // Both fields for record 1 show unmodified, but second record is still modified + expect(actions.classList).not.toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(disable1.classList).toContain('disabled-section'); + expect(disable2.classList).toContain('disabled-section'); + + // Record 2: Change back to original value + input2.value = 'initial2'; + input2.dispatchEvent(new Event("change")); + // Actions are hidden and other sections are now re-enabled + expect(actions.classList).toContain('hidden'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); + expect(disable1.classList).not.toContain('disabled-section'); + expect(disable2.classList).not.toContain('disabled-section'); + }); + }); + }); + + // unable to test disconnect at this stage + // describe("disconnect()", () => { + // it("resets other elements", () => { + // const disable1 = document.getElementById("disable1"); + // const disable2 = document.getElementById("disable2"); + // const controller = document.querySelector('[data-controller="bulk-form"]'); + // const form = document.querySelector('[data-controller="bulk-form"]'); + + // // Form is modified and other sections are disabled + // input1a.value = 'updated1a'; + // input1a.dispatchEvent(new Event("change")); + // expect(disable1.classList).toContain('disabled-section'); + // expect(disable2.classList).toContain('disabled-section'); + + // // form.submit(); //not implemented + // document.body.removeChild(controller); //doesn't trigger disconnect + // controller.innerHTML = ''; //doesn't trigger disconnect + + // expect(disable1.classList).not.toContain('disabled-section'); + // expect(disable2.classList).not.toContain('disabled-section'); + // //TODO: expect window to have no beforeunload event listener + // }); + // }); +}); diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 0a0c83cbe4..598395645b 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -205,6 +205,33 @@ describe 'As an admin, I can see the new product page' do pending expect(page).to have_content "Changes saved" end + + it "can discard changes and reload latest data" do + within row_containing_name("Apples") do + fill_in "Name", with: "Pommes" + end + + # Expect to be alerted when attempting to navigate away. Cancel. + dismiss_confirm do + click_link "Dashboard" + end + within row_containing_name("Apples") do + expect(page).to have_field "Name", with: "Pommes" # Changed value wasn't lost + end + + # Meanwhile, the SKU was updated + product_a.update! sku: "APL-10" + + expect { + click_button "Discard changes" + product_a.reload + }.to_not change { product_a.name } + + within row_containing_name("Apples") do + expect(page).to have_field "Name", with: "Apples" # Changed value wasn't saved + expect(page).to have_field "SKU", with: "APL-10" # Updated value shown + end + end end def expect_page_to_be(page_number)