refactor(validation): un schema a la frontiere des matchs
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.
This commit is contained in:
GGThed
@@ -0,0 +1,322 @@
|
||||
"""Scheduling a match, through the form (ARCH-005).
|
||||
|
||||
Before this, `matches.py` read fifteen fields off `request.form` by hand and
|
||||
believed all of them. What that produced was not loud:
|
||||
|
||||
- `edit_match` caught a bad time and set `start_time = None`, then said the
|
||||
match had been updated. The match lost its time and the calendar showed
|
||||
it at midnight;
|
||||
- `match_type` was accepted as any string. An unknown one created a match
|
||||
with no participants and no complaint;
|
||||
- an end time before the start was stored as given;
|
||||
- `title` is NOT NULL in the model and unvalidated in the route, so an
|
||||
empty one was a 500.
|
||||
|
||||
The route now loads a schema and reports every rejection the same way.
|
||||
"""
|
||||
|
||||
from datetime import date, time
|
||||
|
||||
import pytest
|
||||
|
||||
from app.extensions import db
|
||||
from app.models import (
|
||||
Match,
|
||||
MatchParticipant,
|
||||
OrgTeam,
|
||||
Team,
|
||||
TeamMatch,
|
||||
TeamMember,
|
||||
Tryout,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tryout_setup(app, as_role, make_user):
|
||||
"""A tryout run by the logged-in admin, with two teams and four players."""
|
||||
admin_id = as_role('admin')
|
||||
player_ids = [make_user('player') for _ in range(4)]
|
||||
|
||||
with app.app_context():
|
||||
org_team = OrgTeam(name='Varsity', created_by=admin_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()
|
||||
|
||||
alpha = Team(tryout_id=tryout.id, name='Alpha', created_by=admin_id)
|
||||
beta = Team(tryout_id=tryout.id, name='Beta', created_by=admin_id)
|
||||
db.session.add_all([alpha, beta])
|
||||
db.session.flush()
|
||||
|
||||
db.session.add_all(
|
||||
[
|
||||
TeamMember(team_id=alpha.id, player_id=player_ids[0]),
|
||||
TeamMember(team_id=alpha.id, player_id=player_ids[1]),
|
||||
TeamMember(team_id=beta.id, player_id=player_ids[2]),
|
||||
TeamMember(team_id=beta.id, player_id=player_ids[3]),
|
||||
]
|
||||
)
|
||||
db.session.commit()
|
||||
|
||||
return {
|
||||
'tryout_id': tryout.id,
|
||||
'alpha_id': alpha.id,
|
||||
'beta_id': beta.id,
|
||||
'players': player_ids,
|
||||
}
|
||||
|
||||
|
||||
VALID = {
|
||||
'title': 'Scrim night',
|
||||
'date': '2030-04-01',
|
||||
'start_time': '18:00',
|
||||
'end_time': '20:00',
|
||||
'location': 'Arena',
|
||||
'match_type': 'player_scrim',
|
||||
}
|
||||
|
||||
|
||||
def _create(client, setup, **overrides):
|
||||
payload = dict(VALID)
|
||||
payload.update(overrides)
|
||||
return client.post(f'/matches/create/{setup["tryout_id"]}', data=payload, follow_redirects=True)
|
||||
|
||||
|
||||
def _only_match(app):
|
||||
with app.app_context():
|
||||
return Match.query.one_or_none()
|
||||
|
||||
|
||||
class TestCreating:
|
||||
def test_a_valid_scrim_is_stored_with_typed_values(self, app, client, tryout_setup):
|
||||
_create(client, tryout_setup, player_ids=[str(p) for p in tryout_setup['players'][:2]])
|
||||
|
||||
match = _only_match(app)
|
||||
assert match is not None
|
||||
assert match.title == 'Scrim night'
|
||||
assert match.date == date(2030, 4, 1)
|
||||
assert match.start_time == time(18, 0)
|
||||
assert match.end_time == time(20, 0)
|
||||
|
||||
def test_a_missing_end_time_defaults_to_thirty_minutes(self, app, client, tryout_setup):
|
||||
_create(client, tryout_setup, end_time='')
|
||||
|
||||
assert _only_match(app).end_time == time(18, 30)
|
||||
|
||||
def test_a_team_match_draws_its_players_from_the_teams(self, app, client, tryout_setup):
|
||||
_create(
|
||||
client,
|
||||
tryout_setup,
|
||||
match_type='team_vs_team',
|
||||
team1_id=str(tryout_setup['alpha_id']),
|
||||
team2_id=str(tryout_setup['beta_id']),
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
participants = MatchParticipant.query.all()
|
||||
assert len(participants) == 4
|
||||
assert {p.team_side for p in participants} == {1, 2}
|
||||
|
||||
def test_a_player_vs_player_match_reads_the_comma_separated_ids(
|
||||
self, app, client, tryout_setup
|
||||
):
|
||||
players = tryout_setup['players']
|
||||
_create(
|
||||
client,
|
||||
tryout_setup,
|
||||
match_type='player_vs_player',
|
||||
team1_player_ids=f'{players[0]},{players[1]}',
|
||||
team2_player_ids=f'{players[2]},{players[3]}',
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
sides = {p.player_id: p.team_side for p in MatchParticipant.query.all()}
|
||||
assert sides[players[0]] == 1
|
||||
assert sides[players[3]] == 2
|
||||
|
||||
|
||||
class TestRefusals:
|
||||
"""Every one of these used to be accepted."""
|
||||
|
||||
def test_an_unknown_match_type_is_refused(self, app, client, tryout_setup):
|
||||
"""It used to create a match that no player was ever attached to."""
|
||||
_create(client, tryout_setup, match_type='battle_royale')
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_an_empty_title_is_refused(self, app, client, tryout_setup):
|
||||
"""title is NOT NULL: unvalidated, this was an IntegrityError."""
|
||||
_create(client, tryout_setup, title='')
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_an_end_before_the_start_is_refused(self, app, client, tryout_setup):
|
||||
_create(client, tryout_setup, start_time='20:00', end_time='18:00')
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_a_malformed_time_is_refused(self, app, client, tryout_setup):
|
||||
_create(client, tryout_setup, start_time='six o clock')
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_a_malformed_date_is_refused(self, app, client, tryout_setup):
|
||||
_create(client, tryout_setup, date='next tuesday')
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_a_team_cannot_play_itself(self, app, client, tryout_setup):
|
||||
_create(
|
||||
client,
|
||||
tryout_setup,
|
||||
match_type='team_vs_team',
|
||||
team1_id=str(tryout_setup['alpha_id']),
|
||||
team2_id=str(tryout_setup['alpha_id']),
|
||||
)
|
||||
|
||||
assert _only_match(app) is None
|
||||
|
||||
def test_a_malformed_player_selection_is_refused_not_crashed(self, app, client, tryout_setup):
|
||||
"""'a,b' reached int() unguarded and was a 500."""
|
||||
response = _create(
|
||||
client, tryout_setup, match_type='player_vs_player', team1_player_ids='a,b'
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
assert _only_match(app) is None
|
||||
|
||||
|
||||
class TestEditing:
|
||||
def _edit(self, client, app, tryout_setup, **overrides):
|
||||
_create(client, tryout_setup)
|
||||
match = _only_match(app)
|
||||
payload = dict(VALID, title='Renamed', status='scheduled')
|
||||
payload.update(overrides)
|
||||
return client.post(
|
||||
f'/matches/{match.id}/edit', data=payload, follow_redirects=True
|
||||
), match.id
|
||||
|
||||
def test_a_valid_edit_is_applied(self, app, client, tryout_setup):
|
||||
_, match_id = self._edit(client, app, tryout_setup)
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(Match, match_id).title == 'Renamed'
|
||||
|
||||
def test_a_bad_time_no_longer_wipes_the_one_that_was_there(self, app, client, tryout_setup):
|
||||
"""The defect this whole change exists for.
|
||||
|
||||
`except ValueError: match.start_time = None` — the form was accepted,
|
||||
the time was destroyed, and the page said the match had been updated.
|
||||
"""
|
||||
_, match_id = self._edit(client, app, tryout_setup, start_time='25:99')
|
||||
|
||||
with app.app_context():
|
||||
match = db.session.get(Match, match_id)
|
||||
assert match.start_time == time(18, 0), 'the stored time must survive a bad edit'
|
||||
assert match.title == 'Scrim night', 'and so must everything else on the form'
|
||||
|
||||
def test_an_unknown_status_is_refused_rather_than_ignored(self, app, client, tryout_setup):
|
||||
_, match_id = self._edit(client, app, tryout_setup, status='postponed')
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(Match, match_id).title == 'Scrim night'
|
||||
|
||||
def test_the_match_type_cannot_be_changed_by_the_form(self, app, client, tryout_setup):
|
||||
"""It decides how participants are drawn; changing it would strand
|
||||
the ones already attached."""
|
||||
_, match_id = self._edit(client, app, tryout_setup, match_type='team_vs_team')
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(Match, match_id).match_type == 'player_scrim'
|
||||
|
||||
|
||||
class TestTeamMatchEditing:
|
||||
"""Regular-season matches went through the same treatment.
|
||||
|
||||
Their edit path checked date, start time and end time one at a time,
|
||||
each flashing and redirecting on its own — so a mistyped end time threw
|
||||
away everything the person had typed into the other ten fields.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def team_match(self, app, as_role):
|
||||
admin_id = as_role('admin')
|
||||
with app.app_context():
|
||||
org_team = OrgTeam(name='Varsity', created_by=admin_id)
|
||||
db.session.add(org_team)
|
||||
db.session.flush()
|
||||
|
||||
match = TeamMatch(
|
||||
org_team_id=org_team.id,
|
||||
title='League night',
|
||||
date=date(2030, 5, 1),
|
||||
start_time=time(19, 0),
|
||||
end_time=time(21, 0),
|
||||
created_by=admin_id,
|
||||
)
|
||||
db.session.add(match)
|
||||
db.session.commit()
|
||||
return match.id
|
||||
|
||||
VALID = {
|
||||
'title': 'League night',
|
||||
'date': '2030-05-01',
|
||||
'start_time': '19:00',
|
||||
'end_time': '21:00',
|
||||
'status': 'scheduled',
|
||||
}
|
||||
|
||||
def _edit(self, client, match_id, **overrides):
|
||||
return client.post(
|
||||
f'/team-matches/{match_id}/edit',
|
||||
data=dict(self.VALID, **overrides),
|
||||
follow_redirects=True,
|
||||
)
|
||||
|
||||
def test_a_valid_edit_is_applied(self, app, client, team_match):
|
||||
self._edit(client, team_match, title='Renamed', location='Arena')
|
||||
|
||||
with app.app_context():
|
||||
match = db.session.get(TeamMatch, team_match)
|
||||
assert match.title == 'Renamed'
|
||||
assert match.location == 'Arena'
|
||||
|
||||
def test_a_bad_end_time_changes_nothing_and_says_so_in_place(self, app, client, team_match):
|
||||
"""It used to redirect, one redirect per bad field.
|
||||
|
||||
Known limit, and the reason this asserts on the message rather than
|
||||
on the inputs: the form is re-rendered from the stored record, so the
|
||||
submitted values are not echoed back. Every problem is now reported
|
||||
at once and in place, which is the part that changed.
|
||||
"""
|
||||
response = self._edit(client, team_match, title='Renamed', end_time='half past nine')
|
||||
|
||||
with app.app_context():
|
||||
match = db.session.get(TeamMatch, team_match)
|
||||
assert match.title == 'League night'
|
||||
assert match.end_time == time(21, 0)
|
||||
assert 'alert-danger' in response.get_data(as_text=True)
|
||||
|
||||
def test_an_end_before_the_start_is_refused(self, app, client, team_match):
|
||||
self._edit(client, team_match, start_time='21:00', end_time='19:00')
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(TeamMatch, team_match).start_time == time(19, 0)
|
||||
|
||||
def test_an_empty_title_is_refused(self, app, client, team_match):
|
||||
"""`request.form.get('title', team_match.title)` only defaulted on an
|
||||
absent key, and a form always sends the key — so a cleared title was
|
||||
stored as an empty string in a NOT NULL column."""
|
||||
self._edit(client, team_match, title='')
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(TeamMatch, team_match).title == 'League night'
|
||||
Reference in New Issue
Block a user