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: KHP3-7296-Make relationship person middle name optional and remove notes from relationship payload #547

Conversation

Omoshlawi
Copy link
Collaborator

…emoed notes from relationship payload preventing error

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide.
  • I checked for feature overlap with existing widgets.

Summary

Screenshots

Screenshot 2025-01-15 at 08 13 47

None.

Related Issue

https://thepalladiumgroup.atlassian.net/browse/KHP3-7296

Other

None.

…emoed notes from relationship payload preventing error

const onSubmit: SubmitHandler<FormData> = async (data) => {
try {
await saveRelationship(data, config, session, []);
await saveRelationship({ ...data, ...({ notes: undefined } as any) }, config, session, []); /// Remove notes from payload since endpoint doesnt espect it avoid 400 error
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a typo here

{...field}
invalid={error?.message}
invalidText={error?.message}
className={styles.billingItem}>
Copy link
Contributor

Choose a reason for hiding this comment

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

the css classname here indicates this is a billItem should that be the case

@@ -75,7 +75,7 @@ export const relationshipFormSchema = z.object({
personBInfo: z
.object({
givenName: z.string().min(1, 'Required'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have the error more descriptive e.g Give name is a required field same for familyName field

…move unnecessary notes from relationship payload
@Omoshlawi Omoshlawi requested a review from donaldkibet January 15, 2025 09:52
@donaldkibet
Copy link
Contributor

Screenshot 2025-01-17 at 09 56 24

Fix the lint warning

donaldkibet
donaldkibet previously approved these changes Jan 17, 2025
Copy link
Contributor

@donaldkibet donaldkibet left a comment

Choose a reason for hiding this comment

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

LGTM Merge in once you have fixed the lint errors


const onSubmit: SubmitHandler<FormData> = async (data) => {
try {
await saveRelationship(data, config, session, []);
await saveRelationship({ ...data, ...({ notes: undefined } as any) }, config, session, []); /// Remove notes from payload since endpoint doesn't expect it to avoid 400 error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Additional notes is not supported by relationships model. Why would you still retain the same field on the form @Omoshlawi ? Is this not misleading the users? I thought we should also remove the field from the form.

Copy link
Collaborator Author

@Omoshlawi Omoshlawi Jan 22, 2025

Choose a reason for hiding this comment

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

I was not the one who implemented the other and family relationship initially so I had no Idea why the field was added by initially implementor.But I have removed it completely now

Copy link
Collaborator

@makombe makombe left a comment

Choose a reason for hiding this comment

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

Thanks @Omoshlawi

@makombe makombe merged commit 4fe7fd6 into palladiumkenya:main Jan 23, 2025
4 checks passed
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.

3 participants