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

Jump to bottom

fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] #17835

Merged
merged 7 commits into from
Jul 2, 2024
Merged

fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] #17835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4fa229d
fix: Skip mandatory data value validation for UPDATE [DHIS2-17560]
enricocolasante Jun 19, 2024
73b7d37
More clean up
enricocolasante Jun 19, 2024
fa0bbb9
Fix tests
enricocolasante Jun 19, 2024
5f7adf7
Add tests
enricocolasante Jun 19, 2024
64f40ef
Merge remote-tracking branch 'origin/master' into DHIS2-17560
enricocolasante Jul 2, 2024
e047ccd
Add tests
enricocolasante Jul 2, 2024
2b39767
Add tests
enricocolasante Jul 2, 2024
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
1 change: 1 addition & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/tracker/imports/validation/ValidationCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ public enum ValidationCode {
E1312("Referral events need to have both sides of a relationship."),
E1313(
"Event {0} of an enrollment does not point to an existing tracked entity. The data in your system might be corrupted"),
E1314("Generated by program rule (`{0}`) - DataElement `{1}` is mandatory and cannot be deleted"),
E1315(
"Status `{0}` does not allow defining data values. Statuses that do allow defining data values are: {1}"),
E1316("No event can transition from status `{0}` to status `{1}.`"),
Expand Down
34 changes: 25 additions & 9 deletions ...a/org/hisp/dhis/tracker/imports/programrule/executor/event/SetMandatoryFieldExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
package org.hisp.dhis.tracker.imports.programrule.executor.event;

import static org.hisp.dhis.tracker.imports.programrule.ProgramRuleIssue.error;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.validateDeletionMandatoryDataValue;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.validateMandatoryDataValue;

import java.util.List;
import java.util.Optional;
Expand All @@ -41,7 +43,6 @@
import org.hisp.dhis.tracker.imports.programrule.ProgramRuleIssue;
import org.hisp.dhis.tracker.imports.programrule.executor.RuleActionExecutor;
import org.hisp.dhis.tracker.imports.validation.ValidationCode;
import org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils;

/**
* This executor checks if a field is not empty in the {@link TrackerBundle} @Author Enrico
Expand All @@ -61,15 +62,30 @@ public UID getDataElementUid() {
@Override
public Optional<ProgramRuleIssue> executeRuleAction(TrackerBundle bundle, Event event) {
TrackerPreheat preheat = bundle.getPreheat();
ProgramStage programStage = preheat.getProgramStage(event.getProgramStage());
TrackerIdSchemeParams idSchemes = preheat.getIdSchemes();
ProgramStage programStage = preheat.getProgramStage(event.getProgramStage());

Optional<ProgramRuleIssue> programRuleIssue =
validateDeletionMandatoryDataValue(
event,
programStage,
List.of(
idSchemes.toMetadataIdentifier(preheat.getDataElement(fieldUid.getValue()))))
.stream()
.map(e -> error(ruleUid, ValidationCode.E1314, e.getIdentifierOrAttributeValue()))
.findAny();

if (programRuleIssue.isEmpty()) {
return validateMandatoryDataValue(
bundle,
event,
programStage,
List.of(idSchemes.toMetadataIdentifier(preheat.getDataElement(fieldUid.getValue()))))
.stream()
.map(e -> error(ruleUid, ValidationCode.E1301, e.getIdentifierOrAttributeValue()))
.findAny();
}

return ValidationUtils.validateMandatoryDataValue(
programStage,
event,
List.of(idSchemes.toMetadataIdentifier(preheat.getDataElement(fieldUid.getValue()))))
.stream()
.map(e -> error(ruleUid, ValidationCode.E1301, e.getIdentifierOrAttributeValue()))
.findAny();
return programRuleIssue;
}
}
49 changes: 37 additions & 12 deletions ...ker/src/main/java/org/hisp/dhis/tracker/imports/validation/validator/ValidationUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import org.hisp.dhis.common.CodeGenerator;
import org.hisp.dhis.common.ValueType;
import org.hisp.dhis.common.ValueTypedDimensionalItemObject;
Expand Down Expand Up @@ -99,24 +100,48 @@ public static List<Note> validateNotes(
return notes;
}

public static List<MetadataIdentifier> validateMandatoryDataValue(
ProgramStage programStage, Event event, List<MetadataIdentifier> mandatoryDataElements) {
List<MetadataIdentifier> notPresentMandatoryDataElements = Lists.newArrayList();

public static List<MetadataIdentifier> validateDeletionMandatoryDataValue(
Event event, ProgramStage programStage, List<MetadataIdentifier> mandatoryDataElements) {
if (!needsToValidateDataValues(event, programStage)) {
return notPresentMandatoryDataElements;
return List.of();
Copy link
Contributor

Choose a reason for hiding this comment

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

out of the scope of this PR, but why do we skip the validation if the event status is SCHEDULE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SCHEDULE event is created at some point in time but it is meant to happen later at some point, so when it is created it is kind of empty and it doesn't make sense to require mandatory data values at that point.

Copy link
Contributor

@muilpp muilpp Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

and in that case, are we covered if I create and event this way, and then I provide the values later on?
what I mean by that is, at the moment I provide the values, do we treat it as an import strategy CREATE again to make sure all mandatory fields are set?

if it's something different than CREATE, then I guess I could have mandatory values set to null, couldn't I?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no, you are right.
At the moment if you create a schedule event without the mandatory datavalues and later you just change the status of the event then you can have an empty mandatory datavalues were they are meant to be present.

Will need to check how we manage the transition from a status to another

}
Set<MetadataIdentifier> eventDataElements =
event.getDataValues().stream()
.filter(dv -> dv.getValue() == null)
.map(DataValue::getDataElement)
.collect(Collectors.toSet());

return mandatoryDataElements.stream().filter(eventDataElements::contains).toList();
}

public static List<MetadataIdentifier> validateMandatoryDataValue(
TrackerBundle bundle,
Event event,
ProgramStage programStage,
List<MetadataIdentifier> mandatoryDataElements) {
if (!areDataValuesBeingCreated(bundle, event, programStage)) {
return List.of();
}

Set<MetadataIdentifier> eventDataElements =
event.getDataValues().stream().map(DataValue::getDataElement).collect(Collectors.toSet());

for (MetadataIdentifier mandatoryDataElement : mandatoryDataElements) {
if (!eventDataElements.contains(mandatoryDataElement)) {
notPresentMandatoryDataElements.add(mandatoryDataElement);
}
return mandatoryDataElements.stream().filter(de -> !eventDataElements.contains(de)).toList();
}

public static boolean areDataValuesBeingCreated(
TrackerBundle bundle, Event event, ProgramStage programStage) {
if (!needsToValidateDataValues(event, programStage)) {
return false;
}

if (bundle.getStrategy(event).isCreate()) {
return true;
}

return notPresentMandatoryDataElements;
EventStatus savedStatus = bundle.getPreheat().getEvent(event.getUid()).getStatus();
return EventStatus.STATUSES_WITHOUT_DATA_VALUES.contains(savedStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

do the event statuses OVERDUE and SKIPPED also need to be considered here? or SCHEDULE would be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they do. They have the exact same behaviour as SCHEDULE events, so we need to check them to be sure we are in a consistent state

&& EventStatus.STATUSES_WITH_DATA_VALUES.contains(event.getStatus());
}

public static boolean needsToValidateDataValues(Event event, ProgramStage programStage) {
Expand Down Expand Up @@ -167,8 +192,8 @@ public static boolean eventExist(TrackerBundle bundle, String eventUid) {
}

public static <T extends ValueTypedDimensionalItemObject> void validateOptionSet(
Reporter reporter, TrackerDto dto, T optionalObject, String value) {
if (value == null || !optionalObject.hasOptionSet()) {
Reporter reporter, TrackerDto dto, T optionalObject, @Nonnull String value) {
if (!optionalObject.hasOptionSet()) {
return;
}

Expand Down
153 changes: 40 additions & 113 deletions ...in/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataValuesValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,22 @@
*/
package org.hisp.dhis.tracker.imports.validation.validator.event;

import static com.google.common.base.Preconditions.checkNotNull;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1007;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1009;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1076;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1084;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1302;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1303;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.needsToValidateDataValues;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1304;
import static org.hisp.dhis.tracker.imports.validation.ValidationCode.E1305;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.validateDeletionMandatoryDataValue;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.validateMandatoryDataValue;
import static org.hisp.dhis.tracker.imports.validation.validator.ValidationUtils.validateOptionSet;

import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import org.apache.commons.lang3.StringUtils;
import org.hisp.dhis.dataelement.DataElement;
import org.hisp.dhis.event.EventStatus;
Expand All @@ -56,7 +58,6 @@
import org.hisp.dhis.tracker.imports.validation.Reporter;
import org.hisp.dhis.tracker.imports.validation.ValidationCode;
import org.hisp.dhis.tracker.imports.validation.Validator;
import org.hisp.dhis.tracker.imports.validation.validator.TrackerImporterAssertErrors;

/**
* @author Enrico Colasante
Expand All @@ -66,24 +67,12 @@ class DataValuesValidator implements Validator<Event> {
public void validate(Reporter reporter, TrackerBundle bundle, Event event) {
ProgramStage programStage = bundle.getPreheat().getProgramStage(event.getProgramStage());

checkNotNull(programStage, TrackerImporterAssertErrors.PROGRAM_STAGE_CANT_BE_NULL);

for (DataValue dataValue : event.getDataValues()) {
// event dates (createdAt, updatedAt) are ignored and set by the
// system
TrackerPreheat preheat = bundle.getPreheat();
DataElement dataElement = preheat.getDataElement(dataValue.getDataElement());

if (dataElement == null) {
reporter.addError(event, ValidationCode.E1304, dataValue.getDataElement());
continue;
}

validateDataValue(reporter, bundle, dataElement, dataValue, programStage, event);
validateDataValue(reporter, bundle, dataValue, event);
}

validateEventStatus(reporter, event);
validateMandatoryDataValues(reporter, bundle, event);
validateMandatoryDataValues(reporter, bundle, event, programStage);
validateDataValueDataElementIsConnectedToProgramStage(reporter, bundle, event, programStage);
}

Expand All @@ -98,76 +87,43 @@ private void validateEventStatus(Reporter reporter, Event event) {
}
}

private void validateMandatoryDataValues(Reporter reporter, TrackerBundle bundle, Event event) {
if (event.getProgramStage().isBlank()) {
return;
}

TrackerPreheat preheat = bundle.getPreheat();
ProgramStage programStage = bundle.getPreheat().getProgramStage(event.getProgramStage());
private void validateMandatoryDataValues(
Reporter reporter, TrackerBundle bundle, Event event, ProgramStage programStage) {
final List<MetadataIdentifier> mandatoryDataElements =
programStage.getProgramStageDataElements().stream()
.filter(ProgramStageDataElement::isCompulsory)
.map(de -> preheat.getIdSchemes().toMetadataIdentifier(de.getDataElement()))
.collect(Collectors.toList());
List<MetadataIdentifier> missingDataValue =
validateMandatoryDataValue(programStage, event, mandatoryDataElements);
missingDataValue.forEach(de -> reporter.addError(event, E1303, de));
.map(de -> bundle.getPreheat().getIdSchemes().toMetadataIdentifier(de.getDataElement()))
.toList();

validateMandatoryDataValue(bundle, event, programStage, mandatoryDataElements)
.forEach(de -> reporter.addError(event, E1303, de));
validateDeletionMandatoryDataValue(event, programStage, mandatoryDataElements)
.forEach(de -> reporter.addError(event, E1076, DataElement.class.getSimpleName(), de));
}

private void validateDataValue(
Reporter reporter,
TrackerBundle bundle,
DataElement dataElement,
DataValue dataValue,
ProgramStage programStage,
Event event) {
String status = null;

if (dataElement.getValueType() == null) {
reporter.addError(
event, ValidationCode.E1302, dataElement.getUid(), "data_element_or_type_null_or_empty");
} else if (dataElement.hasOptionSet()) {
validateOptionSet(reporter, event, dataElement, dataValue.getValue());
} else if (dataElement.getValueType().isFile()) {
validateFileNotAlreadyAssigned(reporter, bundle, event, dataValue, dataElement);
} else if (dataElement.getValueType().isOrganisationUnit()) {
validateOrgUnitValueType(reporter, bundle, event, dataValue, dataElement);
} else {
status = ValidationUtils.valueIsValid(dataValue.getValue(), dataElement);
}
Reporter reporter, TrackerBundle bundle, DataValue dataValue, Event event) {
DataElement dataElement = bundle.getPreheat().getDataElement(dataValue.getDataElement());

if (status != null) {
reporter.addError(event, ValidationCode.E1302, dataElement.getUid(), status);
} else {
validateNullDataValues(reporter, dataElement, programStage, dataValue, event);
if (dataElement == null) {
reporter.addError(event, E1304, dataValue.getDataElement());
return;
}
}

private void validateNullDataValues(
Reporter reporter,
DataElement dataElement,
ProgramStage programStage,
DataValue dataValue,
Event event) {
if (dataValue.getValue() != null || !needsToValidateDataValues(event, programStage)) {
if (dataValue.getValue() == null) {
return;
}

Optional<ProgramStageDataElement> optionalPsde =
Optional.of(programStage)
.map(ps -> ps.getProgramStageDataElements().stream())
.flatMap(
psdes ->
psdes
.filter(
psde ->
psde.getDataElement().getUid().equals(dataElement.getUid())
&& psde.isCompulsory())
.findFirst());

if (optionalPsde.isPresent()) {
reporter.addError(event, E1076, DataElement.class.getSimpleName(), dataElement.getUid());
String status = ValidationUtils.valueIsValid(dataValue.getValue(), dataElement);

if (dataElement.hasOptionSet()) {
validateOptionSet(reporter, event, dataElement, dataValue.getValue());
} else if (dataElement.getValueType().isFile()) {
validateFileNotAlreadyAssigned(reporter, bundle, event, dataValue.getValue());
} else if (dataElement.getValueType().isOrganisationUnit()) {
validateOrgUnitValueType(reporter, bundle, event, dataValue.getValue());
} else if (status != null) {
reporter.addError(event, E1302, dataElement.getUid(), status);
}
}

Expand All @@ -184,34 +140,20 @@ private void validateDataValueDataElementIsConnectedToProgramStage(

for (MetadataIdentifier payloadDataElement : payloadDataElements) {
if (!dataElements.contains(payloadDataElement)) {
reporter.addError(event, ValidationCode.E1305, payloadDataElement, programStage.getUid());
reporter.addError(event, E1305, payloadDataElement, programStage.getUid());
}
}
}

private void validateFileNotAlreadyAssigned(
Reporter reporter,
TrackerBundle bundle,
Event event,
DataValue dataValue,
DataElement dataElement) {
boolean isFile = dataElement.getValueType() != null && dataElement.getValueType().isFile();

if (dataValue == null || dataValue.getValue() == null || !isFile) {
return;
}
Reporter reporter, TrackerBundle bundle, Event event, @Nonnull String value) {
FileResource fileResource = bundle.getPreheat().get(FileResource.class, value);

TrackerPreheat preheat = bundle.getPreheat();
FileResource fileResource = preheat.get(FileResource.class, dataValue.getValue());

reporter.addErrorIfNull(fileResource, event, E1084, dataValue.getValue());
reporter.addErrorIfNull(fileResource, event, E1084, value);

if (bundle.getStrategy(event).isCreate()) {
reporter.addErrorIf(
() -> fileResource != null && fileResource.isAssigned(),
event,
E1009,
dataValue.getValue());
() -> fileResource != null && fileResource.isAssigned(), event, E1009, value);
}

if (bundle.getStrategy(event).isUpdate()) {
Expand All @@ -222,27 +164,12 @@ private void validateFileNotAlreadyAssigned(
&& !fileResource.getFileResourceOwner().equals(event.getEvent()),
event,
E1009,
dataValue.getValue());
value);
}
}

private void validateOrgUnitValueType(
Reporter reporter,
TrackerBundle bundle,
Event event,
DataValue dataValue,
DataElement dataElement) {
boolean isOrgUnit =
dataElement.getValueType() != null && dataElement.getValueType().isOrganisationUnit();

if (dataValue == null || dataValue.getValue() == null || !isOrgUnit) {
return;
}

reporter.addErrorIfNull(
bundle.getPreheat().getOrganisationUnit(dataValue.getValue()),
event,
E1007,
dataValue.getValue());
Reporter reporter, TrackerBundle bundle, Event event, @Nonnull String value) {
reporter.addErrorIfNull(bundle.getPreheat().getOrganisationUnit(value), event, E1007, value);
}
}
Loading
Loading