-
Notifications
You must be signed in to change notification settings - Fork 4
feat: reject unenrolments if less than 48h to the event occurrence #505
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -979,7 +979,9 @@ def test_unenrol_occurrence( | |||||||
| occurrence, | ||||||||
| project, | ||||||||
| child_with_random_guardian, | ||||||||
| settings, | ||||||||
| ): | ||||||||
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 0 | ||||||||
| non_authen_executed = api_client.execute( | ||||||||
| UNENROL_OCCURRENCE_MUTATION, variables=ENROL_OCCURRENCE_VARIABLES | ||||||||
| ) | ||||||||
|
|
@@ -1021,7 +1023,8 @@ def test_unenrol_occurrence( | |||||||
| snapshot.assert_match(executed) | ||||||||
|
|
||||||||
|
|
||||||||
| def test_ticket_not_valid_after_unenrol(user_api_client, occurrence, project): | ||||||||
| def test_ticket_not_valid_after_unenrol(user_api_client, occurrence, project, settings): | ||||||||
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 0 | ||||||||
| child = ChildWithGuardianFactory( | ||||||||
| relationship__guardian__user=user_api_client.user, project=project | ||||||||
| ) | ||||||||
|
|
@@ -1059,6 +1062,50 @@ def test_cannot_unenrol_from_occurrence_in_past( | |||||||
| assert_match_error_code(executed, PAST_ENROLMENT_ERROR) | ||||||||
|
|
||||||||
|
|
||||||||
| @freeze_time("2024-01-01 12:00:00") | ||||||||
| def test_cannot_unenrol_within_48_hours( | ||||||||
| guardian_api_client, child_with_user_guardian, settings | ||||||||
| ): | ||||||||
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48 | ||||||||
| # Create occurrence 47 hours in future (within 48 hour limit) | ||||||||
| future_time = timezone.now() + timedelta(hours=47) | ||||||||
| enrolment = EnrolmentFactory( | ||||||||
| occurrence__time=future_time, child=child_with_user_guardian | ||||||||
| ) | ||||||||
|
|
||||||||
| variables = deepcopy(UNENROL_OCCURRENCE_VARIABLES) | ||||||||
| variables["input"]["occurrenceId"] = get_global_id(enrolment.occurrence) | ||||||||
| variables["input"]["childId"] = get_global_id(child_with_user_guardian) | ||||||||
|
|
||||||||
| executed = guardian_api_client.execute( | ||||||||
| UNENROL_OCCURRENCE_MUTATION, variables=variables | ||||||||
| ) | ||||||||
|
|
||||||||
| assert_match_error_code(executed, "TOO_LATE_TO_UNENROL_ERROR") | ||||||||
|
|
||||||||
|
|
||||||||
| @freeze_time("2024-01-01 12:00:00") | ||||||||
| def test_can_unenrol_outside_48_hours( | ||||||||
| guardian_api_client, child_with_user_guardian, settings | ||||||||
| ): | ||||||||
| settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48 | ||||||||
| # Create occurrence 49 hours in future (outside 48 hour limit) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about setting the explicit value for the environment variable here so the test is robust against local .env file changes?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||||||||
| future_time = timezone.now() + timedelta(hours=49) | ||||||||
| enrolment = EnrolmentFactory( | ||||||||
| occurrence__time=future_time, child=child_with_user_guardian | ||||||||
| ) | ||||||||
|
|
||||||||
| variables = deepcopy(UNENROL_OCCURRENCE_VARIABLES) | ||||||||
| variables["input"]["occurrenceId"] = get_global_id(enrolment.occurrence) | ||||||||
| variables["input"]["childId"] = get_global_id(child_with_user_guardian) | ||||||||
|
|
||||||||
| executed = guardian_api_client.execute( | ||||||||
| UNENROL_OCCURRENCE_MUTATION, variables=variables | ||||||||
| ) | ||||||||
|
|
||||||||
| assert "errors" not in executed | ||||||||
|
|
||||||||
|
|
||||||||
| def test_maximum_enrolment( | ||||||||
| guardian_api_client, occurrence, project, child_with_user_guardian | ||||||||
| ): | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about setting the explicit value for the environment variable here so the test is robust against local .env file changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_BEFOREis48. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
settings.KUKKUU_ENROLMENT_UNENROL_HOURS_BEFORE = 48