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 `