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.

Sign up for GitHub

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

PSP-9471: UI UX Clean Up - Contact – Manage Contacts: Allow to save contacts that are set as Inactive without Contact Methods #4552

Merged
merged 18 commits into from
Dec 30, 2024
Merged

PSP-9471: UI UX Clean Up - Contact – Manage Contacts: Allow to save contacts that are set as Inactive without Contact Methods #4552

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
63df019
PSP-9471 fix
stairaku Dec 23, 2024
7c580e6
PSP-9471 unit test
stairaku Dec 23, 2024
eef09b5
updates
stairaku Dec 24, 2024
1f6c9bf
Merge branch 'PSP-9471' of https://github.com/stairaku/PSP into PSP-9471
stairaku Dec 24, 2024
18d9c21
update unit tests
stairaku Dec 24, 2024
914c15e
Merge branch 'dev' into PSP-9471
stairaku Dec 24, 2024
24572ab
Deleting unneccesary imports
stairaku Dec 24, 2024
991b36d
Deleting unneccesary imports
stairaku Dec 24, 2024
acb5b41
Merge branch 'PSP-9471' of https://github.com/stairaku/PSP into PSP-9471
stairaku Dec 24, 2024
7f3641e
Merge branch 'dev' into PSP-9471
stairaku Dec 24, 2024
a51985c
Merge branch 'bcgov:dev' into PSP-9471
stairaku Dec 27, 2024
799ada0
Adding logic on organizations to cover changes on status field
stairaku Dec 27, 2024
e06fc4c
Merge branch 'PSP-9471' of https://github.com/stairaku/PSP into PSP-9471
stairaku Dec 27, 2024
350ae84
Merge branch 'bcgov:dev' into PSP-9471
stairaku Dec 30, 2024
c49de85
changes on organization and person status logic
stairaku Dec 30, 2024
b5b7326
Merge branch 'PSP-9471' of https://github.com/stairaku/PSP into PSP-9471
stairaku Dec 30, 2024
ce0c73f
Changes on contact models to convert isDisabled to string from Api to…
stairaku Dec 30, 2024
6da46e7
Changes on contact models to convert isDisabled to string from Api to…
stairaku Dec 30, 2024
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
6 changes: 3 additions & 3 deletions .../frontend/src/features/contacts/contact/edit/Organization/UpdateOrganizationForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ describe('UpdateOrganizationForm', () => {
const { getSaveButton } = setup({ id: 1 });
const save = getSaveButton();
await act(async () => userEvent.click(save));
expect(updateOrganization).toBeCalledWith(mockOrganization);
expect(updateOrganization).toHaveBeenCalledWith(mockOrganization);
});

it('should save the organization with new values', async () => {
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('UpdateOrganizationForm', () => {
const save = getSaveButton();
await act(async () => userEvent.click(save));

expect(updateOrganization).toBeCalledWith(newValues);
expect(updateOrganization).toHaveBeenCalledWith(newValues);
});

it(`should save the form with address information when 'Other' country selected and no province is supplied`, async () => {
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('UpdateOrganizationForm', () => {
const save = getSaveButton();
await act(async () => userEvent.click(save));

expect(updateOrganization).toBeCalledWith(newValues);
expect(updateOrganization).toHaveBeenCalledWith(newValues);
});
});
});
6 changes: 5 additions & 1 deletion source/frontend/src/features/contacts/contact/edit/Organization/UpdateOrganizationForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,16 @@ const UpdateOrganization: React.FC<FormikProps<IEditableOrganizationForm>> = ({

const organizationId = getIn(values, 'id');
const persons = getIn(values, 'persons') as Partial<ApiGen_Concepts_Person>[];
const isOrganizationDisabled = getIn(values, 'isDisabled');

const onCancel = () => {
history.push(`/contact/O${organizationId}`);
};

const isContactMethodInvalid = useMemo(() => {
if (isOrganizationDisabled === 'true') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels weird that this should be a boolean but the rigth hand operator is a string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let me try with with booleans. I see the tranform method when it is saved and send to the API but not before that.

return false;
}
return (
!!touched.phoneContactMethods &&
!!touched.emailContactMethods &&
Expand All @@ -97,7 +101,7 @@ const UpdateOrganization: React.FC<FormikProps<IEditableOrganizationForm>> = ({
!!touched.billingAddress?.streetAddress1) &&
getIn(errors, 'needsContactMethod')
);
}, [touched, errors]);
}, [touched, errors, isOrganizationDisabled]);

