From ab1d34cd0a8ddb67e7a65cd6a41312e139d75a9c Mon Sep 17 00:00:00 2001 From: Bernard Siebens Date: Mon, 3 Aug 2026 22:42:19 +0200 Subject: [PATCH] Fix a 500 when two roster entries clash on jersey number team/season aren't TeamMembershipForm fields (the view sets them from the URL, not user input), so Django's own validate_unique() silently excludes both of them -- and with them, the whole unique_jersey_number_per_team_per_season constraint. A clashing jersey number reached the database unrejected and came back as a raw IntegrityError. form.clean() now checks it by hand, same as PositionForm already does for its own check constraint; the four roster/staff save paths also get a try/except IntegrityError backstop so any future gap degrades to an error banner instead of a 500. --- management/forms.py | 16 ++++++++++++++++ management/tests.py | 32 ++++++++++++++++++++++++++++++++ management/views.py | 34 +++++++++++++++++++++++++++++----- 3 files changed, 77 insertions(+), 5 deletions(-) diff --git a/management/forms.py b/management/forms.py index 3fe5f60..3609887 100644 --- a/management/forms.py +++ b/management/forms.py @@ -38,6 +38,8 @@ class TeamMembershipForm(forms.ModelForm): def __init__(self, *args, club=None, team=None, season=None, **kwargs): super().__init__(*args, **kwargs) + self.team = team + self.season = season members = Member.objects.filter(member_of__club=club).distinct() if team is not None and season is not None: # Already on this team's roster this season -- offering them again @@ -47,6 +49,20 @@ class TeamMembershipForm(forms.ModelForm): self.fields["member"].queryset = members self.fields["position"].queryset = Position.objects.filter(club=club, staff_position=False) + def clean(self): + cleaned = super().clean() + # team/season aren't form fields (the view sets them from the URL, not user + # input), so Django's automatic validate_unique() excludes both of them -- + # and with them, the whole unique_jersey_number_per_team_per_season check. + # Without this, a clashing jersey number reaches the database unrejected + # and surfaces as a raw IntegrityError instead of a form error. + jersey_number = cleaned.get("jersey_number") + if jersey_number is not None and self.team is not None and self.season is not None: + clash = TeamMembership.objects.filter(team=self.team, season=self.season, jersey_number=jersey_number).exclude(pk=self.instance.pk).exists() + if clash: + self.add_error("jersey_number", _("Another player on this team already has this jersey number this season.")) + return cleaned + class StaffAssignmentForm(forms.ModelForm): """Assign/edit one staff assignment -- team and season come from the view, diff --git a/management/tests.py b/management/tests.py index a7b1608..c68cbec 100644 --- a/management/tests.py +++ b/management/tests.py @@ -386,6 +386,38 @@ class TeamRosterStaffTests(ManagementTestBase): self.assertEqual(response.status_code, 302) self.assertEqual(TeamMembership.objects.filter(team=self.team, season=self.season).count(), 1) + def test_a_duplicate_jersey_number_fails_with_a_form_error_not_a_500(self): + # team/season aren't TeamMembershipForm fields, so Django's own + # validate_unique() can't see unique_jersey_number_per_team_per_season -- + # this constraint only gets checked because the form does it by hand. + other_player = Member.objects.create(first_name="Olly", last_name="Other") + ClubMembership.objects.create(club=self.club, member=other_player, season=self.season, status=ClubMembership.StatusChoices.ACTIVE) + TeamMembership.objects.create(team=self.team, season=self.season, member=self.player, position=self.player_position, jersey_number=7) + self.client.force_login(self.admin_user) + + response = self.club_post("team_roster_add", {"member": str(other_player.pk), "position": str(self.player_position.pk), "jersey_number": "7"}, self.team.pk, self.season.pk) + + self.assertEqual(response.status_code, 302) + self.assertFalse(TeamMembership.objects.filter(team=self.team, season=self.season, member=other_player).exists()) + + def test_editing_a_roster_entry_to_a_clashing_jersey_number_fails_gracefully(self): + other_player = Member.objects.create(first_name="Olly", last_name="Other") + ClubMembership.objects.create(club=self.club, member=other_player, season=self.season, status=ClubMembership.StatusChoices.ACTIVE) + TeamMembership.objects.create(team=self.team, season=self.season, member=self.player, position=self.player_position, jersey_number=7) + other_membership = TeamMembership.objects.create(team=self.team, season=self.season, member=other_player, position=self.player_position, jersey_number=8) + self.client.force_login(self.admin_user) + + response = self.club_post( + "team_roster_update", + {"member": str(other_player.pk), "position": str(self.player_position.pk), "jersey_number": "7"}, + self.team.pk, + other_membership.pk, + ) + + self.assertEqual(response.status_code, 302) + other_membership.refresh_from_db() + self.assertEqual(other_membership.jersey_number, 8) + def test_editing_a_roster_entry_updates_it(self): membership = TeamMembership.objects.create(team=self.team, season=self.season, member=self.player, position=self.player_position, jersey_number=9) self.client.force_login(self.admin_user) diff --git a/management/views.py b/management/views.py index 9318e68..824e847 100644 --- a/management/views.py +++ b/management/views.py @@ -1,4 +1,4 @@ -from django.db import transaction +from django.db import IntegrityError, transaction from django.db.models import Count, ProtectedError from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect, render @@ -808,7 +808,16 @@ class TeamRosterAddView(TeamManagerRequiredMixin, FormView): return redirect(self.team_detail_url()) def form_valid(self, form): - form.save() + try: + form.save() + except IntegrityError: + # Belt and braces: form.clean() already checks jersey-number and + # member uniqueness by hand (team/season aren't form fields, so + # Django's own validate_unique() can't see those constraints) -- + # this is the backstop for whatever that doesn't catch. + notify(self.request, f"e|{_('Could not add player')}|{_('That player could not be added -- please check the details and try again.')}") + return redirect(self.team_detail_url()) + body = _("“%(member)s” added to the roster.") % {"member": form.instance.member} notify(self.request, f"s|{_('Player added')}|{body}") return redirect(self.team_detail_url()) @@ -837,7 +846,12 @@ class TeamRosterUpdateView(TeamManagerRequiredMixin, FormView): return redirect(self.team_detail_url()) def form_valid(self, form): - form.save() + try: + form.save() + except IntegrityError: + notify(self.request, f"e|{_('Could not update player')}|{_('That change could not be saved -- please check the details and try again.')}") + return redirect(self.team_detail_url()) + body = _("“%(member)s” updated.") % {"member": form.instance.member} notify(self.request, f"s|{_('Player updated')}|{body}") return redirect(self.team_detail_url()) @@ -881,7 +895,12 @@ class TeamStaffAddView(TeamManagerRequiredMixin, FormView): return redirect(self.team_detail_url()) def form_valid(self, form): - form.save() + try: + form.save() + except IntegrityError: + notify(self.request, f"e|{_('Could not assign staff')}|{_('That assignment could not be saved -- please check the details and try again.')}") + return redirect(self.team_detail_url()) + body = _("“%(member)s” assigned as staff.") % {"member": form.instance.member} notify(self.request, f"s|{_('Staff assigned')}|{body}") return redirect(self.team_detail_url()) @@ -910,7 +929,12 @@ class TeamStaffUpdateView(TeamManagerRequiredMixin, FormView): return redirect(self.team_detail_url()) def form_valid(self, form): - form.save() + try: + form.save() + except IntegrityError: + notify(self.request, f"e|{_('Could not update staff assignment')}|{_('That change could not be saved -- please check the details and try again.')}") + return redirect(self.team_detail_url()) + body = _("“%(member)s” updated.") % {"member": form.instance.member} notify(self.request, f"s|{_('Staff assignment updated')}|{body}") return redirect(self.team_detail_url())