-
Notifications
You must be signed in to change notification settings - Fork 357
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
refactor: Option dimension in Visualization [DHIS2-18370] #19725
base: master
Are you sure you want to change the base?
Conversation
...pi/src/main/java/org/hisp/dhis/program/ProgramTrackedEntityAttributeOptionDimensionItem.java
Fixed
Show fixed
Hide fixed
dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/ProgramDataElementOptionDimensionItem.java
Show resolved
Hide resolved
...pi/src/main/java/org/hisp/dhis/program/ProgramTrackedEntityAttributeOptionDimensionItem.java
Show resolved
Hide resolved
isNotBlank(aggregationId) ? Aggregation.valueOf(aggregationId) : AGGREGATED; | ||
|
||
if (program == null || attribute == null || option == null) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is the convention, but wouldn't be better to return Option< ProgramTrackedEntityAttributeOptionDimensionItem>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I applied the same convention from all other existing methods. It can be refactored all together later if this is the case.
Quality Gate passedIssues Measures |
Making the option set / item adherent to the existing model objects.
Before this change, the JSON request/response would look like:
Even though, it looks less concise (from the representation point of view) it follows the same standard of existing dimensions. After a conversation with @larshelge , this the preferred way.