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

exclude default categories from dataElements #959

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

eperedo
Copy link
Contributor

@eperedo eperedo commented Jun 8, 2024

📌 References

📝 Implementation

In DHIS 2.38.6 the defaults parameter is not working in the metadata endpoint
/api/metadata.json?fields=id&filter=id:eq:data_element_id&categoryCombo&defaults=INCLUDE/EXCLUDE;
so I'm manually removing the default categoryCombo from dataElements/dataSets

📹 Screenshots/Screen capture

image

🔥 Is there anything the reviewer should know to test it?

📑 Others

#8694rjjmk

@eperedo eperedo requested review from MiquelAdell and xurxodev June 8, 2024 17:16
@ifoche
Copy link
Member

ifoche commented Jun 8, 2024

@eperedo eperedo force-pushed the fix/categories-default branch from c0d94af to 4767357 Compare June 9, 2024 17:30
@MiquelAdell MiquelAdell requested review from tokland and removed request for xurxodev June 10, 2024 06:47
// In DHIS 2.38.5 and above the defaults parameter is not working
// /api/metadata.json?fields=id&filter=id:eq:data_element_id&categoryCombo&defaults=INCLUDE/EXCLUDE; this is not working
// so manually removing the default categoryCombo
const elementWithoutDefaults =
Copy link
Contributor

@tokland tokland Jun 10, 2024

Choose a reason for hiding this comment

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

A use case should not make any reference to DHIS2 (that's an implementation detail). If DHIS2 has a bug (Indeed, I've checked it: 2.36 OK, 2.38 FAIL), any extra processing should be done in the data layer, in the DHIS2 repository implementation (check any file which calls the API with that option defaults: boolean -> apart from the metadata repo, I've found also utils/synchronization.ts; check if more)

Note: checking dhis2-core, DefaultCategoryService.java, the models that have a default object are the 4 expected: category, categoryOption, categoryCombo, categoryOptionCombo.

@eperedo eperedo requested a review from tokland June 10, 2024 14:37
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

[code-only review] All good. I'd just move the file (not a major issue)

@@ -22,3 +25,47 @@ export function getD2APiFromInstance(instance: Instance) {
*/
return new D2Api({ baseUrl: instance.url, auth: instance.auth, backend: "fetch" });
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the file to data/ so it's more CleanArch compliant.

@MiquelAdell MiquelAdell requested a review from tokland June 12, 2024 08:37
@MiquelAdell
Copy link
Contributor

I guess @eperedo commit fixes @tokland request, does it? I've re-requested the review from Arnau

Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

Nothing more to add, it's ok by me now.

@MiquelAdell MiquelAdell merged commit 8f9a7ff into development Jun 12, 2024
1 check passed
@MiquelAdell MiquelAdell deleted the fix/categories-default branch June 12, 2024 09:25
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