fix(data): reparer les trois suppressions cassees

DATA-004, DATA-005, DATA-006. L'audit les classait "forte probabilite" faute
de pouvoir les executer. Les tests les confirment : ce sont des bugs averes,
declenchables par tout manager ou administrateur depuis l'interface.

Erreurs reellement obtenues avant correction :
  NOT NULL constraint failed: match_participants.match_id
  NOT NULL constraint failed: team_matches.org_team_id

Supprimer un match (DATA-004)
  Match.participants n'avait pas de cascade. SQLAlchemy tentait donc de
  detacher les participants en mettant match_id a NULL, ce que la colonne
  refuse. Tout match ayant eu des participants etait indestructible.
  TeamMatch.participants declarait deja delete-orphan ; Match non.

Supprimer un tryout (DATA-006)
  Les PersonalNote pointant vers ses matchs, equipes ou vers lui-meme
  n'etaient pas traitees.

Supprimer une equipe (DATA-005)
  TeamNote.org_team_id et TeamMatch.org_team_id sont NOT NULL et n'etaient
  pas traites du tout. De plus la fonction validait trois fois : un echec au
  troisieme temps laissait les tryouts detaches et les joueurs retires sans
  que l'equipe soit supprimee -- un etat incoherent que rien ne rattrapait.
  Une seule transaction desormais.

Regle appliquee, uniforme
  Ce qui n'a de sens que dans le parent est supprime avec lui : participants,
  membres, notes d'equipe, matchs de saison.
  Ce qui lui survit est seulement detache : les notes personnelles sont les
  observations d'un coach sur un joueur, pas des donnees de tryout. Les
  supprimer avec le tryout detruirait du contenu sans rapport. Idem pour les
  contrats et les demandes de rencontre individuelle.

Fidelite des tests
  conftest.py active PRAGMA foreign_keys=ON. SQLite ignore les cles
  etrangeres par defaut ; PostgreSQL les applique toujours. Sans ce reglage,
  la suite pouvait valider une suppression qui echoue en production --
  precisement la classe de bug corrigee ici. Les 146 tests passent avec les
  contraintes actives.

