diff --git a/hknweb/forms.py b/hknweb/forms.py index 70e4c514..45498f04 100644 --- a/hknweb/forms.py +++ b/hknweb/forms.py @@ -17,6 +17,11 @@ import secrets +import functools +import operator + +from django.db.models import Q + from hknweb.models import User, Profile from hknweb.coursesemester.models import Semester from hknweb.utils import get_rand_photo @@ -200,9 +205,15 @@ def save(self): ) def email_to_username(email: str) -> str: + # Normalize to lowercase so that emails differing only in case (e.g. + # "Jane.Doe@berkeley.edu" vs "jane.doe@berkeley.edu") map to the same + # username, and so that the "@berkeley.edu" suffix check isn't case + # sensitive either. + normalized_email = email.lower() + username = None - if email.endswith(required_email_suffix): - username = email[: -len(required_email_suffix)] + if normalized_email.endswith(required_email_suffix.lower()): + username = normalized_email[: -len(required_email_suffix)] return username @@ -220,11 +231,19 @@ def email_to_username(email: str) -> str: row["username"] = username - existing_usernames = set( - User.objects.filter(username__in=usernames).values_list( + # Compare case-insensitively: a DB user "JaneDoe" should still be treated + # as a duplicate of a newly-derived username "janedoe". + existing_usernames_query = functools.reduce( + operator.or_, + (Q(username__iexact=username) for username in usernames), + Q(pk__in=[]), + ) + existing_usernames = { + username.lower() + for username in User.objects.filter(existing_usernames_query).values_list( "username", flat=True ) - ) + } # Setup account provisioning utils # Get candidate group to add users to @@ -240,7 +259,8 @@ def generate_password() -> str: email_information = [] for row in rows: - # If username is None or already exists, skip provisioning + # If username is None or already exists (including a duplicate earlier + # in this same batch), skip provisioning if (row["username"] is None) or (row["username"] in existing_usernames): continue @@ -257,6 +277,10 @@ def generate_password() -> str: ) user.save() + # Track this username so a later row in the same batch with the same + # email (differing only in case) is treated as a duplicate too. + existing_usernames.add(row["username"]) + # Add user to the candidates group group.user_set.add(user) diff --git a/tests/test_provision_candidates_form.py b/tests/test_provision_candidates_form.py new file mode 100644 index 00000000..c9723466 --- /dev/null +++ b/tests/test_provision_candidates_form.py @@ -0,0 +1,86 @@ +import io + +from django.conf import settings +from django.contrib.auth.models import Group, User +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import TestCase + +from hknweb.forms import ProvisionCandidatesForm + + +def _make_csv_upload(rows): + """Build an in-memory CSV upload matching the form's required columns. + + ``rows`` is a list of (first_name, last_name, email) tuples. + """ + buffer = io.StringIO() + buffer.write("First name,Last name,Berkeley email\r\n") + for first_name, last_name, email in rows: + buffer.write(f"{first_name},{last_name},{email}\r\n") + + return SimpleUploadedFile( + "candidates.csv", + buffer.getvalue().encode("utf-8"), + content_type="text/csv", + ) + + +class ProvisionCandidatesFormTests(TestCase): + def setUp(self): + Group.objects.get_or_create(name=settings.CAND_GROUP) + + def test_duplicate_emails_within_same_batch_differing_only_in_case(self): + # Two rows for the "same" candidate whose email only differs by case. + # Only one account should be created, and the form should not raise. + upload = _make_csv_upload( + [ + ("Jane", "Doe", "Jane.Doe@berkeley.edu"), + ("Jane", "Doe", "jane.doe@berkeley.edu"), + ], + ) + + form = ProvisionCandidatesForm(data={}, files={"file": upload}) + self.assertTrue(form.is_valid(), form.errors) + + form.save() + + self.assertEqual( + User.objects.filter(username__iexact="jane.doe").count(), + 1, + ) + self.assertEqual(len(form.email_information), 1) + + def test_duplicate_email_against_existing_user_differing_only_in_case(self): + # An account already exists with a differently-cased username. A new + # upload deriving the same username (case-insensitively) should be + # skipped instead of raising an IntegrityError. + User.objects.create_user( + username="John.Smith", + email="John.Smith@berkeley.edu", + password="irrelevant", + ) + + upload = _make_csv_upload([("John", "Smith", "john.smith@berkeley.edu")]) + + form = ProvisionCandidatesForm(data={}, files={"file": upload}) + self.assertTrue(form.is_valid(), form.errors) + + form.save() + + self.assertEqual( + User.objects.filter(username__iexact="john.smith").count(), + 1, + ) + self.assertEqual(len(form.email_information), 0) + + def test_new_candidate_is_still_created_normally(self): + upload = _make_csv_upload([("Alice", "Nguyen", "alice.nguyen@berkeley.edu")]) + + form = ProvisionCandidatesForm(data={}, files={"file": upload}) + self.assertTrue(form.is_valid(), form.errors) + + form.save() + + user = User.objects.get(username="alice.nguyen") + self.assertEqual(user.email, "alice.nguyen@berkeley.edu") + self.assertEqual(len(form.email_information), 1)