From 0308eb9eefb171ccd6cd3601b608b93106be7806 Mon Sep 17 00:00:00 2001 From: GGThed Date: Tue, 11 Aug 2026 13:09:00 -0400 Subject: [PATCH] refactor(validation): un schema a la frontiere des matchs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ARCH-005, premiere moitie. matches.py et team_matches.py lisaient une quarantaine de champs sur request.form a la main et les croyaient tous. Ce que ca produisait n etait pas bruyant : - edit_match attrapait une heure invalide et faisait start_time = None, puis annoncait que le match etait mis a jour. Le match perdait son heure et le calendrier l affichait a minuit ; - match_type etait accepte tel quel. Une valeur inconnue creait un match auquel aucun joueur n etait rattache, sans un mot ; - une fin avant le debut etait enregistree telle quelle ; - title est NOT NULL dans le modele et n etait pas verifie dans la route, donc un titre vide etait un 500 ; - 'a,b' dans la selection de joueurs arrivait sur int() sans garde. app/forms.py rassemble les deux fonctions de frontiere, qui vivaient dans users/_shared.py parce que c est la qu elles avaient d abord servi. Elles y restent re-exportees, donc aucun des trente appels n a bouge. Le mixin des schemas lit desormais un champ vide comme un champ absent. C est ce qui rendait ces formulaires invalidables : un formulaire HTML envoie tout ce qu il affiche, donc une date optionnelle non remplie arrive comme '' et non comme rien. Seuls les champs declares optionnels sont concernes ; un champ requis laisse vide doit toujours echouer. Deux duplications absorbees au passage, toutes deux nommees par l audit : la boucle de creation des participants, ecrite deux fois et deja divergee — la copie de edit_match gardait ses identifiants en chaines et appelait int() une ligne plus loin — et le contexte de re-affichage du formulaire, dont les versions courtes faisaient mourir un refus dans tojson sur un Undefined : un message de validation devenait un 500. Limite connue et consignee : le formulaire revient rempli avec les valeurs enregistrees, pas avec la saisie refusee. Reafficher la soumission demande de toucher aux gabarits, c est un autre changement. 19 tests neufs sur ces routes, qui n en avaient aucun. 447 au total. --- app/forms.py | 63 ++++++ app/routes/matches.py | 384 ++++++++++++--------------------- app/routes/team_matches.py | 129 ++++------- app/routes/users/_shared.py | 35 +-- app/validators.py | 194 ++++++++++++++++- tests/test_match_scheduling.py | 322 +++++++++++++++++++++++++++ 6 files changed, 769 insertions(+), 358 deletions(-) create mode 100644 app/forms.py create mode 100644 tests/test_match_scheduling.py diff --git a/app/forms.py b/app/forms.py new file mode 100644 index 0000000..4a435e8 --- /dev/null +++ b/app/forms.py @@ -0,0 +1,63 @@ +"""The boundary between an HTTP form and a validated payload (ARCH-005). + +Every POST in this application arrives as a `werkzeug.MultiDict` of strings. +Turning that into typed, checked values was done inline, differently, in each +route: `int(x) if x else None` here, `datetime.strptime` inside a bare `try` +there, and in several places not at all. The failures that produced were not +loud ones — a bad time silently became `None` and the page said the match had +been updated. + +Two functions here, one schema module next to them (`app.validators`): + + payload = form_payload(list_fields=('player_ids',)) + try: + data = MatchSchema().load(payload) + except ValidationError as err: + flash_validation_errors(err) + return _rerender() + +Both were originally inside `app/routes/users/_shared.py`, which is where +they were first needed. They are re-exported from there so that nothing had +to be renamed when the match and tryout routes started using them too. +""" + +from flask import flash, request +from flask_babel import gettext as _ + + +def flash_validation_errors(err): + """Surface marshmallow errors, one flash per problem. + + The uniform reporting half of ARCH-005: before this, a bad date flashed + 'Invalid date format.' from one route, redirected from another, and was + silently dropped by a third. + """ + for field, messages in err.messages.items(): + for msg in messages: + flash(_('%(field)s: %(msg)s', field=field, msg=msg), 'danger') + + +def form_payload(*, checkboxes=(), list_fields=('games',), optional_blank=('password',)): + """Turn the multi-valued request form into a plain dict for marshmallow. + + request.form.to_dict() keeps only the first value of a repeated key, so + list fields have to be re-read with getlist(). Unchecked HTML checkboxes + are simply absent from the submission, which is not the same as a schema + default, so they are injected explicitly. Blank optional fields are + dropped rather than sent as '' — an empty password means "leave the + current one alone", not "set the password to the empty string". + + Args: + checkboxes: Names to report as True/False on presence. + list_fields: Names to read with getlist(), always producing a list. + optional_blank: Names to drop entirely when submitted empty. + """ + payload = request.form.to_dict() + for name in list_fields: + payload[name] = request.form.getlist(name) + for name in checkboxes: + payload[name] = name in request.form + for name in optional_blank: + if not payload.get(name): + payload.pop(name, None) + return payload diff --git a/app/routes/matches.py b/app/routes/matches.py index 8101bc0..9580dec 100644 --- a/app/routes/matches.py +++ b/app/routes/matches.py @@ -8,9 +8,11 @@ from datetime import datetime, timedelta from flask import Blueprint, flash, jsonify, redirect, render_template, request, url_for from flask_babel import gettext as _ from flask_login import current_user, login_required +from marshmallow import ValidationError from sqlalchemy.orm import joinedload from app.extensions import db +from app.forms import flash_validation_errors, form_payload from app.models import ( Admin, Coach, @@ -29,10 +31,62 @@ from app.models import ( User, ) from app.services.scheduling import notify_participants, zip_participants +from app.validators import MatchEditSchema, MatchSchema matches_bp = Blueprint('matches', __name__, url_prefix='/matches') +def match_form_payload(): + """The match form, shaped for marshmallow. + + `player_ids` is a repeated checkbox, so it needs getlist(); `games` — the + default list field — has nothing to do with this form. + """ + return form_payload(list_fields=('player_ids',), optional_blank=()) + + +#: How long a match lasts when the form gives a start and no end. +DEFAULT_MATCH_MINUTES = 30 + + +def default_end_time(date, start_time): + """End time for a match whose form left it blank.""" + return (datetime.combine(date, start_time) + timedelta(minutes=DEFAULT_MATCH_MINUTES)).time() + + +def create_participants(match, data): + """Attach participants to a match, per its type. + + Was written out twice, in create_match and in edit_match, and had already + drifted: the copy in edit_match kept its player ids as strings and called + int() on them one line later, the one in create_match did not (ARCH-005). + + Returns: + tuple: (player ids to notify, the participant rows created). + """ + sides = [] + if match.match_type == 'team_vs_team': + for side, team_id in ((1, match.team1_id), (2, match.team2_id)): + if team_id: + members = TeamMember.query.filter_by(team_id=team_id).all() + sides.append((side, [m.player_id for m in members])) + elif match.match_type == 'player_vs_player': + sides = [(1, data['team1_player_ids']), (2, data['team2_player_ids'])] + elif match.match_type == 'player_scrim': + sides = [(None, data['player_ids'])] + + player_ids = [] + participant_ids = [] + for side, ids in sides: + for player_id in ids: + participant = MatchParticipant(match_id=match.id, player_id=player_id, team_side=side) + db.session.add(participant) + db.session.flush() + participant_ids.append(participant.id) + player_ids.append(player_id) + return player_ids, participant_ids + + def can_schedule_match(): """Check if user can schedule matches (Admin, Manager, Coach, Scout).""" return isinstance(current_user, (Admin, Manager, Coach, Scout)) @@ -319,129 +373,53 @@ def create_match(tryout_id): all_players = sorted([p for p in all_players if p], key=lambda x: x.username) prefill_date = request.args.get('date', '') + def rerender(): + return render_template( + 'pages/match_form.html', + tryout=tryout, + teams=teams, + all_players=all_players, + prefill_date=prefill_date, + ) + if request.method == 'POST': - title = request.form.get('title') - description = request.form.get('description') - date_str = request.form.get('date') - start_time_str = request.form.get('start_time') - end_time_str = request.form.get('end_time') - location = request.form.get('location') - match_type = request.form.get('match_type') - - if not start_time_str: - flash(_('Start time is required. Please select a time slot.'), 'danger') - return render_template( - 'pages/match_form.html', - tryout=tryout, - teams=teams, - all_players=all_players, - prefill_date=prefill_date, - ) + payload = match_form_payload() + # A tryout match with no date of its own happens on the tryout's day. + payload.setdefault('date', tryout.date.isoformat()) try: - date_obj = datetime.strptime(date_str, '%Y-%m-%d').date() if date_str else tryout.date - except (ValueError, TypeError): - flash(_('Invalid date format.'), 'danger') - return render_template( - 'pages/match_form.html', - tryout=tryout, - teams=teams, - all_players=all_players, - prefill_date=prefill_date, - ) - - start_time = None - end_time = None - try: - start_time = datetime.strptime(start_time_str, '%H:%M').time() - if end_time_str: - end_time = datetime.strptime(end_time_str, '%H:%M').time() - else: - start_dt = datetime.combine(date_obj, start_time) - end_dt = start_dt + timedelta(minutes=30) - end_time = end_dt.time() - except ValueError: - flash(_('Invalid time format.'), 'danger') - return render_template( - 'pages/match_form.html', tryout=tryout, teams=teams, all_players=all_players - ) + data = MatchSchema().load(payload) + except ValidationError as err: + flash_validation_errors(err) + return rerender() match = Match( tryout_id=tryout_id, - title=title, - description=description, - date=date_obj, - start_time=start_time, - end_time=end_time, - location=location, - match_type=match_type, + title=data['title'], + description=data['description'], + date=data['date'], + start_time=data['start_time'], + end_time=data['end_time'] or default_end_time(data['date'], data['start_time']), + location=data['location'], + match_type=data['match_type'], created_by=current_user.id, ) db.session.add(match) db.session.flush() - notified_player_ids = [] - notified_participant_ids = [] + if data['match_type'] == 'team_vs_team': + match.team1_id = data['team1_id'] + match.team2_id = data['team2_id'] - if match_type == 'team_vs_team': - team1_id = request.form.get('team1_id') - team2_id = request.form.get('team2_id') - match.team1_id = int(team1_id) if team1_id else None - match.team2_id = int(team2_id) if team2_id else None - if match.team1_id: - for m in TeamMember.query.filter_by(team_id=match.team1_id).all(): - participant = MatchParticipant( - match_id=match.id, player_id=m.player_id, team_side=1 - ) - db.session.add(participant) - db.session.flush() - notified_participant_ids.append(participant.id) - notified_player_ids.append(m.player_id) - if match.team2_id: - for m in TeamMember.query.filter_by(team_id=match.team2_id).all(): - participant = MatchParticipant( - match_id=match.id, player_id=m.player_id, team_side=2 - ) - db.session.add(participant) - db.session.flush() - notified_participant_ids.append(participant.id) - notified_player_ids.append(m.player_id) - elif match_type == 'player_vs_player': - team1_player_ids = request.form.get('team1_player_ids', '') - team2_player_ids = request.form.get('team2_player_ids', '') - team1_ids = ( - [int(p) for p in team1_player_ids.split(',') if p] if team1_player_ids else [] - ) - team2_ids = ( - [int(p) for p in team2_player_ids.split(',') if p] if team2_player_ids else [] - ) - for pid in team1_ids: - participant = MatchParticipant(match_id=match.id, player_id=pid, team_side=1) - db.session.add(participant) - db.session.flush() - notified_participant_ids.append(participant.id) - for pid in team2_ids: - participant = MatchParticipant(match_id=match.id, player_id=pid, team_side=2) - db.session.add(participant) - db.session.flush() - notified_participant_ids.append(participant.id) - notified_player_ids = team1_ids + team2_ids - elif match_type == 'player_scrim': - player_ids = request.form.getlist('player_ids') - for pid in player_ids: - participant = MatchParticipant(match_id=match.id, player_id=int(pid)) - db.session.add(participant) - db.session.flush() - notified_participant_ids.append(participant.id) - notified_player_ids = [int(p) for p in player_ids] + notified_player_ids, notified_participant_ids = create_participants(match, data) db.session.commit() notify_participants( title=match.title, - date=date_obj, - start_time=start_time, - end_time=end_time, + date=match.date, + start_time=match.start_time, + end_time=match.end_time, participants=zip_participants(notified_player_ids, notified_participant_ids), fallback_id=match.id, ) @@ -449,13 +427,7 @@ def create_match(tryout_id): flash(_('Match scheduled successfully!'), 'success') return redirect(url_for('tryouts.view_tryout', tryout_id=tryout_id)) - return render_template( - 'pages/match_form.html', - tryout=tryout, - teams=teams, - all_players=all_players, - prefill_date=prefill_date, - ) + return rerender() @matches_bp.route('//edit', methods=['GET', 'POST']) @@ -481,126 +453,74 @@ def edit_match(match_id): team1_player_ids = [p.player_id for p in match.participants.filter_by(team_side=1).all()] team2_player_ids = [p.player_id for p in match.participants.filter_by(team_side=2).all()] + def rerender(): + """The form, with everything the template needs. + + One context, used by the GET and by a rejected POST alike. The + rejection paths used to pass a shorter list, and match_form.html + serialises participants_map into a