Skip to content
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

Implement soft validation for member codes in training requests #2549

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions amy/extforms/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class Meta:
fields = (
"review_process",
"member_code",
"member_code_override",
"personal",
"family",
"email",
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
}
Expand All @@ -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(
Expand All @@ -167,6 +171,10 @@ def set_accordion(self, layout: Layout) -> None:
),
)

def set_display_member_code_override(self, visible: bool) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For functions that accept int or boolean arguments I prefer to make them required as keyword by using * in the function definition:
https://realpython.com/python-asterisk-and-slash-special-parameters/#in-short-the-python-asterisk-and-slash-control-how-to-pass-values-to-functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I've never come across this syntax before. I see the appeal

widget = forms.CheckboxInput() if visible else forms.HiddenInput()
self.fields["member_code_override"].widget = widget

def set_hr(self, layout: Layout) -> None:
# add <HR> around "underrepresented*" fields
index = layout.fields.index("underrepresented")
Expand Down Expand Up @@ -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:
# 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)
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put just the line membership = Membership.objects.get(registration_code=code) inside of the try-except block. The conditional if not membership.active_on_date(...) could be outside of that block.

except Membership.DoesNotExist:
errors["member_code"] = ValidationError(error_msg)
return False

return errors
return True

def clean(self):
super().clean()
Expand Down
112 changes: 112 additions & 0 deletions amy/extforms/tests/test_training_request_form.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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."""
Expand All @@ -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):
Expand All @@ -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):
Expand All @@ -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):
Expand All @@ -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):
Expand All @@ -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
)
10 changes: 5 additions & 5 deletions amy/fiscal/tests/test_membership.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions amy/templates/includes/trainingrequest_details.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
<td>{{ object.get_review_process_display|default:"&mdash;" }}</td></tr>
<tr><th>Registration Code:</th>
<td>{{ object.member_code|default:"&mdash;" }}</td></tr>
<tr><th>Continue with registration code marked as invalid:</th>
<td>{{ object.member_code_override|yesno }}</td></tr>
<tr><th>Personal name:</th>
<td>{{ object.personal }}</td></tr>
<tr><th>Middle name:</th>
Expand Down
Original file line number Diff line number Diff line change
@@ -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",
),
),
]
8 changes: 8 additions & 0 deletions amy/workshops/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down