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) make visit information available within expression context #269

Merged
merged 9 commits into from
May 23, 2024

Conversation

arodidev
Copy link
Contributor

@arodidev arodidev commented May 13, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR aims to provide visit information (visitTypeUuid) to the expression context to be used for expression evaluation.

Screenshots

Related Issue

Other

@arodidev arodidev changed the title (refactor) fetching visit within the component (refactor) fetching visit within the root component May 13, 2024
@arodidev arodidev requested a review from samuelmale May 13, 2024 20:50
@samuelmale samuelmale requested a review from ibacher May 13, 2024 21:10
@@ -42,7 +42,7 @@ interface EncounterFormProps {
formSessionDate: Date;
provider: string;
location: SessionLocation;
visit?: Visit;
visit: Visit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we now making the visit mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since it is now not being passed as a prop but rather being fetched within the root component, it wouldn't make sense to have it as optional anymore yet it is always available.

visit={visit}
/>,
);
render(<FormEngine formJson={formJson} formUUID={formUUID} patientUUID={patientUUID} formSessionIntent={intent} />);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if its RDE? How do you know the visit to use if the visit uuid or visit is not passed as a prop?

Copy link
Contributor Author

@arodidev arodidev May 15, 2024

Choose a reason for hiding this comment

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

I am not entirely sure what RDE is, please explain. For the second part, currently I am picking the current visit.

@samuelmale
Copy link
Member

@arodidev does it makes sense to consider investigating why we end up with an undefined visit assuming the form is being launched from the workspace? I'm just worried of the potential performance implications unless the underlying hook is optimised. cc: @ibacher

@arodidev
Copy link
Contributor Author

why we end up with an undefined visit assuming the form is being launched from the workspace?

@samuelmale This doesn't seem to be the case from my side.

Screenshot 2024-05-15 at 21 49 58

Screenshot 2024-05-15 at 21 51 38

Could you show how to replicate your findings above?

@samuelmale
Copy link
Member

@arodidev I didn't understand why it is necessary for the form-engine to load the visit by itself; what are the issues with the current setup?

@arodidev
Copy link
Contributor Author

@arodidev I didn't understand why it is necessary for the form-engine to load the visit by itself; what are the issues with the current setup?

Well, considering we are getting the visit from the useVisit hook, deprecating the other one was just a means of removing redundancy. Should we maintain both?

@ibacher
Copy link
Member

ibacher commented May 16, 2024

Ideally, I think the form engine wouldn't be calling useVisit(), but have the visit passed in. Adding it here is essentially adding a redundant API call. Both visit and encounter should be determined from the context the form is being entered in, rather than by the form engine itself, so it's reasonable to expect visit and (optionally) encounter to be passed as props.

@arodidev arodidev changed the title (refactor) fetching visit within the root component (refactor) make visit information available within expression context May 18, 2024
@arodidev arodidev changed the title (refactor) make visit information available within expression context (fix) make visit information available within expression context May 18, 2024
@@ -60,6 +64,7 @@ export function evaluateExpression(
sex,
age,
HD,
visitTypeUuid,
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add this to the evaluateAsyncExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuelmale samuelmale merged commit b5fa586 into openmrs:main May 23, 2024
4 checks passed
vasharma05 pushed a commit that referenced this pull request May 27, 2024
* MVP

* Cleanup

* Adapt tests to new format

* revert to visit as a prop

* remove unnecessary visit

* expand context for async-exp
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.

4 participants