fix: Add event validation when updating status [DHIS2-17658]#17890
fix: Add event validation when updating status [DHIS2-17658]#17890enricocolasante merged 6 commits intomasterfrom
Conversation
enricocolasante
force-pushed
the
DHIS2-17658
branch
from
9dbc015 to
4b0c473
Compare
| return eventStatus; | ||
| } | ||
| } | ||
| public static final Set<EventStatus> ALLOW_DATA_VALUES_STATUSES = |
There was a problem hiding this comment.
wouldn't it make more sense to name it ALLOWED_STATUSES?
we are not referencing event data values here, are we?
There was a problem hiding this comment.
Anyway, is this used somewhere?
There was a problem hiding this comment.
Not yet :)
I extracted this PR from the other one, so it will be used at some point.
There was a problem hiding this comment.
ALLOWED_STATUSES is not true. These are the statuses that allow to have data values defined.
ALLOWED_STATUSES gets you the idea that those statuses are not allowed at all, which is not true.
There was a problem hiding this comment.
How about STATUSES_ALLOWING_DATA_VALUES or STATUSES_WITH_DATA_VALUES?
| public static boolean isExistingEvent(EventStatus status) { | ||
| return status != null && (COMPLETED.equals(status) || VISITED.equals(status)); | ||
| } | ||
| public static final Set<EventStatus> NO_ALLOW_DATA_VALUES_STATUSES = |
There was a problem hiding this comment.
I'd call this one DISALLOWED_DATA_VALUES_STATUSES, and in case the comment above makes sense, just DISALLOWED_STATUSES
There was a problem hiding this comment.
I think not allowed and disallowed are not synonyms, but we could ask a native speaker about it.
The idea here is the same as above, these statuses are valid, the just do not permit to define data values.
Maybe because this was extracted from a bigger PR is not clear, but we will add another validation that is simply going to fail if the event has a status in NO_ALLOW_DATA_VALUES_STATUSES and dataValues is not empty
There was a problem hiding this comment.
STATUSES_DISALLOWING_DATA_VALUES or STATUSES_WITHOUT_DATA_VALUES
| return eventStatus; | ||
| } | ||
| } | ||
| public static final Set<EventStatus> ALLOW_DATA_VALUES_STATUSES = |
There was a problem hiding this comment.
How about STATUSES_ALLOWING_DATA_VALUES or STATUSES_WITH_DATA_VALUES?
| public static boolean isExistingEvent(EventStatus status) { | ||
| return status != null && (COMPLETED.equals(status) || VISITED.equals(status)); | ||
| } | ||
| public static final Set<EventStatus> NO_ALLOW_DATA_VALUES_STATUSES = |
There was a problem hiding this comment.
STATUSES_DISALLOWING_DATA_VALUES or STATUSES_WITHOUT_DATA_VALUES
dhis-2/dhis-api/src/main/java/org/hisp/dhis/tracker/imports/validation/ValidationCode.java
Outdated
Show resolved
Hide resolved
|
|
||
| private boolean checkInvalidStatusTransition(EventStatus fromStatus, EventStatus toStatus) { | ||
| return switch (fromStatus) { | ||
| case VISITED, ACTIVE, COMPLETED -> |
There was a problem hiding this comment.
These are ALLOW_DATA_VALUES_STATUSES, should we not use that then? like you do with NO_ALLOW_DATA_VALUES_STATUSES.
.../main/java/org/hisp/dhis/tracker/imports/validation/validator/event/DataStatusValidator.java
Outdated
Show resolved
Hide resolved
|
* fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments
….41) (#18015) * fix: Add event validation when updating status [DHIS2-17658] (#17890) * fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments * fix: Validate no datavalues for some event statuses [DHIS2-17658] (#17934) * fix: Validate no datavalues for some event statuses [DHIS2-17658] * Fix tests * Fix code review comments * Update dhis-2/dhis-api/src/main/java/org/hisp/dhis/tracker/imports/validation/ValidationCode.java Co-authored-by: teleivo <teleivo@users.noreply.github.com> --------- Co-authored-by: teleivo <teleivo@users.noreply.github.com> * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] (#17835) * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] * More clean up * Fix tests * Add tests * Add tests --------- Co-authored-by: teleivo <teleivo@users.noreply.github.com>
* fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments
….40) (#18017) * fix: Add event validation when updating status [DHIS2-17560] * fix: Add event validation when updating status [DHIS2-17658] (#17890) * fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] (#17835) * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] * More clean up * Fix tests * Add tests * Add tests
* fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments
….39) (#18018) * fix: Add event validation when updating status [DHIS2-17560] * fix: Add event validation when updating status [DHIS2-17658] (#17890) * fix: Add event validation when updating status [DHIS2-17658] * Fix sonar issues * Fix code review comments * Fix code review comments * Fix code review comments * Fix code review comments * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] (#17835) * fix: Skip mandatory data value validation for UPDATE [DHIS2-17560] * More clean up * Fix tests * Add tests * Add tests * Fix test
4 participants
This is the first PR to fix https://dhis2.atlassian.net/browse/DHIS2-17658.
We were missing the validation on event status update.
Some statuses are meant to be used for events that will be handled at a later point in time ('SCHEDULE', 'SKIPPED', 'OVERDUE') and they do not allow data values to be defined and other statuses are meant to be used for events that are being worked on ('ACTIVE', 'COMPLETED', 'VISITED').
Updating from the first ones to the second ones is always allowed while going back from the second ones to the first ones does not make sense and it has to be blocked.