dhis2: Implement http namespace, a generic request function, and remove all callbacks

[hunterachieng]

Summary

Move all the generic functions into a http namespace, remove callbacks and add a generic request function.

Fixes #978

Details

All the functions such as post, get, and patch are moved to their own http name-spaced file to separate them from the other specific functions, and a generic request function was added as well.

All callbacks are being removed since the same behaviour can be achieved with promise chaining

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 ?
• If this PR includes breaking changes, do we need to update any jobs in
  production? Is it safe to release?
• 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?

[josephjclark]

I am having a hard time seperating "move http methods out into a clean namespace" from "fix all the problems with DHIS2 http APIs"

Docs are worrying me, base64 is worrying me, and I wonder if handleResponse needs to move into here.

I need to break here today but I'll think about this stuff more tomorrow.

It doesn't technically matter which PR fixes these problems - but it does matter what's in the next release. Dhis2 7 really needs to have a clean, well documented API. And do get to there we'll need more work.

I'll come back with a fresh brain.

[josephjclark]

ooh this is really annoying

Because we're using the http.get() operation directly, we have to feed state back into the call here.

it's an antipattern really. We certainly don't want to encourage it, and this is a big, brand-defining adaptor.

We don't have to do this in this PR, but what's the cost in using like util.request instead of http.request? Of using a function rather than an operation?

Also: if asbase64 is true, I think this stuff might be broken? Because response.data is a string and all these tests will fail 😬

[josephjclark]

Worried about these docs but they're not technically part of this PR. Let me think about it. Referenced here: #975 (comment)

[josephjclark]

@mtuchi can you help resolve this? Should we just remove patch?

[mtuchi]

Hiya @josephjclark this is what i was able to gather regarding PATCH method in DHIS2

The DHIS2 API supports the PATCH method for various metadata endpoints.

For example, you can use PATCH to:

• Update properties of metadata objects
• Add or remove items from collections
• Change domain types and value types
• Remove specific organization units from groups

When using the PATCH method with any of these endpoints, you must use the content type application/json-patch+json in your request headers as per the JSON patch specification. See DHIS2 Documentation - Partial Updates DHIS2 Documentation - Bulk Sharing

Meanwhile, you can use PUT on:

• Metadata endpoints - For updating various metadata objects like data elements, organization units, etc. DHIS2 Documentation - Metadata CRUD
• Data Store endpoints - For updating key-value pairs DHIS2 Documentation - Data Store
• Maintenance endpoints - For performing system maintenance operations DHIS2 Documentation - Maintenance
• Visualization endpoints - For updating visualizations DHIS2 Documentation - Visualizations

That being said, I have never used the API endpoints in this discovery but i guess it doesn't hurt to maintain both patch and put or if we decide to remove them we should have a http.request function which will still allow me to specify patch or put when there is a need for that

[josephjclark]

Thank you for the thorough report @mtuchi!

So this must be a JSON patch. That's interesting.

If we wanted, we could sit down a workout a nice easy JSON patch API. But we should not do that for a function that no-one is using.

How about:

• We keep the patch function
• We default the content type to application/json-patch+json
• We document that data must be in JSON Patch format and link to DHIS2 docs

That's a reasonable position to take right now. Maybe later we can make this a bit slicker.

[mtuchi]

I am happy with that, and since the link to DHIS2 docs is subjected to either metadata apis or sharing apis maybe we add an example with JSON Patch format. For example we should convert this example 👇🏽 to openfn job code

PATCH /api/dataElements/{id}

[
  {"op": "add", "path": "/name", "value": "New Name"},
  {"op": "add", "path": "/valueType", "value": "INTEGER"}
]

[josephjclark]

@hunterachieng after you've addressed my comments, you'll want to rebase this on top of #1059 (or maybe merge is easier). It might be a hard rebase so it might be easier to start again? You'll need to take a look and decide.

[josephjclark]

Hi @hunterachieng , can we please restore the get function and remove the reponse from the main get(), create(), update(), upsert() etc functions.

basically the http namespace should include response, but the other functions should not.

Thanks!

[josephjclark]

@hunterachieng something really weird has happened on this diff? The history is all messed up. I think you need to revert that merge.

[hunterachieng]

@josephjclark I have implemented the changes

[josephjclark]

@hunterachieng can you please take the http tests and split them out into a new file http.test.js? The test file is very big now and it gets a bit confusing to know which tests you're looking at.

[josephjclark]

Great, thank you @hunterachieng

I'm still confused about the main namespace get(). It just doesn't sit well with http.get().

I'm going to have to work out what to do and raise an new issue for next sprint.