Feature/change management draft (Test) #974
Conversation
…changed flag only for genuine modifications.
mohomeoffice
requested review from
jeff-horton-ho-sas,
kiran-varma-home-office,
farrukhishaq and
parvinba
There was a problem hiding this comment.
There are a number of changes I'm recommending, but overall this is good. Key things to focus on:
- Get the redux state using the mechanisms provided by redux/react so that changes to the state are correctly synced to the components and update the UI. This will also help with testing as the state is then passed into the change diff functions, so a mock of redux doesn't need to be provided
- The test cases should be automated
- Review the comments to make sure they're adding context and not merely repeating what the code is already stating
Also it looks like there is a bug in eslint causing the linting to fail (https://drone-gh.acp.homeoffice.gov.uk/UKHomeOffice/asl-projects/4771/1/3). It looks like this might be fixed in more recent versions eslint/eslint#14172
| // Compare values and log debugging information | ||
| console.log(`Field: ${key}`); | ||
| console.log(` Saved: ${sanitizedSaved}`); | ||
| console.log(` Current: ${sanitizedCurrent}`); | ||
| console.log(` Initial: ${sanitizedInitial}`); |
There was a problem hiding this comment.
This would be more useful as a single log entry, with a label for where it is from. Something like:
| // Compare values and log debugging information | |
| console.log(`Field: ${key}`); | |
| console.log(` Saved: ${sanitizedSaved}`); | |
| console.log(` Current: ${sanitizedCurrent}`); | |
| console.log(` Initial: ${sanitizedInitial}`); | |
| console.log('compareFields debug info', {key, sanitizedSaved, sanitizedCurrent, sanitizedInitial}); |
Same for other logs.
There was a problem hiding this comment.
even as a single log entry, would this not add noice to overall logs?
How do we know which project/license fields we are comparing here?
Does console.log show nicely in our montoring tools? If we really think this logging will add value to investigation there are logging libraries which enhances logging!
| /** | ||
| * 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 normalizeValue(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. | ||
| } | ||
|
|
||
| /** | ||
| * Sanitise a given value by normalising it and removing double quotes. | ||
| * | ||
| * - First, the value is normalised using `normalizeValue`, 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 sanitise. | ||
| * @returns {string} - A sanitised string with double quotes removed. | ||
| */ | ||
| function sanitizeValue(value) { | ||
| return normalizeValue(value).replace(/["]+/g, ""); // Remove all double quotes from the normalised value. | ||
| } |
There was a problem hiding this comment.
sanitizeValueimplies this is making the input safe from injection attacks, which this doesn't do. I think including this in the normalise function would be better.- 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 toJSON.stringify(value).replace(/["]+/g, "").trim();so we can unify the two branches.value === null || value === undefinedis equivalent tovalue == nullwhich is the more idiomatic way of doing that check.
| /** | |
| * 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 normalizeValue(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. | |
| } | |
| /** | |
| * Sanitise a given value by normalising it and removing double quotes. | |
| * | |
| * - First, the value is normalised using `normalizeValue`, 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 sanitise. | |
| * @returns {string} - A sanitised string with double quotes removed. | |
| */ | |
| function sanitizeValue(value) { | |
| return normalizeValue(value).replace(/["]+/g, ""); // Remove all double quotes from the normalised value. | |
| } | |
| /** | |
| * 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(); | |
| } |
There was a problem hiding this comment.
After jeff's suggestion, please move it to some common place. It doesn't seem to belong in this file.
There was a problem hiding this comment.
Test cases should be automated and live in https://github.com/UKHomeOffice/asl-projects/tree/master/test/specs.
| // console.log(`Field Name: ${name}`); | ||
| // console.log(`Stored Value:`, adjustedStoredValue); | ||
| // console.log(`Actual Current Value:`, actualCurrentValue); | ||
| // console.log(`Normalized Stored Value:`, normalizedStoredValue); | ||
| // console.log(`Normalized Current Value:`, normalizedCurrentValue); | ||
| // console.log(`Value for this field ${name}:`, this.props.values[name]); |
There was a problem hiding this comment.
| // console.log(`Field Name: ${name}`); | |
| // console.log(`Stored Value:`, adjustedStoredValue); | |
| // console.log(`Actual Current Value:`, actualCurrentValue); | |
| // console.log(`Normalized Stored Value:`, normalizedStoredValue); | |
| // console.log(`Normalized Current Value:`, normalizedCurrentValue); | |
| // console.log(`Value for this field ${name}:`, this.props.values[name]); |
| // console.log(`Field Name: ${fieldName}`); | ||
| // console.log(`Stored Value:`, speciesStoredValue); | ||
| // console.log(`Current Value:`, speciesCurrentValue); |
There was a problem hiding this comment.
| // console.log(`Field Name: ${fieldName}`); | |
| // console.log(`Stored Value:`, speciesStoredValue); | |
| // console.log(`Current Value:`, speciesCurrentValue); |
There was a problem hiding this comment.
What is using this? I can't see anything calling it.
| this.reduxState = reduxState; | ||
| } | ||
|
|
||
| getReduxState() { |
There was a problem hiding this comment.
The idiomatic way to get redux state would be to use connect to wrap the component, or via the useSelector() hook. That way changes to the state properly cause react to rerender the components.
There was a problem hiding this comment.
please check other view components for examples.
| import { store } from '../store'; | ||
|
|
||
| class DatabaseService { | ||
| constructor(apiClient, reduxState) { |
There was a problem hiding this comment.
This constructor is never called with arguments and there's no other code that initialises the api client. This suggests:
- The redux state has always had what is needed so far in testing, so the apiClient is redundant
- If there is an edge case where the apiClient is needed then it will fail with apiClient.get is not a function
| console.log("Fields with values:", fieldsWithValues); | ||
| console.log("Fields without values:", fieldsWithoutValues); |
There was a problem hiding this comment.
Condense into one log entry
| * @param {object} data - The section or field data to process. | ||
| * @returns {boolean} - True if any field differs, false otherwise. | ||
| */ | ||
| const compareFields = (data) => { |
There was a problem hiding this comment.
This functionality is nicely extracted out 👍 . I prefer for it to be out of this file and tests added
|
|
||
|
|
||
|
|
||
| const hasChangedFields = (fields, currentValues, initialValues) => { |
There was a problem hiding this comment.
again my preference is for this function to be out of this file and tests added
| @@ -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); | |||
There was a problem hiding this comment.
not sure I understand reason to move it outside the scope where its used !?
|
|
||
| class Review extends React.Component { | ||
|
|
||
| hasDatabaseChange() { | ||
| const normalizeValue = value => { |
There was a problem hiding this comment.
is this normalizeValue different to normalizeValue function in other file? there seems to be some common checks? could we reuse?
| return value; // Return all other values as-is | ||
| }; | ||
|
|
||
| const normalizeDuration = value => { |
There was a problem hiding this comment.
could we maybe put all duration logic in 1 place please (if possible)? We have similar logic in calculate-expiry-date.js i think.
| const rawValue = field.name.includes('.') | ||
| ? field.name.split('.').reduce((acc, key) => acc?.[key], values) | ||
| : values?.[field.name]; |
There was a problem hiding this comment.
could you please give me an example of field object?
Please do not approve this PR
Change Management Summary :
Change Detection:
Detects changes at all levels (field, sub-field, sub-subsection, subsection, section).
Handles all field types (text, text area, radio, select, true/false, duration, int, etc.).
Handles cases like conditional fields, read-only fields, and system-generated fields.
Handles detecting changes on sections, sub section, sub sub sections
Handles detecting changes on protocol, protocl sections, protocl sub section, protocl sub sub sections
Handles detecting changes on steps
Changed Badge Display:
Shows badges at all levels when changes are detected.
Removes badges when changes are reverted.
Persists badges across saves and sessions.
Clears badges after approval or rejection.
Comparison Functionality:
Test Cases :
Change Detection at Field Level
Test Case: Verify that the system detects changes to a text field when its value is modified from empty to non-empty, default to non-default, or existing value to a new value.
Test Case: Verify that the system detects changes to a text area field when its content is modified, including additions, deletions, or updates.
Test Case: Verify that the system detects changes to a radio button field when a different option is selected.
Test Case: Verify that the system detects changes to a select dropdown field when a different option is selected.
Test Case: Verify that the system detects changes to a true/false field when its value is toggled.
Test Case: Verify that the system detects changes to a duration field when the time value is updated.
Test Case: Verify that the system detects changes to an integer field when the numeric value is updated.
Test Case: Verify that the system detects changes to a sub-field when its value is modified, and propagates the change badge to the parent field.
Test Case: Verify that the system does not detect a change if a field’s value is updated to the same value (e.g., re-saving without modification).
Change Badge Display at Section, Subsection, and Sub-Subsection Levels
Test Case: Verify that a change badge is displayed at the field level when its value is modified.
Test Case: Verify that a change badge is displayed at the sub-field level when its value is modified, and the parent field also shows a change badge.
Test Case: Verify that a change badge is displayed at the sub-subsection level when any field or sub-field within it is modified.
Test Case: Verify that a change badge is displayed at the subsection level when any field, sub-field, or sub-subsection within it is modified.
Test Case: Verify that a change badge is displayed at the section level when any field, sub-field, subsection, or sub-subsection within it is modified.
Test Case: Verify that the change badge is removed at all levels (field, sub-field, sub-subsection, subsection, section) when a field is reverted to its original value.
Test Case: Verify that the change badge is persisted at all levels even after the application is saved and reopened.
Test Case: Verify that the change badge is cleared at all levels after the inspector approves or rejects the application.
Test Cases for Comparison and highlight
Test Case: Verify that the inspector can compare the current value of a field with:
Test Case: Verify that the inspector can view changed data with highlighted badge: