From e7f7f33f474d6be49d7ab60abff8d1a8cdc4445e Mon Sep 17 00:00:00 2001 From: Arthur Deierlein Date: Thu, 13 Aug 2026 12:00:13 +0200 Subject: [PATCH] fix(backend): refactor use of NumberFilter/BaseInFilter --- backend/timed/employment/filters.py | 63 ++++++------ backend/timed/filters.py | 30 ++++++ backend/timed/projects/filters.py | 49 +++++----- backend/timed/reports/filters.py | 30 +++--- backend/timed/subscription/filters.py | 9 +- backend/timed/tracking/filters.py | 97 +++++++------------ .../tests/__snapshots__/test_report.ambr | 74 ++++++++++++++ backend/timed/tracking/tests/test_activity.py | 32 +++--- backend/timed/tracking/tests/test_report.py | 26 +++++ 9 files changed, 261 insertions(+), 149 deletions(-) create mode 100644 backend/timed/filters.py diff --git a/backend/timed/employment/filters.py b/backend/timed/employment/filters.py index b3b92a533..bb8892e5e 100644 --- a/backend/timed/employment/filters.py +++ b/backend/timed/employment/filters.py @@ -4,23 +4,25 @@ from django.db.models import Q from django_filters.constants import EMPTY_VALUES -from django_filters.rest_framework import DateFilter, Filter, FilterSet, NumberFilter +from django_filters.rest_framework import ( + BooleanFilter, + DateFilter, + Filter, + FilterSet, +) from timed.employment import models from timed.employment.models import User +from timed.filters import IdFilter if TYPE_CHECKING: - from typing import TypeVar - from django.db.models import QuerySet - T = TypeVar("T", QuerySet) - class YearFilter(Filter): """Filter to filter a queryset by year.""" - def filter(self, qs: T, value: int) -> T: + def filter[T: QuerySet](self, qs: T, value: int) -> T: if value in EMPTY_VALUES: return qs @@ -48,7 +50,7 @@ class Meta: class AbsenceTypeFilterSet(FilterSet): - fill_worktime = NumberFilter(field_name="fill_worktime") + fill_worktime = BooleanFilter(field_name="fill_worktime") class Meta: """Meta information for the public holiday filter set.""" @@ -58,31 +60,34 @@ class Meta: class UserFilterSet(FilterSet): - active = NumberFilter(field_name="is_active") - supervisor = NumberFilter(field_name="supervisors") - is_reviewer = NumberFilter(method="filter_is_reviewer") - is_supervisor = NumberFilter(method="filter_is_supervisor") - is_accountant = NumberFilter(field_name="is_accountant") - is_external = NumberFilter(method="filter_is_external") - - def filter_is_external( - self, queryset: QuerySet[models.User], _name: str, value: int - ) -> QuerySet[models.User]: - return queryset.filter(employments__is_external=value) + active = BooleanFilter(field_name="is_active") + supervisor = IdFilter(field_name="supervisors") + is_reviewer = BooleanFilter(method="filter_is_reviewer") + is_supervisor = BooleanFilter(method="filter_is_supervisor") + is_accountant = BooleanFilter(field_name="is_accountant") + is_external = BooleanFilter(field_name="employments__is_external") def filter_is_reviewer( - self, queryset: QuerySet[models.User], _name: str, value: int - ) -> QuerySet[models.User]: + self, + queryset: QuerySet[User], + _name: str, + value: bool, # noqa: FBT001 + ) -> QuerySet[User]: + is_reviewer = Q(pk__in=User.objects.all_reviewers()) if value: - return queryset.filter(pk__in=User.objects.all_reviewers()) - return queryset.exclude(pk__in=User.objects.all_reviewers()) + return queryset.filter(is_reviewer) + return queryset.exclude(is_reviewer) def filter_is_supervisor( - self, queryset: QuerySet[models.User], _name: str, value: int - ) -> QuerySet[models.User]: + self, + queryset: QuerySet[User], + _name: str, + value: bool, # noqa: FBT001 + ) -> QuerySet[User]: + is_supervisor = Q(pk__in=User.objects.all_supervisors()) if value: - return queryset.filter(pk__in=User.objects.all_supervisors()) - return queryset.exclude(pk__in=User.objects.all_supervisors()) + return queryset.filter(is_supervisor) + return queryset.exclude(is_supervisor) class Meta: model = models.User @@ -148,8 +153,8 @@ class Meta: class WorktimeBalanceFilterSet(FilterSet): - user = NumberFilter(field_name="id") - supervisor = NumberFilter(field_name="supervisors") + user = IdFilter(field_name="id") + supervisor = IdFilter(field_name="supervisors") class Meta: model = models.User @@ -157,7 +162,7 @@ class Meta: class AbsenceBalanceFilterSet(FilterSet): - absence_type = NumberFilter(field_name="id") + absence_type = IdFilter(field_name="id") class Meta: model = models.AbsenceType diff --git a/backend/timed/filters.py b/backend/timed/filters.py new file mode 100644 index 000000000..357d5092b --- /dev/null +++ b/backend/timed/filters.py @@ -0,0 +1,30 @@ +"""In historic versions of timed, we used `NumberFilter`s or `BaseInFilter`s for filtering by one or more `id`s. + +`NumberFilter` defaults to `django.forms.DecimalField`s, therefore our `id`s were actually `Decimal`s, which allowed for bogus negative and/or non-whole numbers + +`BaseInFilter` is supposed to be combined with another filter, which it wasn't, not even with a `NumberFilter`, in newer versions we only accept positive integers in both. +""" + +from django.forms import IntegerField +from django_filters.rest_framework import BaseInFilter, NumberFilter + + +class PositiveIntegerField(IntegerField): + """`PositiveIntegerField` for use with `NumberFilter`s. + + `NumberFilter` defaults to `forms.DecimalField`, which isn't what we want in most cases. + """ + + def __init__(self, *args, **kwargs): + kwargs.setdefault("min_value", 1) + super().__init__(*args, **kwargs) + + +class IdFilter(NumberFilter): + """A filter to use for primary keys (ids).""" + + field_class = PositiveIntegerField + + +class IdInFilter(BaseInFilter, IdFilter): + """In filter to use with primary keys (ids).""" diff --git a/backend/timed/projects/filters.py b/backend/timed/projects/filters.py index f1f4ee299..38f3b7d2e 100644 --- a/backend/timed/projects/filters.py +++ b/backend/timed/projects/filters.py @@ -7,22 +7,19 @@ from django.db.models import Count, Q from django_filters.constants import EMPTY_VALUES -from django_filters.rest_framework import BaseInFilter, Filter, FilterSet, NumberFilter +from django_filters.rest_framework import BooleanFilter, Filter, FilterSet +from timed.filters import IdFilter, IdInFilter from timed.projects import models if TYPE_CHECKING: from django.db.models import QuerySet -class NumberInFilter(BaseInFilter, NumberFilter): - pass - - class CustomerFilterSet(FilterSet): """Filter set for the customers endpoint.""" - archived = NumberFilter(field_name="archived") + archived = BooleanFilter(field_name="archived") class Meta: """Meta information for the customer filter set.""" @@ -37,13 +34,16 @@ class Meta: class ProjectFilterSet(FilterSet): """Filter set for the projects endpoint.""" - archived = NumberFilter(field_name="archived") - has_manager = NumberFilter(method="filter_has_manager") - has_reviewer = NumberFilter(method="filter_has_reviewer") - customer = NumberInFilter(field_name="customer") + archived = BooleanFilter(field_name="archived") + has_manager = IdFilter(method="filter_has_manager") + has_reviewer = IdFilter(method="filter_has_reviewer") + customer = IdInFilter(field_name="customer") def filter_has_manager( - self, queryset: QuerySet[models.Project], _name: str, value: int + self, + queryset: QuerySet[models.Project], + _name: str, + value: int, ) -> QuerySet[models.Project]: if not value: # pragma: no cover return queryset @@ -61,7 +61,10 @@ def filter_has_manager( ) def filter_has_reviewer( - self, queryset: QuerySet[models.Project], _name: str, value: int + self, + queryset: QuerySet[models.Project], + _name: str, + value: int, ) -> QuerySet[models.Project]: if not value: # pragma: no cover return queryset @@ -128,8 +131,8 @@ class TaskFilterSet(FilterSet): """Filter set for the tasks endpoint.""" my_most_frequent = MyMostFrequentTaskFilter() - archived = NumberFilter(field_name="archived") - project = NumberInFilter(field_name="project") + archived = BooleanFilter(field_name="archived") + project = IdInFilter(field_name="project") class Meta: """Meta information for the task filter set.""" @@ -147,9 +150,9 @@ class Meta: class TaskAssigneeFilterSet(FilterSet): """Filter set for the task assignees endpoint.""" - task = NumberFilter(field_name="task") - tasks = NumberInFilter(field_name="task") - user = NumberFilter(field_name="user") + task = IdFilter(field_name="task") + tasks = IdInFilter(field_name="task") + user = IdFilter(field_name="user") class Meta: """Meta information for the task assignee filter set.""" @@ -167,9 +170,9 @@ class Meta: class ProjectAssigneeFilterSet(FilterSet): """Filter set for the project assignees endpoint.""" - project = NumberFilter(field_name="project") - projects = NumberInFilter(field_name="project") - user = NumberFilter(field_name="user") + project = IdFilter(field_name="project") + projects = IdInFilter(field_name="project") + user = IdFilter(field_name="user") class Meta: """Meta information for the project assignee filter set.""" @@ -187,9 +190,9 @@ class Meta: class CustomerAssigneeFilterSet(FilterSet): """Filter set for the customer assignees endpoint.""" - customer = NumberFilter(field_name="customer") - customers = NumberInFilter(field_name="customer") - user = NumberFilter(field_name="user") + customer = IdFilter(field_name="customer") + customers = IdInFilter(field_name="customer") + user = IdFilter(field_name="user") class Meta: """Meta information for the customer assignee filter set.""" diff --git a/backend/timed/reports/filters.py b/backend/timed/reports/filters.py index 29d8784f4..c82d44df4 100644 --- a/backend/timed/reports/filters.py +++ b/backend/timed/reports/filters.py @@ -5,12 +5,12 @@ from django.db.models import DurationField, F, Q, Sum, Value from django.db.models.functions import Coalesce from django_filters.rest_framework import ( - BaseInFilter, + BooleanFilter, DateFilter, FilterSet, - NumberFilter, ) +from timed.filters import IdFilter, IdInFilter from timed.projects.models import CustomerAssignee, ProjectAssignee, TaskAssignee if TYPE_CHECKING: @@ -129,23 +129,23 @@ def statistic_filterset_builder( "to_date": DateFilter( field_name=f"{reports_prefix}date", lookup_expr="lte" ), - "project": NumberFilter(field_name=f"{project_prefix}pk"), - "customer": NumberFilter(field_name=f"{customer_prefix}pk"), - "review": NumberFilter(field_name=f"{reports_prefix}review"), - "not_billable": NumberFilter(field_name=f"{reports_prefix}not_billable"), - "billed": NumberFilter(field_name=f"{reports_prefix}billed"), - "verified": NumberFilter( + "project": IdFilter(field_name=f"{project_prefix}pk"), + "customer": IdFilter(field_name=f"{customer_prefix}pk"), + "review": BooleanFilter(field_name=f"{reports_prefix}review"), + "not_billable": BooleanFilter(field_name=f"{reports_prefix}not_billable"), + "billed": BooleanFilter(field_name=f"{reports_prefix}billed"), + "verified": BooleanFilter( field_name=f"{reports_prefix}verified_by_id", lookup_expr="isnull", exclude=True, ), - "verifier": NumberFilter(field_name=f"{reports_prefix}verified_by"), - "billing_type": NumberFilter(field_name=f"{project_prefix}billing_type"), - "user": NumberFilter(field_name=f"{reports_prefix}user_id"), - "rejected": NumberFilter(field_name=f"{reports_prefix}rejected"), - "id": BaseInFilter(), - "cost_center": NumberFilter(method="filter_cost_center"), - "reviewer": NumberFilter(method="filter_has_reviewer"), + "verifier": IdFilter(field_name=f"{reports_prefix}verified_by"), + "billing_type": IdFilter(field_name=f"{project_prefix}billing_type"), + "user": IdFilter(field_name=f"{reports_prefix}user_id"), + "rejected": BooleanFilter(field_name=f"{reports_prefix}rejected"), + "id": IdInFilter(), + "cost_center": IdFilter(method="filter_cost_center"), + "reviewer": IdFilter(method="filter_has_reviewer"), }, ) diff --git a/backend/timed/subscription/filters.py b/backend/timed/subscription/filters.py index b2675ad11..34d9bab5e 100644 --- a/backend/timed/subscription/filters.py +++ b/backend/timed/subscription/filters.py @@ -2,8 +2,9 @@ from typing import TYPE_CHECKING -from django_filters import FilterSet, NumberFilter +from django_filters.rest_framework import BooleanFilter, FilterSet +from timed.filters import IdFilter from timed.projects.models import Project from . import models @@ -13,7 +14,7 @@ class PackageFilter(FilterSet): - customer = NumberFilter(method="filter_customer") + customer = IdFilter(method="filter_customer") def filter_customer( self, queryset: QuerySet[models.Package], _name: str, value: int @@ -30,8 +31,8 @@ class Meta: class OrderFilter(FilterSet): - customer = NumberFilter(field_name="project__customer") - acknowledged = NumberFilter(field_name="acknowledged") + customer = IdFilter(field_name="project__customer") + acknowledged = BooleanFilter(field_name="acknowledged") class Meta: model = models.Order diff --git a/backend/timed/tracking/filters.py b/backend/timed/tracking/filters.py index 22a9714d9..1468ca59c 100644 --- a/backend/timed/tracking/filters.py +++ b/backend/timed/tracking/filters.py @@ -2,77 +2,42 @@ from __future__ import annotations -from functools import wraps from typing import TYPE_CHECKING from django.contrib.postgres.search import SearchQuery from django.db.models import Q -from django_filters.constants import EMPTY_VALUES from django_filters.rest_framework import ( - BaseInFilter, BooleanFilter, CharFilter, DateFilter, - Filter, FilterSet, - NumberFilter, ) +from timed.filters import IdFilter, IdInFilter from timed.projects.models import CustomerAssignee, ProjectAssignee, TaskAssignee from timed.tracking import models if TYPE_CHECKING: - from collections.abc import Callable - from typing import TypeVar - from django.db.models import QuerySet - T = TypeVar("T") # used for self - G = TypeVar("G", QuerySet) # used for qs - - -def boolean_filter(func: Callable[[T, G, str], G]) -> Callable[[T, G, bool], G]: - """Cast the passed query parameter into a boolean. - - :param function func: The function to decorate - :return: The function called with a boolean - :rtype: function - """ - - @wraps(func) - def wrapper(self, qs, value): - if value in EMPTY_VALUES: - return qs - - value = value.lower() not in ("1", "true", "yes") - return func(self, qs, value) - - return wrapper - - -class ActivityActiveFilter(Filter): - """Filter to filter activities by being currently active or not. +class ActivityFilterSet(FilterSet): + """Filter set for the activities endpoint.""" - An activity is active, as soon as they have at least on activity - block which does not have to_time. - """ + active = BooleanFilter(method="filter_active") + day = DateFilter(field_name="date") - @boolean_filter - def filter( + def filter_active( self, qs: QuerySet[models.Activity], - _value: bool, # noqa: FBT001 + _name: str, + value: bool, # noqa: FBT001 ) -> QuerySet[models.Activity]: - """Filter the queryset.""" - return qs.filter(to_time__exact=None).distinct() - - -class ActivityFilterSet(FilterSet): - """Filter set for the activities endpoint.""" - - active = ActivityActiveFilter() - day = DateFilter(field_name="date") + """Filter activities by being currently active or not.""" + active_activities = Q(to_time__exact=None) + if not value: + return qs.exclude(active_activities) + return qs.filter(active_activities) class Meta: """Meta information for the activity filter set.""" @@ -97,24 +62,25 @@ class Meta: class ReportFilterSet(FilterSet): """Filter set for the reports endpoint.""" - id = BaseInFilter() + id = IdInFilter() from_date = DateFilter(field_name="date", lookup_expr="gte") to_date = DateFilter(field_name="date", lookup_expr="lte") - project = NumberFilter(field_name="task__project") - customer = NumberFilter(field_name="task__project__customer") - review = NumberFilter(field_name="review") - editable = NumberFilter(method="filter_editable") - not_billable = NumberFilter(field_name="not_billable") - billed = NumberFilter(field_name="billed") + project = IdFilter(field_name="task__project") + customer = IdFilter(field_name="task__project__customer") + review = BooleanFilter() + editable = BooleanFilter(method="filter_editable") + not_billable = BooleanFilter() + billed = BooleanFilter() verified = BooleanFilter( field_name="verified_by_id", lookup_expr="isnull", exclude=True ) - reviewer = NumberFilter(method="filter_has_reviewer") - verifier = NumberFilter(field_name="verified_by") - billing_type = NumberFilter(field_name="task__project__billing_type") - user = NumberFilter(field_name="user_id") - cost_center = NumberFilter(method="filter_cost_center") - rejected = NumberFilter(field_name="rejected") + reviewer = IdFilter(method="filter_has_reviewer") + verifier = IdFilter(field_name="verified_by") + billing_type = IdFilter(field_name="task__project__billing_type") + user = IdFilter(field_name="user_id") + cost_center = IdFilter(method="filter_cost_center") + + rejected = BooleanFilter() comment = CharFilter(method="filter_comment") def filter_has_reviewer( @@ -174,12 +140,15 @@ def filter_has_reviewer( ) def filter_editable( - self, queryset: QuerySet[models.Report], _name: str, value: int + self, + queryset: QuerySet[models.Report], + _name: str, + value: bool, # noqa: FBT001 ) -> QuerySet[models.Report]: """Filter reports whether they are editable by current user. - When set to `1` filter all results to what is editable by current - user. If set to `0` to not editable. + When set `True` filter all results to what is editable by current + user. If set to `False` to not editable. """ user = self.request.user assignee_filter = ( diff --git a/backend/timed/tracking/tests/__snapshots__/test_report.ambr b/backend/timed/tracking/tests/__snapshots__/test_report.ambr index 01d5f8866..dbe4f973b 100644 --- a/backend/timed/tracking/tests/__snapshots__/test_report.ambr +++ b/backend/timed/tracking/tests/__snapshots__/test_report.ambr @@ -1,4 +1,78 @@ # serializer version: 1 +# name: test_report_filters[-1-None-400] + dict({ + 'errors': list([ + dict({ + 'code': 'min_value', + 'detail': 'Ensure this value is greater than or equal to 1.', + 'source': dict({ + 'pointer': '/data/attributes/id', + }), + 'status': '400', + }), + ]), + }) +# --- +# name: test_report_filters[1,2-0.5-400] + dict({ + 'errors': list([ + dict({ + 'code': 'invalid', + 'detail': 'Enter a whole number.', + 'source': dict({ + 'pointer': '/data/attributes/reviewer', + }), + 'status': '400', + }), + ]), + }) +# --- +# name: test_report_filters[1,2.6-1-400] + dict({ + 'errors': list([ + dict({ + 'code': 'invalid', + 'detail': 'Enter a whole number.', + 'source': dict({ + 'pointer': '/data/attributes/id', + }), + 'status': '400', + }), + ]), + }) +# --- +# name: test_report_filters[1-None-200] + dict({ + 'data': list([ + ]), + 'meta': dict({ + 'total-time': '00:00:00', + }), + }) +# --- +# name: test_report_filters[None--1-400] + dict({ + 'errors': list([ + dict({ + 'code': 'min_value', + 'detail': 'Ensure this value is greater than or equal to 1.', + 'source': dict({ + 'pointer': '/data/attributes/reviewer', + }), + 'status': '400', + }), + ]), + }) +# --- +# name: test_report_filters[None-None-200] + dict({ + 'data': list([ + ]), + 'meta': dict({ + 'total-time': '00:00:00', + }), + }) +# --- # name: test_report_intersection_partial dict({ 'data': dict({ diff --git a/backend/timed/tracking/tests/test_activity.py b/backend/timed/tracking/tests/test_activity.py index b3c59da63..e1c4cf107 100644 --- a/backend/timed/tracking/tests/test_activity.py +++ b/backend/timed/tracking/tests/test_activity.py @@ -199,20 +199,6 @@ def test_activity_delete( assert response.status_code == expected -def test_activity_list_filter_active(internal_employee_client, activity_factory): - user = internal_employee_client.user - activity1 = activity_factory(user=user) - activity2 = activity_factory(user=user, to_time=None, task=activity1.task) - - url = reverse("activity-list") - - response = internal_employee_client.get(url, data={"active": "true"}) - assert response.status_code == status.HTTP_200_OK - json = response.json() - assert len(json["data"]) == 1 - assert json["data"][0]["id"] == str(activity2.id) - - def test_activity_list_filter_day(internal_employee_client, activity_factory): user = internal_employee_client.user day = date(2016, 2, 2) @@ -396,3 +382,21 @@ def test_activity_transfer_ends_it(internal_employee_client, activity_factory): activity.refresh_from_db() assert activity.to_time assert activity.transferred + + +@pytest.mark.parametrize("is_active", [True, False]) +def test_activity_list_filter_active( + internal_employee_client, activity_factory, is_active +): + user = internal_employee_client.user + active_activity = activity_factory(user=user, to_time=None) + other_activity = activity_factory(user=user, task=active_activity.task) + + url = reverse("activity-list") + + response = internal_employee_client.get(url, query_params={"active": is_active}) + assert response.status_code == status.HTTP_200_OK + json = response.json() + assert len(json["data"]) == 1 + activity = active_activity if is_active else other_activity + assert json["data"][0]["id"] == str(activity.id) diff --git a/backend/timed/tracking/tests/test_report.py b/backend/timed/tracking/tests/test_report.py index 4ffd11d77..f70d94d45 100644 --- a/backend/timed/tracking/tests/test_report.py +++ b/backend/timed/tracking/tests/test_report.py @@ -2380,3 +2380,29 @@ def test_report_split( comment=new_comment, duration=new_duration, task=new_report_task ).exists() assert original_report_count == Report.objects.count() + + +@pytest.mark.parametrize( + ("ids", "reviewer", "expected_status"), + [ + (None, None, status.HTTP_200_OK), + ("1", None, status.HTTP_200_OK), + ("-1", None, status.HTTP_400_BAD_REQUEST), + (None, "-1", status.HTTP_400_BAD_REQUEST), + ("1,2", "0.5", status.HTTP_400_BAD_REQUEST), + ("1,2.6", "1", status.HTTP_400_BAD_REQUEST), + ], +) +def test_report_filters( + internal_employee_client, + snapshot, + ids: str | None, + reviewer: int | None, + expected_status: int, +) -> None: + params = {"id": ids, "reviewer": reviewer} + params = {name: value for name, value in params.items() if value is not None} + url = reverse("report-list") + response = internal_employee_client.get(url, query_params=params) + assert response.status_code == expected_status + assert response.json() == snapshot