fix: [DHIS2-15272] Fix program expiryDays check with periodType for events #19297
Conversation
| @@ -246,6 +246,24 @@ void testEventIsNotValidWhenDateBelongsToExpiredPeriod() { | |||
| assertHasError(reporter, event, E1047); | |||
| } | |||
|
|
|||
| @Test | |||
| void testEventIsValidWhenScheduledDateBelongsToFuturePeriod() { | |||
There was a problem hiding this comment.
Some time ago, we decided to use the convention for our test names: should[ExpectedResult]When[Action]
There was a problem hiding this comment.
Thanks @muilpp ! Good catch on the expiryDays to 0 meaning indefinite. I'll update the code and add a test case for that scenario as well!
There was a problem hiding this comment.
@ameenhere, I didn't know how the exipiryDays work, so I read our docs, but I couldn't find anything about setting its value to 0, so I asked AI. That said, I'm not sure we should rely on it, wouldn't be the first time it lies. Just wanted to mention it here before we make any decision about it.
There was a problem hiding this comment.
I do remember expiryDays set to 0 is indeed the default which means "there is no expiry".
But I was hoping that expiryDays is only (and always) applicable if a "expiryPeriodType" is set to something.
Meaning if expiryPeriodType is null, then that means there is no expiry (whatever the expiryDays is configured as).
But if expiryPeriodType is a specific periodType, then expiryDays would be used (even if it is 0) to limit the number days after the end of the period to lock the event. This would also make it possible to actually say, "If the period has ended, lock it!" . This was my assumption. But I realize that may not be existing behaviour.
For resolving the original bug atleast, I think it is safe that I do not change the existing behaviour. Whether or not we should change the behaviour to my assumption, we can decide later. So reverted the if clause change.
|
This is what our AI told me when I asked what happens if I set a program's expiry days to 0.
|
| private Program getProgramWithRegistration() { | ||
| @Test | ||
| void shouldPassValidationForEventWhenDateBelongsToPastPeriodWithZeroExpiryDays() { | ||
| // given |
There was a problem hiding this comment.
We don't add such comments. 3 AAA (arrange=setup, act, assert) style paragraphs conveys the same.
| } | ||
|
|
||
| @Test | ||
| void shouldFailValidationForEventWhenScheduledDateBelongsToFuturePeriod() { |
There was a problem hiding this comment.
Either the name of the test or the assertion is wrong here.
AFAIK the name seems correct but we should get an error than in the reporter.
There was a problem hiding this comment.
I think the name was incorrect. ScheduledDate should be allowed for future periods. I have changed the name to reflect that is should pass.
|
There exists a configuration for setting a expiryPeriodType and expiryDays for a program.
As per the documentation
The validation that existed ignored the expiryDays part of the configuration.
This PR fixes this by making sure the current date is always before the last day of period in which the event took place plus the expiryDays count. Meaning if the event occuredAt October and the expiryDays is 10, then the validation is updated so that it verifies currentDate is before ( October 31+10 days = Nov 10)
Some additional changes done