-
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
Metadata for user orgunits in analytics response [2.41-dhis2-15933] #15451
Conversation
...ice-analytics/src/main/java/org/hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java
Fixed
Show fixed
Hide fixed
...ice-analytics/src/main/java/org/hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java
Fixed
Show fixed
Hide fixed
Codecov Report
@@ Coverage Diff @@
## master #15451 +/- ##
============================================
+ Coverage 66.10% 66.13% +0.02%
- Complexity 31115 31138 +23
============================================
Files 3484 3486 +2
Lines 129448 129525 +77
Branches 15095 15104 +9
============================================
+ Hits 85577 85655 +78
+ Misses 36792 36788 -4
- Partials 7079 7082 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
import org.hisp.dhis.common.EventsAnalyticsQueryCriteria; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class OrganisationUnitCriteriaUtilsTest { |
Check notice
Code scanning / CodeQL
Unused classes and interfaces Note test
dhis-2/dhis-api/src/main/java/org/hisp/dhis/analytics/AnalyticsMetaDataKey.java
Show resolved
Hide resolved
dhis-2/dhis-api/src/main/java/org/hisp/dhis/analytics/AnalyticsMetaDataKey.java
Show resolved
Hide resolved
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.
Added a few suggestions 👍
Also, this same logic should be applied to the TEI/Cross Program API (I believe). The UI requests are built using the same components and are read on the same page.
dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/OrganisationUnitCriteriaUtils.java
Outdated
Show resolved
Hide resolved
dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/OrganisationUnitCriteriaUtils.java
Outdated
Show resolved
Hide resolved
dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/OrganisationUnitCriteriaUtils.java
Outdated
Show resolved
Hide resolved
dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/OrganisationUnitCriteriaUtils.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* Transform criteria in the string |
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.
What does it mean ("Transform criteria in the string")?
Would be nice to describe what the method actually does, including an example of a possible output.
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.
done
: StringUtils.EMPTY; | ||
} | ||
|
||
private static boolean hasDimensions(Set<String> dimensions) { |
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.
You don't need to create this additional method...
You could use CollectionUtils.isNotEmpty(criteria.getDimension())
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.
done
dhis-2/dhis-api/src/main/java/org/hisp/dhis/util/OrganisationUnitCriteriaUtils.java
Outdated
Show resolved
Hide resolved
currentUserService.getCurrentUser(), params.getUserOrganisationUnitsCriteria()) | ||
.forEach(items::putAll); | ||
|
||
if (params.isComingFromQuery()) { |
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.
Could we concentrate all the logic of the if/else related to
params.isComingFromQuery()
on the same block? ie:
if (params.isComingFromQuery()) {
items.putAll(getMetadataItems(params, periodKeywords, optionItems, grid));
metadata.put(
DIMENSIONS.getKey(), getDimensionItems(params, Optional.of(optionsPresentInGrid)));
} else {
items.putAll(getMetadataItems(params));
metadata.put(DIMENSIONS.getKey(), getDimensionItems(params, empty()));
}
metadata.put(ITEMS.getKey(), items);
Or is it going to have a side affect?
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.
The items will change the place with dimensions. I thing this should not be a problem, should I change it?
Do not forget, it will affect most likely our e2e tests and som unit tests as well.
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.
So, it may have a side effect. We never know if someone is relying on this order.
Let's leave it for now.
...ice-analytics/src/main/java/org/hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java
Outdated
Show resolved
Hide resolved
...ice-analytics/src/main/java/org/hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java
Outdated
Show resolved
Hide resolved
…nitCriteriaUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…hisp/dhis/analytics/util/AnalyticsOrganisationUnitUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…nitCriteriaUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…nitCriteriaUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…nitCriteriaUtils.java Co-authored-by: Maikel Arabori <[email protected]>
…nitCriteriaUtils.java Co-authored-by: Maikel Arabori <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
currentUserService.getCurrentUser(), params.getUserOrganisationUnitsCriteria()) | ||
.forEach(items::putAll); | ||
|
||
if (params.isComingFromQuery()) { |
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.
So, it may have a side effect. We never know if someone is relying on this order.
Let's leave it for now.
* Transform request criteria into a comma separated string | ||
* (USER_ORG_UNIT,USER_ORGUNIT_CHILDREN,USER_ORGUNIT_GRANDCHILDREN) | ||
* | ||
* @param dimensions, set of the requested dimensions |
Check notice
Code scanning / CodeQL
Spurious Javadoc @param tags Note
please see https://dhis2.atlassian.net/browse/DHIS2-15933