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

fix(applicant): add more logic to disable/enable save and close button #2918

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

sirtawast
Copy link
Collaborator

@sirtawast sirtawast commented Apr 5, 2024

Description ✨

Save and close button was disabled if any field on dataset employee or company was changed to an invalid value.

Logic is as follows:

Rather than trying to compare touched, a single field, against a errors data set:

  1. flatten both nested object structures to single level objects where the key is of dot notation
  2. use substring to match the touched and errored field key
  3. ... and use substring rather than equals to ignore any min, max, required, ... validation related additional data which the original errors object contains.

@EmiliaMakelaVincit if you know TypeScript any better then feel free to format the function to a more prettier one. This is the best I can do πŸ˜“

@terovirtanen
Copy link
Contributor

API branch is deployed to platta: https://helsinkilisa-pr2918.api.dev.hel.ninja/healthz πŸš€πŸš€πŸš€

@terovirtanen
Copy link
Contributor

HANDLER branch is deployed to platta: https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja πŸš€πŸš€πŸš€

@terovirtanen
Copy link
Contributor

APPLICANT branch is deployed to platta: https://helsinkilisa-ui-pr2918.dev.hel.ninja πŸš€πŸš€πŸš€

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja πŸ˜†πŸŽ‰πŸŽ‰πŸŽ‰

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://helsinkilisa-ui-pr2918.dev.hel.ninja πŸ˜†πŸŽ‰πŸŽ‰πŸŽ‰

Copy link
Collaborator

@EmiliaMakelaVincit EmiliaMakelaVincit left a comment

Choose a reason for hiding this comment

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

Functionally, seems good. Do consider using the given suggestion for the type woes before merging.

Comment on lines 4 to 25
type FlattenedObject = Record<string, string>;

function flattenObject(
obj: FlattenedObject,
parentKey = '',
result: FlattenedObject = {}
): FlattenedObject {
const appendResult = result;
Object.keys(obj).forEach((key) => {
const newKey = `${parentKey}${parentKey ? '.' : ''}${key}`;
if (
typeof obj[key] === 'object' &&
obj[key] !== null &&
!Array.isArray(obj[key])
) {
flattenObject(obj[key] as unknown as FlattenedObject, newKey, result);
} else {
appendResult[newKey] = obj[key];
}
});
return result;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The implementation is sound but the typing is iffy, yeah. The input is already assigned to FlattenedObject – assumed to be an object with only string values – which doesn't make sense since the whole point of the function is to flatten nested objects onto one level.

Here's how I would type this. The effective FormikTouched and FormikErrors types already are the same shape as the SimpleObject type given, which means the casts in the next function can also be removed altogether.

type SimpleObjectNode = SimpleObject | Array<SimpleObjectNode> | string | number | boolean | null;
type SimpleObject = { [prop: string]: SimpleObjectNode };

function isSimpleObject(a: SimpleObjectNode): a is SimpleObject {
  return typeof a === 'object' && a !== null && !Array.isArray(a);
}

function flattenObject(
  obj: SimpleObject,
  parentKey = '',
  result: { [prop: string]: Exclude<SimpleObjectNode, SimpleObject> } = {}
): SimpleObjectNode {
  const appendResult = result;
  Object.keys(obj).forEach((key) => {
    const newKey = `${parentKey}${parentKey ? '.' : ''}${key}`;
    const item = obj[key];
    if (isSimpleObject(item)) {
      flattenObject(item, newKey, result);
    } else {
      appendResult[newKey] = item;
    }
  });
  return result;
}

In our use case, where we only care about the object-ness of each leaf and the keys of the output, we could even define SimpleObjectNode as SimpleObject | unknown and I believe it would still work.

Comment on lines 38 to 40
Object.keys(flattenObject(touched as FlattenedObject)).filter((field) =>
Object.keys(flattenObject(errors as FlattenedObject)).some((errorField) =>
errorField.includes(field)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the above suggestion, these could just be passed as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, and flattenObject is being run for errors quite a few times if there are many touched fields. I doubt there's much of a reason to fear performance issues here, but the flattened objects could also be optionally assigned to variables first before this two-level lookup happens.

Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed for 'yjdh'

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed for 'yjdh'

Issues
22 New issues
7 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.4% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed for 'yjdh'

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 11, 2024

@terovirtanen
Copy link
Contributor

API branch is deployed to platta: https://helsinkilisa-pr2918.api.dev.hel.ninja/healthz πŸš€πŸš€πŸš€

@terovirtanen
Copy link
Contributor

HANDLER branch is deployed to platta: https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja πŸš€πŸš€πŸš€

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://helsinkilisa-ui-handler-pr2918.dev.hel.ninja πŸ˜†πŸŽ‰πŸŽ‰πŸŽ‰

@sirtawast sirtawast merged commit ef38ce4 into main Apr 11, 2024
6 checks passed
@sirtawast sirtawast deleted the hl-1160 branch April 11, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants