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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 3, 2023
Merged

fix: Return all visible events when no program specified [TECH-1663] #15549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0467bdd
fix: Return all visible events when no program specified [TECH-1663]
muilpp Oct 31, 2023
6345c68
Merge remote-tracking branch 'origin/master' into TECH-1663-return-al…
muilpp Oct 31, 2023
33cb9ca
fix: Return search scope and capture scope in old api [TECH-1663]
muilpp Oct 31, 2023
d303dcf
fix: Return all visible events when mode selected [TECH-1663]
muilpp Oct 31, 2023
7145c22
fix: Return all visible events when mode selected [TECH-1663]
muilpp Nov 1, 2023
19f89b8
fix: Return all visible events when mode selected [TECH-1663]
muilpp Nov 1, 2023
7ffeb9d
Merge branch 'master' into TECH-1663-return-all-events-when-no-program
muilpp Nov 1, 2023
bc761a4
fix: Add capture scope org unit to integration test [TECH-1633]
muilpp Nov 1, 2023
252ad18
Merge branch 'master' into TECH-1663-return-all-events-when-no-program
muilpp Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 60 additions & 39 deletions ...ervice-dxf2/src/main/java/org/hisp/dhis/dxf2/deprecated/tracker/event/JdbcEventStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,16 @@ public class JdbcEventStore implements EventStore {

private static final String AND = " AND ";

private static final String COLUMN_USER_UID = "u_uid";

private static final String COLUMN_ORG_UNIT_PATH = "ou_path";

private static final String USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY =
" ou.path like CONCAT(orgunit.path, '%') ";

private static final String CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY =
" ou.path like CONCAT(:" + COLUMN_ORG_UNIT_PATH + ", '%' ) ";

private static final Map<String, String> QUERY_PARAM_COL_MAP =
ImmutableMap.<String, String>builder()
.put(EVENT_ID, "psi_uid")
Expand Down Expand Up @@ -296,10 +306,6 @@ public class JdbcEventStore implements EventStore {

private static final String UPDATE_EVENT_SQL;

private static final String COLUMN_USER_UID = "u_uid";

private static final String COLUMN_ORG_UNIT_PATH = "ou_path";

private static final String PERCENTAGE_SIGN = ", '%' ";

/**
Expand Down Expand Up @@ -2140,13 +2146,7 @@ private String createAccessibleSql(
}

mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return " EXISTS(SELECT ss.organisationunitid "
+ " FROM userteisearchorgunits ss "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = ss.organisationunitid "
+ " JOIN userinfo u ON u.userinfoid = ss.userinfoid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ " AND ou.path like CONCAT(orgunit.path, '%')) ";
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

private String createDescendantsSql(
Expand All @@ -2155,54 +2155,54 @@ private String createDescendantsSql(

if (isProgramRestricted(params.getProgram())) {
return createCaptureScopeQuery(
user,
mapSqlParameterSource,
" AND ou.path like CONCAT(:" + COLUMN_ORG_UNIT_PATH + PERCENTAGE_SIGN + ")");
user, mapSqlParameterSource, AND + CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

return " ou.path like CONCAT(:" + COLUMN_ORG_UNIT_PATH + PERCENTAGE_SIGN + ") ";
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

private String createChildrenSql(
User user, EventSearchParams params, MapSqlParameterSource mapSqlParameterSource) {
mapSqlParameterSource.addValue(COLUMN_ORG_UNIT_PATH, params.getOrgUnit().getPath());

String customChildrenQuery =
" AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " ) ";

if (isProgramRestricted(params.getProgram())) {
String childrenSqlClause =
" AND ou.path like CONCAT(:"
+ COLUMN_ORG_UNIT_PATH
+ PERCENTAGE_SIGN
+ ") "
+ " AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " )";

return createCaptureScopeQuery(user, mapSqlParameterSource, childrenSqlClause);
}

return " ou.path like CONCAT(:"
+ COLUMN_ORG_UNIT_PATH
+ PERCENTAGE_SIGN
+ ") "
+ " AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " ) ";
return createCaptureScopeQuery(
user,
mapSqlParameterSource,
AND + CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY + customChildrenQuery);
}

mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(
CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY + customChildrenQuery);
}

private String createSelectedSql(
User user, EventSearchParams params, MapSqlParameterSource mapSqlParameterSource) {
mapSqlParameterSource.addValue(COLUMN_ORG_UNIT_PATH, params.getOrgUnit().getPath());

String orgUnitPathEqualsMatchQuery =
" ou.path = :"
+ COLUMN_ORG_UNIT_PATH
+ " "
+ AND
+ USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY;

if (isProgramRestricted(params.getProgram())) {
String customSelectedClause = " AND ou.path = :" + COLUMN_ORG_UNIT_PATH + " ";
return createCaptureScopeQuery(user, mapSqlParameterSource, customSelectedClause);
}

return " ou.path = :" + COLUMN_ORG_UNIT_PATH + " ";
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(orgUnitPathEqualsMatchQuery);
}

private boolean isProgramRestricted(Program program) {
Expand All @@ -2227,4 +2227,25 @@ private String createCaptureScopeQuery(
+ customClause
+ ") ";
}

private static String getSearchAndCaptureScopeOrgUnitPathMatchQuery(String orgUnitMatcher) {
return " (EXISTS(SELECT ss.organisationunitid "
+ " FROM userteisearchorgunits ss "
+ " JOIN userinfo u ON u.userinfoid = ss.userinfoid "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = ss.organisationunitid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ AND
+ orgUnitMatcher
+ " AND p.accesslevel in ('OPEN', 'AUDITED')) "
+ " OR EXISTS(SELECT cs.organisationunitid "
+ " FROM usermembership cs "
+ " JOIN userinfo u ON u.userinfoid = cs.userinfoid "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = cs.organisationunitid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ AND
+ orgUnitMatcher
+ " )) ";
}
}
113 changes: 74 additions & 39 deletions ...dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/event/JdbcEventStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ class JdbcEventStore implements EventStore {
private static final String COLUMN_USER_UID = "u_uid";
private static final String COLUMN_ORG_UNIT_PATH = "ou_path";
private static final String DEFAULT_ORDER = COLUMN_EVENT_ID + " desc";
private static final String ORG_UNIT_PATH_LIKE_MATCH_QUERY =
private static final String USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY =
" ou.path like CONCAT(orgunit.path, '%') ";
private static final String CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY =
" ou.path like CONCAT(:" + COLUMN_ORG_UNIT_PATH + ", '%' ) ";

/**
Expand Down Expand Up @@ -1097,14 +1099,7 @@ private String createAccessibleSql(
}

mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return " EXISTS(SELECT ss.organisationunitid "
+ " FROM userteisearchorgunits ss "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = ss.organisationunitid "
+ " JOIN userinfo u ON u.userinfoid = ss.userinfoid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ " AND ou.path like CONCAT(orgunit.path, '%') "
+ ") ";
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

private String createDescendantsSql(
Expand All @@ -1113,60 +1108,64 @@ private String createDescendantsSql(

if (isProgramRestricted(params.getProgram())) {
return createCaptureScopeQuery(
user, mapSqlParameterSource, AND + ORG_UNIT_PATH_LIKE_MATCH_QUERY);
user, mapSqlParameterSource, AND + CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

return ORG_UNIT_PATH_LIKE_MATCH_QUERY;
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY);
}

private String createChildrenSql(
User user, EventQueryParams params, MapSqlParameterSource mapSqlParameterSource) {
mapSqlParameterSource.addValue(COLUMN_ORG_UNIT_PATH, params.getOrgUnit().getPath());

String customChildrenQuery =
" AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " ) ";

if (isProgramRestricted(params.getProgram())) {
String childrenSqlClause =
AND
+ ORG_UNIT_PATH_LIKE_MATCH_QUERY
+ " AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " )";

return createCaptureScopeQuery(user, mapSqlParameterSource, childrenSqlClause);
return createCaptureScopeQuery(
user,
mapSqlParameterSource,
AND + CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY + customChildrenQuery);
}

return ORG_UNIT_PATH_LIKE_MATCH_QUERY
+ " AND (ou.hierarchylevel = "
+ params.getOrgUnit().getHierarchyLevel()
+ " OR ou.hierarchylevel = "
+ (params.getOrgUnit().getHierarchyLevel() + 1)
+ " ) ";
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
Copy link
Contributor

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

Copy link
Contributor Author

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.

return getSearchAndCaptureScopeOrgUnitPathMatchQuery(
CUSTOM_ORG_UNIT_PATH_LIKE_MATCH_QUERY + customChildrenQuery);
}

private String createSelectedSql(
User user, EventQueryParams params, MapSqlParameterSource mapSqlParameterSource) {
mapSqlParameterSource.addValue(COLUMN_ORG_UNIT_PATH, params.getOrgUnit().getPath());

String orgUnitPathEqualsMatchQuery = " ou.path = :" + COLUMN_ORG_UNIT_PATH + " ";
String orgUnitPathEqualsMatchQuery =
" ou.path = :"
+ COLUMN_ORG_UNIT_PATH
+ " "
+ AND
+ USER_SCOPE_ORG_UNIT_PATH_LIKE_MATCH_QUERY;

if (isProgramRestricted(params.getProgram())) {
String customSelectedClause = AND + orgUnitPathEqualsMatchQuery;
return createCaptureScopeQuery(user, mapSqlParameterSource, customSelectedClause);
}

return orgUnitPathEqualsMatchQuery;
}

private boolean isProgramRestricted(Program program) {
return program != null && (program.isProtected() || program.isClosed());
}

private boolean isUserSearchScopeNotSet(User user) {
return user.getTeiSearchOrganisationUnits().isEmpty();
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());
return getSearchAndCaptureScopeOrgUnitPathMatchQuery(orgUnitPathEqualsMatchQuery);
}

/**
* Generates a sql to match the org unit event to the org unit(s) in the user's capture scope
*
* @param orgUnitMatcher specific condition to add depending on the ou mode
* @return a sql clause to add to the main query
*/
private String createCaptureScopeQuery(
User user, MapSqlParameterSource mapSqlParameterSource, String customClause) {
User user, MapSqlParameterSource mapSqlParameterSource, String orgUnitMatcher) {
mapSqlParameterSource.addValue(COLUMN_USER_UID, user.getUid());

return " EXISTS(SELECT cs.organisationunitid "
Expand All @@ -1176,10 +1175,46 @@ private String createCaptureScopeQuery(
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ " AND ou.path like CONCAT(orgunit.path, '%') "
+ customClause
+ orgUnitMatcher
+ ") ";
}

/**
* Generates a sql to match the org unit event to the org unit(s) in the user's search and capture
* scope
*
* @param orgUnitMatcher specific condition to add depending on the ou mode
* @return a sql clause to add to the main query
*/
private static String getSearchAndCaptureScopeOrgUnitPathMatchQuery(String orgUnitMatcher) {
return " (EXISTS(SELECT ss.organisationunitid "
+ " FROM userteisearchorgunits ss "
+ " JOIN userinfo u ON u.userinfoid = ss.userinfoid "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = ss.organisationunitid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ AND
+ orgUnitMatcher
+ " AND p.accesslevel in ('OPEN', 'AUDITED')) "
+ " OR EXISTS(SELECT cs.organisationunitid "
+ " FROM usermembership cs "
+ " JOIN userinfo u ON u.userinfoid = cs.userinfoid "
+ " JOIN organisationunit orgunit ON orgunit.organisationunitid = cs.organisationunitid "
+ " WHERE u.uid = :"
+ COLUMN_USER_UID
+ AND
+ orgUnitMatcher
+ " )) ";
}

private boolean isProgramRestricted(Program program) {
return program != null && (program.isProtected() || program.isClosed());
}

private boolean isUserSearchScopeNotSet(User user) {
return user.getTeiSearchOrganisationUnits().isEmpty();
}

/**
* For dataElement params, restriction is set in inner join. For query params, restriction is set
* in where clause.
Expand Down
Loading
Loading