Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 156 additions & 2 deletions client/components/application-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,48 @@ import Submit from './submit';
import { selector } from './sync-handler';
import HoldingPage from './holding-page';


/**
* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested previously in other PR, I find it easier to read code and write tests when functions like this can be extracted out to different file. This file is too long for my liking :)


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.

}
mohomeoffice marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +24 to +46
Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().


/**
* Clean a given value by normalising it and removing double quotes.
*
* - First, the value is normalised using `normaliseValue`, ensuring it is a consistent string.
* - Then, any double quotes (`"`) in the string are removed using a regular expression.
*
* @param {any} value - The input value to clean.
* @returns {string} - A sanitised string with double quotes removed.
*/
function cleanValue(value) {

// Remove all double quotes from the normalised value.
return normaliseValue(value).replace(/["]+/g, "");
}



const mapStateToProps = ({
project,
comments,
Expand Down Expand Up @@ -65,6 +107,10 @@ const ApplicationSummary = () => {
const [errors, setErrors] = useState(false);
const ref = useRef(null);





useEffect(() => {
if (submitted && !isSyncing) { submit(); }
}, [isSyncing, submitted]);
Expand Down Expand Up @@ -205,6 +251,109 @@ const ApplicationSummary = () => {
return <HoldingPage />;
}


/**
* Recursively find all fields within a section and compare their values.
* @param {object} props - Contains all saved and current data (savedValues, currentValues, initialValues).
* @param {object} sectionData - The section object containing fields and nested subsections.
* @returns {boolean} - True if any field in the section differs, false otherwise.
*/
const hasSectionChangedDeep = (props, sectionData) => {
const { savedValues, currentValues, initialValues } = props;

if (!savedValues || !currentValues || !initialValues) {
return false;
}

/**
* Recursive helper function to compare values.
* @param {object} data - The section or field data to process.
* @returns {boolean} - True if any field differs, false otherwise.
*/
const compareFields = (data) => {
let changed = false;

Object.entries(data).forEach(([key, value]) => {
// Handle nested objects (e.g., subsections)
if (value && typeof value === "object" && !Array.isArray(value)) {
if (compareFields(value)) {
changed = true;
}
} else {
// Compare leaf fields
const savedValue = savedValues[key];
const currentValue = currentValues[key];
const initialValue = initialValues[key];

const sanitisedSaved = cleanValue(savedValue);
const sanitisedCurrent = cleanValue(currentValue);
const sanitisedInitial = cleanValue(initialValue);

if (
sanitisedSaved !== sanitisedCurrent ||
sanitisedSaved !== sanitisedInitial
) {
changed = true;
}
}
});

return changed;
};

return compareFields(sectionData);
};




/**
* Determines if any fields in a given set have changed by comparing their current and initial values.
*
* This function is designed to handle various field types, including:
*
* - **Text & textarea fields:** Compared as normalised and sanitised strings.
* - **Checkboxes & multi-selects:** Compared as sorted arrays to ensure order does not affect the result.
* - **Radio buttons & boolean fields:** Directly compared as boolean values.
*
* The function iterates through all specified fields and returns `true` if any have changed.
*
* @param {Array} fields - The list of field names to check for changes.
* @param {Object} currentValues - The current values of the fields, typically from user input or state.
* @param {Object} initialValues - The initial values of the fields, typically from the database or original state.
* @returns {boolean} - Returns `true` if at least one field has changed, otherwise `false`.
*/
const hasChangedFields = (fields, currentValues, initialValues) => {
return fields.some(field => {
// Normalise and sanitise current and initial values
const currentValue = cleanValue(currentValues[field]);
const initialValue = cleanValue(initialValues[field]);

// 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;
}
Comment on lines +332 to +346
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}


// General scalar value comparison (e.g., text, textarea)
const changed = currentValue !== initialValue;

return changed;
});
};



return (
<div className="application-summary" ref={ref}>
<ErrorSummary />
Expand All @@ -229,8 +378,10 @@ const ApplicationSummary = () => {
<tbody>
{
subsections.map(key => {

const subsection = section.subsections[key];
const fields = Object.values(fieldsBySection[key] || []);

if (key === 'protocols') {
fields.push('reusableSteps');
}
Expand All @@ -245,7 +396,10 @@ const ApplicationSummary = () => {
</td>
<td className="controls">
<Comments subsection={key} />
<ChangedBadge fields={fields} />
{

hasChangedFields(fields, values, project.initialValues || {}) && <ChangedBadge fields={fields} />
}
<CompleteBadge isComplete={isComplete(subsection, key)} />
</td>
</tr>;
Expand All @@ -266,4 +420,4 @@ const ApplicationSummary = () => {

};

export default ApplicationSummary;
export default ApplicationSummary;
8 changes: 4 additions & 4 deletions client/components/checkbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ const NtsCheckBoxWithModal = (props) => {
const values = [...(value || [])];
const itemRemoved = values.includes(checkboxValue);

if (itemRemoved) {
const {hasData} = hasExistingDataForCheckbox(project, checkboxValue);
const {hasData} = hasExistingDataForCheckbox(project, checkboxValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mohomeoffice - I have commented on the PR you closed but this is still not addressed. Could you please check all comments from other PR?

I don't see a reason why hasData scope has changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you revert this, there is no change in this file and can be reverted to master.


if (itemRemoved) {
if (hasData) {
// If there's existing data, show the modal
setShowModal(true);
Expand Down Expand Up @@ -54,7 +54,7 @@ const NtsCheckBoxWithModal = (props) => {
};
const customTextMap = {
'set-free': 'Set free',
'kept-alive': 'Kept alive at a licensed establishment for non-regulated purposes or possible reuse',
'kept-alive': 'Kept alive for non-regulated purposes or possible reuse',
'used-in-other-projects': 'Continued use on other projects',
'killed': 'Killed',
'rehomed': 'Rehomed'
Expand Down Expand Up @@ -104,4 +104,4 @@ const NtsCheckBoxWithModal = (props) => {
);
};

export default NtsCheckBoxWithModal;
export default NtsCheckBoxWithModal;
2 changes: 1 addition & 1 deletion client/components/editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,4 @@ class TextEditor extends Component {
}
}

export default connect(null, { throwError })(TextEditor);
export default connect(null, { throwError })(TextEditor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change in this file, please revert it to master.

2 changes: 1 addition & 1 deletion client/components/review-field.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,4 @@ const mapStateToProps = ({ project, settings }, props) => {
};
};

export default connect(mapStateToProps)(ReviewField);
export default connect(mapStateToProps)(ReviewField);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change in this file, please revert it to master.

Loading