diff --git a/app/models/match_model/match.py b/app/models/match_model/match.py index caa22e1..7d63eb3 100644 --- a/app/models/match_model/match.py +++ b/app/models/match_model/match.py @@ -16,7 +16,13 @@ class Match(BaseMatch): tryout = db.relationship('Tryout', backref='matches') team1 = db.relationship('Team', foreign_keys=[team1_id], backref='matches_as_team1') 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): return [p.player_id for p in self.participants.all()] \ No newline at end of file diff --git a/app/routes/matches.py b/app/routes/matches.py index 6c42699..35b931c 100644 --- a/app/routes/matches.py +++ b/app/routes/matches.py @@ -10,7 +10,7 @@ from app.models import ( Admin, Manager, Coach, Player, Scout, User, Tryout, Match, MatchParticipant, Team, TeamMember, TryoutRegistration, PlayerDisponibility, - OneOnOneRequest, + OneOnOneRequest, PersonalNote, ) from datetime import datetime, timedelta from app.discord_bot import send_schedule_notification @@ -507,6 +507,14 @@ def delete_match(match_id): if tryout.is_ended: flash('This tryout has ended. Matches can no longer be deleted.', 'danger') 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.commit() flash('Match deleted successfully.', 'success') diff --git a/app/routes/teams.py b/app/routes/teams.py index 93c1ef5..bd7e75c 100644 --- a/app/routes/teams.py +++ b/app/routes/teams.py @@ -9,6 +9,7 @@ from app.extensions import db from app.models import ( Admin, Manager, Coach, Player, OrgTeam, User, PersonalNote, TeamNote, Tryout, TeamPlayer, + TeamMatch, Contract, OneOnOneRequest, ) from datetime import datetime @@ -209,13 +210,28 @@ def delete_team(team_id): team = OrgTeam.query.get_or_404(team_id) name = team.name - tryouts = Tryout.query.filter_by(target_org_team_id=team_id).all() - for t in tryouts: - t.target_org_team_id = None - db.session.commit() + # One transaction. This used to commit three times, so a failure at the + # third step left the tryouts detached and the players removed without + # the team being deleted — an inconsistent state nothing could undo. + # + # 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() - db.session.commit() + # Entities that only make sense as part of the team. + 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.commit() diff --git a/app/routes/tryouts.py b/app/routes/tryouts.py index 501c204..2a2c36c 100644 --- a/app/routes/tryouts.py +++ b/app/routes/tryouts.py @@ -10,7 +10,7 @@ from app.extensions import db from app.models import ( Admin, Manager, Coach, Player, Scout, User, Tryout, TryoutRegistration, Evaluation, Team, TeamMember, - OrgTeam, Match, MatchParticipant, + OrgTeam, Match, MatchParticipant, PersonalNote, ESPORT_GAMES, GAME_POSITIONS, ) from datetime import datetime @@ -505,28 +505,34 @@ def delete_tryout(tryout_id): flash('You do not have permission to delete this tryout.', 'danger') 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()] + 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: MatchParticipant.query.filter( MatchParticipant.match_id.in_(match_ids) ).delete(synchronize_session=False) - # Delete matches 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: TeamMember.query.filter( TeamMember.team_id.in_(team_ids) ).delete(synchronize_session=False) - # Delete teams Team.query.filter(Team.id.in_(team_ids)).delete(synchronize_session=False) - # Delete registrations TryoutRegistration.query.filter_by(tryout_id=tryout_id).delete() - - # Delete evaluations Evaluation.query.filter_by(tryout_id=tryout_id).delete() db.session.delete(tryout) diff --git a/tests/conftest.py b/tests/conftest.py index cfa021b..12c9d44 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,11 +14,31 @@ import pytest # Make the project root importable as the 'app' package. 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.extensions import db as _db # 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 = { 'admin': Admin, 'manager': Manager, diff --git a/tests/test_deletions.py b/tests/test_deletions.py new file mode 100644 index 0000000..1d04b85 --- /dev/null +++ b/tests/test_deletions.py @@ -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