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

openmrs: add a generic http request #923

Merged
merged 11 commits into from
Jan 20, 2025
Merged

Conversation

hunterachieng
Copy link
Contributor

@hunterachieng hunterachieng commented Jan 17, 2025

Summary

Implement a generic http request to allow both fhir and non-fhir requests to be made

Fixes #889

Details

Add technical details of what you've changed (and why).

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
/**
* Make a HTTP request to any OpenMRS endpoint
* @example
* request("GET","http://msf-ocg-openmrs3-dev.westeurope.cloudapp.azure.com/openmrs/ws/rest/v1/patient/d3f7e1a8-0114-4de6-914b-41a11fc8a1a8", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❗ Important! We do not want to allow absolute URLs here. Users should set state.configuration.instanceUrl

I actually think we should throw if an absolute URL is passed

Am I right in thinking that the common adaptor's request will throw if the url is absolute and baseURL has a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Common does not throw an error, but rather commcare flags with a 404 if both the instanceUrl and path are absolute urls

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@josephjclark
Copy link
Collaborator

QA note: check the built docs locally and make sure everything is OK

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@hunterachieng
Copy link
Contributor Author

@josephjclark The import method is throwing an error. I am researching for another method to reuse the typedef

@josephjclark
Copy link
Collaborator

@hunterachieng dang, that's a shame.

Don't spend much time on this. I was actually hoping the typedef would be available "globally" and you wouldn't need to do anything to be able to use it. Or do some simple namespacing.

So if you can't resolve it in less than half an hour, please just copy and paste the typedef

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@hunterachieng
Copy link
Contributor Author

@josephjclark I have tried other methods like globally but the import is the only one that would have work except its failing on build. I have just copy pasted instead

@@ -7,6 +7,7 @@ import {

/**
* Options object
* @global
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably remove this if we're not keeping it

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@hunterachieng hunterachieng marked this pull request as ready for review January 20, 2025 14:13
@josephjclark
Copy link
Collaborator

@hunterachieng can you please generate a Minor changeset for this feature?

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@hunterachieng
Copy link
Contributor Author

@hunterachieng can you please generate a Minor changeset for this feature?

Done

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Just a few adjustments please :)

packages/openmrs/src/http.js Outdated Show resolved Hide resolved
packages/openmrs/src/http.js Outdated Show resolved Hide resolved
packages/openmrs/src/http.js Outdated Show resolved Hide resolved
packages/openmrs/test/index.test.js Outdated Show resolved Hide resolved
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
@josephjclark josephjclark merged commit ed4ec04 into main Jan 20, 2025
2 checks passed
@josephjclark josephjclark deleted the feature/openmrs-rework-889 branch January 20, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Phase 1: Add generic HTTP request
2 participants