diff --git a/backend/accounts/tests/test_accounts_api.py b/backend/accounts/tests/test_accounts_api.py index c0830c2f..cf1647df 100644 --- a/backend/accounts/tests/test_accounts_api.py +++ b/backend/accounts/tests/test_accounts_api.py @@ -8,6 +8,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile from accounts.models import Account, AccountEmail, AccountEmailLog from common.models import Attachments, Comment, Tags, Teams from contacts.models import Contact +from invoices.models import Estimate, Invoice, RecurringInvoice @pytest.mark.django_db @@ -223,6 +224,38 @@ class TestAccountDetailView: assert response.status_code == 200 assert not Account.objects.filter(id=account.id).exists() + @pytest.mark.parametrize( + ("model", "extra"), + [ + (Invoice, {"invoice_number": "INV-1"}), + (Estimate, {"estimate_number": "EST-1"}), + (RecurringInvoice, {"title": "Monthly retainer"}), + ], + ids=["invoice", "estimate", "recurring-invoice"], + ) + def test_delete_account_with_billing_records_is_refused( + self, admin_client, org_a, model, extra + ): + """Each PROTECT relation answers 409, and the message covers all three. + + Unguarded, the ProtectedError left the view as a 500. The message is + asserted on because it is the only thing telling an admin which list to + go and empty, and it named two of the three relations for a while. + """ + account = Account.objects.create(name="Has Billing", org=org_a) + model.objects.create(org=org_a, account=account, **extra) + + response = admin_client.delete(f"/api/accounts/{account.id}/") + + assert response.status_code == 409 + body = response.json() + assert body["error"] is True + assert body["errors"] == ( + "This account can't be deleted while it still has invoices, " + "estimates or recurring invoices linked to it." + ) + assert Account.objects.filter(id=account.id).exists() + def test_get_account_cross_org(self, org_b_client, org_a): account = Account.objects.create(name="Org A Account", org=org_a) response = org_b_client.get(f"/api/accounts/{account.id}/") diff --git a/backend/accounts/views.py b/backend/accounts/views.py index d94489ee..85db43a2 100644 --- a/backend/accounts/views.py +++ b/backend/accounts/views.py @@ -571,11 +571,15 @@ class AccountDetailView(APIView): try: self.object.delete() except ProtectedError: + # All three of Invoice, Estimate and RecurringInvoice point here with + # on_delete=PROTECT, so all three must be named. An account blocked + # only by a recurring invoice used to be told to go and look at two + # lists that were already empty. return Response( { "error": True, "errors": "This account can't be deleted while it still has " - "invoices or estimates linked to it.", + "invoices, estimates or recurring invoices linked to it.", }, status=status.HTTP_409_CONFLICT, ) diff --git a/backend/cases/kanban_views.py b/backend/cases/kanban_views.py index ae96a420..854993d2 100644 --- a/backend/cases/kanban_views.py +++ b/backend/cases/kanban_views.py @@ -132,10 +132,10 @@ class CaseKanbanView(APIView): queryset = queryset.filter(tags__id=tags) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) return queryset.distinct() def _get_status_kanban(self, queryset): diff --git a/backend/cases/views.py b/backend/cases/views.py index 6107f964..14b0a31f 100644 --- a/backend/cases/views.py +++ b/backend/cases/views.py @@ -128,10 +128,10 @@ def apply_case_list_filters(queryset, params): ) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) if params.get("sla_breached") == "true": # Wall-clock approximation matching the mobile card's # `isFirstResponseSlaBreached` getter; `Case.is_sla_*_breached` uses diff --git a/backend/common/tests/test_created_at_filter_end_day.py b/backend/common/tests/test_created_at_filter_end_day.py new file mode 100644 index 00000000..e0a283df --- /dev/null +++ b/backend/common/tests/test_created_at_filter_end_day.py @@ -0,0 +1,125 @@ +"""A `created_at` range includes the day at each end of it. + +`created_at` is a DateTimeField and `?created_at__lte=` carries a date, so +comparing the two directly put the upper bound at midnight and dropped +everything recorded during the end day itself. A user filtering "up to today" +saw none of today. Filtering on the date part instead makes the range inclusive +by calendar day, and resolves it in the org's timezone rather than the server's, +since `__date` answers in whichever timezone `GetProfileAndOrg` activated. + +Every endpoint accepting the pair is covered here rather than in each app's own +suite, because the defect was one shape repeated eight times. Seven of the eight +were still live after the first was fixed in isolation, which is the case for +pinning them together: a new list view that copies the old two lines fails here +immediately. + +Each assertion is paired with its mirror, so a filter that quietly stopped being +applied at all fails the pair rather than passing the half that only asks +whether the record is present. +""" + +import datetime + +import pytest + +from accounts.models import Account +from cases.models import Case +from contacts.models import Contact +from leads.models import Lead +from opportunity.models import Opportunity +from tasks.models import Task + +# Midday, so the record sits inside its day rather than on either edge. The org +# fixtures keep the default UTC timezone, so this is the 10th for the filter too. +CREATED_AT = datetime.datetime(2026, 3, 10, 12, 0, tzinfo=datetime.timezone.utc) +THE_DAY = "2026-03-10" +DAY_BEFORE = "2026-03-09" +DAY_AFTER = "2026-03-11" + + +def _account(org, user): + return Account.objects.create(name="Filter Me", org=org, created_by=user) + + +def _lead(org, user): + # `status` is nullable on the model, and the kanban builds one column per + # LEAD_STATUS value, so a lead without one is counted by the board and shown + # in none of its columns. Set it, or the kanban case would assert nothing. + return Lead.objects.create( + first_name="Filter", last_name="Me", status="assigned", org=org, created_by=user + ) + + +def _contact(org, user): + return Contact.objects.create( + first_name="Filter", last_name="Me", org=org, created_by=user + ) + + +def _case(org, user): + return Case.objects.create( + name="Filter Me", status="New", priority="Normal", org=org, created_by=user + ) + + +def _opportunity(org, user): + return Opportunity.objects.create( + name="Filter Me", stage="QUALIFICATION", org=org, created_by=user + ) + + +def _task(org, user): + return Task.objects.create( + title="Filter Me", status="New", priority="Medium", org=org, created_by=user + ) + + +ENDPOINTS = [ + ("/api/accounts/", _account), + ("/api/leads/", _lead), + ("/api/leads/kanban/", _lead), + ("/api/contacts/", _contact), + ("/api/cases/", _case), + ("/api/cases/kanban/", _case), + ("/api/opportunities/", _opportunity), + ("/api/tasks/", _task), +] +IDS = [url for url, _factory in ENDPOINTS] + + +def _record(factory, org, user): + """One record whose `created_at` is a known instant. + + `created_at` is `auto_now_add`, so it can only be moved by an UPDATE that + goes around the field's default. + """ + obj = factory(org, user) + type(obj).objects.filter(pk=obj.pk).update(created_at=CREATED_AT) + return obj + + +def _shows(response, obj): + """Whether the record came back. + + Matched on the raw body rather than a results key: the list and kanban + endpoints answer with different shapes, and the question here is only + whether the filter kept the row. + """ + assert response.status_code == 200 + return str(obj.id) in response.content.decode() + + +@pytest.mark.django_db +@pytest.mark.parametrize(("url", "factory"), ENDPOINTS, ids=IDS) +def test_lte_includes_the_end_day(admin_client, org_a, admin_user, url, factory): + obj = _record(factory, org_a, admin_user) + assert _shows(admin_client.get(f"{url}?created_at__lte={THE_DAY}"), obj) + assert not _shows(admin_client.get(f"{url}?created_at__lte={DAY_BEFORE}"), obj) + + +@pytest.mark.django_db +@pytest.mark.parametrize(("url", "factory"), ENDPOINTS, ids=IDS) +def test_gte_includes_the_start_day(admin_client, org_a, admin_user, url, factory): + obj = _record(factory, org_a, admin_user) + assert _shows(admin_client.get(f"{url}?created_at__gte={THE_DAY}"), obj) + assert not _shows(admin_client.get(f"{url}?created_at__gte={DAY_AFTER}"), obj) diff --git a/backend/common/tests/test_healthz.py b/backend/common/tests/test_healthz.py new file mode 100644 index 00000000..8db92a3f --- /dev/null +++ b/backend/common/tests/test_healthz.py @@ -0,0 +1,24 @@ +"""The liveness probe answers without a tenant. + +`RequireOrgContext` 403s any resolvable path with no org context, and +`/healthz/` has no org to carry: it is fetched by load balancers, uptime +monitors and container orchestrators, none of which hold a JWT. While it was +gated, every health check read the service as down. + +Paired with a request that must still be refused, so the exemption is pinned as +narrow rather than as "the middleware stopped enforcing". +""" + +import pytest + + +@pytest.mark.django_db +class TestHealthzNoOrgContext: + def test_probe_is_reachable_unauthenticated(self, unauthenticated_client): + response = unauthenticated_client.get("/healthz/") + assert response.status_code == 200 + + def test_other_paths_still_require_org_context(self, unauthenticated_client): + # Same client, same absence of a token. Only /healthz/ is exempt. + response = unauthenticated_client.get("/api/accounts/") + assert response.status_code == 403 diff --git a/backend/common/tests/test_list_filter_params.py b/backend/common/tests/test_list_filter_params.py index 3dde2cab..100d91e6 100644 --- a/backend/common/tests/test_list_filter_params.py +++ b/backend/common/tests/test_list_filter_params.py @@ -88,6 +88,35 @@ class TestAMalformedRangeFilterIsRefusedNotCrashed: assert resp.status_code == http.HTTP_200_OK + @pytest.mark.parametrize("url,param", DATE_PARAMS) + def test_a_day_that_does_not_exist_is_a_400(self, admin_client, org_a, url, param): + """A date-shaped value for a day that never happened. + + `parse_date` raises `ValueError` on it rather than answering None, so it + went straight past the check that was only looking for None and took the + endpoint down the same way "banana" used to. + """ + resp = admin_client.get(url, {param: "2026-02-30"}) + + assert resp.status_code == http.HTTP_400_BAD_REQUEST + assert param in resp.data + + @pytest.mark.parametrize("url,param", DATE_PARAMS) + def test_a_full_timestamp_still_filters(self, admin_client, org_a, url, param): + """A date-valued filter also has to take a timestamp. + + `mobile/lib/providers/tickets_provider.dart` sends + `createdAfter.toUtc().toIso8601String()`, so this is the live shape of + the ticket list's date filter, not a hypothetical one. Neither lookup + will parse it out of the query string: a DateField raises on anything + that is not YYYY-MM-DD, and so does the `__date` lookup the datetime + columns are filtered through. `date_param` resolves the calendar day + before either sees it. + """ + resp = admin_client.get(url, {param: "2026-08-01T12:00:00.000Z"}) + + assert resp.status_code == http.HTTP_200_OK + @pytest.mark.parametrize("url,param", NUMBER_PARAMS) def test_a_word_where_a_number_belongs_is_a_400( self, admin_client, org_a, url, param diff --git a/backend/common/tests/test_users.py b/backend/common/tests/test_users.py index a1a822d7..cd21d027 100644 --- a/backend/common/tests/test_users.py +++ b/backend/common/tests/test_users.py @@ -4,9 +4,14 @@ Tests for user management views: list users, user detail, update user, status. Run with: pytest common/tests/test_users.py -v """ +from unittest.mock import patch + import pytest +from django.utils import timezone from rest_framework import status +from cases.approvals import Approval, ApprovalRule +from cases.models import Case, TimeEntry from common.models import Address, Profile from common.permissions import is_org_admin @@ -431,6 +436,53 @@ class TestUserDetailView: response = user_client.delete(self._url(admin_user.id)) assert response.status_code == status.HTTP_403_FORBIDDEN + @patch("common.views.user_views.send_email_user_delete") + def test_delete_user_tells_them_they_were_removed( + self, send_email, admin_client, org_a, regular_user, user_profile + ): + """The notice goes out, and only once the removal has actually happened.""" + response = admin_client.delete(self._url(regular_user.id)) + + assert response.status_code == status.HTTP_200_OK + send_email.delay.assert_called_once_with( + regular_user.email, deleted_by="admin@test.com" + ) + + @pytest.mark.parametrize("relation", ["time-entry", "approval"]) + @patch("common.views.user_views.send_email_user_delete") + def test_delete_user_with_protected_history_is_refused( + self, send_email, admin_client, org_a, regular_user, user_profile, relation + ): + """A member who logged time or raised an approval cannot be deleted. + + Both relations are on_delete=PROTECT, so the delete raises and the view + used to answer 500. The email assertion is the other half of it: the + notice was sent before the delete was attempted, so a user who was still + in the org was told they had been removed from it. + """ + case = Case.objects.create( + name="Blocking Case", status="New", priority="Normal", org=org_a + ) + if relation == "time-entry": + TimeEntry.objects.create( + org=org_a, case=case, profile=user_profile, started_at=timezone.now() + ) + else: + rule = ApprovalRule.objects.create(name="Discount sign-off", org=org_a) + Approval.objects.create( + org=org_a, case=case, rule=rule, requested_by=user_profile + ) + + response = admin_client.delete(self._url(regular_user.id)) + + assert response.status_code == status.HTTP_409_CONFLICT + assert response.data["errors"] == ( + "This user can't be deleted while they still have time entries or " + "approvals linked to them." + ) + assert Profile.objects.filter(id=user_profile.id).exists() + send_email.delay.assert_not_called() + @pytest.mark.django_db class TestUserStatusView: diff --git a/backend/common/validators.py b/backend/common/validators.py index 08d7b2c5..89917800 100644 --- a/backend/common/validators.py +++ b/backend/common/validators.py @@ -2,6 +2,7 @@ Common validators for CRM models. """ +import datetime import json import re import uuid as uuid_module @@ -10,6 +11,7 @@ from zoneinfo import available_timezones from django.core.exceptions import ValidationError as DjangoValidationError from django.core.validators import RegexValidator +from django.utils import timezone from django.utils.dateparse import parse_date, parse_datetime from django.utils.translation import gettext_lazy as _ from rest_framework.exceptions import ValidationError as DRFValidationError @@ -129,30 +131,49 @@ def uuid_list_param(params, field: str) -> list: return validate_uuid_list(params.get(field), field) -def validate_date(value, field: str) -> str: - """Return ``value`` unchanged if the ORM will be able to parse it as a date. +def validate_date(value, field: str) -> datetime.date: + """The calendar day a date-valued query parameter names. The same defect ``validate_uuid`` closes, wearing a different type. Range filters (``due_date__gte``, ``created_at__lte``, ``close_date__gte``, ...) - take raw query-string text and hand it to a date or datetime lookup. - Django parses it while building the query and raises + take raw query-string text and hand it to a date lookup. Django parses it + while building the query and raises ``django.core.exceptions.ValidationError`` on anything malformed, which DRF's handler does not translate, so ``?due_date__gte=banana`` answered 500 - rather than 400. + rather than 400. ``2026-02-30`` is the same 500 by a different route: + ``parse_date`` raises ``ValueError`` on a day that does not exist rather + than answering ``None``, so a check looking only for ``None`` walks past it. - Accepts a plain date and a full timestamp alike, because the datetime - columns here (``created_at``) are filtered with both. The value is returned - as text rather than as a parsed object so the lookup keeps behaving exactly - as it did: passing a ``date`` into a ``DateTimeField`` lookup changes how - Django interprets midnight, and this is a 500-to-400 fix, not a semantics - change. + A full timestamp is accepted as well as a plain date, because callers send + both: ``mobile/lib/providers/tickets_provider.dart`` sends + ``toUtc().toIso8601String()``. Neither column type will take that text. + A DateField parses only ``YYYY-MM-DD``, and the datetime columns are + filtered through ``__date`` so their range covers the end day, which parses + only ``YYYY-MM-DD`` too. Resolving the day here rather than returning the + text is what lets one parameter serve both. + + An aware timestamp resolves in the active timezone, the org's, which is the + timezone ``__date`` resolves the rows against. """ text = str(value).strip() - if parse_date(text) is None and parse_datetime(text) is None: + try: + day = parse_date(text) + if day is None: + moment = parse_datetime(text) + if moment is not None: + day = ( + timezone.localtime(moment).date() + if timezone.is_aware(moment) + else moment.date() + ) + except ValueError: + # Well formed, no such moment: "2026-02-30", "2026-02-30T10:00:00". + day = None + if day is None: raise DRFValidationError( {field: [f"'{value}' is not a valid date. Use YYYY-MM-DD."]} ) from None - return text + return day def date_param(params, field: str): diff --git a/backend/common/views/user_views.py b/backend/common/views/user_views.py index b2182889..8c7ebe09 100644 --- a/backend/common/views/user_views.py +++ b/backend/common/views/user_views.py @@ -1,6 +1,6 @@ from django.conf import settings from django.db import IntegrityError, transaction -from django.db.models import Count, Q +from django.db.models import Count, ProtectedError, Q from django.shortcuts import get_object_or_404 from django.utils import timezone from drf_spectacular.utils import extend_schema, inline_serializer @@ -520,11 +520,24 @@ class UserDetailView(APIView): status=status.HTTP_403_FORBIDDEN, ) deleted_by = self.request.profile.user.email - send_email_user_delete.delay( - self.object.user.email, - deleted_by=deleted_by, - ) - self.object.delete() + recipient = self.object.user.email + try: + self.object.delete() + except ProtectedError: + # TimeEntry.profile, Approval.requested_by and Approval.approver all + # point here with on_delete=PROTECT, so a member who has logged time + # or touched an approval cannot be removed. Unguarded this was a 500, + # and the notice below had already gone out by then: the user was + # told they had been removed from an org they were still in. + return Response( + { + "error": True, + "errors": "This user can't be deleted while they still have " + "time entries or approvals linked to them.", + }, + status=status.HTTP_409_CONFLICT, + ) + send_email_user_delete.delay(recipient, deleted_by=deleted_by) return Response({"status": "success"}, status=status.HTTP_200_OK) diff --git a/backend/contacts/views.py b/backend/contacts/views.py index 4756b512..d3883b4a 100644 --- a/backend/contacts/views.py +++ b/backend/contacts/views.py @@ -101,10 +101,10 @@ class ContactsListView(APIView, LimitOffsetPagination): ) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) # Custom-field filters: ?cf_= -> custom_fields contains pair. for raw_key, raw_value in params.items(): if raw_key.startswith("cf_") and raw_value: diff --git a/backend/leads/views/kanban_views.py b/backend/leads/views/kanban_views.py index d8ba3662..79cf4dc2 100644 --- a/backend/leads/views/kanban_views.py +++ b/backend/leads/views/kanban_views.py @@ -116,10 +116,10 @@ class LeadKanbanView(APIView): queryset = queryset.filter(source=params.get("source")) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) return queryset def _get_status_kanban(self, queryset): diff --git a/backend/leads/views/lead_views.py b/backend/leads/views/lead_views.py index 8ab0a059..29dfef56 100644 --- a/backend/leads/views/lead_views.py +++ b/backend/leads/views/lead_views.py @@ -159,10 +159,10 @@ class LeadListView(APIView, LimitOffsetPagination): ) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) close_date_gte = date_param(params, "close_date__gte") if close_date_gte: queryset = queryset.filter(close_date__gte=close_date_gte) diff --git a/backend/opportunity/views/opportunity_views.py b/backend/opportunity/views/opportunity_views.py index 183e645d..eb254605 100644 --- a/backend/opportunity/views/opportunity_views.py +++ b/backend/opportunity/views/opportunity_views.py @@ -172,10 +172,10 @@ class OpportunityListView(APIView, LimitOffsetPagination): queryset = queryset.filter(name__icontains=params.get("search")) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) closed_on_gte = date_param(params, "closed_on__gte") if closed_on_gte: queryset = queryset.filter(closed_on__gte=closed_on_gte) diff --git a/backend/tasks/views/task_views.py b/backend/tasks/views/task_views.py index 7a0e8c9c..53392ea1 100644 --- a/backend/tasks/views/task_views.py +++ b/backend/tasks/views/task_views.py @@ -121,10 +121,10 @@ class TaskListView(APIView, LimitOffsetPagination): queryset = queryset.filter(due_date__lte=due_date_lte) created_at_gte = date_param(params, "created_at__gte") if created_at_gte: - queryset = queryset.filter(created_at__gte=created_at_gte) + queryset = queryset.filter(created_at__date__gte=created_at_gte) created_at_lte = date_param(params, "created_at__lte") if created_at_lte: - queryset = queryset.filter(created_at__lte=created_at_lte) + queryset = queryset.filter(created_at__date__lte=created_at_lte) for related in ("account", "opportunity", "case", "lead"): related_id = uuid_param(params, related) if related_id: diff --git a/backend/uv.lock b/backend/uv.lock index f8bcf3bc..6dc95932 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -583,7 +583,7 @@ wheels = [ [[package]] name = "django-crm" -version = "1.3.1" +version = "1.4.0" source = { editable = "." } dependencies = [ { name = "cairocffi" },