From 2586f15ae80a7be6bcfd3330a5c6813b13ff6191 Mon Sep 17 00:00:00 2001 From: Alfredo Di Stasio Date: Tue, 10 Mar 2026 15:53:55 +0100 Subject: [PATCH] Make invalid search input explicit in UI and API --- apps/api/views.py | 38 ++++++++++++---- apps/players/views.py | 19 ++++---- templates/players/index.html | 13 ++++++ templates/players/partials/results.html | 14 ++++++ tests/test_api.py | 25 +++++++++++ tests/test_players_views.py | 58 +++++++++++++++++++++++++ 6 files changed, 150 insertions(+), 17 deletions(-) diff --git a/apps/api/views.py b/apps/api/views.py index 53d3190..cc66ea6 100644 --- a/apps/api/views.py +++ b/apps/api/views.py @@ -1,5 +1,7 @@ from rest_framework import generics +from rest_framework import status from rest_framework.pagination import PageNumberPagination +from rest_framework.response import Response from rest_framework.throttling import AnonRateThrottle, UserRateThrottle from apps.competitions.models import Competition, Season @@ -39,17 +41,35 @@ class PlayerSearchApiView(ReadOnlyBaseAPIView, generics.ListAPIView): serializer_class = PlayerListSerializer pagination_class = ApiPagination + def get_search_form(self): + if not hasattr(self, "_search_form"): + self._search_form = PlayerSearchForm(self.request.query_params) + return self._search_form + + def _validation_error_response(self): + form = self.get_search_form() + return Response( + { + "detail": "Invalid search parameters.", + "errors": form.errors.get_json_data(escape_html=True), + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + def list(self, request, *args, **kwargs): + form = self.get_search_form() + if form.is_bound and not form.is_valid(): + return self._validation_error_response() + return super().list(request, *args, **kwargs) + def get_queryset(self): - form = PlayerSearchForm(self.request.query_params or None) + form = self.get_search_form() queryset = base_player_queryset() - if form.is_valid(): - queryset = filter_players(queryset, form.cleaned_data) - sort_key = form.cleaned_data.get("sort", "name_asc") - if sort_key in METRIC_SORT_KEYS: - queryset = annotate_player_metrics(queryset, form.cleaned_data) - queryset = apply_sorting(queryset, sort_key) - else: - queryset = queryset.order_by("full_name", "id") + queryset = filter_players(queryset, form.cleaned_data) + sort_key = form.cleaned_data.get("sort", "name_asc") + if sort_key in METRIC_SORT_KEYS: + queryset = annotate_player_metrics(queryset, form.cleaned_data) + queryset = apply_sorting(queryset, sort_key) return queryset diff --git a/apps/players/views.py b/apps/players/views.py index c0969b6..930ec23 100644 --- a/apps/players/views.py +++ b/apps/players/views.py @@ -33,7 +33,7 @@ class PlayerSearchView(ListView): def get_form(self): if not hasattr(self, "_search_form"): - self._search_form = PlayerSearchForm(self.request.GET or None) + self._search_form = PlayerSearchForm(self.request.GET) return self._search_form def get_paginate_by(self, queryset): @@ -44,20 +44,23 @@ class PlayerSearchView(ListView): def get_queryset(self): form = self.get_form() + form_valid = form.is_valid() + if form.is_bound and not form_valid: + return Player.objects.none() + queryset = base_player_queryset() - if form.is_valid(): - queryset = filter_players(queryset, form.cleaned_data) - queryset = annotate_player_metrics(queryset, form.cleaned_data) - queryset = apply_sorting(queryset, form.cleaned_data.get("sort", "name_asc")) - else: - queryset = annotate_player_metrics(queryset).order_by("full_name", "id") + queryset = filter_players(queryset, form.cleaned_data) + queryset = annotate_player_metrics(queryset, form.cleaned_data) + queryset = apply_sorting(queryset, form.cleaned_data.get("sort", "name_asc")) return queryset def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - context["search_form"] = self.get_form() + search_form = self.get_form() + context["search_form"] = search_form + context["search_has_errors"] = search_form.is_bound and bool(search_form.errors) context["favorite_player_ids"] = set() if self.request.user.is_authenticated: player_ids = [player.id for player in context["players"]] diff --git a/templates/players/index.html b/templates/players/index.html index 09e84ad..87d0044 100644 --- a/templates/players/index.html +++ b/templates/players/index.html @@ -6,6 +6,19 @@

Player Search

Filter players by profile, origin, context, and production metrics.

+ {% if search_has_errors %} +
+

Please correct the highlighted filters.

+ {% for field in search_form %} + {% for error in field.errors %} +

{{ field.label }}: {{ error }}

+ {% endfor %} + {% endfor %} + {% for error in search_form.non_field_errors %} +

{{ error }}

+ {% endfor %} +
+ {% endif %}
+{% if search_has_errors %} +
+

Search filters are invalid.

+ {% for field in search_form %} + {% for error in field.errors %} +

{{ field.label }}: {{ error }}

+ {% endfor %} + {% endfor %} + {% for error in search_form.non_field_errors %} +

{{ error }}

+ {% endfor %} +
+{% endif %} + {% if request.user.is_authenticated %} {% include "scouting/partials/save_search_form.html" %} {% endif %} diff --git a/tests/test_api.py b/tests/test_api.py index 7a8e95b..d5a30b7 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -196,3 +196,28 @@ def test_api_combined_filters_respect_same_player_season_context(client): ) assert response.status_code == 200 assert response.json()["count"] == 0 + + +@pytest.mark.django_db +def test_players_api_returns_400_for_invalid_numeric_filter(client): + response = client.get(reverse("api:players"), data={"points_per_game_min": "abc"}) + assert response.status_code == 400 + payload = response.json() + assert payload["detail"] == "Invalid search parameters." + assert "points_per_game_min" in payload["errors"] + + +@pytest.mark.django_db +def test_players_api_returns_400_for_invalid_choice_filter(client): + response = client.get(reverse("api:players"), data={"sort": "not-a-sort"}) + assert response.status_code == 400 + payload = response.json() + assert "sort" in payload["errors"] + + +@pytest.mark.django_db +def test_players_api_returns_400_for_invalid_range_combination(client): + response = client.get(reverse("api:players"), data={"age_min": 30, "age_max": 20}) + assert response.status_code == 400 + payload = response.json() + assert "age_max" in payload["errors"] diff --git a/tests/test_players_views.py b/tests/test_players_views.py index 5748634..d055b86 100644 --- a/tests/test_players_views.py +++ b/tests/test_players_views.py @@ -135,3 +135,61 @@ def test_player_detail_page_loads(client): body = response.content.decode() assert "Paul Martin" in body assert "P. Martin" in body + + +@pytest.mark.django_db +def test_player_search_invalid_numeric_filter_shows_errors_and_no_broad_fallback(client): + nationality = Nationality.objects.create(name="Belgium", iso2_code="BE", iso3_code="BEL") + position = Position.objects.create(code="PG", name="Point Guard") + role = Role.objects.create(code="playmaker", name="Playmaker") + Player.objects.create( + first_name="Any", + last_name="Player", + full_name="Any Player", + birth_date=date(2000, 1, 1), + nationality=nationality, + nominal_position=position, + inferred_role=role, + ) + + response = client.get(reverse("players:index"), data={"points_per_game_min": "abc", "q": "Any"}) + + assert response.status_code == 200 + assert list(response.context["players"]) == [] + assert response.context["search_has_errors"] is True + assert "points per game min" in response.content.decode().lower() + assert response.context["search_form"]["q"].value() == "Any" + + +@pytest.mark.django_db +def test_player_search_invalid_choice_filter_shows_errors(client): + response = client.get(reverse("players:index"), data={"sort": "bad-sort"}) + assert response.status_code == 200 + assert list(response.context["players"]) == [] + assert response.context["search_has_errors"] is True + assert "select a valid choice" in response.content.decode().lower() + + +@pytest.mark.django_db +def test_player_search_invalid_range_combination_shows_errors(client): + response = client.get(reverse("players:index"), data={"age_min": 30, "age_max": 20}) + assert response.status_code == 200 + assert list(response.context["players"]) == [] + assert response.context["search_has_errors"] is True + body = response.content.decode().lower() + assert "age max" in body + assert "must be >=" in body or "must be >=" in body + + +@pytest.mark.django_db +def test_player_search_htmx_invalid_filters_return_validation_feedback(client): + response = client.get( + reverse("players:index"), + HTTP_HX_REQUEST="true", + data={"points_per_game_min": "abc"}, + ) + + assert response.status_code == 200 + body = response.content.decode().lower() + assert "search filters are invalid" in body + assert "points per game min" in body