From 4f287ffe05632d65da7cb84f91b595385da1211c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Mon, 27 May 2024 16:38:55 +1000
Subject: [PATCH] When dropdown fields don't allow blank, but are blank, show
 as changed

They were already counted as changed by the javascript, but didn't have a 'changed' class to indicate it.
The reason they are 'changed', is because the dropdown has no blank option, and is forced to select the first item in the list.

This is purely to cover the case of invalid data, but should help a lot when debugging data issues. I don't think it's any less efficient, because the extra 'classList.toggle' calls don't do anything on unchanged fields.
---
 .../controllers/bulk_form_controller.js       | 16 ++++--
 .../stimulus/bulk_form_controller_test.js     | 53 +++++++++++++++++++
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 0af566b271..bd1ab3b854 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -63,7 +63,7 @@ export default class BulkFormController extends Controller {
     // For each record, check if any fields are changed
     // TODO: optimise basd on current state. if field is changed, but form already changed, no need to update (and vice versa)
     const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
-      elements.some(this.#isChanged),
+      elements.some(this.#checkIsChanged.bind(this)),
     ).length;
     this.formChanged = changedRecordCount > 0 || this.errorValue;
 
@@ -131,11 +131,17 @@ export default class BulkFormController extends Controller {
     });
   }
 
-  #isChanged(element) {
-    if(!element.isConnected) {
-      return false;
+  // Check if changed, and mark with class if it is.
+  #checkIsChanged(element) {
+    if(!element.isConnected) return false;
 
-    } else if (element.type == "checkbox") {
+    const changed = this.#isChanged(element);
+    element.classList.toggle("changed", changed);
+    return changed;
+  }
+
+  #isChanged(element) {
+     if (element.type == "checkbox") {
       return element.defaultChecked !== undefined && element.checked != element.defaultChecked;
 
     } else if (element.type == "select-one") {
diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js
index 82d83c7271..2f1e8ea429 100644
--- a/spec/javascripts/stimulus/bulk_form_controller_test.js
+++ b/spec/javascripts/stimulus/bulk_form_controller_test.js
@@ -99,6 +99,59 @@ describe("BulkFormController", () => {
         expect(input1b.classList).not.toContain('changed');
         expect(input2.classList).toContain('changed');
       });
+
+      describe("select not include_blank", () => {
+        beforeEach(() => {
+          document.body.innerHTML = `
+            <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
+              <div data-record-id="1">
+                <select id="select1">
+                  <option value="1">one</option>
+                  <option value="2">two</option>
+                </select>
+              </div>
+              <input type="submit">
+            </form>
+          `;
+        });
+
+        it("shows as changed", () => {
+          // Expect select to show changed (select-one always has something selected)
+          expect(select1.classList).toContain('changed');
+
+          // Change selection
+          select1.options[0].selected = false;
+          select1.options[1].selected = true;
+          select1.dispatchEvent(new Event("input"));
+          expect(select1.classList).toContain('changed');
+        });
+      });
+
+      describe("select-one with include_blank", () => {
+        beforeEach(() => {
+          document.body.innerHTML = `
+            <form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
+              <div data-record-id="1">
+                <select id="select1">
+                  <option value="">blank</option>
+                  <option value="1">one</option>
+                </select>
+              </div>
+              <input type="submit">
+            </form>
+          `;
+        });
+
+        it("does not show as changed", () => {
+          expect(select1.classList).not.toContain('changed');
+
+          // Change selection
+          select1.options[0].selected = false;
+          select1.options[1].selected = true;
+          select1.dispatchEvent(new Event("input"));
+          expect(select1.classList).toContain('changed');
+        });
+      });
     })
 
     describe("activating sections, and showing a summary", () => {