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: Include time when filtering in tracker exporter [DHIS2-16019] #15474

Merged
merged 7 commits into from
Oct 27, 2023
+517 −52
Merged

fix: Include time when filtering in tracker exporter [DHIS2-16019] #15474

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5904d6
fix: Include time when filtering in old tracker exporter [DHIS2-16019]
enricocolasante Oct 6, 2023
fd2845a
fix: Include time when filtering in tracker exporter [DHIS2-16019]
enricocolasante Oct 24, 2023
df617cb
Add tests
enricocolasante Oct 27, 2023
0eb1a91
Merge remote-tracking branch 'origin/master' into DHIS2-16019
enricocolasante Oct 27, 2023
1999d45
Fix code review comments
enricocolasante Oct 27, 2023
8dccabb
Fix code review comments
enricocolasante Oct 27, 2023
74064bc
Fix code review comments
enricocolasante Oct 27, 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
11 changes: 4 additions & 7 deletions ...acker/src/main/java/org/hisp/dhis/tracker/export/enrollment/HibernateEnrollmentStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@

import static org.hisp.dhis.common.IdentifiableObjectUtils.getUids;
import static org.hisp.dhis.commons.util.TextUtils.getQuotedCommaDelimitedString;
import static org.hisp.dhis.util.DateUtils.getLongDateString;
import static org.hisp.dhis.util.DateUtils.getLongGmtDateString;
import static org.hisp.dhis.util.DateUtils.getMediumDateString;
import static org.hisp.dhis.util.DateUtils.nowMinusDuration;

import java.util.List;
Expand Down Expand Up @@ -170,10 +170,7 @@ private QueryWithOrderBy buildEnrollmentHql(EnrollmentQueryParams params) {
+ "'";
} else if (params.hasLastUpdated()) {
hql +=
hlp.whereAnd()
+ "en.lastUpdated >= '"
+ getMediumDateString(params.getLastUpdated())
+ "'";
hlp.whereAnd() + "en.lastUpdated >= '" + getLongDateString(params.getLastUpdated()) + "'";
}