9 tests, dont trois qui verifient que les entites survivantes survivent
vraiment : une note garde son contenu et perd son contexte, un tryout
survit a l'equipe qu'il visait.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
2026-08-07 20:37:04 -04:00
co-authored by Claude Opus 5
parent a92600c305
commit d15a3de2a1
6 changed files with 269 additions and 17 deletions
+7 -1
View File
@@ -16,7 +16,13 @@ class Match(BaseMatch):
tryout = db.relationship('Tryout', backref='matches') tryout = db.relationship('Tryout', backref='matches')
team1 = db.relationship('Team', foreign_keys=[team1_id], backref='matches_as_team1') team1 = db.relationship('Team', foreign_keys=[team1_id], backref='matches_as_team1')
team2 = db.relationship('Team', foreign_keys=[team2_id], backref='matches_as_team2') team2 = db.relationship('Team', foreign_keys=[team2_id], backref='matches_as_team2')
participants = db.relationship('MatchParticipant', backref='match', lazy='dynamic') # delete-orphan: without it, SQLAlchemy tries to detach participants by
# setting match_id to NULL, which the NOT NULL column refuses — so
# deleting any match that had participants raised IntegrityError.
# TeamMatch.participants already declared this; Match did not.
participants = db.relationship(
'MatchParticipant', backref='match', lazy='dynamic',
cascade='all, delete-orphan')
def get_participating_players(self): def get_participating_players(self):
return [p.player_id for p in self.participants.all()] return [p.player_id for p in self.participants.all()]
+9 -1
View File
@@ -10,7 +10,7 @@ from app.models import (
Admin, Manager, Coach, Player, Scout, Admin, Manager, Coach, Player, Scout,
User, Tryout, Match, MatchParticipant, Team, TeamMember, User, Tryout, Match, MatchParticipant, Team, TeamMember,
TryoutRegistration, PlayerDisponibility, TryoutRegistration, PlayerDisponibility,
OneOnOneRequest, OneOnOneRequest, PersonalNote,
) )
from datetime import datetime, timedelta from datetime import datetime, timedelta
from app.discord_bot import send_schedule_notification from app.discord_bot import send_schedule_notification
@@ -507,6 +507,14 @@ def delete_match(match_id):
if tryout.is_ended: if tryout.is_ended:
flash('This tryout has ended. Matches can no longer be deleted.', 'danger') flash('This tryout has ended. Matches can no longer be deleted.', 'danger')
return redirect(url_for('tryouts.view_tryout', tryout_id=tryout.id)) return redirect(url_for('tryouts.view_tryout', tryout_id=tryout.id))
# Notes outlive the match they were taken during: a coach's observation
# keeps its value, and deleting it here would destroy unrelated content.
# Only the context link is dropped. Participants go through the
# relationship's delete-orphan cascade.
PersonalNote.query.filter_by(match_id=match_id).update(
{'match_id': None}, synchronize_session=False)
db.session.delete(match) db.session.delete(match)
db.session.commit() db.session.commit()
flash('Match deleted successfully.', 'success') flash('Match deleted successfully.', 'success')
+22 -6
View File
@@ -9,6 +9,7 @@ from app.extensions import db
from app.models import ( from app.models import (
Admin, Manager, Coach, Player, Admin, Manager, Coach, Player,
OrgTeam, User, PersonalNote, TeamNote, Tryout, TeamPlayer, OrgTeam, User, PersonalNote, TeamNote, Tryout, TeamPlayer,
TeamMatch, Contract, OneOnOneRequest,
) )
from datetime import datetime from datetime import datetime
@@ -209,13 +210,28 @@ def delete_team(team_id):
team = OrgTeam.query.get_or_404(team_id) team = OrgTeam.query.get_or_404(team_id)
name = team.name name = team.name
tryouts = Tryout.query.filter_by(target_org_team_id=team_id).all() # One transaction. This used to commit three times, so a failure at the
for t in tryouts: # third step left the tryouts detached and the players removed without
t.target_org_team_id = None # the team being deleted — an inconsistent state nothing could undo.
db.session.commit() #
# TeamNote.org_team_id and TeamMatch.org_team_id are NOT NULL, and were
# not handled at all: deleting a team that had ever been used raised
# IntegrityError. Contract.team_id and OneOnOneRequest.org_team_id are
# nullable, and the rows outlive the team, so they are only detached.
TeamPlayer.query.filter_by(org_team_id=team_id).delete() # Entities that only make sense as part of the team.
db.session.commit() TeamNote.query.filter_by(org_team_id=team_id).delete(synchronize_session=False)
for team_match in TeamMatch.query.filter_by(org_team_id=team_id).all():
db.session.delete(team_match) # participants follow by cascade
TeamPlayer.query.filter_by(org_team_id=team_id).delete(synchronize_session=False)
# Entities that survive it.
Tryout.query.filter_by(target_org_team_id=team_id).update(
{'target_org_team_id': None}, synchronize_session=False)
Contract.query.filter_by(team_id=team_id).update(
{'team_id': None}, synchronize_session=False)
OneOnOneRequest.query.filter_by(org_team_id=team_id).update(
{'org_team_id': None}, synchronize_session=False)
db.session.delete(team) db.session.delete(team)
db.session.commit() db.session.commit()
+15 -9
View File
@@ -10,7 +10,7 @@ from app.extensions import db
from app.models import ( from app.models import (
Admin, Manager, Coach, Player, Scout, Admin, Manager, Coach, Player, Scout,
User, Tryout, TryoutRegistration, Evaluation, Team, TeamMember, User, Tryout, TryoutRegistration, Evaluation, Team, TeamMember,
OrgTeam, Match, MatchParticipant, OrgTeam, Match, MatchParticipant, PersonalNote,
ESPORT_GAMES, GAME_POSITIONS, ESPORT_GAMES, GAME_POSITIONS,
) )
from datetime import datetime from datetime import datetime
@@ -505,28 +505,34 @@ def delete_tryout(tryout_id):
flash('You do not have permission to delete this tryout.', 'danger') flash('You do not have permission to delete this tryout.', 'danger')
return redirect(url_for('tryouts.list_tryouts')) return redirect(url_for('tryouts.list_tryouts'))
# Delete match participants for all matches in this tryout
match_ids = [m.id for m in Match.query.filter_by(tryout_id=tryout_id).all()] match_ids = [m.id for m in Match.query.filter_by(tryout_id=tryout_id).all()]
team_ids = [t.id for t in Team.query.filter_by(tryout_id=tryout_id).all()]
# Personal notes outlive the tryout: they are a coach's observations
# about a player, not tryout data. Only their context links are cleared.
# Missing this step made the deletion fail on the foreign keys below.
PersonalNote.query.filter_by(tryout_id=tryout_id).update(
{'tryout_id': None}, synchronize_session=False)
if match_ids:
PersonalNote.query.filter(PersonalNote.match_id.in_(match_ids)).update(
{'match_id': None}, synchronize_session=False)
if team_ids:
PersonalNote.query.filter(PersonalNote.team_id.in_(team_ids)).update(
{'team_id': None}, synchronize_session=False)
if match_ids: if match_ids:
MatchParticipant.query.filter( MatchParticipant.query.filter(
MatchParticipant.match_id.in_(match_ids) MatchParticipant.match_id.in_(match_ids)
).delete(synchronize_session=False) ).delete(synchronize_session=False)
# Delete matches
Match.query.filter(Match.id.in_(match_ids)).delete(synchronize_session=False) Match.query.filter(Match.id.in_(match_ids)).delete(synchronize_session=False)
# Delete team members for all teams in this tryout
team_ids = [t.id for t in Team.query.filter_by(tryout_id=tryout_id).all()]
if team_ids: if team_ids:
TeamMember.query.filter( TeamMember.query.filter(
TeamMember.team_id.in_(team_ids) TeamMember.team_id.in_(team_ids)
).delete(synchronize_session=False) ).delete(synchronize_session=False)
# Delete teams
Team.query.filter(Team.id.in_(team_ids)).delete(synchronize_session=False) Team.query.filter(Team.id.in_(team_ids)).delete(synchronize_session=False)
# Delete registrations
TryoutRegistration.query.filter_by(tryout_id=tryout_id).delete() TryoutRegistration.query.filter_by(tryout_id=tryout_id).delete()
# Delete evaluations
Evaluation.query.filter_by(tryout_id=tryout_id).delete() Evaluation.query.filter_by(tryout_id=tryout_id).delete()
db.session.delete(tryout) db.session.delete(tryout)
+20
View File
@@ -14,11 +14,31 @@ import pytest
# Make the project root importable as the 'app' package. # Make the project root importable as the 'app' package.
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
from sqlalchemy import event # noqa: E402
from sqlalchemy.engine import Engine # noqa: E402
from app.app import create_app # noqa: E402 from app.app import create_app # noqa: E402
from app.extensions import db as _db # noqa: E402 from app.extensions import db as _db # noqa: E402
from app.models import Admin, Coach, Manager, Player, Scout # noqa: E402 from app.models import Admin, Coach, Manager, Player, Scout # noqa: E402
@event.listens_for(Engine, 'connect')
def _enforce_sqlite_foreign_keys(dbapi_connection, connection_record):
"""Make SQLite behave like PostgreSQL about foreign keys.
SQLite ignores foreign key constraints unless asked not to. Production
runs on PostgreSQL, which always enforces them, so without this a test
suite could pass over a deletion that fails in production exactly the
class of bug DATA-004 and DATA-005 turned out to be.
"""
import sqlite3
if isinstance(dbapi_connection, sqlite3.Connection):
cursor = dbapi_connection.cursor()
cursor.execute('PRAGMA foreign_keys=ON')
cursor.close()
ROLE_CLASSES = { ROLE_CLASSES = {
'admin': Admin, 'admin': Admin,
'manager': Manager, 'manager': Manager,
+196
View File
@@ -0,0 +1,196 @@
"""Deletion of entities that other rows point at.
DATA-004, DATA-005, DATA-006. Three delete routes clean up some of their
dependants and ignore others. Every ignored one is a foreign key pointing
at the row being removed, so the delete raises an IntegrityError, which
surfaces as a 500 and rolls back the entity simply cannot be deleted once
it has been used.
These are reachable through the interface by any manager or admin, and the
audit could only rate them "likely" without executing them. That is what
this file is for.
"""
from datetime import date, time
import pytest
from app.extensions import db
from app.models import (
Match, MatchParticipant, OrgTeam, PersonalNote, Team, TeamMatch,
TeamMember, TeamNote, TeamPlayer, Tryout, TryoutRegistration,
)
@pytest.fixture
def world(app, make_user):
"""A tryout with a team, a match, a participant and a note on each.
Deliberately mirrors what an actual tryout looks like after a session:
the empty case already worked, the populated one is what breaks.
"""
admin_id = make_user('admin')
coach_id = make_user('coach')
player_id = make_user('player')
with app.app_context():
org_team = OrgTeam(name='Varsity', created_by=admin_id, coach_id=coach_id)
db.session.add(org_team)
db.session.flush()
tryout = Tryout(title='Spring', game='Valorant', date=date(2030, 4, 1),
created_by=admin_id, target_org_team_id=org_team.id)
db.session.add(tryout)
db.session.flush()
team = Team(tryout_id=tryout.id, name='Alpha', created_by=admin_id)
db.session.add(team)
db.session.flush()
match = Match(tryout_id=tryout.id, title='Scrim', date=date(2030, 4, 2),
start_time=time(18, 0), match_type='player_scrim',
created_by=admin_id)
db.session.add(match)
db.session.flush()
db.session.add_all([
TryoutRegistration(tryout_id=tryout.id, player_id=player_id),
TeamMember(team_id=team.id, player_id=player_id),
MatchParticipant(match_id=match.id, player_id=player_id),
TeamPlayer(player_id=player_id, org_team_id=org_team.id),
TeamNote(org_team_id=org_team.id, coach_id=coach_id, content='Team note'),
TeamMatch(org_team_id=org_team.id, title='Season match',
date=date(2030, 4, 5), created_by=admin_id),
# A note referencing all three contexts at once.
PersonalNote(player_id=player_id, coach_id=coach_id,
content='Watch the entries',
match_id=match.id, team_id=team.id, tryout_id=tryout.id),
])
db.session.commit()
return {
'admin_id': admin_id, 'coach_id': coach_id, 'player_id': player_id,
'org_team_id': org_team.id, 'tryout_id': tryout.id,
'team_id': team.id, 'match_id': match.id,
}
def _login_admin(client, app, admin_id, login):
from app.models import User
with app.app_context():
username = db.session.get(User, admin_id).username
login(username)
class TestDeleteMatch:
"""DATA-004 — Match.participants had no cascade, and PersonalNote.match_id
references matches.id."""
def test_a_populated_match_can_be_deleted(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
response = client.post(f"/matches/{world['match_id']}/delete",
follow_redirects=False)
assert response.status_code < 500, 'deleting a used match raised'
with app.app_context():
assert db.session.get(Match, world['match_id']) is None
def test_its_participants_go_with_it(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
client.post(f"/matches/{world['match_id']}/delete", follow_redirects=True)
with app.app_context():
assert MatchParticipant.query.filter_by(
match_id=world['match_id']).count() == 0
def test_notes_survive_but_lose_their_match_context(self, app, client, world, login):
"""A coach's observation keeps its value once the match is gone;
deleting it with the match would destroy unrelated content."""
_login_admin(client, app, world['admin_id'], login)
client.post(f"/matches/{world['match_id']}/delete", follow_redirects=True)
with app.app_context():
note = PersonalNote.query.filter_by(player_id=world['player_id']).one()
assert note.content == 'Watch the entries'
assert note.match_id is None
class TestDeleteTryout:
"""DATA-006 — delete_tryout removed matches, teams, registrations and
evaluations, but not the PersonalNote rows pointing at any of them."""
def test_a_populated_tryout_can_be_deleted(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
response = client.post(f"/tryouts/{world['tryout_id']}/delete",
follow_redirects=False)
assert response.status_code < 500
with app.app_context():
assert db.session.get(Tryout, world['tryout_id']) is None
def test_its_matches_and_teams_go_with_it(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
client.post(f"/tryouts/{world['tryout_id']}/delete", follow_redirects=True)
with app.app_context():
assert db.session.get(Match, world['match_id']) is None
assert db.session.get(Team, world['team_id']) is None
assert TryoutRegistration.query.filter_by(
tryout_id=world['tryout_id']).count() == 0
def test_notes_survive_the_tryout(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
client.post(f"/tryouts/{world['tryout_id']}/delete", follow_redirects=True)
with app.app_context():
note = PersonalNote.query.filter_by(player_id=world['player_id']).one()
assert note.tryout_id is None
assert note.match_id is None
assert note.team_id is None
class TestDeleteTeam:
"""DATA-005 — delete_team handled tryouts and TeamPlayer, but not
TeamNote or TeamMatch, both of which are NOT NULL foreign keys. It also
committed three times, so a failure at the third step left the tryouts
detached and the players removed without deleting the team."""
def test_a_populated_team_can_be_deleted(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
response = client.post(f"/teams/{world['org_team_id']}/delete",
follow_redirects=False)
assert response.status_code < 500
with app.app_context():
assert db.session.get(OrgTeam, world['org_team_id']) is None
def test_its_notes_and_season_matches_go_with_it(self, app, client, world, login):
_login_admin(client, app, world['admin_id'], login)
client.post(f"/teams/{world['org_team_id']}/delete", follow_redirects=True)
with app.app_context():
assert TeamNote.query.filter_by(
org_team_id=world['org_team_id']).count() == 0
assert TeamMatch.query.filter_by(
org_team_id=world['org_team_id']).count() == 0
assert TeamPlayer.query.filter_by(
org_team_id=world['org_team_id']).count() == 0
def test_the_tryout_survives_and_is_detached(self, app, client, world, login):
"""A tryout outlives the team it was aimed at."""
_login_admin(client, app, world['admin_id'], login)
client.post(f"/teams/{world['org_team_id']}/delete", follow_redirects=True)
with app.app_context():
tryout = db.session.get(Tryout, world['tryout_id'])
assert tryout is not None
assert tryout.target_org_team_id is None