[ASSB-1334] Detect Actual Changes and Display Changed Flag at field, sub field, protocol , sections #973

mohomeoffice · 2025-01-28T08:56:03Z

This PR implements the following changes:

Detect actual changes at both field and sub-field levels.
Display the changed flag only when genuine modifications occur.
Updated the following components: application-summary, checkbox, editor, review-field, review, static-section, protocol-sections, steps, and reducers.

…changed flag only for genuine modifications.

jeff-horton-ho-sas

Mostly looks good, thanks.

Remove debug / commented out console logs
Some refactorings for readability
It would be good to see some test cases added for hasDatabaseChanges, hasSectionChangedDeep, and hasChangedFields. There are some existing tests in https://github.com/UKHomeOffice/asl-projects/tree/master/test/specs as an example.
Remove debug / commented out console logs :
All unnecessary debug statements and commented-out logs have been removed.
Some refactorings for readability :
Some refactorings have been applied as per your suggestions.
However, in certain places, explicit checks ("", 0, false, null, undefined) were necessary due to the way our data structure works. These explicit conditions ensure accuracy when handling falsy values that could otherwise be misinterpreted as unchanged.
It would be good to see some test cases added for hasDatabaseChanges, hasSectionChangedDeep, and hasChangedFields. There are some existing tests in https://github.com/UKHomeOffice/asl-projects/tree/master/test/specs as an example.

hasDatabaseChanges - I agree with you passing values as param would be better code design
I will implement this in the second part of the task as part of the structural improvement along with test cases you have mention.

client/components/application-summary.js

jeff-horton-ho-sas · 2025-01-29T09:07:09Z

client/components/review.js

+    const actualCurrentValue = currentValue !== undefined && currentValue !== null
+        ? currentValue
+        : values[name] !== undefined
+        ? values[name]
+        : null; // Fallback to `null` if no value is provided


This is hard to read and can use the nullish coalescing operator to make it clearer

Suggested change

const actualCurrentValue = currentValue !== undefined && currentValue !== null

? currentValue

: values[name] !== undefined

? values[name]

: null; // Fallback to `null` if no value is provided

const actualCurrentValue = currentValue ?? values[name] ?? null

Jeff, I initially used the nullish operator for readability, but the explicit ternary check ensures falsy values like 0, "", and false are preserved, preventing unintended fallbacks, so I had to keep it this way.

So does ?? - it only uses the RHS if the LHS is null or undefined. || has the behaviour you're describing, but that isn't what i'm suggesting

client/components/review.js

client/components/application-summary.js

client/components/review.js

client/components/application-summary.js

jeff-horton-ho-sas · 2025-01-29T13:22:36Z

