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.

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: Fix enrollment and event tables [DHIS2-12600] #19193

Merged
merged 7 commits into from
Dec 4, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
</list>

<many-to-one name="organisationUnit" class="org.hisp.dhis.organisationunit.OrganisationUnit" column="organisationunitid"
foreign-key="fk_programinstance_organisationunitid" />
foreign-key="fk_programinstance_organisationunitid" not-null="true"/>

</class>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
<property name="occurredDate" column="occurreddate" type="timestamp" index="programstageinstance_executiondate" />

<many-to-one name="organisationUnit" class="org.hisp.dhis.organisationunit.OrganisationUnit" column="organisationunitid"
foreign-key="fk_programstageinstance_organisationunitid" index="programstageinstance_organisationunitid" />
foreign-key="fk_programstageinstance_organisationunitid" index="programstageinstance_organisationunitid" not-null="true"/>

<property name="status" column="status" type="org.hisp.dhis.program.EventStatusUserType" not-null="true" />

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
import org.hisp.dhis.feedback.ErrorCode;
import org.hisp.dhis.feedback.ErrorReport;
import org.hisp.dhis.organisationunit.OrganisationUnitService;
import org.hisp.dhis.preheat.PreheatIdentifier;
import org.hisp.dhis.program.Enrollment;
import org.hisp.dhis.program.EnrollmentStatus;
Expand All @@ -58,6 +59,8 @@ public class ProgramObjectBundleHook extends AbstractObjectBundleHook<Program> {

private final ProgramStageService programStageService;

private final OrganisationUnitService organisationUnitService;

private final AclService aclService;

private final IdentifiableObjectManager identifiableObjectManager;
Expand Down Expand Up @@ -133,6 +136,8 @@ private void addProgramInstance(Program program) {
enrollment.setProgram(program);
enrollment.setStatus(EnrollmentStatus.ACTIVE);
enrollment.setStoredBy("system-process");
enrollment.setOrganisationUnit(
organisationUnitService.getRootOrganisationUnits().iterator().next());
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like there could be multiple root org units? I guess it still does not matter that we arbitrarily chose one of the root org unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised as well that the method is potentially returning more that one root.
In our case, it doesn't matter, we need just a value in order to make the column not null, any random value is actually fine in there.


identifiableObjectManager.save(enrollment);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import org.hisp.dhis.common.IdentifiableObjectManager;
import org.hisp.dhis.feedback.ErrorCode;
import org.hisp.dhis.feedback.ErrorReport;
import org.hisp.dhis.organisationunit.OrganisationUnit;
import org.hisp.dhis.organisationunit.OrganisationUnitService;
import org.hisp.dhis.program.Enrollment;
import org.hisp.dhis.program.EnrollmentStatus;
import org.hisp.dhis.program.EventProgramEnrollmentService;
Expand All @@ -71,6 +73,8 @@ class ProgramObjectBundleHookTest {

@Mock private ProgramStageService programStageService;

@Mock private OrganisationUnitService organisationUnitService;

@Mock private AclService aclService;

@Mock private IdentifiableObjectManager identifiableObjectManager;
Expand All @@ -83,6 +87,7 @@ public void setUp() {
new ProgramObjectBundleHook(
eventProgramEnrollmentService,
programStageService,
organisationUnitService,
aclService,
identifiableObjectManager);

Expand All @@ -106,6 +111,8 @@ void verifyMissingBundleIsIgnored() {

@Test
void verifyProgramInstanceIsSavedForEventProgram() {
when(organisationUnitService.getRootOrganisationUnits())
.thenReturn(List.of(new OrganisationUnit()));
ArgumentCaptor<Enrollment> argument = ArgumentCaptor.forClass(Enrollment.class);

programA.setProgramType(ProgramType.WITHOUT_REGISTRATION);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.hisp.dhis.organisationunit.OrganisationUnit;
import org.hisp.dhis.program.Enrollment;
import org.hisp.dhis.program.EnrollmentStatus;
import org.hisp.dhis.program.ProgramType;
import org.hisp.dhis.security.acl.AclService;
import org.hisp.dhis.tracker.export.Order;
import org.hisp.dhis.tracker.export.Page;
Expand Down Expand Up @@ -192,6 +193,9 @@ private QueryWithOrderBy buildEnrollmentHql(EnrollmentQueryParams params) {
hql += hlp.whereAnd() + "en.program.uid = '" + params.getProgram().getUid() + "'";
}

// TODO(DHIS2-17961) This will be removed when dummy enrollments will not exist anymore
hql += hlp.whereAnd() + "en.program.programType = '" + ProgramType.WITH_REGISTRATION + "'";

if (params.hasEnrollmentStatus()) {
hql += hlp.whereAnd() + "en." + STATUS + " = '" + params.getEnrollmentStatus() + "'";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,7 @@ public void validate(
// If event is newly created, or going to be deleted, capture scope
// has to be checked
if (program.isWithoutRegistration() || strategy.isCreate() || strategy.isDelete()) {
if (organisationUnit == null) {
log.warn(ORG_UNIT_NO_USER_ASSIGNED, event.getUid());
} else {
checkOrgUnitInCaptureScope(reporter, event, organisationUnit, bundle.getUser());
}
checkOrgUnitInCaptureScope(reporter, event, organisationUnit, bundle.getUser());
}

UID teUid = getTeUidFromEvent(bundle, event, program);
Expand Down Expand Up @@ -304,9 +300,7 @@ private void checkEventOrgUnitWriteAccess(
OrganisationUnit eventOrgUnit,
boolean isCreatableInSearchScope,
UserDetails user) {
if (eventOrgUnit == null) {
log.warn(ORG_UNIT_NO_USER_ASSIGNED, event.getUid());
} else if (isCreatableInSearchScope
if (isCreatableInSearchScope
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I'm missing something...
but here we validate the user capture scope if the strategy is not an UPDATE.
since this method is only called when the strategy is not update, do we need to validate the org unit scope again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole validator is quite messy and difficult to follow.
We already have a bug to double check that everything in here is correct.
https://dhis2.atlassian.net/browse/DHIS2-17335

? !user.isInUserEffectiveSearchOrgUnitHierarchy(eventOrgUnit.getPath())
: !user.isInUserHierarchy(eventOrgUnit.getPath())) {
reporter.addError(event, ValidationCode.E1000, user, eventOrgUnit);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
DO $$
enricocolasante marked this conversation as resolved.
Show resolved Hide resolved
BEGIN
-- Set null organisation unit of dummy enrollments to root organisation unit
update enrollment en
set organisationunitid = (select organisationunitid from organisationunit order by hierarchylevel limit 1)
where en.organisationunitid is null
and en.programid in (select programid from program where type = 'WITHOUT_REGISTRATION');

alter table if exists enrollment alter column organisationunitid set not null;

alter table if exists event alter column organisationunitid set not null;

EXCEPTION
WHEN not_null_violation THEN
RAISE EXCEPTION 'There are inconsistent data in your DB. Please check https://github.com/dhis2/dhis2-releases/blob/master/releases/2.42/migration-notes.md#null-organisation-unit to have more information on the issue and to find ways to fix it. Detailed error message: %', SQLERRM;
enricocolasante marked this conversation as resolved.
Show resolved Hide resolved

END;
$$;
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ void testDeleteSoftDeletedEnrollmentLinkedToAnEventDataValueChangeLog()
eventA.setScheduledDate(enrollmentDate);
eventA.setUid("UID-A");
eventA.setAttributeOptionCombo(coA);
eventA.setOrganisationUnit(organisationUnit);
manager.save(eventA);
eventChangeLogService.addDataValueChangeLog(
eventA, dataElement, "", "value", ChangeLogType.UPDATE, getCurrentUsername());
Expand Down Expand Up @@ -362,6 +363,7 @@ void testDeleteSoftDeletedEventLinkedToARelationshipItem() {
eventA.setScheduledDate(enrollmentDate);
eventA.setUid(UID.generate().getValue());
eventA.setAttributeOptionCombo(coA);
eventA.setOrganisationUnit(organisationUnit);
manager.save(eventA);
long idA = eventA.getId();
Relationship r = new Relationship();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ void setUp() {

enrollmentA = new Enrollment(new Date(), new Date(), trackedEntityB, programA);
enrollmentA.setUid(CodeGenerator.generateUid());
enrollmentA.setOrganisationUnit(orgUnitA);

eventA = createEvent(stageA, enrollmentA, orgUnitA);
eventA.setScheduledDate(new Date());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,28 @@ void setUp() {
Date enrollmentDate = testDate2.toDate();
enrollmentA = new Enrollment(enrollmentDate, incidenDate, trackedEntityA, programA);
enrollmentA.setUid("UID-PIA");
enrollmentA.setOrganisationUnit(organisationUnitA);
manager.save(enrollmentA);
enrollmentB = new Enrollment(enrollmentDate, incidenDate, trackedEntityB, programB);
enrollmentB.setOrganisationUnit(organisationUnitB);
manager.save(enrollmentB);
Event eventA = new Event(enrollmentA, stageA);
Event eventA = new Event(enrollmentA, stageA, organisationUnitA);
eventA.setScheduledDate(enrollmentDate);
eventA.setUid("UID-A");
eventA.setAttributeOptionCombo(coA);
Event eventB = new Event(enrollmentA, stageB);
Event eventB = new Event(enrollmentA, stageB, organisationUnitA);
eventB.setScheduledDate(enrollmentDate);
eventB.setUid("UID-B");
eventB.setAttributeOptionCombo(coA);
Event eventC = new Event(enrollmentB, stageC);
Event eventC = new Event(enrollmentB, stageC, organisationUnitA);
eventC.setScheduledDate(enrollmentDate);
eventC.setUid("UID-C");
eventC.setAttributeOptionCombo(coA);
Event eventD1 = new Event(enrollmentB, stageD);
Event eventD1 = new Event(enrollmentB, stageD, organisationUnitA);
eventD1.setScheduledDate(enrollmentDate);
eventD1.setUid("UID-D1");
eventD1.setAttributeOptionCombo(coA);
Event eventD2 = new Event(enrollmentB, stageD);
Event eventD2 = new Event(enrollmentB, stageD, organisationUnitA);
eventD2.setScheduledDate(enrollmentDate);
eventD2.setUid("UID-D2");
eventD2.setAttributeOptionCombo(coA);
Expand Down Expand Up @@ -281,15 +283,15 @@ void testGetEventsWithScheduledNotifications() {
cal.add(Calendar.DATE, -2);
Date yesterday = cal.getTime();
// Events
Event eventA = new Event(enrollmentA, stageA);
Event eventA = new Event(enrollmentA, stageA, organisationUnitA);
eventA.setScheduledDate(tomorrow);
eventA.setAttributeOptionCombo(coA);
manager.save(eventA);
Event eventB = new Event(enrollmentB, stageB);
Event eventB = new Event(enrollmentB, stageB, organisationUnitA);
eventB.setScheduledDate(today);
eventB.setAttributeOptionCombo(coA);
manager.save(eventB);
Event eventC = new Event(enrollmentB, stageC);
Event eventC = new Event(enrollmentB, stageC, organisationUnitA);
eventC.setScheduledDate(yesterday);
eventC.setAttributeOptionCombo(coA);
manager.save(eventC);
Expand Down Expand Up @@ -415,8 +417,10 @@ void testGetEnrollmentsWithScheduledNotifications() {
Date aWeekAgo = cal.getTime();
// Enrollments
Enrollment enrollmentC = new Enrollment(today, tomorrow, trackedEntityX, programA);
enrollmentC.setOrganisationUnit(organisationUnitA);
manager.save(enrollmentC);
Enrollment enrollmentD = new Enrollment(aWeekAgo, yesterday, trackedEntityY, programA);
enrollmentD.setOrganisationUnit(organisationUnitA);
manager.save(enrollmentD);
// Queries
List<Enrollment> results;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ void testEmptyEventFormHasIntegrityIssues() {
"id": "%s",
"name": "Test",
"shortName": "Test",
"programType": "WITHOUT_REGISTRATION",
"programType": "WITH_REGISTRATION",
"categoryCombo": {"id": "%s"},
"programStages": [{"id": "%s"}]
}
Expand Down
Loading