feat: reject unenrolments if less than 48h to the event occurrence#505
Conversation
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr505.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr505.api.dev.hel.ninja 😆🎉🎉🎉 |
nikomakela
force-pushed
the
KK-1436-reject-unenrolments-48h
branch
from
5ec6294 to
bf2d67d
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr505.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr505.api.dev.hel.ninja 😆🎉🎉🎉 |
|
|
||
| @freeze_time("2024-01-01 12:00:00") | ||
| def test_cannot_unenrol_within_48_hours(guardian_api_client, child_with_user_guardian): | ||
| # Create occurrence 47 hours in future (within 48 hour limit) |
There was a problem hiding this comment.
How about setting the explicit value for the environment variable here so the test is robust against local .env file changes?
| # Create occurrence 47 hours in future (within 48 hour limit) | |
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48 | |
| # Create occurrence 47 hours in future (within 48 hour limit) |
There was a problem hiding this comment.
Without it, it's testing the default behaviour and that's what I think we should use. I have not added the environment variable to pipeline configurations at all, so I'm trusting in default value.
There was a problem hiding this comment.
If the tests assume that the value is always set to 48 then how about adding a simple test case for that? I mean a test case just for testing that KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE is 48. Otherwise there is a hidden dependency that one can not override the environment variable to any other value in any environment when running the tests or these cases will fail. They will do so with the added test case also though, but at least then the dependency on the value being 48 in environment would be documented by it.
There was a problem hiding this comment.
added settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48
|
|
||
| @freeze_time("2024-01-01 12:00:00") | ||
| def test_can_unenrol_outside_48_hours(guardian_api_client, child_with_user_guardian): | ||
| # Create occurrence 49 hours in future (outside 48 hour limit) |
There was a problem hiding this comment.
How about setting the explicit value for the environment variable here so the test is robust against local .env file changes?
| # Create occurrence 49 hours in future (outside 48 hour limit) | |
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48 | |
| # Create occurrence 49 hours in future (outside 48 hour limit) |
There was a problem hiding this comment.
added settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48
There was a problem hiding this comment.
The KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE variable could be explicitly set in the test cases that need it to be set to 48 for the test case to pass for added robustness but otherwise looks ok.
I assume Ratkaisutoimisto would want to code review this PR on their own before merging is possible.
|
Before merging this, the City-of-Helsinki/kukkuu-ui#693 should be merged, so also the UI reacts to this change. |
I agree with Kari, the tests shouldn't be counting on same value that can be overridden by |
|
|
||
| # Get the unenrol hours limit from settings, 0 means no time limit | ||
| # (only past occurrences are not allowed to unenrol). | ||
| hours_before = getattr(settings, "KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE", 48) |
There was a problem hiding this comment.
We can trust the settings default value and let it fail / error if it for some reason is None
KK-1436. Configure `KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE` with environment variable and use it to limit unenrolments too late before an occurrence start time. The default value is set to 48h. Use 0 as a value to disable this new feature. The UI changes are implemented in City-of-Helsinki/kukkuu-ui#693.
nikomakela
force-pushed
the
KK-1436-reject-unenrolments-48h
branch
from
bf2d67d to
79ad9bc
Compare
|
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr505.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr505.api.dev.hel.ninja 😆🎉🎉🎉 |
5 participants
KK-1436.
Configure
KUKKUU_ENROLMENT_UNENROL_HOURS_BEFOREwith environment variable and use it to limit unenrolments too late before an occurrence start time. The default value is set to 48h. Use 0 as a value to disable this new feature.The UI changes are implemented in
City-of-Helsinki/kukkuu-ui#693.