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

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
Changes from 6 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
Original file line number Diff line number Diff line change
@@ -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 () => {
@@ -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 () => {
@@ -201,7 +201,7 @@ describe('UpdateOrganizationForm', () => {
const save = getSaveButton();
await act(async () => userEvent.click(save));

expect(updateOrganization).toBeCalledWith(newValues);
expect(updateOrganization).toHaveBeenCalledWith(newValues);
});
});
});
Original file line number Diff line number Diff line change
@@ -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) {
return false;
}
return (
!!touched.phoneContactMethods &&
!!touched.emailContactMethods &&
@@ -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;
Original file line number Diff line number Diff line change
@@ -13,7 +13,15 @@ import { ApiGen_Concepts_Organization } from '@/models/api/generated/ApiGen_Conc
import { ApiGen_Concepts_Person } from '@/models/api/generated/ApiGen_Concepts_Person';
import { ApiGen_Concepts_PersonAddress } from '@/models/api/generated/ApiGen_Concepts_PersonAddress';
import { lookupCodesSlice } from '@/store/slices/lookupCodes';
import { act, fillInput, render, RenderOptions, userEvent } from '@/utils/test-utils';
import {
act,
fillInput,
findByText,
queryByText,
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure we are using these 2 imports in the test file here...

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 check and will update my PR

render,
RenderOptions,
userEvent,
} from '@/utils/test-utils';

import UpdatePersonForm from './UpdatePersonForm';
import { ApiGen_CodeTypes_AddressUsageTypes } from '@/models/api/generated/ApiGen_CodeTypes_AddressUsageTypes';
@@ -241,7 +249,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 () => {
@@ -287,7 +295,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 () => {
Original file line number Diff line number Diff line change
@@ -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');
@@ -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 &&
@@ -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(() => {
Original file line number Diff line number Diff line change
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

) {
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,
@@ -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 });

Loading