feat(perf): borner les vues de liste, et choisir le motif une bonne fois
MNT-14. Chaque vue de liste faisait .all() sur sa table. L'audit evaluait l'impact a nul -- justement, a l'echelle d'une association etudiante -- et recommandait de choisir le motif maintenant plutot que de le retro-adapter plus tard. C'est ce que ceci est. Applique a list_users, list_evaluations et team_matches.list_matches. Pour cette derniere, la pagination borne aussi la boucle sur les participants, qui est le N+1 que le constat designait comme le premier a se degrader. Trois decisions, parce que ce sont celles qui se prennent deux fois differemment sinon. error_out=False : les numeros de page arrivent par l'URL, donc ?page=999 est une chose qu'on tape ou qu'un signet perime contient. Le defaut de Flask-SQLAlchemy y repond par un 404, ce qui est deroutant pour quelqu'un qui est simplement alle une page trop loin. Un plafond sur per_page : c'est aussi un parametre d'URL, et sans plafond ?per_page=100000 redonne a la main exactement la requete non bornee que la pagination existe pour empecher. page_url est un global Jinja plutot qu'une valeur que chaque vue passe. Ce qui se rate avec des liens de pagination, c'est le reste de la chaine de requete : la liste d'evaluations porte sort et order, celle des matchs d'equipe porte team_id, et un lien qui les perd reinitialise silencieusement la vue que la personne regardait. Les deux tests qui l'epinglent tombent si page_url cesse de les recopier -- verifie par mutation. Les tris sont completes par une cle unique : une requete paginee sans ORDER BY stable peut montrer la meme ligne deux fois et jamais une autre. Au passage, huit entrees fuzzy corrigees dans les catalogues, dont deux laissees par le commit SEC-16 : une entree fuzzy est ignoree a l'execution, donc ces messages retombaient en anglais. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
@@ -0,0 +1,161 @@
|
||||
"""Bounded list views, and the links between their pages.
|
||||
|
||||
MNT-14. Every list view ran `.all()` on its table. The audit rated the impact
|
||||
as nil at the scale of a student club and recommended choosing the pattern
|
||||
now rather than retro-fitting one — which is what this is.
|
||||
|
||||
The tests worth having are not "does page 2 exist". They are the three ways
|
||||
pagination is normally got wrong:
|
||||
|
||||
- a link that drops the rest of the query string, silently resetting the
|
||||
sort or the filter the person was using;
|
||||
- `?page=999` answering 404 to someone who went one page too far;
|
||||
- `?per_page=100000`, which hands back by hand exactly the unbounded query
|
||||
the pagination was added to prevent.
|
||||
"""
|
||||
|
||||
import re
|
||||
|
||||
import pytest
|
||||
|
||||
from app.extensions import db
|
||||
from app.pagination import DEFAULT_PER_PAGE, MAX_PER_PAGE
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def many_users(app, make_user):
|
||||
"""Enough accounts to need a second page at a small per_page."""
|
||||
for _index in range(7):
|
||||
make_user('player')
|
||||
return 7
|
||||
|
||||
|
||||
class TestTheUserList:
|
||||
def test_per_page_bounds_the_rows(self, app, client, as_role, many_users):
|
||||
as_role('admin')
|
||||
|
||||
response = client.get('/users?per_page=3')
|
||||
|
||||
assert response.status_code == 200
|
||||
# One <tr> per user in the body, plus the header row.
|
||||
rows = re.findall(r'<tr[ >]', response.get_data(as_text=True))
|
||||
assert len(rows) <= 4, 'the page returned more rows than per_page allows'
|
||||
|
||||
def test_the_second_page_holds_different_people(self, app, client, as_role, many_users):
|
||||
as_role('admin')
|
||||
|
||||
first = client.get('/users?per_page=3').get_data(as_text=True)
|
||||
second = client.get('/users?per_page=3&page=2').get_data(as_text=True)
|
||||
|
||||
assert first != second
|
||||
|
||||
def test_a_page_past_the_end_is_empty_not_a_404(self, app, client, as_role, many_users):
|
||||
"""Page numbers come from the URL, so a stale bookmark is a normal
|
||||
thing to receive. Flask-SQLAlchemy's default answers it with a 404."""
|
||||
as_role('admin')
|
||||
|
||||
response = client.get('/users?page=999')
|
||||
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_per_page_is_capped(self, app, client, as_role, many_users):
|
||||
"""Without the cap, `?per_page=100000` is the unbounded query again."""
|
||||
as_role('admin')
|
||||
|
||||
response = client.get(f'/users?per_page={MAX_PER_PAGE * 100}')
|
||||
|
||||
assert response.status_code == 200
|
||||
|
||||
def test_a_nonsense_page_does_not_crash(self, app, client, as_role, many_users):
|
||||
as_role('admin')
|
||||
|
||||
assert client.get('/users?page=abc').status_code == 200
|
||||
assert client.get('/users?page=-4').status_code == 200
|
||||
assert client.get('/users?per_page=0').status_code == 200
|
||||
|
||||
|
||||
class TestThePageLinksKeepTheQueryString:
|
||||
"""The failure that is easy to ship and hard to notice: the person sorts
|
||||
a column, clicks "next", and lands on an unsorted page 2."""
|
||||
|
||||
def test_evaluations_links_carry_the_sort(self, app, client, as_role, make_user):
|
||||
from datetime import date
|
||||
|
||||
from app.models import Evaluation, Tryout
|
||||
|
||||
# One evaluation per player: the table has a unique constraint on
|
||||
# (tryout, player, evaluator), which is DB-006 working as intended.
|
||||
player_ids = [make_user('player') for _ in range(3)]
|
||||
admin_id = as_role('admin')
|
||||
with app.app_context():
|
||||
tryout = Tryout(
|
||||
title='Spring',
|
||||
game='Valorant',
|
||||
date=date(2030, 4, 1),
|
||||
created_by=admin_id,
|
||||
)
|
||||
db.session.add(tryout)
|
||||
db.session.flush()
|
||||
for player_id in player_ids:
|
||||
db.session.add(
|
||||
Evaluation(
|
||||
tryout_id=tryout.id,
|
||||
player_id=player_id,
|
||||
evaluator_id=admin_id,
|
||||
overall_score=5,
|
||||
)
|
||||
)
|
||||
db.session.commit()
|
||||
|
||||
body = client.get('/evaluations?sort=player&order=asc&per_page=1').get_data(as_text=True)
|
||||
|
||||
links = re.findall(r'href="([^"]*page=\d+[^"]*)"', body)
|
||||
assert links, 'no pagination link was rendered; this test proves nothing'
|
||||
assert all('sort=player' in link for link in links), (
|
||||
f'a page link dropped the sort: {links}'
|
||||
)
|
||||
assert all('order=asc' in link for link in links)
|
||||
|
||||
def test_team_matches_links_carry_the_team_filter(self, app, client, as_role, make_user):
|
||||
from datetime import date
|
||||
|
||||
from app.models import OrgTeam, TeamMatch
|
||||
|
||||
admin_id = as_role('admin')
|
||||
with app.app_context():
|
||||
team = OrgTeam(name='Varsity', created_by=admin_id)
|
||||
db.session.add(team)
|
||||
db.session.flush()
|
||||
team_id = team.id
|
||||
for index in range(3):
|
||||
db.session.add(
|
||||
TeamMatch(
|
||||
title=f'Match {index}',
|
||||
date=date(2030, 5, 1),
|
||||
org_team_id=team_id,
|
||||
created_by=admin_id,
|
||||
)
|
||||
)
|
||||
db.session.commit()
|
||||
|
||||
body = client.get(f'/team-matches?team_id={team_id}&per_page=1').get_data(as_text=True)
|
||||
|
||||
links = re.findall(r'href="([^"]*page=\d+[^"]*)"', body)
|
||||
assert links, 'no pagination link was rendered; this test proves nothing'
|
||||
assert all(f'team_id={team_id}' in link for link in links), (
|
||||
f'a page link dropped the team filter: {links}'
|
||||
)
|
||||
|
||||
|
||||
class TestTheControlsStayOutOfTheWayWhenThereIsOnePage:
|
||||
def test_no_controls_for_a_short_list(self, app, client, as_role):
|
||||
as_role('admin')
|
||||
|
||||
body = client.get('/users').get_data(as_text=True)
|
||||
|
||||
assert 'aria-label' not in body or 'page=2' not in body
|
||||
|
||||
|
||||
class TestTheDefault:
|
||||
def test_it_is_a_page_size_not_the_whole_table(self):
|
||||
assert 0 < DEFAULT_PER_PAGE <= MAX_PER_PAGE
|
||||
Reference in New Issue
Block a user