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

dhis2: Update tracked entities endpoints to V42 #819

Merged
merged 41 commits into from
Nov 28, 2024

Conversation

hunterachieng
Copy link
Contributor

@hunterachieng hunterachieng commented Nov 11, 2024

Summary

Update tracked entities endpoints to the new v42.

Fixes #754

Details

Updates create, update, upsert and destroy so that resources are mapped to the new tracker endpoints.

HTTP helpers are not affected.

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 <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
---
'@openfn/language-dhis2': patch
---
Removed support for DHIS2 v42
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changeset needs updating. Is this the old changeset from the last fixes?

Anyway this needs to be a major update

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this changelog doesn't apply it needs to be removed, because it'll create an inaccurate release note and confuse users

packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
.changeset/breezy-tigers-attend.md Outdated Show resolved Hide resolved
---
'@openfn/language-dhis2': patch
---
Removed support for DHIS2 v42
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this changelog doesn't apply it needs to be removed, because it'll create an inaccurate release note and confuse users

packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
packages/dhis2/src/Adaptor.js Show resolved Hide resolved
packages/dhis2/test/index.test.js Show resolved Hide resolved
packages/dhis2/test/index.test.js Outdated Show resolved Hide resolved
packages/dhis2/test/index.test.js Outdated Show resolved Hide resolved
@josephjclark josephjclark marked this pull request as ready for review November 14, 2024 14:36
Signed-off-by: Hunter Achieng <[email protected]>
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.

I think we're about there @hunterachieng. Sorry this is late, and I won't be able to get to it now until Wednesday.

I'm a bit worried about some of the docs. Big docs changes are out of scope for this work but I also think there are some innaccuracies.

Unless anyone else volunteers before Wednesday, I'll do a light sweep of the docs, do a final check, and we can release.

@@ -41,6 +41,11 @@ export function execute(...operations) {
`WARNING: This adaptor is incompatible with DHIS2 API version 42+. See here: https://docs.dhis2.org/en/develop/using-the-api/dhis-core-version-master/tracker.html.`
);

if (+version < 42)
console.warn(
`WARNING: This adaptor is incompatible with DHIS2 tracker API version less than 42. See here: https://docs.dhis2.org/en/develop/using-the-api/dhis-core-version-master/tracker.html.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think version 36+ is OK?

The new tracker API, which we use for eg create, was introduced in v36. So it should work with anything from 36+.

Also, please remove the trailing fullstop at the end of the URL. It's unnecessary and will break the URL if someone accidentally copies it into the address bar

packages/dhis2/src/Adaptor.js Outdated Show resolved Hide resolved
@josephjclark
Copy link
Collaborator

@hunterachieng have you called the actual DHIS2 sandbox with the new APIs, or have you only tested against unit tests?

@hunterachieng
Copy link
Contributor Author

@hunterachieng have you called the actual DHIS2 sandbox with the new APIs, or have you only tested against unit tests?

@josephjclark Yes I did

@josephjclark
Copy link
Collaborator

@hunterachieng the PR body is very out of date with the changes here, which is a big problem for audit trails and working in the open.

I'm going to start getting more strict about this - it's super important that PRs actually do what they say they do when they are merged.

I've updated the description to be more accurate.

Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
@josephjclark
Copy link
Collaborator

TODO: remove all support for trackedEntityInstances - ie, we should NOT map it to the new tracker, and we should have examples or unit tests which use this key.

This is because the datastructure for an entity on the new tracker is different to the datastructure to the old one. It's not worth pretending to maintain support.

@josephjclark
Copy link
Collaborator

TODO: nestArray does NOT nest new tracker objects into the right format. In the create etc we need to ensure the data is in an array. Use util.ensureArray

@josephjclark
Copy link
Collaborator

TODO: on the new tracker, we need to pass async: false so that the operation completes synchronously and returns data to us.

TODO: raise an issue to later support async: true as an option. This is kind of a big deal because we need to poll the endpoints and then return data. We need to make this seamless so that we just return the completed data to the job code.

@mtuchi
Copy link
Collaborator

mtuchi commented Nov 26, 2024

Hiya @josephjclark, please give this another round

@mtuchi mtuchi requested a review from josephjclark November 26, 2024 14:31
@mtuchi mtuchi requested a review from josephjclark November 27, 2024 17:11
@josephjclark josephjclark merged commit a1f5722 into main Nov 28, 2024
2 checks passed
@josephjclark josephjclark deleted the feat/754-dhis2-update branch November 28, 2024 12:33
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.

DHIS2 new tracker api
3 participants