-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: Return all visible events when no program specified [TECH-1663] #15549
fix: Return all visible events when no program specified [TECH-1663] #15549
Conversation
…l-events-when-no-program
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.
Just a style suggestion
+ " OR ou.hierarchylevel = " | ||
+ (params.getOrgUnit().getHierarchyLevel() + 1) | ||
+ " ) "; | ||
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid()); |
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.
Would it make sense to move
mapSqlParameterSource.addValue(COLUMN_ORG_UNIT_PATH, params.getOrgUnit().getPath()); mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
to getOrgUnitSql
method and remove the addValue operation from the single methods?
It seems to me that it would simplify the code quite a bit even if we will end up with a value in the parameter source that is not used
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 problem with that would be that modes CAPTURE
and ACCESSIBLE
don't have an orgUnit in the request, so calling params.getOrgUnit().getPath()
in getOrgUnitSql
would result in a NPE for these two cases.
Also, there's the scheduler running tasks with a null user. Since it's a system user, we assume it will use the mode ALL
, but in that case calling user.getUid()
would fail too.
Kudos, SonarCloud Quality Gate passed! |
Codecov Report
@@ Coverage Diff @@
## master #15549 +/- ##
=========================================
Coverage 66.22% 66.22%
- Complexity 31234 31237 +3
=========================================
Files 3485 3485
Lines 129764 129769 +5
Branches 15141 15141
=========================================
+ Hits 85931 85937 +6
+ Misses 36750 36749 -1
Partials 7083 7083
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
When no program present in
/events
, we need to return everything visible to the user according to its search scope and capture scope, that means:The changes are applied to both the new and old API.
Ticket: https://dhis2.atlassian.net/browse/TECH-1663