bernard/RosterChief
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix a 500 when two roster entries clash on jersey number

Browse Source 
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.
This commit is contained in:
Bernard Siebens
2026-08-03 22:42:19 +02:00
parent 054e1cb1be
commit ab1d34cd0a
3 changed files with 77 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
management/forms.py
View File

@@ -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,

32
management/tests.py
View File

@@ -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)

26
management/views.py
View File

@@ -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):
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):
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):
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):
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())