if (params.hasTrackedEntity()) {
Expand Down Expand Up @@ -226,15 +223,15 @@ private QueryWithOrderBy buildEnrollmentHql(EnrollmentQueryParams params) {
hql +=
hlp.whereAnd()
+ "en.enrollmentDate >= '"
+ getMediumDateString(params.getProgramStartDate())
+ getLongDateString(params.getProgramStartDate())
+ "'";
}

if (params.hasProgramEndDate()) {
hql +=
hlp.whereAnd()
+ "en.enrollmentDate <= '"
+ getMediumDateString(params.getProgramEndDate())
+ getLongDateString(params.getProgramEndDate())
+ "'";
}

Expand Down
18 changes: 9 additions & 9 deletions ...src/main/java/org/hisp/dhis/tracker/export/trackedentity/HibernateTrackedEntityStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@
import static org.hisp.dhis.commons.util.TextUtils.getCommaDelimitedString;
import static org.hisp.dhis.commons.util.TextUtils.getQuotedCommaDelimitedString;
import static org.hisp.dhis.util.DateUtils.addDays;
import static org.hisp.dhis.util.DateUtils.getLongDateString;
import static org.hisp.dhis.util.DateUtils.getLongGmtDateString;
import static org.hisp.dhis.util.DateUtils.getMediumDateString;

import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -480,14 +480,14 @@ private String getFromSubQueryTrackedEntityConditions(
trackedEntity
.append(whereAnd.whereAnd())
.append(" TE.lastupdated >= '")
.append(getMediumDateString(params.getLastUpdatedStartDate()))
.append(getLongDateString(params.getLastUpdatedStartDate()))
.append(SINGLE_QUOTE);
}
if (params.hasLastUpdatedEndDate()) {
trackedEntity
.append(whereAnd.whereAnd())
.append(" TE.lastupdated < '")
.append(getMediumDateString(addDays(params.getLastUpdatedEndDate(), 1)))
.append(getLongDateString(addDays(params.getLastUpdatedEndDate(), 1)))
.append(SINGLE_QUOTE);
}
}
Expand Down Expand Up @@ -707,28 +707,28 @@ private String getFromSubQueryEnrollmentConditions(
if (params.hasProgramEnrollmentStartDate()) {
program
.append("AND EN.enrollmentdate >= '")
.append(getMediumDateString(params.getProgramEnrollmentStartDate()))
.append(getLongDateString(params.getProgramEnrollmentStartDate()))
.append("' ");
}

if (params.hasProgramEnrollmentEndDate()) {
program
.append("AND EN.enrollmentdate <= '")
.append(getMediumDateString(params.getProgramEnrollmentEndDate()))
.append(getLongDateString(params.getProgramEnrollmentEndDate()))
.append("' ");
}

if (params.hasProgramIncidentStartDate()) {
program
.append("AND EN.incidentdate >= '")
.append(getMediumDateString(params.getProgramIncidentStartDate()))
.append(getLongDateString(params.getProgramIncidentStartDate()))
.append("' ");
}

if (params.hasProgramIncidentEndDate()) {
program
.append("AND EN.incidentdate <= '")
.append(getMediumDateString(params.getProgramIncidentEndDate()))
.append(getLongDateString(params.getProgramIncidentEndDate()))
.append("' ");
}

Expand Down Expand Up @@ -765,8 +765,8 @@ private String getFromSubQueryEvent(TrackedEntityQueryParams params) {
}

if (params.hasEventStatus()) {
String start = getMediumDateString(params.getEventStartDate());
String end = getMediumDateString(params.getEventEndDate());
String start = getLongDateString(params.getEventStartDate());
String end = getLongDateString(params.getEventEndDate());

if (params.isEventStatus(EventStatus.COMPLETED)) {
events
Expand Down
59 changes: 59 additions & 0 deletions ...on/src/test/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.time.Instant;
import java.time.ZoneId;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -555,6 +556,64 @@ void shouldReturnTrackedEntityIfGivenFilterMatches()
assertContainsOnly(List.of(trackedEntityA), trackedEntities);
}

@Test
void shouldReturnTrackedEntityIfTEWasUpdatedAfterPassedDateAndTime()
throws ForbiddenException, NotFoundException, BadRequestException {
Date oneHourBeforeLastUpdated =
Date.from(
trackedEntityA
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not following how this date is supposed to test all changes we are applying. Can we make a test where we actually have a date match? Or maybe there is and i do not see it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are just testing that the filter on updatedAt is working.
We try to get all the TE that were updated in the last hour and we want to assert that they are present.
I am not sure we should assert on the date as the next test is doing the opposite and filter on something that will return no TEs and so there we will not have any date to assert

Copy link
Contributor

Choose a reason for hiding this comment

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

ok but i am a bit surprised that we made these changes and there are no tests failing. So maybe there is no test coverage or it is not fine-grained to time. I see many where conditions are changed now so maybe we could think of some test coverage for that. But maybe i am wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right.
There are no failures just because this change is not a breaking one, so any case working before is still working but we need to cover more cases.
I am adding the new test cases for all the changes

.getLastUpdated()
.toInstant()
.atZone(ZoneId.systemDefault())
.toLocalDateTime()
.minusHours(1)
.atZone(ZoneId.systemDefault())
.toInstant());

TrackedEntityOperationParams operationParams =
TrackedEntityOperationParams.builder()
.organisationUnits(Set.of(orgUnitA.getUid()))
.orgUnitMode(SELECTED)
.trackedEntityTypeUid(trackedEntityTypeA.getUid())
.lastUpdatedStartDate(oneHourBeforeLastUpdated)
.user(user)
.build();

List<TrackedEntity> trackedEntities =
trackedEntityService.getTrackedEntities(operationParams).getTrackedEntities();

assertContainsOnly(List.of(trackedEntityA), trackedEntities);
}

@Test
void shouldReturnEmptyIfTEWasUpdatedBeforePassedDateAndTime()
throws ForbiddenException, NotFoundException, BadRequestException {
Date oneHourAfterLastUpdated =
Date.from(
trackedEntityA
.getLastUpdated()
.toInstant()
.atZone(ZoneId.systemDefault())
.toLocalDateTime()
.plusHours(1)
.atZone(ZoneId.systemDefault())
.toInstant());

TrackedEntityOperationParams operationParams =
TrackedEntityOperationParams.builder()
.organisationUnits(Set.of(orgUnitA.getUid()))
.orgUnitMode(SELECTED)
.trackedEntityTypeUid(trackedEntityTypeA.getUid())
.lastUpdatedStartDate(oneHourAfterLastUpdated)
.user(user)
.build();

List<TrackedEntity> trackedEntities =
trackedEntityService.getTrackedEntities(operationParams).getTrackedEntities();

assertIsEmpty(trackedEntities);
}

@Test
void shouldReturnEmptyCollectionIfGivenFilterDoesNotMatch()
throws ForbiddenException, NotFoundException, BadRequestException {
Expand Down
Loading