It looks like there is a bug in eslint causing the linting to fail (https://drone-gh.acp.homeoffice.gov.uk/UKHomeOffice/asl-projects/4773/1/3). It looks like this might be fixed in more recent versions eslint/eslint#14172

client/components/application-summary.js

jeff-horton-ho-sas

It would be good to see some test cases added for hasDatabaseChanges, hasSectionChangedDeep, and hasChangedFields. There are some existing tests in https://github.com/UKHomeOffice/asl-projects/tree/master/test/specs as an example.

I'd still like to see some unit tests for the main change detection functions.

It looks like there is a bug in eslint causing the linting to fail (https://drone-gh.acp.homeoffice.gov.uk/UKHomeOffice/asl-projects/4773/1/3). It looks like this might be fixed in more recent versions eslint/eslint#14172

The eslint issue still needs fixing

jeff-horton-ho-sas · 2025-01-31T12:59:41Z

client/components/application-summary.js

+/**
+ * Normalise a given value into a consistent string format.
+ * 
+ * - If the value is `null` or `undefined`, it returns an empty string.
+ * - If the value is an object, it converts it into a JSON string.
+ * - For all other types, it converts the value into a string and trims any extra whitespace.
+ *
+ * @param {any} value - The input value to normalise.
+ * @returns {string} - A normalised string representation of the input value.
+ */
+function normaliseValue(value) {
+
+if (value === null || value === undefined) {
+  return ""; // Return an empty string for null or undefined values.
+}
+
+if (typeof value === "object") {
+  return JSON.stringify(value); // Convert objects to their JSON string representation.
+}
+
+return String(value).trim(); // Convert other types to string and trim whitespace.
+
+}


You've only addressed the first of my points here. Please see the others.

Clean and normalise are doing two parts of one thing, and shouldn't be separate.

Comments don't need to be included if they are repeating trivial code - adding redundant comments makes code harder to read, and makes useful comments harder to pick out from the noise.

String(value).trim().replace(/["]+/g, "") is equivalent to JSON.stringify(value).replace(/["]+/g, "").trim(); so we can unify the two branches.

value === null || value === undefined is equivalent to value == null which is the more idiomatic way of doing that check.

Suggested change

/**

* Normalise a given value into a consistent string format.

*

* - If the value is `null` or `undefined`, it returns an empty string.

* - If the value is an object, it converts it into a JSON string.

* - For all other types, it converts the value into a string and trims any extra whitespace.

*

* @param {any} value - The input value to normalise.

* @returns {string} - A normalised string representation of the input value.

*/

function normaliseValue(value) {

if (value === null || value === undefined) {

return ""; // Return an empty string for null or undefined values.

}

if (typeof value === "object") {

return JSON.stringify(value); // Convert objects to their JSON string representation.

}

return String(value).trim(); // Convert other types to string and trim whitespace.

}

/**

* Normalise a given value into a consistent string format.

*

* - If the value is `null` or `undefined`, it returns an empty string.

* - For other values it uses JSON stringify, removing double quotes.

*

* @param {any} value - The input value to normalise.

* @returns {string} - A normalised string representation of the input value.

*/

function normaliseValue(value) {

if (value == null) {

return "";

}

return JSON.stringify(value)

.replace(/["]+/g, "")

.trim();

}

New question: do we need to be doing some form of deep normalisation?

Say for example we have a section with an object like:

const old = {"property": "value "}; // normalises to "{property:value }" const new = {"property": "value"}; // normalises to "{property:value}" - different

but when calculating the difference between the two values for the change badge for that field we end up comparing value vs value because the } is no longer present to block the .trim(). This then results in a section that shows a change flag, but none of the inputs in that section show as changed.

See also my comment below about merging the two normalise functions and using json-stable-stringify instead of JSON.stringify().

jeff-horton-ho-sas · 2025-01-31T13:13:46Z

client/components/application-summary.js

+    // Special handling for array-based fields (e.g., checkboxes, multi-selects)
+    if (Array.isArray(currentValue) || Array.isArray(initialValue)) {
+      const currentArray = Array.isArray(currentValue) ? currentValue : [];
+      const initialArray = Array.isArray(initialValue) ? initialValue : [];
+      const changed = JSON.stringify(currentArray.sort()) !== JSON.stringify(initialArray.sort());
+
+      return changed;
+    }
+
+    // Special handling for boolean fields (e.g., checkboxes, radios)
+    if (typeof currentValue === "boolean" || typeof initialValue === "boolean") {
+      const changed = currentValue !== initialValue;
+
+      return changed;
+    }


After normalisation current value and initial value are always strings so this is redundant and misleading.

Suggested change

// Special handling for array-based fields (e.g., checkboxes, multi-selects)

if (Array.isArray(currentValue) || Array.isArray(initialValue)) {

const currentArray = Array.isArray(currentValue) ? currentValue : [];

const initialArray = Array.isArray(initialValue) ? initialValue : [];

const changed = JSON.stringify(currentArray.sort()) !== JSON.stringify(initialArray.sort());

return changed;

}

// Special handling for boolean fields (e.g., checkboxes, radios)

if (typeof currentValue === "boolean" || typeof initialValue === "boolean") {

const changed = currentValue !== initialValue;

return changed;

}

jeff-horton-ho-sas · 2025-01-31T13:29:13Z

client/components/review.js

+    const normaliseValue = value => {
+        if (value === null || value === undefined || (Array.isArray(value) && value.length === 0)) {
+            // Normalise `null`, `undefined`, and empty arrays to a consistent empty state
+            return null;
+        }
+        if (Array.isArray(value)) {
+            // Sort arrays to ensure order doesn't affect comparison
+            return value.sort();
+        }
+        if (typeof value === 'object' && value !== null) {
+            // Ensure consistent ordering of object keys for generic objects
+            return JSON.stringify(value, Object.keys(value).sort());
+        }
+        return value; // Return all other values as-is
+    };


This is different to normaliseValue in application-summary.js. We should probably extract both into a library method and have a consistent normalised form.

if (typeof value === 'object' && value !== null) { // Ensure consistent ordering of object keys for generic objects return JSON.stringify(value, Object.keys(value).sort()); }

This is a shallow normalisation, any nested objects could still have inconsistent ordering of keys.

We already depend on json-stable-stringify via shasum to calculate if there's a mismatch between server and client versions when applying a patch. (https://github.com/UKHomeOffice/asl-projects/blob/master/client/actions/projects.js#L325-L331) we could add this as a direct dependency and use that rather than writing our own.

jeff-horton-ho-sas · 2025-01-31T13:31:08Z

client/components/review.js

+    const normaliseDuration = value => {
+    if (value && typeof value === 'object' && ('years' in value || 'months' in value)) {
+        // Normalise duration objects specifically
+        return {
+            years: value.years ?? 5, // Default to 5 if undefined or null
+            months: value.months ?? 0 // Default to 0 if undefined or null
+        };
+    }
+
+      // Default for completely missing duration
+      return { years: 5, months: 0 };
+  };


Suggested change

const normaliseDuration = value => {

if (value && typeof value === 'object' && ('years' in value || 'months' in value)) {

// Normalise duration objects specifically

return {

years: value.years ?? 5, // Default to 5 if undefined or null

months: value.months ?? 0 // Default to 0 if undefined or null

};

}

// Default for completely missing duration

return { years: 5, months: 0 };

};

const normaliseDuration = value => ({

years: value?.years ?? 5,

months: value?.months ?? 0

});

jeff-horton-ho-sas · 2025-01-31T13:33:14Z

client/components/review.js

+    const actualCurrentValue = currentValue !== undefined && currentValue !== null
+        ? currentValue
+        : values[name] !== undefined
+        ? values[name]
+        : null; // Fallback to `null` if no value is provided


So does ?? - it only uses the RHS if the LHS is null or undefined. || has the behaviour you're describing, but that isn't what i'm suggesting

jeff-horton-ho-sas · 2025-01-31T13:36:11Z

client/components/review.js

+    // Adjust storedValue for duration fields
+    const adjustedStoredValue =
+        name === 'duration' ? normaliseDuration(storedValue) : storedValue;


Redundant because of lines 79 and 81.

Suggested change

// Adjust storedValue for duration fields

const adjustedStoredValue =

name === 'duration' ? normaliseDuration(storedValue) : storedValue;

jeff-horton-ho-sas · 2025-01-31T13:38:38Z

client/components/review.js

+    // Detect changes by comparing normalized values
+    let hasChange = !isEqual(normalisedStoredValue, normalisedCurrentValue);
+
+
+    // Add logic for species-based dynamic fields
+    const species = values?.species || [];
+    if (species.length === 0) {
+
+    } else {
+        species.forEach(speciesName => {
+            const fieldName = `reduction-quantities-${speciesName}`;
+            const speciesStoredValue = values.storedValue?.[fieldName] || null;
+            const speciesCurrentValue = values[fieldName] || null;
+
+            if (!isEqual(normaliseValue(speciesStoredValue), normaliseValue(speciesCurrentValue))) {
+
+              // any change in dynamic fields sets hasChange to true
+                hasChange = true; 
+            } else {
+
+            }
+        });
+    }
+
+    return hasChange;


Suggested change

// Detect changes by comparing normalized values

let hasChange = !isEqual(normalisedStoredValue, normalisedCurrentValue);

// Add logic for species-based dynamic fields

const species = values?.species || [];

if (species.length === 0) {

} else {

species.forEach(speciesName => {

const fieldName = `reduction-quantities-${speciesName}`;

const speciesStoredValue = values.storedValue?.[fieldName] || null;

const speciesCurrentValue = values[fieldName] || null;

if (!isEqual(normaliseValue(speciesStoredValue), normaliseValue(speciesCurrentValue))) {

// any change in dynamic fields sets hasChange to true

hasChange = true;

} else {

}

});

}

return hasChange;

if (!isEqual(normalizedStoredValue, normalizedCurrentValue)) {

return true

}

const hasSpeciesChange = (speciesName) => {

const fieldName = `reduction-quantities-${speciesName}`;

const speciesStoredValue = values.storedValue?.[fieldName] || null;

const speciesCurrentValue = values[fieldName] || null;

return !isEqual(normalizeValue(speciesStoredValue), normalizeValue(speciesCurrentValue));

}

const species = values?.species || [];

return species.find(species => hasSpeciesChange(species)) != null;

Also future work: create a ticket to move field specific overrides like this to the schema for that field.

jeff-horton-ho-sas · 2025-01-31T13:42:59Z

client/pages/sections/protocols/sections.js

+  const fieldsWithoutValues = [];
+
+  section.fields?.forEach((field, index) => {
+    console.log("Processing field:", field.name);


These logs are going to be very noisy in opensearch when doing server-side rendering of the page, and could potentially be logging PII which would be a security risk

Suggested change

console.log("Processing field:", field.name);

jeff-horton-ho-sas · 2025-01-31T13:44:06Z

client/pages/sections/protocols/sections.js

+      fieldValue = rawValue || null;
+    }
+
+    console.log(`Field ${index + 1}: ${field.name} = ${fieldValue}`);


These logs are going to be very noisy in opensearch when doing server-side rendering of the page, and could potentially be logging PII which would be a security risk

jeff-horton-ho-sas · 2025-01-31T13:44:12Z

client/pages/sections/protocols/sections.js

+
+  console.log("Fields with values:", fieldsWithValues);
+  console.log("Fields without values:", fieldsWithoutValues);


These logs are going to be very noisy in opensearch when doing server-side rendering of the page, and could potentially be logging PII which would be a security risk

Detect actual changes at field and sub-field levels, and display the …

e592e4e

…changed flag only for genuine modifications.

mohomeoffice requested review from jeff-horton-ho-sas and farrukhishaq January 28, 2025 08:56

removed console log

f4a2fc8

jeff-horton-ho-sas requested changes Jan 29, 2025

View reviewed changes

added suggested improvements by jeff

8ccdcbd

mohomeoffice requested a review from jeff-horton-ho-sas January 30, 2025 09:28

kiran-varma-home-office reviewed Jan 30, 2025

View reviewed changes

client/components/application-summary.js Show resolved Hide resolved

jeff-horton-ho-sas requested changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASSB-1334] Detect Actual Changes and Display Changed Flag at field, sub field, protocol , sections #973

[ASSB-1334] Detect Actual Changes and Display Changed Flag at field, sub field, protocol , sections #973

mohomeoffice commented Jan 28, 2025

jeff-horton-ho-sas left a comment •

edited by mohomeoffice

Loading

jeff-horton-ho-sas Jan 29, 2025

mohomeoffice Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas commented Jan 29, 2025

jeff-horton-ho-sas left a comment

jeff-horton-ho-sas Jan 31, 2025 •

edited

Loading

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

jeff-horton-ho-sas Jan 31, 2025

	// Adjust storedValue for duration fields
	const adjustedStoredValue =
	name === 'duration' ? normaliseDuration(storedValue) : storedValue;


		console.log("Fields with values:", fieldsWithValues);
		console.log("Fields without values:", fieldsWithoutValues);

[ASSB-1334] Detect Actual Changes and Display Changed Flag at field, sub field, protocol , sections #973

Are you sure you want to change the base?

[ASSB-1334] Detect Actual Changes and Display Changed Flag at field, sub field, protocol , sections #973

Conversation

mohomeoffice commented Jan 28, 2025

jeff-horton-ho-sas left a comment • edited by mohomeoffice Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-horton-ho-sas commented Jan 29, 2025

jeff-horton-ho-sas left a comment

Choose a reason for hiding this comment

jeff-horton-ho-sas Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-horton-ho-sas left a comment •

edited by mohomeoffice

Loading

jeff-horton-ho-sas Jan 31, 2025 •

edited

Loading