From 9166d8abeb5ea9f8059b019d22fdb4d2cb053593 Mon Sep 17 00:00:00 2001 From: GGThed Date: Tue, 11 Aug 2026 20:20:25 -0400 Subject: [PATCH] fix(data): une saisie d heure refusee n efface plus la disponibilite MNT-12. Le meme bloc de parsing date/heure vivait dans quatre modules avec trois reponses differentes a la meme saisie invalide : signaler et rediriger, mettre la valeur a None et annoncer la reussite, ou passer au suivant en silence. Les vagues E et G ont ferme le cote matchs avec des schemas ; il restait la disponibilite, les creneaux de coach et les demandes individuelles. Deux choses trouvees en appliquant, aucune dans le constat. OneOnOneRequestSchema et DisponibilityAddSchema etaient definis dans validators.py et appeles NULLE PART : aucun import, aucun test. C'est le motif SEC-AUTHZ-001 -- une politique de validation ecrite et non appliquee -- qui survivait dans un coin que personne n'avait rouvert. Les deux passaient d'ailleurs par des fields.String + Regexp, qui verifient la forme et laissent l'appelant convertir ; fields.Date et fields.Time font les deux. Et manage_coach_availability supprimait tous les creneaux existants avant de reajouter ceux qu'il savait lire, ignorant les autres en silence et repondant {'success': true}. Un envoi malforme effacait donc les heures reservables d'un coach en annoncant la reussite -- et les demandes individuelles sont refusees contre exactement cette table, donc le coach devenait injoignable sans que rien ne le dise. Une operation de remplacement doit tout valider avant de rien supprimer : le lot est refuse en entier. Trouve aussi : le controle de disponibilite comparait les chaines du formulaire aux chaines serialisees, ce qui ne marchait que parce que les deux cotes etaient en HH:MM a zero non significatif. La comparaison porte desormais sur des objets time. Et le lint a rattrape une regression que la relecture avait manquee -- sixieme fois : datetime retire de one_on_one.py alors que deux fonctions non touchees l'utilisaient encore. Co-Authored-By: Claude Opus 5 --- app/routes/users/availability.py | 149 +++++++++------- app/routes/users/one_on_one.py | 63 +++---- app/timeslots.py | 60 +++++++ app/validators.py | 90 +++++++--- tests/test_timeslots.py | 288 +++++++++++++++++++++++++++++++ 5 files changed, 539 insertions(+), 111 deletions(-) create mode 100644 app/timeslots.py create mode 100644 tests/test_timeslots.py diff --git a/app/routes/users/availability.py b/app/routes/users/availability.py index c87c497..ee8fe2f 100644 --- a/app/routes/users/availability.py +++ b/app/routes/users/availability.py @@ -5,22 +5,52 @@ weekly availability blocks, and a coach's bookable slots for one-on-one sessions. """ -from datetime import datetime, timedelta - from flask import 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 app.api import json_endpoint from app.extensions import db +from app.forms import form_payload from app.models import Coach, CoachAvailability, PlayerDisponibility, User from app.routes.users.blueprint import users_bp - -DAY_NAMES = ['Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday'] +from app.timeslots import day_name, slot_end +from app.validators import TimeSlotSchema -def add_30_minutes(t): - return (datetime.combine(datetime.today(), t) + timedelta(minutes=30)).time() +def _load_slots(payload_slots): + """Validate a batch of posted slots, keeping the rejects. + + Both bulk endpoints used to `continue` past anything malformed and then + answer `{'success': True}`. The client had no way to learn that a slot + had been dropped — and for coach availability that is destructive, since + the route deletes every existing slot before re-adding the ones it + accepted. A payload the browser mangled could therefore wipe a coach's + bookable hours and report success (MNT-12). + + Args: + payload_slots: Whatever arrived under the `slots` key. + + Returns: + tuple[list[dict], list[str]]: Accepted slots, and one message per + rejected one. + """ + schema = TimeSlotSchema() + accepted, rejected = [], [] + for index, raw in enumerate(payload_slots or []): + if not isinstance(raw, dict): + rejected.append(f'slot {index}: expected an object') + continue + try: + accepted.append(schema.load(raw)) + except ValidationError as err: + details = '; '.join( + f'{field}: {" ".join(str(m) for m in messages)}' + for field, messages in err.messages.items() + ) + rejected.append(f'slot {index}: {details}') + return accepted, rejected @users_bp.route('/disponibilities') @@ -43,7 +73,7 @@ def get_disponibilities(): { 'id': d.id, 'day_of_week': d.day_of_week, - 'day_name': DAY_NAMES[d.day_of_week], + 'day_name': day_name(d.day_of_week), 'start_time': d.start_time.strftime('%H:%M'), 'end_time': d.end_time.strftime('%H:%M'), } @@ -68,7 +98,7 @@ def get_my_disponibilities(): { 'id': d.id, 'day_of_week': d.day_of_week, - 'day_name': DAY_NAMES[d.day_of_week], + 'day_name': day_name(d.day_of_week), 'start_time': d.start_time.strftime('%H:%M'), 'end_time': d.end_time.strftime('%H:%M'), } @@ -81,21 +111,18 @@ def get_my_disponibilities(): @login_required def add_disponibility(): """Add a disponibility block for the current player.""" - day_of_week = request.form.get('day_of_week', type=int) - start_time_str = request.form.get('start_time') - if day_of_week is None or day_of_week < 0 or day_of_week > 6: - return jsonify({'error': 'Invalid day of week'}), 400 try: - start_time = datetime.strptime(start_time_str, '%H:%M').time() - except (ValueError, TypeError): - return jsonify({'error': 'Invalid time format'}), 400 + slot = TimeSlotSchema().load(form_payload()) + except ValidationError as err: + return jsonify({'error': 'Invalid slot', 'details': err.messages}), 400 - end_time = add_30_minutes(start_time) + day_of_week = slot['day_of_week'] + start_time = slot['start_time'] disponibility = PlayerDisponibility( player_id=current_user.id, day_of_week=day_of_week, start_time=start_time, - end_time=end_time, + end_time=slot_end(start_time), ) db.session.add(disponibility) db.session.commit() @@ -103,7 +130,7 @@ def add_disponibility(): { 'id': disponibility.id, 'day_of_week': disponibility.day_of_week, - 'day_name': DAY_NAMES[disponibility.day_of_week], + 'day_name': day_name(disponibility.day_of_week), 'start_time': disponibility.start_time.strftime('%H:%M'), 'end_time': disponibility.end_time.strftime('%H:%M'), } @@ -115,31 +142,23 @@ def add_disponibility(): @login_required def add_disponibilities_bulk(): """Add multiple disponibility blocks at once.""" - data = request.get_json() - slots = data.get('slots', []) - created = [] - for slot in slots: - day_of_week = slot.get('day_of_week') - start_time_str = slot.get('start_time') - if day_of_week is None or day_of_week < 0 or day_of_week > 6: - continue - try: - start_time = datetime.strptime(start_time_str, '%H:%M').time() - except (ValueError, TypeError): - continue + data = request.get_json(silent=True) or {} + accepted, rejected = _load_slots(data.get('slots')) - end_time = add_30_minutes(start_time) + created = [] + for slot in accepted: + start_time = slot['start_time'] existing = PlayerDisponibility.query.filter_by( player_id=current_user.id, - day_of_week=day_of_week, + day_of_week=slot['day_of_week'], start_time=start_time, ).first() if not existing: disponibility = PlayerDisponibility( player_id=current_user.id, - day_of_week=day_of_week, + day_of_week=slot['day_of_week'], start_time=start_time, - end_time=end_time, + end_time=slot_end(start_time), ) db.session.add(disponibility) db.session.flush() @@ -147,12 +166,16 @@ def add_disponibilities_bulk(): { 'id': disponibility.id, 'day_of_week': disponibility.day_of_week, - 'day_name': DAY_NAMES[disponibility.day_of_week], + 'day_name': day_name(disponibility.day_of_week), 'start_time': disponibility.start_time.strftime('%H:%M'), } ) db.session.commit() - return jsonify({'success': True, 'created': created}) + # `rejected` is reported rather than swallowed. What was accepted is + # still saved — dropping a whole batch because one cell was malformed + # would be its own kind of surprise — but the client can now tell the + # difference between "nine slots saved" and "ten sent, nine saved". + return jsonify({'success': True, 'created': created, 'rejected': rejected}) @users_bp.route('/disponibilities/clear', methods=['POST']) @@ -188,36 +211,42 @@ def manage_coach_availability(): return redirect(url_for('main.dashboard')) if request.method == 'POST': - data = request.get_json() - slots = data.get('slots', []) if data else [] + data = request.get_json(silent=True) or {} + accepted, rejected = _load_slots(data.get('slots')) + + # Validate everything before deleting anything. + # + # This route replaces the coach's availability: it deleted every + # existing slot and then re-added the ones it could parse, skipping + # the rest in silence and answering `{'success': true}`. A payload + # the browser mangled therefore wiped a coach's bookable hours and + # reported success — and one-on-one requests are refused against + # exactly this table, so the coach became unbookable with nothing to + # show for it. Refusing the whole batch is the only safe answer when + # the operation is a replacement (MNT-12). + if rejected: + return jsonify( + { + 'error': 'Invalid slots; nothing was changed.', + 'rejected': rejected, + } + ), 400 - # Clear existing availability CoachAvailability.query.filter_by(coach_id=current_user.id).delete() - # Add new slots - for slot in slots: - day_of_week = slot.get('day_of_week') - start_time_str = slot.get('start_time') - if day_of_week is None or day_of_week < 0 or day_of_week > 6: - continue - try: - start_time = datetime.strptime(start_time_str, '%H:%M').time() - end_time = ( - datetime.combine(datetime.today(), start_time) + timedelta(minutes=30) - ).time() - except (ValueError, TypeError): - continue - - availability = CoachAvailability( - coach_id=current_user.id, - day_of_week=day_of_week, - start_time=start_time, - end_time=end_time, + for slot in accepted: + start_time = slot['start_time'] + db.session.add( + CoachAvailability( + coach_id=current_user.id, + day_of_week=slot['day_of_week'], + start_time=start_time, + end_time=slot_end(start_time), + ) ) - db.session.add(availability) db.session.commit() - return jsonify({'success': True}) + return jsonify({'success': True, 'saved': len(accepted)}) existing_availability = CoachAvailability.query.filter_by( coach_id=current_user.id, diff --git a/app/routes/users/one_on_one.py b/app/routes/users/one_on_one.py index 6ff2829..6fe50b7 100644 --- a/app/routes/users/one_on_one.py +++ b/app/routes/users/one_on_one.py @@ -5,11 +5,14 @@ from datetime import datetime from flask import flash, 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 app.extensions import db +from app.forms import flash_validation_errors, form_payload from app.models import Coach, CoachAvailability, OneOnOneRequest, PersonalNote, Player, TeamNote from app.routes.users.blueprint import users_bp from app.services.notifications import send_discord_notification +from app.validators import OneOnOneRequestSchema @users_bp.route('/one-on-one', methods=['GET', 'POST']) @@ -46,44 +49,44 @@ def one_on_one(): .all() ) - coach_availability = [] - if coach: - availabilities = CoachAvailability.query.filter_by(coach_id=coach.id).all() - coach_availability = [ - { - 'day_of_week': av.day_of_week, - 'start_time': av.start_time.strftime('%H:%M'), - 'end_time': av.end_time.strftime('%H:%M'), - } - for av in availabilities - ] + # Kept as model objects for the availability check below, and serialised + # separately for the page. They used to be the same list of strings, + # which is what made the check compare '9:00' with '10:00' as text. + availabilities = CoachAvailability.query.filter_by(coach_id=coach.id).all() if coach else [] + coach_availability = [ + { + 'day_of_week': av.day_of_week, + 'start_time': av.start_time.strftime('%H:%M'), + 'end_time': av.end_time.strftime('%H:%M'), + } + for av in availabilities + ] if request.method == 'POST': - date_str = request.form.get('date') - start_time_str = request.form.get('start_time') - end_time_str = request.form.get('end_time') - points = request.form.get('points', '').strip() - if not coach: flash(_('Cannot request One on One - no coach assigned.'), 'danger') return redirect(url_for('users.one_on_one')) try: - date_obj = datetime.strptime(date_str, '%Y-%m-%d').date() - start_time = datetime.strptime(start_time_str, '%H:%M').time() - end_time = datetime.strptime(end_time_str, '%H:%M').time() - except (ValueError, TypeError): - flash(_('Invalid date or time format.'), 'danger') + data = OneOnOneRequestSchema().load(form_payload()) + except ValidationError as err: + flash_validation_errors(err) return redirect(url_for('users.one_on_one')) - check_date = datetime.strptime(date_str, '%Y-%m-%d') - day_of_week = check_date.weekday() + date_obj = data['date'] + start_time = data['start_time'] + end_time = data['end_time'] + points = data['points'] or '' + # Compared as times, not as strings. The old code parsed the three + # form fields into objects and then compared the *original strings* + # against the serialised availability — which worked only because + # both sides happened to be zero-padded HH:MM. is_available = any( - av['day_of_week'] == day_of_week - and av['start_time'] <= start_time_str - and av['end_time'] >= end_time_str - for av in coach_availability + av.day_of_week == date_obj.weekday() + and av.start_time <= start_time + and av.end_time >= end_time + for av in availabilities ) if not is_available: @@ -105,9 +108,9 @@ def one_on_one(): send_discord_notification( player_name=current_user.full_name, points=points, - date_str=date_str, - start_time_str=start_time_str, - end_time_str=end_time_str, + date_str=date_obj.strftime('%Y-%m-%d'), + start_time_str=start_time.strftime('%H:%M'), + end_time_str=end_time.strftime('%H:%M'), team_name=org_team.name if org_team else 'Unknown Team', coach_name=coach.full_name, coach_discord=coach.discord_username or '', diff --git a/app/timeslots.py b/app/timeslots.py new file mode 100644 index 0000000..5858807 --- /dev/null +++ b/app/timeslots.py @@ -0,0 +1,60 @@ +"""The half-hour slot, in one place (MNT-12). + +Availability, coach bookings and matches are all built from the same rule: +*a slot given a start and no end lasts thirty minutes*. It was written five +times, in four modules, and the copies had drifted in the way that matters — +not in the arithmetic, but in what each did when the input was wrong: + + matches.py flashed an error and redirected + matches.py, later set start_time to None and reported success + team_matches.py `except ValueError: pass`, silently + availability.py `continue`, dropping the slot without a word + +Waves E and G closed the match side by putting a marshmallow schema at the +form boundary. This module is the other half: the constant and the two +functions the remaining callers need, so that the availability routes can +use the same schema treatment without each one re-deciding what a slot is. + +`DEFAULT_SLOT_MINUTES` is deliberately not configurable. It is a product +decision written into the UI — the availability grid draws half-hour cells — +and a setting would let the two disagree. +""" + +from datetime import date as date_cls +from datetime import datetime, timedelta + +#: How long a slot lasts when only its start is given. +DEFAULT_SLOT_MINUTES = 30 + +#: Monday-first, matching `datetime.weekday()` and the availability grid. +DAY_NAMES = ('Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday') + + +def slot_end(start_time, minutes=DEFAULT_SLOT_MINUTES, on=None): + """The end of a slot starting at `start_time`. + + Args: + start_time: A `datetime.time`. + minutes: Slot length; defaults to DEFAULT_SLOT_MINUTES. + on: The date the slot falls on. Only matters for a slot that would + cross midnight, where the arbitrary date the old helpers used + (`datetime.today()`) made the result depend on when the code ran. + + Returns: + datetime.time: The end of the slot, wrapping past midnight like the + previous implementations did. + """ + anchor = on or date_cls(2000, 1, 1) + return (datetime.combine(anchor, start_time) + timedelta(minutes=minutes)).time() + + +def day_name(day_of_week): + """Name of a weekday index, or a readable fallback. + + Every caller indexed a module-level list directly, so an out-of-range + day — which nothing prevented before the schemas — was an IndexError + inside a JSON route, i.e. a 500 with an HTML body. + """ + if 0 <= day_of_week < len(DAY_NAMES): + return DAY_NAMES[day_of_week] + return f'Day {day_of_week}' diff --git a/app/validators.py b/app/validators.py index 277006f..2b6738f 100644 --- a/app/validators.py +++ b/app/validators.py @@ -461,28 +461,41 @@ class UploadContractSchema(StripMixin): class OneOnOneRequestSchema(StripMixin): - """Validate One on One session request form input. + """A player asking their coach for a session (MNT-12). - Fields: - date: Date string (YYYY-MM-DD), required. - start_time: Time string (HH:MM), required. - end_time: Time string (HH:MM), required. - points: Optional text. + This schema existed before and **was never called**: not imported by any + route, not exercised by any test. `one_on_one.py` parsed the same three + fields by hand, three lines apart, and parsed the date a second time to + get its weekday. That is the pattern the audit named as SEC-AUTHZ-001 — + a validation policy that is written down and not applied — surviving in + a corner nobody had looked at since. + + The fields were `String` + `Regexp`, which checked the shape and left the + caller to convert. `Date` and `Time` do both, so the route receives the + objects it is going to store and the "is this a date" question is asked + once, in one place. """ - date = fields.String( + date = fields.Date( required=True, - validate=validate.Regexp( - r'^\d{4}-\d{2}-\d{2}$', error=_l('Date must be in YYYY-MM-DD format.') - ), + error_messages={ + 'invalid': _l('Date must be in YYYY-MM-DD format.'), + 'required': _l('A date is required.'), + }, ) - start_time = fields.String( + start_time = fields.Time( required=True, - validate=validate.Regexp(r'^\d{2}:\d{2}$', error=_l('Start time must be in HH:MM format.')), + error_messages={ + 'invalid': _l('Start time must be in HH:MM format.'), + 'required': _l('A start time is required.'), + }, ) - end_time = fields.String( + end_time = fields.Time( required=True, - validate=validate.Regexp(r'^\d{2}:\d{2}$', error=_l('End time must be in HH:MM format.')), + error_messages={ + 'invalid': _l('End time must be in HH:MM format.'), + 'required': _l('An end time is required.'), + }, ) points = fields.String( validate=validate.Length(max=2000, error=_l('Points must be 2000 characters or less.')), @@ -490,22 +503,57 @@ class OneOnOneRequestSchema(StripMixin): load_default=None, ) + @validates_schema + def validate_window(self, data, **kwargs): + """A session cannot end before it starts. -class DisponibilityAddSchema(StripMixin): - """Validate disponibility block addition. + Nothing checked this. The form's own `