From 92d72e4d4875d25e66864f8ab08ba046bb125d9d Mon Sep 17 00:00:00 2001 From: GGThed Date: Sat, 8 Aug 2026 14:06:19 -0400 Subject: [PATCH] refactor(authz): un seul point de verite pour les autorisations d equipe ARCH-002. La question "a quelles equipes ce coach est-il rattache ?" etait posee a huit endroits, de cinq facons differentes, et trois d entre elles donnaient une mauvaise reponse en production. Le motif fautif, present tel quel dans six routes : OrgTeam.query.filter_by(coach_id=user.id).first() Il repond au plus une equipe, et seulement par la colonne heritee. Deux pannes en decoulaient, silencieuses -- les pages s affichaient, vides : - un coach rattache uniquement par la relation many-to-many n avait aucune equipe, donc aucun joueur, aucun contrat, aucune note d equipe, aucun match a venir sur son tableau de bord ; - un coach de deux equipes n en voyait qu une. Le formulaire de contrat lui proposait la moitie de son effectif, alors que la route POST acceptait l autre moitie. app/permissions.py devient le module ou la question se pose une fois : coach_org_teams, manager_org_teams, attached_org_teams, visible_org_teams, can_manage_org_team, org_team_player_ids, coach_player_ids, can_manage_player_contract, coach_tryouts, coach_manages_tryout. Toutes lisent les deux rattachements et toutes les equipes. Le meme ecart existait dans le modele : Coach.get_visible_tryouts ne lisait que la relation m2m -- calendrier vide pour un coach rattache par la colonne -- et can_manage_this_tryout ignorait la colonne pour l equipe cible. Les deux delegent desormais. Corrections de portee, au passage - one_on_one lisait org_team.coach_id : un joueur dont l equipe declare ses coachs par la relation etait informe qu il n avait pas de coach, et le formulaire de demande restait ferme. Passe par get_coaches(), qui retombe deja sur la colonne heritee. - notes_dashboard conditionnait les notes personnelles du coach a l existence d une equipe : un coach sans equipe ne voyait pas ses propres notes. - can_manage_team_match reformulait can_manage_this_org_team ; la reformulation avait derive. Elle appelle maintenant la regle. Limite assumee : le panneau de notes d equipe reste ecrit pour une seule equipe et affiche donc la premiere. La resolution est corrigee, la mise en page multi-equipes ne l est pas -- c est un choix produit, pas un bug. 14 tests ajoutes. Cinq echouent sur le code d avant, verifie en remettant les routes et le modele a leur etat precedent. ARCH-001 fera disparaitre la colonne heritee ; cela demande une migration de donnees, donc Alembic. D ici la, ce module est ce qui rend la duplication inoffensive. Co-Authored-By: Claude Opus 5 --- app/models/user_model/coach.py | 47 ++---- app/permissions.py | 291 +++++++++++++++++++++++++++++---- app/routes/main.py | 14 +- app/routes/team_matches.py | 47 ++---- app/routes/teams.py | 23 +-- app/routes/users.py | 92 +++++------ tests/test_permissions.py | 203 ++++++++++++++++++++++- 7 files changed, 549 insertions(+), 168 deletions(-) diff --git a/app/models/user_model/coach.py b/app/models/user_model/coach.py index 57c129d..693d8a0 100644 --- a/app/models/user_model/coach.py +++ b/app/models/user_model/coach.py @@ -1,10 +1,18 @@ -"""Coach — evaluates, schedules matches, manages their own org team.""" +"""Coach — evaluates, schedules matches, manages their own org teams.""" from app.models.user_model.user import User -from app.extensions import db class Coach(User): - """Coach — evaluates, schedules matches, manages their own org team.""" + """Coach — evaluates, schedules matches, manages their own org teams. + + Every question about *which* teams or tryouts belong to this coach is + delegated to ``app.permissions``. Two of the three methods below used to + answer it themselves, each considering a different subset of the two + ways a coach can be attached to a team: ``can_manage_this_tryout`` + ignored the legacy ``coach_id`` column when checking the target team, + and ``get_visible_tryouts`` ignored it entirely. A coach attached only + by that column therefore saw an empty calendar (ARCH-002). + """ __mapper_args__ = {'polymorphic_identity': 'coach'} def can_evaluate(self): @@ -17,37 +25,14 @@ class Coach(User): return True def can_manage_this_tryout(self, tryout): - from app.models.org_team.org_team import OrgTeam - if tryout.target_org_team_id: - is_coach_of_target = OrgTeam.query.filter( - OrgTeam.id == tryout.target_org_team_id, - OrgTeam.coaches.any(id=self.id), - ).first() is not None - if is_coach_of_target: - return True - # Check many-to-many coaches relationship - if any(c.id == self.id for c in tryout.coaches): - return True - # Backward compat: check deprecated coach_id - if tryout.coach_id == self.id: - return True - return False + from app.permissions import coach_manages_tryout + return coach_manages_tryout(self, tryout) def can_manage_this_org_team(self, org_team): if org_team.coaches.filter_by(id=self.id).first(): return True - if org_team.coach_id == self.id: - return True - return False + return org_team.coach_id == self.id def get_visible_tryouts(self): - from app.models.tryout.tryout import Tryout - team_ids = [t.id for t in self.coached_org_teams.all()] - conditions = [] - if team_ids: - conditions.append(Tryout.target_org_team_id.in_(team_ids)) - # Check many-to-many coaches - conditions.append(Tryout.coaches.any(id=self.id)) - # Backward compat: check deprecated coach_id - conditions.append(Tryout.coach_id == self.id) - return Tryout.query.filter(db.or_(*conditions)).order_by(Tryout.date).all() + from app.permissions import coach_tryouts + return coach_tryouts(self) diff --git a/app/permissions.py b/app/permissions.py index 85a741b..5f30c59 100644 --- a/app/permissions.py +++ b/app/permissions.py @@ -1,28 +1,40 @@ -"""Shared access-control rules. +"""Shared access-control rules — the single point of truth (ARCH-002). -Authorisation logic currently lives inline in eight route modules, and the -same question — "may this coach act on this player?" — is answered -differently depending on which route you reach. This module is the first -step towards a single point of truth (ARCH-002); rules move here as they -are unified, rather than in one sweeping change. +Authorisation used to live inline in eight route modules, and the same +question could get a different answer depending on which URL you reached. +This module now holds the rules themselves; routes and models call it. -An important subtlety this module hides from callers: a coach can be -attached to a team two different ways. +The subtlety it hides from callers: a coach or a manager can be attached to +a team two different ways. OrgTeam.coach_id the original single-coach column OrgTeam.coaches the many-to-many relationship added later -Both are still populated, and route code reads one or the other with no -apparent pattern. Reading only `coach_id` — which most of users.py does — -silently locks out every coach who is not the first one on their team. -coach_org_team_ids() always considers both. +Both are still populated. Reading only ``coach_id`` — which most of +users.py did — silently locked out every coach who was not the first one on +their team, and reading ``.first()`` on top of it locked a coach out of +every team but one. Both defects were live in production. Every function +here considers both attachment routes and every team, so the fix applies +once instead of at each call site. + +ARCH-001 will collapse the two columns into one for good; that needs a data +migration, so until then this module is what makes the duplication +harmless. + +Functions take the acting user explicitly rather than reading +``current_user``: it keeps them callable from models, from the Discord bot, +and from tests without a request context. """ from app.extensions import db -def coach_org_team_ids(coach): - """IDs of the organisation teams a coach is attached to. +# --------------------------------------------------------------------------- +# Team attachment +# --------------------------------------------------------------------------- + +def coach_org_teams(coach): + """Organisation teams a coach is attached to, ordered by name. Considers the many-to-many relationship *and* the legacy column, so the second coach of a team is not treated as belonging to nothing. @@ -31,17 +43,148 @@ def coach_org_team_ids(coach): coach: The user to inspect. Returns: - list[int]: Team IDs, possibly empty. + list[OrgTeam]: Possibly empty. """ from app.models import OrgTeam - teams = OrgTeam.query.filter( + return OrgTeam.query.filter( db.or_( OrgTeam.coaches.any(id=coach.id), OrgTeam.coach_id == coach.id, ) - ).all() - return [team.id for team in teams] + ).order_by(OrgTeam.name).all() + + +def coach_org_team_ids(coach): + """IDs of the organisation teams a coach is attached to. + + Args: + coach: The user to inspect. + + Returns: + list[int]: Team IDs, possibly empty. + """ + return [team.id for team in coach_org_teams(coach)] + + +def manager_org_teams(manager): + """Organisation teams a manager is attached to, ordered by name. + + Symmetric to :func:`coach_org_teams`: ``manager_id`` and ``managers`` + carry the same duplication. + + Args: + manager: The user to inspect. + + Returns: + list[OrgTeam]: Possibly empty. + """ + from app.models import OrgTeam + + return OrgTeam.query.filter( + db.or_( + OrgTeam.managers.any(id=manager.id), + OrgTeam.manager_id == manager.id, + ) + ).order_by(OrgTeam.name).all() + + +def attached_org_teams(user): + """Teams the user is personally attached to, whatever their role. + + A president is attached to none in particular — they administer them + all — so this returns an empty list for them. Callers that want "the + teams to display" want :func:`visible_org_teams` instead. + + Args: + user: The acting user. + + Returns: + list[OrgTeam]: Possibly empty. + """ + from app.models import Coach, Manager, Player + + if isinstance(user, Coach): + return coach_org_teams(user) + if isinstance(user, Manager): + return manager_org_teams(user) + if isinstance(user, Player): + return user.get_org_teams() + return [] + + +def visible_org_teams(user): + """Organisation teams the user may see, ordered by name. + + A president sees every team; a coach or a manager sees the ones they + are attached to; a player sees the ones they play on; anyone else sees + none. + + Args: + user: The acting user. + + Returns: + list[OrgTeam]: Possibly empty. + """ + from app.models import Admin, OrgTeam + + if isinstance(user, Admin): + return OrgTeam.query.order_by(OrgTeam.name).all() + return attached_org_teams(user) + + +def can_manage_org_team(user, org_team): + """Whether the user may administer this organisation team. + + Delegates to the polymorphic model method, which is the role-level + rule; this wrapper exists so route code has one name to call and never + has to know which subclass it is holding. + + Args: + user: The acting user. + org_team: The team concerned. + + Returns: + bool + """ + return bool(org_team) and user.can_manage_this_org_team(org_team) + + +# --------------------------------------------------------------------------- +# Reach over players +# --------------------------------------------------------------------------- + +def org_team_player_ids(team_ids): + """IDs of the players placed on any of these teams. + + Args: + team_ids: Team primary keys. + + Returns: + list[int]: Player IDs, possibly empty, without duplicates. + """ + from app.models import TeamPlayer + + if not team_ids: + return [] + rows = TeamPlayer.query.filter(TeamPlayer.org_team_id.in_(team_ids)).all() + return list({row.player_id for row in rows}) + + +def coach_player_ids(coach): + """IDs of the players on *all* of a coach's teams. + + The routes this replaces looked at one team — the first row matching + the legacy column — so a coach of two teams could act on half of their + squad and no more. + + Args: + coach: The acting coach. + + Returns: + list[int]: Player IDs, possibly empty. + """ + return org_team_player_ids(coach_org_team_ids(coach)) def coach_can_access_player(coach, player_id): @@ -59,9 +202,7 @@ def coach_can_access_player(coach, player_id): Returns: bool """ - from app.models import ( - Match, MatchParticipant, TeamPlayer, Tryout, TryoutRegistration, - ) + from app.models import Match, MatchParticipant, TeamPlayer, TryoutRegistration if not player_id: return False @@ -75,16 +216,7 @@ def coach_can_access_player(coach, player_id): if on_team: return True - # Tryouts the coach manages, through any of the three routes the model - # supports: target team, many-to-many, or the deprecated coach_id. - conditions = [ - Tryout.coaches.any(id=coach.id), - Tryout.coach_id == coach.id, - ] - if team_ids: - conditions.append(Tryout.target_org_team_id.in_(team_ids)) - - tryout_ids = [t.id for t in Tryout.query.filter(db.or_(*conditions)).all()] + tryout_ids = coach_tryout_ids(coach, team_ids=team_ids) if not tryout_ids: return False @@ -100,3 +232,100 @@ def coach_can_access_player(coach, player_id): Match.tryout_id.in_(tryout_ids), ).first() return plays_a_match is not None + + +def can_manage_player_contract(user, player_id): + """Whether the user may upload or replace a contract for this player. + + Presidents and managers may do so for anyone. A coach may do so for the + players on their teams — the working relationship a contract implies. + + Args: + user: The acting user. + player_id: Primary key of the player concerned. + + Returns: + bool + """ + from app.models import Admin, Coach, Manager + + if not player_id: + return False + if isinstance(user, (Admin, Manager)): + return True + if isinstance(user, Coach): + return player_id in coach_player_ids(user) + return False + + +# --------------------------------------------------------------------------- +# Tryouts +# --------------------------------------------------------------------------- + +def coach_tryouts(coach, team_ids=None): + """Tryouts a coach manages, ordered by date. + + A coach reaches a tryout through any of the three routes the model + supports: it targets one of their teams, they are named on the + many-to-many relationship, or the deprecated ``coach_id`` points at + them. + + Args: + coach: The acting coach. + team_ids: Pre-computed team IDs, to avoid querying twice when the + caller already has them. + + Returns: + list[Tryout]: Possibly empty. + """ + from app.models import Tryout + + if team_ids is None: + team_ids = coach_org_team_ids(coach) + + conditions = [ + Tryout.coaches.any(id=coach.id), + Tryout.coach_id == coach.id, + ] + if team_ids: + conditions.append(Tryout.target_org_team_id.in_(team_ids)) + + return Tryout.query.filter(db.or_(*conditions)).order_by(Tryout.date).all() + + +def coach_tryout_ids(coach, team_ids=None): + """IDs of the tryouts a coach manages. + + Args: + coach: The acting coach. + team_ids: Pre-computed team IDs, see :func:`coach_tryouts`. + + Returns: + list[int]: Possibly empty. + """ + return [tryout.id for tryout in coach_tryouts(coach, team_ids=team_ids)] + + +def coach_manages_tryout(coach, tryout): + """Whether a coach manages this particular tryout. + + Same three routes as :func:`coach_tryouts`, asked about one row. + Written as a membership test rather than a query so that a tryout not + yet flushed to the database still answers correctly. + + Args: + coach: The acting coach. + tryout: The tryout concerned. + + Returns: + bool + """ + if tryout is None: + return False + if tryout.coach_id == coach.id: + return True + if any(c.id == coach.id for c in tryout.coaches): + return True + if tryout.target_org_team_id: + return tryout.target_org_team_id in coach_org_team_ids(coach) + return False diff --git a/app/routes/main.py b/app/routes/main.py index 7e4a79f..727b563 100644 --- a/app/routes/main.py +++ b/app/routes/main.py @@ -10,8 +10,9 @@ from app.extensions import db from app.models import ( Admin, Manager, Coach, Player, Scout, User, Tryout, Evaluation, TryoutRegistration, TeamMember, - Match, MatchParticipant, OrgTeam, + Match, MatchParticipant, ) +from app.permissions import coach_tryout_ids from sqlalchemy import func from datetime import date @@ -97,13 +98,14 @@ def dashboard(): stats['my_recent_evaluations'] = Evaluation.query.filter_by( evaluator_id=user.id).order_by(Evaluation.created_at.desc()).limit(10).all() today = date.today() - org_team = OrgTeam.query.filter_by(coach_id=user.id).first() - coach_tryout_ids = [t.id for t in Tryout.query.filter_by( - target_org_team_id=org_team.id).all()] if org_team else [] + # Was: the first team matching the legacy coach_id column, and only + # the tryouts targeting it. A coach attached by the many-to-many + # relationship, or coaching a second team, saw no upcoming match. + tryout_ids = coach_tryout_ids(user) stats['upcoming_matches'] = Match.query.filter( - Match.tryout_id.in_(coach_tryout_ids), + Match.tryout_id.in_(tryout_ids), Match.status == 'scheduled', Match.date >= today, - ).order_by(Match.date, Match.start_time).limit(5).all() if coach_tryout_ids else [] + ).order_by(Match.date, Match.start_time).limit(5).all() if tryout_ids else [] elif isinstance(user, Player): stats['my_tryouts'] = TryoutRegistration.query.filter_by(player_id=user.id).count() diff --git a/app/routes/team_matches.py b/app/routes/team_matches.py index 138080d..1032295 100644 --- a/app/routes/team_matches.py +++ b/app/routes/team_matches.py @@ -11,6 +11,7 @@ from app.models import ( Admin, Manager, Coach, Player, OrgTeam, TeamMatch, TeamMatchParticipant, TeamPlayer, ) +from app.permissions import can_manage_org_team, coach_org_teams, visible_org_teams from datetime import datetime, timedelta from app.discord_bot import send_schedule_notification @@ -18,17 +19,12 @@ team_matches_bp = Blueprint('team_matches', __name__, url_prefix='/team-matches' def can_manage_team_match(team): - """Check if current user can manage matches for this team.""" - if isinstance(current_user, Admin): - return True - if isinstance(current_user, Manager): - return True - if isinstance(current_user, Coach): - if team.coaches.filter_by(id=current_user.id).first(): - return True - if team.coach_id == current_user.id: - return True - return False + """Whether the current user can manage matches for this team. + + Same rule as administering the team itself, so it is the same call. + This function used to restate it, and the restatement drifted. + """ + return can_manage_org_team(current_user, team) @team_matches_bp.route('') @@ -37,29 +33,17 @@ def list_matches(): """List all team matches visible to the current user.""" filter_team_id = request.args.get('team_id', type=int) - if isinstance(current_user, Admin): + # A manager administers every team, so the listing shows them all; + # visible_org_teams() only reports the teams they are attached to. + if isinstance(current_user, (Admin, Manager)): teams = OrgTeam.query.order_by(OrgTeam.name).all() matches_query = TeamMatch.query - elif isinstance(current_user, Manager): - teams = OrgTeam.query.order_by(OrgTeam.name).all() - matches_query = TeamMatch.query - elif isinstance(current_user, Coach): - teams = OrgTeam.query.filter( - db.or_( - OrgTeam.coaches.any(id=current_user.id), - OrgTeam.coach_id == current_user.id, - ) - ).order_by(OrgTeam.name).all() + elif isinstance(current_user, (Coach, Player)): + teams = visible_org_teams(current_user) team_ids = [t.id for t in teams] matches_query = TeamMatch.query.filter( TeamMatch.org_team_id.in_(team_ids), ) if team_ids else TeamMatch.query.filter(TeamMatch.id == -1) - elif isinstance(current_user, Player): - player_team_ids = [tp.org_team_id for tp in current_user.team_placements] - teams = OrgTeam.query.filter(OrgTeam.id.in_(player_team_ids)).all() if player_team_ids else [] - matches_query = TeamMatch.query.filter( - TeamMatch.org_team_id.in_(player_team_ids), - ) if player_team_ids else TeamMatch.query.filter(TeamMatch.id == -1) else: teams = [] matches_query = TeamMatch.query.filter(TeamMatch.id == -1) @@ -274,12 +258,7 @@ def api_manageable_teams(): if isinstance(current_user, (Admin, Manager)): teams = OrgTeam.query.order_by(OrgTeam.name).all() elif isinstance(current_user, Coach): - teams = OrgTeam.query.filter( - db.or_( - OrgTeam.coaches.any(id=current_user.id), - OrgTeam.coach_id == current_user.id, - ) - ).order_by(OrgTeam.name).all() + teams = coach_org_teams(current_user) else: return jsonify([]) diff --git a/app/routes/teams.py b/app/routes/teams.py index fb41938..81ae8c2 100644 --- a/app/routes/teams.py +++ b/app/routes/teams.py @@ -12,6 +12,7 @@ from app.models import ( OrgTeam, User, PersonalNote, TeamNote, Tryout, TeamPlayer, TeamMatch, Contract, OneOnOneRequest, ) +from app.permissions import visible_org_teams from datetime import datetime teams_bp = Blueprint('teams', __name__, url_prefix='/teams') @@ -23,29 +24,15 @@ def list_teams(): """List all organization teams visible to the current user.""" can_manage = current_user.can_manage_teams() - if isinstance(current_user, Admin): - teams = OrgTeam.query.order_by(OrgTeam.name).all() - elif isinstance(current_user, Coach): - teams = OrgTeam.query.filter( - db.or_( - OrgTeam.coaches.any(id=current_user.id), - OrgTeam.coach_id == current_user.id, - ) - ).order_by(OrgTeam.name).all() - elif isinstance(current_user, Manager): - teams = OrgTeam.query.filter( - db.or_( - OrgTeam.managers.any(id=current_user.id), - OrgTeam.manager_id == current_user.id, - ) - ).order_by(OrgTeam.name).all() - elif isinstance(current_user, Player): + if isinstance(current_user, Player): flash(_('Use My Team(s) to view your teams.'), 'info') return redirect(url_for('teams.my_teams')) - else: + if not isinstance(current_user, (Admin, Coach, Manager)): flash(_('You do not have permission to view teams.'), 'danger') return redirect(url_for('main.dashboard')) + teams = visible_org_teams(current_user) + coaches = User.query.filter_by(role='coach', is_active_account=True).order_by(User.username).all() managers = User.query.filter_by(role='manager', is_active_account=True).order_by(User.username).all() all_players = User.query.filter_by(role='player').order_by(User.username).all() diff --git a/app/routes/users.py b/app/routes/users.py index ca4408f..7218ffd 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -26,7 +26,10 @@ from app.validators import ( UploadContractSchema, ) from app.logging_config import log_auth_event -from app.permissions import coach_can_access_player +from app.permissions import ( + can_manage_player_contract, coach_can_access_player, coach_org_teams, + coach_player_ids, +) import requests ALLOWED_CONTRACT_EXTENSIONS = {'pdf'} @@ -575,19 +578,19 @@ def delete_disponibility(disponibility_id): # Contracts # --------------------------------------------------------------------------- -def can_manage_player_contract(user, player_id): - """Check if a user can upload contracts for a specific player.""" - if isinstance(user, Admin): - return True - if isinstance(user, Manager): - return True - if isinstance(user, Coach): - org_team = OrgTeam.query.filter_by(coach_id=user.id).first() - if org_team: - tp = TeamPlayer.query.filter_by(player_id=player_id, org_team_id=org_team.id).first() - if tp: - return True - return False +def _manageable_players(): + """Players the current user may attach a contract to. + + A coach used to see the squad of one team — the first row matching the + legacy coach_id column — so a coach of two teams could file a contract + for half of their players and no more, and a coach attached only by the + many-to-many relationship for none at all. + """ + if isinstance(current_user, Coach): + player_ids = coach_player_ids(current_user) + return User.query.filter(User.id.in_(player_ids)).order_by( + User.username).all() if player_ids else [] + return User.query.filter_by(role='player').order_by(User.username).all() @users_bp.route('/contracts') @@ -602,14 +605,7 @@ def list_contracts(): player_id=current_user.id, ).order_by(Contract.uploaded_at.desc()).all() elif isinstance(current_user, (Admin, Manager, Coach)): - players = [] - if isinstance(current_user, Coach): - org_team = OrgTeam.query.filter_by(coach_id=current_user.id).first() - if org_team: - player_ids = [tp.player_id for tp in TeamPlayer.query.filter_by(org_team_id=org_team.id).all()] - players = User.query.filter(User.id.in_(player_ids)).all() if player_ids else [] - else: - players = User.query.filter_by(role='player').all() + players = _manageable_players() if players: player_ids = [p.id for p in players] @@ -629,15 +625,7 @@ def upload_contract(): flash(_('Only presidents, managers, and coaches can upload contracts.'), 'danger') return redirect(url_for('users.list_contracts')) - if isinstance(current_user, Coach): - org_team = OrgTeam.query.filter_by(coach_id=current_user.id).first() - if org_team: - player_ids = [tp.player_id for tp in TeamPlayer.query.filter_by(org_team_id=org_team.id).all()] - players = User.query.filter(User.id.in_(player_ids)).all() if player_ids else [] - else: - players = [] - else: - players = User.query.filter_by(role='player').all() + players = _manageable_players() if request.method == 'POST': contract_schema = UploadContractSchema() @@ -826,7 +814,12 @@ def one_on_one(): org_teams = current_user.get_org_teams() org_team = org_teams[0] if org_teams else None - coach = User.query.get(org_team.coach_id) if org_team and org_team.coach_id else None + # Reading org_team.coach_id directly told every player whose team lists + # its coaches through the many-to-many relationship — the newer of the + # two ways — that they had no coach, and closed the page to them. + # get_coaches() falls back to the legacy column when the list is empty. + team_coaches = org_team.get_coaches() if org_team else [] + coach = team_coaches[0] if team_coaches else None if not coach: flash(_('You do not have a coach assigned to your team.'), 'info') @@ -1123,15 +1116,18 @@ def notes_dashboard(): flash(_('Only coaches can access the notes dashboard.'), 'danger') return redirect(url_for('main.dashboard')) - org_team = OrgTeam.query.filter_by(coach_id=current_user.id).first() + # The team-notes panel is still written against a single team; the + # player list is not, and used to be narrowed to one team's squad while + # the POST routes accepted every player the coach works with. The form + # offered fewer players than the handler would take. + org_teams = coach_org_teams(current_user) + org_team = org_teams[0] if org_teams else None - players = [] - if org_team: - player_ids = [tp.player_id for tp in TeamPlayer.query.filter_by(org_team_id=org_team.id).all()] - players = User.query.filter(User.id.in_(player_ids)).all() if player_ids else [] + player_ids = coach_player_ids(current_user) + players = User.query.filter(User.id.in_(player_ids)).order_by( + User.username).all() if player_ids else [] team_notes = [] - personal_notes = [] latest_team_note = None if org_team: team_notes = TeamNote.query.filter_by( @@ -1139,16 +1135,16 @@ def notes_dashboard(): ).order_by(TeamNote.created_at.desc()).all() latest_team_note = team_notes[0] if team_notes else None - personal_notes = PersonalNote.query.filter_by( - coach_id=current_user.id, - ).order_by(PersonalNote.created_at.desc()).all() + # A coach's own notes belong to them whether or not they hold a team; + # this list was gated on org_team and came back empty without one. + personal_notes = PersonalNote.query.filter_by( + coach_id=current_user.id, + ).order_by(PersonalNote.created_at.desc()).all() - # One on One requests from team players one_on_one_requests = [] - if org_team and players: - player_ids_list = [p.id for p in players] + if player_ids: one_on_one_requests = OneOnOneRequest.query.filter( - OneOnOneRequest.player_id.in_(player_ids_list) + OneOnOneRequest.player_id.in_(player_ids) ).order_by(OneOnOneRequest.created_at.desc()).all() # For context selectors in the form @@ -1184,10 +1180,12 @@ def manage_team_notes(): flash(_('Only coaches can manage team notes.'), 'danger') return redirect(url_for('main.dashboard')) - org_team = OrgTeam.query.filter_by(coach_id=current_user.id).first() - if not org_team: + # Same team the dashboard displays notes for, resolved the same way. + org_teams = coach_org_teams(current_user) + if not org_teams: flash(_('You are not assigned to a team.'), 'danger') return redirect(url_for('users.notes_dashboard')) + org_team = org_teams[0] content = request.form.get('content', '').strip() if content: diff --git a/tests/test_permissions.py b/tests/test_permissions.py index df8360a..feebf8d 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -21,7 +21,10 @@ from app.extensions import db from app.models import ( Contract, OrgTeam, PersonalNote, TeamPlayer, Tryout, TryoutRegistration, ) -from app.permissions import coach_can_access_player, coach_org_team_ids +from app.permissions import ( + can_manage_player_contract, coach_can_access_player, coach_org_team_ids, + coach_player_ids, visible_org_teams, +) @pytest.fixture @@ -236,3 +239,201 @@ class TestContractVisibility: from app.models import User contract = db.session.get(Contract, contract_id) assert contract.can_view(db.session.get(User, admin_id)) + + +class TestSecondTeam: + """ARCH-002 — the routes resolved a coach's team with + + OrgTeam.query.filter_by(coach_id=...).first() + + which answers with at most one team, and only ever the legacy column. + A coach of two teams therefore reached half of their squad; a coach + attached by the relationship only reached none of it. Both defects were + live, and silent: the pages rendered, just empty.""" + + def test_a_coach_of_two_teams_reaches_both_squads( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + first_player = make_user('player') + second_player = make_user('player') + team_factory('Varsity', legacy_coach_id=coach_id, player_ids=[first_player]) + team_factory('JV', coach_id=coach_id, player_ids=[second_player]) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert sorted(coach_player_ids(coach)) == sorted([first_player, second_player]) + + def test_a_contract_may_be_filed_for_a_player_of_the_second_team( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + team_factory('Varsity', legacy_coach_id=coach_id) + second_player = make_user('player') + team_factory('JV', coach_id=coach_id, player_ids=[second_player]) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert can_manage_player_contract(coach, second_player) + + def test_a_contract_is_still_refused_for_an_unrelated_player( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + stranger_id = make_user('player') + team_factory('Varsity', coach_id=coach_id) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert not can_manage_player_contract(coach, stranger_id) + + def test_the_contract_page_lists_the_players_of_every_team( + self, app, client, as_role, make_user, team_factory + ): + coach_id = as_role('coach') + first_player = make_user('player', username='alpha') + second_player = make_user('player', username='bravo') + team_factory('Varsity', legacy_coach_id=coach_id, player_ids=[first_player]) + team_factory('JV', coach_id=coach_id, player_ids=[second_player]) + + body = client.get('/users/contracts/upload').get_data(as_text=True) + assert 'alpha' in body + assert 'bravo' in body + + def test_the_notes_dashboard_lists_the_players_of_every_team( + self, app, client, as_role, make_user, team_factory + ): + coach_id = as_role('coach') + first_player = make_user('player', username='charlie') + second_player = make_user('player', username='delta') + team_factory('Varsity', legacy_coach_id=coach_id, player_ids=[first_player]) + team_factory('JV', coach_id=coach_id, player_ids=[second_player]) + + body = client.get('/users/notes-dashboard').get_data(as_text=True) + assert 'charlie' in body + assert 'delta' in body + + +class TestTeamVisibility: + def test_a_coach_attached_by_the_relationship_sees_their_team( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + team_factory('Varsity', coach_id=coach_id) + + with app.app_context(): + from app.models import User + teams = visible_org_teams(db.session.get(User, coach_id)) + assert [t.name for t in teams] == ['Varsity'] + + def test_a_president_sees_every_team(self, app, make_user, team_factory): + admin_id = make_user('admin') + other_coach = make_user('coach') + team_factory('Varsity', coach_id=other_coach) + team_factory('JV', legacy_coach_id=other_coach) + + with app.app_context(): + from app.models import User + teams = visible_org_teams(db.session.get(User, admin_id)) + assert [t.name for t in teams] == ['JV', 'Varsity'] + + def test_a_scout_sees_none(self, app, make_user, team_factory): + scout_id = make_user('scout') + coach_id = make_user('coach') + team_factory('Varsity', coach_id=coach_id) + + with app.app_context(): + from app.models import User + assert visible_org_teams(db.session.get(User, scout_id)) == [] + + def test_the_team_listing_shows_the_relationship_team( + self, app, client, as_role, team_factory + ): + coach_id = as_role('coach') + team_factory('Northern Lights', coach_id=coach_id) + + body = client.get('/teams').get_data(as_text=True) + assert 'Northern Lights' in body + + +class TestTryoutVisibility: + """Coach.get_visible_tryouts() read the many-to-many relationship only, + so a coach attached by the legacy column had an empty calendar.""" + + @staticmethod + def _tryout(app, *, creator_id, title, target_team_id=None, legacy_coach_id=None): + with app.app_context(): + tryout = Tryout(title=title, game='Valorant', date=date(2030, 5, 1), + created_by=creator_id, target_org_team_id=target_team_id, + coach_id=legacy_coach_id) + db.session.add(tryout) + db.session.commit() + return tryout.id + + def test_a_tryout_targeting_a_legacy_team_is_visible( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + team_id = team_factory('Varsity', legacy_coach_id=coach_id) + self._tryout(app, creator_id=coach_id, title='Spring intake', + target_team_id=team_id) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert [t.title for t in coach.get_visible_tryouts()] == ['Spring intake'] + + def test_the_same_tryout_is_manageable(self, app, make_user, team_factory): + coach_id = make_user('coach') + team_id = team_factory('Varsity', legacy_coach_id=coach_id) + tryout_id = self._tryout(app, creator_id=coach_id, title='Spring intake', + target_team_id=team_id) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert coach.can_manage_this_tryout(db.session.get(Tryout, tryout_id)) + + def test_another_coachs_tryout_stays_out_of_reach( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + other_id = make_user('coach') + team_factory('Varsity', coach_id=coach_id) + other_team = team_factory('JV', coach_id=other_id) + tryout_id = self._tryout(app, creator_id=other_id, title='Their intake', + target_team_id=other_team) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert coach.get_visible_tryouts() == [] + assert not coach.can_manage_this_tryout(db.session.get(Tryout, tryout_id)) + + +class TestOneOnOnePage: + """A player whose team lists its coaches through the relationship was + told they had no coach, and the request form stayed closed.""" + + #: The request form and its availability payload are rendered under + #: `{% if coach %}`. Asserting on this marker rather than on wording + #: keeps the test about the capability, not about a translated string. + REQUEST_FORM = 'id="coach-availability-data"' + + def test_a_player_finds_the_coach_attached_by_the_relationship( + self, app, client, as_role, make_user, team_factory + ): + coach_id = make_user('coach') + player_id = as_role('player') + team_factory('Varsity', coach_id=coach_id, player_ids=[player_id]) + + body = client.get('/users/one-on-one').get_data(as_text=True) + assert self.REQUEST_FORM in body + + def test_a_teamless_player_gets_no_request_form(self, app, client, as_role): + as_role('player') + body = client.get('/users/one-on-one').get_data(as_text=True) + assert self.REQUEST_FORM not in body