From e1e287f13e1e22fd6780f5bb72c1c2bc5089b9b0 Mon Sep 17 00:00:00 2001 From: Eemeli Ranta Date: Wed, 11 Dec 2024 20:03:46 +0200 Subject: [PATCH] Add more filter options to `unitsAll` endpoint --- .../test_unit/test_filtering.py | 116 +++++++++++++----- .../test_unit/test_query_all.py | 53 -------- .../test_unit/test_query_permissions.py | 18 ++- .../api/graphql/types/unit/filtersets.py | 82 +++++++------ 4 files changed, 145 insertions(+), 124 deletions(-) delete mode 100644 tests/test_graphql_api/test_unit/test_query_all.py diff --git a/tests/test_graphql_api/test_unit/test_filtering.py b/tests/test_graphql_api/test_unit/test_filtering.py index e8262289d..82c2e8f42 100644 --- a/tests/test_graphql_api/test_unit/test_filtering.py +++ b/tests/test_graphql_api/test_unit/test_filtering.py @@ -5,13 +5,13 @@ import freezegun import pytest from django.utils.timezone import get_default_timezone -from graphene_django_extensions.testing import build_query +from graphene_django.settings import graphene_settings from tilavarauspalvelu.enums import ReservationKind from tests.factories import ReservationFactory, ReservationUnitFactory, UnitFactory, UserFactory -from .helpers import units_query +from .helpers import units_all_query, units_query # Applied to all tests pytestmark = [ @@ -19,22 +19,28 @@ ] -def test_units__filter__by_name(graphql): +@pytest.mark.parametrize("gql_query", [units_query, units_all_query]) +def test_units__filter__by_name(graphql, gql_query): unit = UnitFactory.create(name_fi="1111") UnitFactory.create(name_fi="2222") UnitFactory.create(name_fi="3333") graphql.login_with_superuser() - response = graphql(units_query(nameFi="111")) + response = graphql(gql_query(nameFi="111")) - assert response.has_errors is False + assert response.has_errors is False, response.errors - assert len(response.edges) == 1 - assert response.node(0) == {"pk": unit.pk} + if gql_query == units_query: + assert len(response.edges) == 1 + assert response.node(0) == {"pk": unit.pk} + else: + assert len(response.first_query_object) == 1 + assert response.first_query_object[0] == {"pk": unit.pk} @freezegun.freeze_time("2021-01-01T12:00:00Z") -def test_units__filter__by_published_reservation_units(graphql): +@pytest.mark.parametrize("gql_query", [units_query, units_all_query]) +def test_units__filter__by_published_reservation_units(graphql, gql_query): unit_1 = UnitFactory.create(name="1") unit_2 = UnitFactory.create(name="2") unit_3 = UnitFactory.create(name="3") @@ -53,18 +59,24 @@ def test_units__filter__by_published_reservation_units(graphql): graphql.login_with_superuser() - query = build_query("units", connection=True, published_reservation_units=True, order_by="nameFiAsc") - response = graphql(query) + response = graphql(gql_query(published_reservation_units=True, order_by="nameFiAsc")) assert response.has_errors is False - assert len(response.edges) == 3 - assert response.node(0) == {"pk": unit_1.pk} - assert response.node(1) == {"pk": unit_2.pk} - assert response.node(2) == {"pk": unit_3.pk} + if gql_query == units_query: + assert len(response.edges) == 3 + assert response.node(0) == {"pk": unit_1.pk} + assert response.node(1) == {"pk": unit_2.pk} + assert response.node(2) == {"pk": unit_3.pk} + else: + assert len(response.first_query_object) == 3 + assert response.first_query_object[0] == {"pk": unit_1.pk} + assert response.first_query_object[1] == {"pk": unit_2.pk} + assert response.first_query_object[2] == {"pk": unit_3.pk} -def test_units__filter__by_own_reservations(graphql): +@pytest.mark.parametrize("gql_query", [units_query, units_all_query]) +def test_units__filter__by_own_reservations(graphql, gql_query): unit_1 = UnitFactory.create(name="1") unit_2 = UnitFactory.create(name="2") unit_3 = UnitFactory.create(name="3") @@ -85,22 +97,32 @@ def test_units__filter__by_own_reservations(graphql): graphql.force_login(user_1) # Own reservations = True - response = graphql(units_query(own_reservations=True)) + response = graphql(gql_query(own_reservations=True)) assert response.has_errors is False - assert len(response.edges) == 2 - assert response.node(0) == {"pk": unit_1.pk} - assert response.node(1) == {"pk": unit_2.pk} + if gql_query == units_query: + assert len(response.edges) == 2 + assert response.node(0) == {"pk": unit_1.pk} + assert response.node(1) == {"pk": unit_2.pk} + else: + assert len(response.first_query_object) == 2 + assert response.first_query_object[0] == {"pk": unit_1.pk} + assert response.first_query_object[1] == {"pk": unit_2.pk} # Own reservations = False - response = graphql(units_query(own_reservations=False)) + response = graphql(gql_query(own_reservations=False)) assert response.has_errors is False - assert len(response.edges) == 1 - assert response.node(0) == {"pk": unit_3.pk} + if gql_query == units_query: + assert len(response.edges) == 1 + assert response.node(0) == {"pk": unit_3.pk} + else: + assert len(response.first_query_object) == 1 + assert response.first_query_object[0] == {"pk": unit_3.pk} -def test_units__filter__by_only_direct_bookable(graphql): +@pytest.mark.parametrize("gql_query", [units_query, units_all_query]) +def test_units__filter__by_only_direct_bookable(graphql, gql_query): # Has only direct reservation unit, should be included unit_1 = UnitFactory.create(name="1") ReservationUnitFactory.create(unit=unit_1, reservation_kind=ReservationKind.DIRECT) @@ -121,17 +143,24 @@ def test_units__filter__by_only_direct_bookable(graphql): # Has no reservation units, should be excluded UnitFactory.create(name="5") - query = units_query(only_direct_bookable=True) + query = gql_query(only_direct_bookable=True) response = graphql(query) assert response.has_errors is False - assert len(response.edges) == 3 - assert response.node(0) == {"pk": unit_1.pk} - assert response.node(1) == {"pk": unit_3.pk} - assert response.node(2) == {"pk": unit_4.pk} + if gql_query == units_query: + assert len(response.edges) == 3 + assert response.node(0) == {"pk": unit_1.pk} + assert response.node(1) == {"pk": unit_3.pk} + assert response.node(2) == {"pk": unit_4.pk} + else: + assert len(response.first_query_object) == 3 + assert response.first_query_object[0] == {"pk": unit_1.pk} + assert response.first_query_object[1] == {"pk": unit_3.pk} + assert response.first_query_object[2] == {"pk": unit_4.pk} -def test_units__filter__by_only_seasonal_bookable(graphql): +@pytest.mark.parametrize("gql_query", [units_query, units_all_query]) +def test_units__filter__by_only_seasonal_bookable(graphql, gql_query): # Has only direct reservation unit, should be excluded unit_1 = UnitFactory.create(name="1") ReservationUnitFactory.create(unit=unit_1, reservation_kind=ReservationKind.DIRECT) @@ -152,11 +181,30 @@ def test_units__filter__by_only_seasonal_bookable(graphql): # Has no reservation units, should be excluded UnitFactory.create(name="5") - query = units_query(only_seasonal_bookable=True) + query = gql_query(only_seasonal_bookable=True) response = graphql(query) assert response.has_errors is False - assert len(response.edges) == 3 - assert response.node(0) == {"pk": unit_2.pk} - assert response.node(1) == {"pk": unit_3.pk} - assert response.node(2) == {"pk": unit_4.pk} + if gql_query == units_query: + assert len(response.edges) == 3 + assert response.node(0) == {"pk": unit_2.pk} + assert response.node(1) == {"pk": unit_3.pk} + assert response.node(2) == {"pk": unit_4.pk} + else: + assert len(response.first_query_object) == 3 + assert response.first_query_object[0] == {"pk": unit_2.pk} + assert response.first_query_object[1] == {"pk": unit_3.pk} + assert response.first_query_object[2] == {"pk": unit_4.pk} + + +def test_unit_all__no_pagination_limit(graphql): + graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 1 + + UnitFactory.create_batch(2) + + graphql.login_with_superuser() + query = units_all_query(fields="pk nameFi nameEn nameSv tprekId") + response = graphql(query) + + assert response.has_errors is False, response.errors + assert len(response.first_query_object) == 2 diff --git a/tests/test_graphql_api/test_unit/test_query_all.py b/tests/test_graphql_api/test_unit/test_query_all.py deleted file mode 100644 index 358278a5f..000000000 --- a/tests/test_graphql_api/test_unit/test_query_all.py +++ /dev/null @@ -1,53 +0,0 @@ -from __future__ import annotations - -import pytest -from graphene_django.settings import graphene_settings - -from tests.factories import UnitFactory, UserFactory -from tests.test_graphql_api.test_unit.helpers import units_all_query - -# Applied to all tests -pytestmark = [ - pytest.mark.django_db, -] - - -def test_unit_all__no_pagination_limit(graphql): - graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 1 - - UnitFactory.create_batch(2) - - graphql.login_with_superuser() - query = units_all_query(fields="pk nameFi nameEn nameSv tprekId") - response = graphql(query) - - assert response.has_errors is False, response.errors - assert len(response.first_query_object) == 2 - - -def test_unit_all__filter__by_name_fi(graphql): - unit = UnitFactory.create(name_fi="foo") - UnitFactory.create(name_fi="bar") - - query = units_all_query(nameFi="foo") - response = graphql(query) - - assert response.has_errors is False, response.errors - assert len(response.first_query_object) == 1 - assert response.first_query_object[0] == {"pk": unit.pk} - - -def test_unit_all__filter__only_with_permission__general_admin(graphql): - unit_1 = UnitFactory.create() - unit_2 = UnitFactory.create() - - user = UserFactory.create_with_general_role() - graphql.force_login(user) - - query = units_all_query(onlyWithPermission=True) - response = graphql(query) - - assert response.has_errors is False, response.errors - assert len(response.first_query_object) == 2 - assert response.first_query_object[0] == {"pk": unit_1.pk} - assert response.first_query_object[1] == {"pk": unit_2.pk} diff --git a/tests/test_graphql_api/test_unit/test_query_permissions.py b/tests/test_graphql_api/test_unit/test_query_permissions.py index 50f906893..4c5668ac0 100644 --- a/tests/test_graphql_api/test_unit/test_query_permissions.py +++ b/tests/test_graphql_api/test_unit/test_query_permissions.py @@ -4,7 +4,7 @@ from tests.factories import PaymentMerchantFactory, UnitFactory, UnitGroupFactory, UserFactory -from .helpers import units_query +from .helpers import units_all_query, units_query # Applied to all tests pytestmark = [ @@ -91,3 +91,19 @@ def test_units__query__show_payment_merchant_with_permissions(graphql): assert len(response.edges) == 1 assert response.node(0) == {"nameFi": unit.name, "paymentMerchant": {"name": unit.payment_merchant.name}} + + +def test_unit_all__filter__only_with_permission__general_admin(graphql): + unit_1 = UnitFactory.create() + unit_2 = UnitFactory.create() + + user = UserFactory.create_with_general_role() + graphql.force_login(user) + + query = units_all_query(onlyWithPermission=True) + response = graphql(query) + + assert response.has_errors is False, response.errors + assert len(response.first_query_object) == 2 + assert response.first_query_object[0] == {"pk": unit_1.pk} + assert response.first_query_object[1] == {"pk": unit_2.pk} diff --git a/tilavarauspalvelu/api/graphql/types/unit/filtersets.py b/tilavarauspalvelu/api/graphql/types/unit/filtersets.py index 5ece7c772..b05529085 100644 --- a/tilavarauspalvelu/api/graphql/types/unit/filtersets.py +++ b/tilavarauspalvelu/api/graphql/types/unit/filtersets.py @@ -15,7 +15,7 @@ from django.db import models from tilavarauspalvelu.models.unit.queryset import UnitQuerySet - from tilavarauspalvelu.typing import AnyUser + from tilavarauspalvelu.typing import AnyUser, WSGIRequest __all__ = [ "UnitAllFilterSet", @@ -24,6 +24,8 @@ class UnitFilterSetMixin: + request: WSGIRequest + def filter_by_only_with_permission(self, qs: models.QuerySet, name: str, value: bool) -> models.QuerySet: if not value: return qs @@ -44,6 +46,28 @@ def filter_by_only_with_permission(self, qs: models.QuerySet, name: str, value: return qs.filter(Q(id__in=u_ids) | Q(unit_groups__in=g_ids)).distinct() + # These filters use information across the relationship between Unit and ReservationUnit. + # FilterSets apply individual filters in separate `queryset.filter(...)` calls. + # + # Due to how Django works when spanning `to-many` relationships in qs.filter(...) calls, + # (see https://docs.djangoproject.com/en/5.0/topics/db/queries/#spanning-multi-valued-relationships), + # this means that using a combination of these filters would result in a queryset where + # ANY of their conditions are true for ANY ReservationUnit linked to a given Unit. + # e.g. using `published_reservation_units=True` and `only_direct_bookable=True` + # would return all Units where ANY of their ReservationUnit are EITHER published OR directly bookable. + # + # In this case, this is incorrect, since we want less permissive behavior: + # e.g. the aforementioned filter should return all Units where ANY of their ReservationUnit are + # BOTH published AND directly bookable. + # + # The incorrect behavior also has the side effect of adding multiple SQL joins to the + # many-to-many though table between Unit and ReservationUnit, which makes the query + # slower, since more duplication is needed. + # + # To prevent this, we need these filters to execute in order, and set `queryset.query.filter_is_sticky = True` + # to indicate to the queryset that it should reuse joins it found from one filter to the next. + # Note that this only applies until the next time the queryset is cloned, (e.g. when using + # `queryset.filter(...)` or `.distinct()`) and thus needs to be reapplied between them. @staticmethod def filter_by_published_reservation_units(qs: models.QuerySet, name: str, value: bool) -> models.QuerySet: now = local_datetime() @@ -92,6 +116,18 @@ def filter_by_only_seasonal_bookable(qs: models.QuerySet, name: str, value: bool qs.query.filter_is_sticky = True return qs.distinct() + def filter_by_own_reservations(self, qs: models.QuerySet, name: str, value: bool) -> models.QuerySet: + user = self.request.user + + if user.is_anonymous: + return qs.none() + + # Prevent multiple joins, see explanation above. + qs.query.filter_is_sticky = True + qs = qs.filter(Q(reservation_units__reservations__user=user, _negated=not value)) + qs.query.filter_is_sticky = True + return qs.distinct() + class UnitFilterSet(ModelFilterSet, UnitFilterSetMixin): pk = IntMultipleChoiceFilter() @@ -101,29 +137,6 @@ class UnitFilterSet(ModelFilterSet, UnitFilterSetMixin): name_sv = django_filters.CharFilter(field_name="name_sv", lookup_expr="istartswith") only_with_permission = django_filters.BooleanFilter(method="filter_by_only_with_permission") - - # These filters use information across the relationship between Unit and ReservationUnit. - # FilterSets apply individual filters in separate `queryset.filter(...)` calls. - # - # Due to how Django works when spanning `to-many` relationships in qs.filter(...) calls, - # (see https://docs.djangoproject.com/en/5.0/topics/db/queries/#spanning-multi-valued-relationships), - # this means that using a combination of these filters would result in a queryset where - # ANY of their conditions are true for ANY ReservationUnit linked to a given Unit. - # e.g. using `published_reservation_units=True` and `only_direct_bookable=True` - # would return all Units where ANY of their ReservationUnit are EITHER published OR directly bookable. - # - # In this case, this is incorrect, since we want less permissive behavior: - # e.g. the aforementioned filter should return all Units where ANY of their ReservationUnit are - # BOTH published AND directly bookable. - # - # The incorrect behavior also has the side effect of adding multiple SQL joins to the - # many-to-many though table between Unit and ReservationUnit, which makes the query - # slower, since more duplication is needed. - # - # To prevent this, we need these filters to execute in order, and set `queryset.query.filter_is_sticky = True` - # to indicate to the queryset that it should reuse joins it found from one filter to the next. - # Note that this only applies until the next time the queryset is cloned, (e.g. when using - # `queryset.filter(...)` or `.distinct()`) and thus needs to be reapplied between them. published_reservation_units = django_filters.BooleanFilter(method="filter_by_published_reservation_units") own_reservations = django_filters.BooleanFilter(method="filter_by_own_reservations") only_direct_bookable = django_filters.BooleanFilter(method="filter_by_only_direct_bookable") @@ -149,18 +162,6 @@ class Meta: "unit_group_name_sv", ] - def filter_by_own_reservations(self, qs: models.QuerySet, name: str, value: bool) -> models.QuerySet: - user = self.request.user - - if user.is_anonymous: - return qs.none() - - # Prevent multiple joins, see explanation above. - qs.query.filter_is_sticky = True - qs = qs.filter(Q(reservation_units__reservations__user=user, _negated=not value)) - qs.query.filter_is_sticky = True - return qs.distinct() - @staticmethod def order_by_reservation_units_count(qs: UnitQuerySet, desc: bool) -> models.QuerySet: return qs.order_by_reservation_units_count(desc=desc) @@ -184,7 +185,16 @@ def order_by_unit_group_name_sv(qs: UnitQuerySet, desc: bool) -> models.QuerySet class UnitAllFilterSet(ModelFilterSet, UnitFilterSetMixin): unit = IntMultipleChoiceFilter() + + name_fi = django_filters.CharFilter(field_name="name_fi", lookup_expr="istartswith") + name_en = django_filters.CharFilter(field_name="name_en", lookup_expr="istartswith") + name_sv = django_filters.CharFilter(field_name="name_sv", lookup_expr="istartswith") + only_with_permission = django_filters.BooleanFilter(method="filter_by_only_with_permission") + published_reservation_units = django_filters.BooleanFilter(method="filter_by_published_reservation_units") + own_reservations = django_filters.BooleanFilter(method="filter_by_own_reservations") + only_direct_bookable = django_filters.BooleanFilter(method="filter_by_only_direct_bookable") + only_seasonal_bookable = django_filters.BooleanFilter(method="filter_by_only_seasonal_bookable") class Meta: model = Unit