From 00b0c32165ecd63a7a6e1f67360d4b51c8829543 Mon Sep 17 00:00:00 2001 From: Eli Chadwick Date: Thu, 19 Oct 2023 11:34:44 +0100 Subject: [PATCH 1/4] add field to override invalid code for soft validation --- amy/extforms/forms.py | 39 ++++++++++++++++--- .../includes/trainingrequest_details.html | 2 + ...iningrequest_accept_invalid_member_code.py | 22 +++++++++++ amy/workshops/models.py | 8 ++++ 4 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 amy/workshops/migrations/0267_trainingrequest_accept_invalid_member_code.py diff --git a/amy/extforms/forms.py b/amy/extforms/forms.py index ae21df061..c98622fdc 100644 --- a/amy/extforms/forms.py +++ b/amy/extforms/forms.py @@ -37,6 +37,7 @@ class Meta: fields = ( "review_process", "member_code", + "member_code_override", "personal", "family", "email", @@ -109,6 +110,7 @@ def __init__(self, *args, **kwargs): self.set_other_fields(self.helper.layout) self.set_fake_required_fields() self.set_accordion(self.helper.layout) + self.set_display_member_code_override(False) self.set_hr(self.helper.layout) def set_other_field(self, field_name: str, layout: Layout) -> None: @@ -143,6 +145,7 @@ def set_accordion(self, layout: Layout) -> None: self["review_process"].field.widget.subfields = { # type: ignore "preapproved": [ self["member_code"], + self["member_code_override"], ], "open": [], # this option doesn't require any additional fields } @@ -155,6 +158,7 @@ def set_accordion(self, layout: Layout) -> None: layout.fields.remove("review_process") layout.fields.remove("member_code") + layout.fields.remove("member_code_override") # insert div+field at previously saved position layout.insert( @@ -167,6 +171,10 @@ def set_accordion(self, layout: Layout) -> None: ), ) + def set_display_member_code_override(self, visible: bool) -> None: + widget = forms.CheckboxInput() if visible else forms.HiddenInput() + self.fields["member_code_override"].widget = widget + def set_hr(self, layout: Layout) -> None: # add
around "underrepresented*" fields index = layout.fields.index("underrepresented") @@ -202,30 +210,51 @@ def validate_member_code( ) -> None | dict[str, ValidationError]: errors = dict() member_code = self.cleaned_data.get("member_code", "") + member_code_override = self.cleaned_data.get("member_code_override", False) error_msg = ( "This code is invalid. " "This may be due to a typo, an expired code, " "or a code that has not yet been activated. " "Please confirm that you have copied the code correctly, " - "or confirm the code with the Membership Contact for your group." + "or confirm the code with the Membership Contact for your group. " + "If the code seems to be correct, tick the checkbox below to ignore " + "this message." ) if not member_code: return None + member_code_valid = self.member_code_valid(member_code) + if member_code_valid and member_code_override: + # checkbox doesn't need to be ticked, so correct it quietly and continue + self.cleaned_data["member_code_override"] = False + self.set_display_member_code_override(False) + elif not member_code_valid: + self.set_display_member_code_override(True) + if not member_code_override: + # user must either correct the code or tick the override + errors["member_code"] = ValidationError(error_msg) + + return errors + + def member_code_valid(self, code: str) -> bool: + """Returns True if the code matches an active Membership, + including a grace period of 90 days before and after the Membership dates. + """ + try: # find relevant membership - may raise Membership.DoesNotExist - membership = Membership.objects.get(registration_code=member_code) + membership = Membership.objects.get(registration_code=code) # confirm that membership is currently active # grace period: 90 days before and after if not membership.active_on_date( date.today(), grace_before=90, grace_after=90 ): - errors["member_code"] = ValidationError(error_msg) + return False except Membership.DoesNotExist: - errors["member_code"] = ValidationError(error_msg) + return False - return errors + return True def clean(self): super().clean() diff --git a/amy/templates/includes/trainingrequest_details.html b/amy/templates/includes/trainingrequest_details.html index 5bfb508f0..c874c1dfc 100644 --- a/amy/templates/includes/trainingrequest_details.html +++ b/amy/templates/includes/trainingrequest_details.html @@ -26,6 +26,8 @@ {{ object.get_review_process_display|default:"—" }} Registration Code: {{ object.member_code|default:"—" }} + Continue with registration code marked as invalid: + {{ object.member_code_override|yesno }} Personal name: {{ object.personal }} Middle name: diff --git a/amy/workshops/migrations/0267_trainingrequest_accept_invalid_member_code.py b/amy/workshops/migrations/0267_trainingrequest_accept_invalid_member_code.py new file mode 100644 index 000000000..870243c14 --- /dev/null +++ b/amy/workshops/migrations/0267_trainingrequest_accept_invalid_member_code.py @@ -0,0 +1,22 @@ +# Generated by Django 3.2.20 on 2023-10-19 07:37 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("workshops", "0266_rename_group_name_trainingrequest_member_code"), + ] + + operations = [ + migrations.AddField( + model_name="trainingrequest", + name="member_code_override", + field=models.BooleanField( + blank=True, + default=False, + help_text="A member of our team will check the code and follow up with you if there are any problems that require your attention.", + verbose_name="Continue with registration code marked as invalid", + ), + ), + ] diff --git a/amy/workshops/models.py b/amy/workshops/models.py index 686dba82f..a4becefb0 100644 --- a/amy/workshops/models.py +++ b/amy/workshops/models.py @@ -2123,6 +2123,14 @@ class TrainingRequest( "a Carpentries member site or for a specific scheduled " "event, please enter it here:", ) + member_code_override = models.BooleanField( + null=False, + default=False, + blank=True, + verbose_name="Continue with registration code marked as invalid", + help_text="A member of our team will check the code and follow up with you if " + "there are any problems that require your attention.", + ) personal = models.CharField( max_length=STR_LONG, From fbb5880de41366dd78fa942134ab60212a11bb99 Mon Sep 17 00:00:00 2001 From: Eli Chadwick Date: Thu, 19 Oct 2023 12:56:14 +0100 Subject: [PATCH 2/4] add tests for soft validation --- amy/extforms/forms.py | 2 +- .../tests/test_training_request_form.py | 112 ++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/amy/extforms/forms.py b/amy/extforms/forms.py index c98622fdc..2692a1379 100644 --- a/amy/extforms/forms.py +++ b/amy/extforms/forms.py @@ -226,6 +226,7 @@ def validate_member_code( member_code_valid = self.member_code_valid(member_code) if member_code_valid and member_code_override: + # case where a user corrects their code but ticks the box anyway # checkbox doesn't need to be ticked, so correct it quietly and continue self.cleaned_data["member_code_override"] = False self.set_display_member_code_override(False) @@ -241,7 +242,6 @@ def member_code_valid(self, code: str) -> bool: """Returns True if the code matches an active Membership, including a grace period of 90 days before and after the Membership dates. """ - try: # find relevant membership - may raise Membership.DoesNotExist membership = Membership.objects.get(registration_code=code) diff --git a/amy/extforms/tests/test_training_request_form.py b/amy/extforms/tests/test_training_request_form.py index 55fee4997..a945f46c0 100644 --- a/amy/extforms/tests/test_training_request_form.py +++ b/amy/extforms/tests/test_training_request_form.py @@ -1,6 +1,7 @@ from datetime import date, timedelta from django.core import mail +from django.forms import CheckboxInput, HiddenInput from django.test import override_settings from django.urls import reverse @@ -134,6 +135,11 @@ def test_review_process_validation__preapproved_code_empty(self): rv, "Registration code is required for pre-approved training review process.", ) + # test that override field is not visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + HiddenInput, + ) def test_review_process_validation__open_code_nonempty(self): """Shouldn't pass when review_process requires *NO* member_code.""" @@ -151,6 +157,27 @@ def test_review_process_validation__open_code_nonempty(self): self.assertContains( rv, "Registration code must be empty for open training review process." ) + # test that override field is not visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + HiddenInput, + ) + + @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", False)]}) + def test_member_code_validation__not_enforced(self): + """Invalid code should pass if enforcement is not enabled.""" + # Arrange + data = { + "review_process": "preapproved", + "member_code": "invalid", + } + + # Act + rv = self.client.post(reverse("training_request"), data=data) + + # Assert + self.assertEqual(rv.status_code, 200) + self.assertNotContains(rv, self.INVALID_MEMBER_CODE_ERROR) @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) def test_member_code_validation__code_valid(self): @@ -168,6 +195,11 @@ def test_member_code_validation__code_valid(self): # Assert self.assertEqual(rv.status_code, 200) self.assertNotContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is not visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + HiddenInput, + ) @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) def test_member_code_validation__code_invalid(self): @@ -184,6 +216,11 @@ def test_member_code_validation__code_invalid(self): # Assert self.assertEqual(rv.status_code, 200) self.assertContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + CheckboxInput, + ) @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) def test_member_code_validation__code_inactive_early(self): @@ -203,6 +240,11 @@ def test_member_code_validation__code_inactive_early(self): # Assert self.assertEqual(rv.status_code, 200) self.assertContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + CheckboxInput, + ) @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) def test_member_code_validation__code_inactive_late(self): @@ -222,3 +264,73 @@ def test_member_code_validation__code_inactive_late(self): # Assert self.assertEqual(rv.status_code, 200) self.assertContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + CheckboxInput, + ) + + @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) + def test_member_code_validation__code_invalid_override(self): + """Invalid member code should be accepted when the override is ticked.""" + # Arrange + data = { + "review_process": "preapproved", + "member_code": "invalid", + "member_code_override": True, + } + + # Act + rv = self.client.post(reverse("training_request"), data=data) + + # Assert + self.assertEqual(rv.status_code, 200) + self.assertNotContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + CheckboxInput, + ) + + @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) + def test_member_code_validation__code_valid_override(self): + """Override should be quietly hidden if a valid code is used.""" + # Arrange + self.setUpMembership() + data = { + "review_process": "preapproved", + "member_code": "valid123", + "member_code_override": True, + } + + # Act + rv = self.client.post(reverse("training_request"), data=data) + + # Assert + self.assertEqual(rv.status_code, 200) + self.assertNotContains(rv, self.INVALID_MEMBER_CODE_ERROR) + # test that override field is not visible + self.assertEqual( + rv.context["form"].fields["member_code_override"].widget.__class__, + HiddenInput, + ) + + @override_settings(FLAGS={"ENFORCE_MEMBER_CODES": [("boolean", True)]}) + def test_member_code_validation__code_valid_override_full_request(self): + """Override should be quietly changed to False if a valid code is used + in a successful submission.""" + # Arrange + self.setUpMembership() + self.data["member_code"] = "valid123" + self.data["member_code_override"] = True + self.passCaptcha(self.data) + + # Act + rv = self.client.post(reverse("training_request"), data=self.data, follow=True) + + # Assert + self.assertEqual(rv.status_code, 200) + self.assertEqual(rv.resolver_match.view_name, "training_request_confirm") + self.assertFalse( + TrainingRequest.objects.get(member_code="valid123").member_code_override + ) From 551184da6fdc8bf7ab87195a72ac2c564bb02576 Mon Sep 17 00:00:00 2001 From: Eli Chadwick Date: Thu, 19 Oct 2023 15:57:31 +0100 Subject: [PATCH 3/4] appease the linter --- amy/fiscal/tests/test_membership.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/amy/fiscal/tests/test_membership.py b/amy/fiscal/tests/test_membership.py index da74a85dd..4e2c9bb18 100644 --- a/amy/fiscal/tests/test_membership.py +++ b/amy/fiscal/tests/test_membership.py @@ -1129,11 +1129,11 @@ def test_comment_added(self): comment = "Everything is awesome." data = {"new_agreement_end": date(2021, 3, 31), "comment": comment} today = date.today() - expected_comment = f"""Extended membership by 30 days on {today} (new end date: 2021-03-31). - ----- - -{comment}""" + expected_comment = ( + f"Extended membership by 30 days on {today} (new end date: 2021-03-31)." + "\n\n----\n\n" + f"{comment}" + ) # Act self.client.post(reverse("membership_extend", args=[membership.pk]), data=data) From 7dafdf93198014eb6aae54faa46a938d94d1a0d6 Mon Sep 17 00:00:00 2001 From: Eli Chadwick Date: Fri, 20 Oct 2023 11:13:58 +0100 Subject: [PATCH 4/4] improvements from code review --- amy/extforms/forms.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/amy/extforms/forms.py b/amy/extforms/forms.py index 2692a1379..d580e9940 100644 --- a/amy/extforms/forms.py +++ b/amy/extforms/forms.py @@ -110,7 +110,7 @@ def __init__(self, *args, **kwargs): self.set_other_fields(self.helper.layout) self.set_fake_required_fields() self.set_accordion(self.helper.layout) - self.set_display_member_code_override(False) + self.set_display_member_code_override(visible=False) self.set_hr(self.helper.layout) def set_other_field(self, field_name: str, layout: Layout) -> None: @@ -171,7 +171,7 @@ def set_accordion(self, layout: Layout) -> None: ), ) - def set_display_member_code_override(self, visible: bool) -> None: + def set_display_member_code_override(self, *, visible: bool) -> None: widget = forms.CheckboxInput() if visible else forms.HiddenInput() self.fields["member_code_override"].widget = widget @@ -229,9 +229,9 @@ def validate_member_code( # case where a user corrects their code but ticks the box anyway # checkbox doesn't need to be ticked, so correct it quietly and continue self.cleaned_data["member_code_override"] = False - self.set_display_member_code_override(False) + self.set_display_member_code_override(visible=False) elif not member_code_valid: - self.set_display_member_code_override(True) + self.set_display_member_code_override(visible=True) if not member_code_override: # user must either correct the code or tick the override errors["member_code"] = ValidationError(error_msg) @@ -245,15 +245,14 @@ def member_code_valid(self, code: str) -> bool: try: # find relevant membership - may raise Membership.DoesNotExist membership = Membership.objects.get(registration_code=code) - # confirm that membership is currently active - # grace period: 90 days before and after - if not membership.active_on_date( - date.today(), grace_before=90, grace_after=90 - ): - return False except Membership.DoesNotExist: return False + # confirm that membership is currently active + # grace period: 90 days before and after + if not membership.active_on_date(date.today(), grace_before=90, grace_after=90): + return False + return True def clean(self):