From daefada5a9b9fc9ab05e5122863bea5c5f62165d Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 8 Sep 2023 13:12:48 +1000 Subject: [PATCH] Show summary of modified records I found myself trying to write Ruby in Javascript, and it's not nearly as pretty.. Javascript now has more advanced data structures like Map, but it's rather useless because it doesn't have the usual iterator methods (such as filter, map, reduce etc). Also for the spec I wasn't sure of the best approach, so will gladly recieve feedback. --- app/views/admin/products_v3/_table.html.haml | 6 +- .../controllers/bulk_form_controller.js | 32 ++++++- config/locales/en.yml | 4 + .../stimulus/bulk_form_controller_test.js | 96 +++++++++++++++---- 4 files changed, 115 insertions(+), 23 deletions(-) diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index a200a8cd41..8e54f978ac 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,10 +1,10 @@ = 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', 'data-controller': "bulk-form"} do |form| - %fieldset.form-actions + %fieldset.form-actions{ '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 @@ -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 index 163e46cde1..d812f907dc 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -2,6 +2,9 @@ import { Controller } from "stimulus"; // Manages "modified" state for a form with multiple records export default class BulkFormController extends Controller { + static targets = ["actions", "modifiedSummary"]; + recordElements = {}; + connect() { this.form = this.element; @@ -10,12 +13,37 @@ export default class BulkFormController extends Controller { 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); + } } } toggleModified(e) { const element = e.target; - const changed = element.value != element.defaultValue; - element.classList.toggle("modified", changed); + 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; + + // Display number of records modified + const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; + if (key) { + this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); + } } } 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 index 0a2ce229da..7134251b0e 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -11,39 +11,99 @@ 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("#toggleModified", () => { - it("marks a changed element as modified", () => { - // const form = document.getElementsByTagName("form")[0]; - const input1 = document.getElementById("input1"); + describe("Modifying input values", () => { + // This is more of a behaviour spec. Jest doesn't have all the niceties of RSpec so lots of code + // would be repeated if these were broken into multiple examples. So it seems impractical to + // write individual unit tests. + it("counts modified fields and records", () => { + const modified_summary = document.getElementById("modified_summary"); + const input1a = document.getElementById("input1a"); + const input1b = document.getElementById("input1b"); const input2 = document.getElementById("input2"); - expect(input1.classList).not.toContain('modified'); + // Record 1: First field changed (we're not simulating a user in a browser here; we're testing DOM events directly) + input1a.value = 'updated1a'; + input1a.dispatchEvent(new Event("change")); + // Expect only first field to show modified, and show modified summary translation + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - // Value has been changed (we're not simulating a user in a browser here; we're testing DOM events directly) - input1.value = 'updated1'; - input1.dispatchEvent(new Event("change")); - // form.dispatchEvent(new Event("change")); - - expect(input1.classList).toContain('modified'); + // Record 1: Second field changed + input1b.value = 'updated1b'; + input1b.dispatchEvent(new Event("change")); + // Expect to show modified, and same summary translation + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); - // Change back to original value - input1.value = 'initial1'; - input1.dispatchEvent(new Event("change")); - // form.dispatchEvent(new Event("change")); + // Record 2: has been changed + input2.value = 'updated2'; + input2.dispatchEvent(new Event("change")); + // Expect all fields to show modified, summary counts both records + expect(input1a.classList).toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); - expect(input1.classList).not.toContain('modified'); + // Record 1: Change first field back to original value + input1a.value = 'initial1a'; + input1a.dispatchEvent(new Event("change")); + // Expect first field to not show modified. But both records are still modified. + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).toContain('modified'); + expect(input2.classList).toContain('modified'); + 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(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); + expect(input2.classList).toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + + // Record 2: Change back to original value + input2.value = 'initial2'; + input2.dispatchEvent(new Event("change")); + // No fields or records are modified + expect(input1a.classList).not.toContain('modified'); + expect(input1b.classList).not.toContain('modified'); expect(input2.classList).not.toContain('modified'); + expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); }); }); });