const checkState = useCallback(() => {
return dirty && !isSubmitting;
Expand Down
4 changes: 2 additions & 2 deletions source/frontend/src/features/contacts/contact/edit/Person/UpdatePersonForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ describe('UpdatePersonForm', () => {

const expectedPerson = { ...mockPerson };
expectedPerson!.personOrganizations![0].organization = null;
expect(updatePerson).toBeCalledWith(expectedPerson);
expect(updatePerson).toHaveBeenCalledWith(expectedPerson);
});

it('should save the form with updated values', async () => {
Expand Down Expand Up @@ -287,7 +287,7 @@ describe('UpdatePersonForm', () => {
const save = getSaveButton();
await act(async () => userEvent.click(save));

expect(updatePerson).toBeCalledWith(newValues);
expect(updatePerson).toHaveBeenCalledWith(newValues);
});

it(`should save the form with address information when 'Other' country selected and no province is supplied`, async () => {
Expand Down
6 changes: 5 additions & 1 deletion source/frontend/src/features/contacts/contact/edit/Person/UpdatePersonForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const UpdatePersonComponent: React.FC<
> = ({ values, errors, touched, dirty, submitForm, setFieldValue, isSubmitting }) => {
const history = useHistory();
const { getOrganization } = useApiContacts();
const isPersonDisabled = getIn(values, 'isDisabled');

const personId = getIn(values, 'id');
const organizationId = getIn(values, 'organization.id');
Expand All @@ -90,6 +91,9 @@ const UpdatePersonComponent: React.FC<
};

const isContactMethodInvalid = useMemo(() => {
if (isPersonDisabled === 'true') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

one is a boolean, the right hand is a string. probably need to check the right type?

return false;
}
return (
!!touched.phoneContactMethods &&
!!touched.emailContactMethods &&
Expand All @@ -98,7 +102,7 @@ const UpdatePersonComponent: React.FC<
!!touched.billingAddress?.streetAddress1) &&
getIn(errors, 'needsContactMethod')
);
}, [touched, errors]);
}, [touched, errors, isPersonDisabled]);

// update mailing address sub-form when "useOrganizationAddress" checkbox is toggled
useEffect(() => {
Expand Down
18 changes: 14 additions & 4 deletions source/frontend/src/features/contacts/contact/utils/contactUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ export const onValidateOrganization = (
const errors = {} as any;
try {
// combine yup schema validation with custom rules
if (!hasEmail(values) && !hasPhoneNumber(values) && !hasAddress(values)) {
if (
!hasEmail(values) &&
!hasPhoneNumber(values) &&
!hasAddress(values) &&
(values.isDisabled === 'false' || values.isDisabled === false)
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you follow the pattern I described in my other comment, then here you would only need to check for the string "false"

errors.needsContactMethod =
'Contacts must have a minimum of one method of contact to be saved. (ex: email,phone or address)';
'Contacts must have a minimum of one method of contact to be saved. (ex: email, phone or address)';
}
validateYupSchema(values, OrganizationValidationSchema, true, {
otherCountry: otherCountryId,
Expand All @@ -40,9 +45,14 @@ export const onValidatePerson = (values: IEditablePersonForm, otherCountryId?: s
const errors = {} as any;
try {
// combine yup schema validation with custom rules
if (!hasEmail(values) && !hasPhoneNumber(values) && !hasAddress(values)) {
if (
!hasEmail(values) &&
!hasPhoneNumber(values) &&
!hasAddress(values) &&
(values.isDisabled === 'false' || values.isDisabled === false)
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment here re: checking for the string representation value

errors.needsContactMethod =
'Contacts must have a minimum of one method of contact to be saved. (ex: email,phone or address)';
'Contacts must have a minimum of one method of contact to be saved. (ex: email, phone or address)';
}
validateYupSchema(values, PersonValidationSchema, true, { otherCountry: otherCountryId });

Expand Down
2 changes: 1 addition & 1 deletion source/frontend/src/features/contacts/formModels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export class IEditableOrganizationForm {
alias?: string;
incorporationNumber?: string;
comment?: string;
isDisabled: boolean;
isDisabled: string | boolean = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the comments and lead me to a better solution. I'm updating this PR with the suggested changes. Thank you!!!!

persons: Partial<ApiGen_Concepts_Person>[];
emailContactMethods: IEditableContactMethodForm[];
phoneContactMethods: IEditableContactMethodForm[];
Expand Down
Loading