PSP-9471: UI UX Clean Up - Contact – Manage Contacts: Allow to save contacts that are set as Inactive without Contact Methods #4552
Conversation
| findByText, | ||
| queryByText, |
There was a problem hiding this comment.
not sure we are using these 2 imports in the test file here...
There was a problem hiding this comment.
Let me check and will update my PR
| !hasEmail(values) && | ||
| !hasPhoneNumber(values) && | ||
| !hasAddress(values) && | ||
| values.isDisabled === false |
There was a problem hiding this comment.
the validatePerson() method below has the following logic:
if (
... &&
(values.isDisabled === 'false' || values.isDisabled === false)
) {
...
}But the validateOrganization() has a different logic: values.isDisabled === false
wouldn't it make sense to have the same check for disabled for person and organization?
There was a problem hiding this comment.
I agree, but I'll have to do a refactor on the logic we have now on contacts. I'll give it a go and update my PR.
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
1 similar comment
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
1 similar comment
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
|
||
| const onCancel = () => { | ||
| history.push(`/contact/O${organizationId}`); | ||
| }; | ||
|
|
||
| const isContactMethodInvalid = useMemo(() => { | ||
| if (isOrganizationDisabled === 'true') { |
There was a problem hiding this comment.
Feels weird that this should be a boolean but the rigth hand operator is a string?
There was a problem hiding this comment.
This is because Formik values for dropdowns (SELECT input field) are always strings. We have several utility methods to convert from "stringy" booleans to "real" booleans
There was a problem hiding this comment.
let me try with with booleans. I see the tranform method when it is saved and send to the API but not before that.
| @@ -90,6 +91,9 @@ const UpdatePersonComponent: React.FC< | |||
| }; | |||
|
|
|||
| const isContactMethodInvalid = useMemo(() => { | |||
| if (isPersonDisabled === 'true') { | |||
There was a problem hiding this comment.
one is a boolean, the right hand is a string. probably need to check the right type?
| @@ -183,7 +183,7 @@ export class IEditableOrganizationForm { | |||
| alias?: string; | |||
| incorporationNumber?: string; | |||
| comment?: string; | |||
| isDisabled: boolean; | |||
| isDisabled: string | boolean = false; | |||
There was a problem hiding this comment.
This field should always be string. Yes, it will contain string values of "true" or "false" but those are string representations of boolean values. We need this field to be a string because it is used in Formik to render the "disabled" dropdown (SELECT input) which always takes string values (so no boolean values are accepted for select)
The pattern we follow is to use the utility method booleanToString() to convert from the API value (which is boolean) into the string representation that the dropdown can render. We do this in the fromApi() method.
Then in the corresponding toApi() method we call the "inverse" function - stringToBoolean() to convert the string representation from formik into the API boolean value.
From what I see the code in contact is doing this half-way. The "toApi" is correctly converting from string to boolean. BUT the "fromApi" is missing the conversion from a boolean. The solution is to make "isDisabled" here a string and follow the pattern from "UpdatePropertyDetailsFormModel.ts" below:
There was a problem hiding this comment.
Thank you for the comments and lead me to a better solution. I'm updating this PR with the suggested changes. Thank you!!!!
| (values.isDisabled === 'false' || values.isDisabled === false) | ||
| ) { |
There was a problem hiding this comment.
if you follow the pattern I described in my other comment, then here you would only need to check for the string "false"
| (values.isDisabled === 'false' || values.isDisabled === false) | ||
| ) { |
There was a problem hiding this comment.
same comment here re: checking for the string representation value
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4552 |
No description provided.