diff --git a/backend/accounts/urls.py b/backend/accounts/urls.py index 6815dc80..46a421a1 100644 --- a/backend/accounts/urls.py +++ b/backend/accounts/urls.py @@ -6,8 +6,8 @@ app_name = "api_accounts" urlpatterns = [ path("", views.AccountsListView.as_view()), - path("/", views.AccountDetailView.as_view()), - path("/create_mail/", views.AccountCreateMailView.as_view()), - path("comment//", views.AccountCommentView.as_view()), - path("attachment//", views.AccountAttachmentView.as_view()), + path("/", views.AccountDetailView.as_view()), + path("/create_mail/", views.AccountCreateMailView.as_view()), + path("comment//", views.AccountCommentView.as_view()), + path("attachment//", views.AccountAttachmentView.as_view()), ] diff --git a/backend/business_hours/urls.py b/backend/business_hours/urls.py index 2200c3ff..d8c53f4e 100644 --- a/backend/business_hours/urls.py +++ b/backend/business_hours/urls.py @@ -7,17 +7,17 @@ app_name = "api_business_hours" urlpatterns = [ path("calendar/", views.BusinessCalendarView.as_view(), name="calendar_default"), path( - "calendar//", + "calendar//", views.BusinessCalendarView.as_view(), name="calendar_detail", ), path( - "calendar//holidays/", + "calendar//holidays/", views.BusinessHolidayListView.as_view(), name="holiday_list", ), path( - "calendar//holidays//", + "calendar//holidays//", views.BusinessHolidayDetailView.as_view(), name="holiday_detail", ), diff --git a/backend/cases/time_entry_urls.py b/backend/cases/time_entry_urls.py index f7ca165a..c8588663 100644 --- a/backend/cases/time_entry_urls.py +++ b/backend/cases/time_entry_urls.py @@ -10,7 +10,7 @@ from cases import time_views app_name = "api_time_entries" urlpatterns = [ - # Timesheet must come before / to avoid the catchall. + # Timesheet must come before / to avoid the catchall. path( "timesheet/", time_views.TimesheetView.as_view(), @@ -22,12 +22,12 @@ urlpatterns = [ name="time_entries_unbilled", ), path( - "/stop/", + "/stop/", time_views.TimeEntryStopView.as_view(), name="time_entry_stop", ), path( - "/", + "/", time_views.TimeEntryDetailView.as_view(), name="time_entry_detail", ), diff --git a/backend/cases/urls.py b/backend/cases/urls.py index 1e96c2e8..1e9534c2 100644 --- a/backend/cases/urls.py +++ b/backend/cases/urls.py @@ -25,7 +25,7 @@ app_name = "api_cases" urlpatterns = [ # Cases list endpoint path("", views.CaseListView.as_view()), - # Kanban endpoints (must be before / to avoid conflicts) + # Kanban endpoints (must be before / to avoid conflicts) path("kanban/", kanban_views.CaseKanbanView.as_view(), name="case_kanban"), # Pipeline management path( @@ -34,81 +34,81 @@ urlpatterns = [ name="pipeline_list_create", ), path( - "pipelines//", + "pipelines//", kanban_views.CasePipelineDetailView.as_view(), name="pipeline_detail", ), path( - "pipelines//stages/", + "pipelines//stages/", kanban_views.CaseStageCreateView.as_view(), name="stage_create", ), path( - "pipelines//stages/reorder/", + "pipelines//stages/reorder/", kanban_views.CaseStageReorderView.as_view(), name="stage_reorder", ), # Stage management path( - "stages//", + "stages//", kanban_views.CaseStageDetailView.as_view(), name="stage_detail", ), - # Solutions (Knowledge Base) endpoints (must be before / to avoid conflicts) + # Solutions (Knowledge Base) endpoints (must be before / to avoid conflicts) path( "solutions/", solution_views.SolutionListView.as_view(), name="solutions_list" ), path( - "solutions//", + "solutions//", solution_views.SolutionDetailView.as_view(), name="solution_detail", ), path( - "solutions//publish/", + "solutions//publish/", solution_views.SolutionPublishView.as_view(), name="solution_publish", ), path( - "solutions//unpublish/", + "solutions//unpublish/", solution_views.SolutionUnpublishView.as_view(), name="solution_unpublish", ), - # Reopen policy (admin singleton, must be before / patterns) + # Reopen policy (admin singleton, must be before / patterns) path( "reopen-policy/", views.ReopenPolicyView.as_view(), name="reopen_policy", ), - # Escalation policies (admin CRUD, must be before / patterns) + # Escalation policies (admin CRUD, must be before / patterns) path( "escalation-policies/", escalation_views.EscalationPolicyListCreateView.as_view(), name="escalation_policy_list_create", ), path( - "escalation-policies//", + "escalation-policies//", escalation_views.EscalationPolicyDetailView.as_view(), name="escalation_policy_detail", ), - # Routing rules (admin CRUD + dry-run test endpoint, must be before / patterns) + # Routing rules (admin CRUD + dry-run test endpoint, must be before / patterns) path( "routing-rules/", routing_views.RoutingRuleListCreateView.as_view(), name="routing_rule_list_create", ), path( - "routing-rules//", + "routing-rules//", routing_views.RoutingRuleDetailView.as_view(), name="routing_rule_detail", ), path( - "routing-rules//test/", + "routing-rules//test/", routing_views.RoutingRuleTestView.as_view(), name="routing_rule_test", ), # Inbound email: public webhook (SNS-signed) + admin mailbox CRUD. path( - "inbound//", + "inbound//", inbound_views.InboundMailboxWebhookView.as_view(), name="inbound_webhook", ), @@ -118,17 +118,17 @@ urlpatterns = [ name="inbound_mailbox_list_create", ), path( - "mailboxes//", + "mailboxes//", inbound_views.InboundMailboxDetailView.as_view(), name="inbound_mailbox_detail", ), - # Watching list (must be before / patterns) + # Watching list (must be before / patterns) path( "watching/", watcher_views.WatchingListView.as_view(), name="cases_watching", ), - # Analytics dashboards (must be before / patterns) + # Analytics dashboards (must be before / patterns) path( "analytics/frt/", analytics_views.AnalyticsFrtView.as_view(), @@ -169,13 +169,13 @@ urlpatterns = [ analytics_views.AnalyticsExportView.as_view(), name="analytics_export", ), - # CSAT aggregate (must be before / patterns) + # CSAT aggregate (must be before / patterns) path( "csat/aggregate/", csat_views.CsatAggregateView.as_view(), name="csat_aggregate", ), - # Bulk endpoints (must be before / patterns) + # Bulk endpoints (must be before / patterns) path( "bulk/update/", bulk_views.BulkUpdateCasesView.as_view(), @@ -186,7 +186,7 @@ urlpatterns = [ bulk_views.BulkDeleteCasesView.as_view(), name="cases_bulk_delete", ), - # CSV import (must be before / patterns) + # CSV import (must be before / patterns) path( "import/preview/", import_views.CaseImportPreviewView.as_view(), @@ -197,91 +197,91 @@ urlpatterns = [ import_views.CaseImportCommitView.as_view(), name="cases_import_commit", ), - # Case ↔ Solution linking (M2M endpoints, must be before / patterns) + # Case ↔ Solution linking (M2M endpoints, must be before / patterns) path( - "/solutions/", + "/solutions/", views.CaseSolutionLinkView.as_view(), name="case_solution_link", ), path( - "/solutions//", + "/solutions//", views.CaseSolutionLinkView.as_view(), name="case_solution_unlink", ), - # Audit-log feed (must be before / pattern) + # Audit-log feed (must be before / pattern) path( - "/activities/", + "/activities/", views.CaseActivityListView.as_view(), name="case_activities", ), - # KB suggester typeahead (must be before / pattern) + # KB suggester typeahead (must be before / pattern) path( - "/solution-suggestions/", + "/solution-suggestions/", kb_views.SolutionSuggestionsView.as_view(), name="case_solution_suggestions", ), - # Watch toggle + watchers list (must be before / pattern) + # Watch toggle + watchers list (must be before / pattern) path( - "/watch/", + "/watch/", watcher_views.WatchView.as_view(), name="case_watch", ), path( - "/watchers/", + "/watchers/", watcher_views.WatchersListView.as_view(), name="case_watchers", ), - # Merge endpoint (must be before / pattern) + # Merge endpoint (must be before / pattern) path( - "/merge//", + "/merge//", merge_views.CaseMergeView.as_view(), name="case_merge", ), path( - "/unmerge/", + "/unmerge/", unmerge_views.CaseUnmergeView.as_view(), name="case_unmerge", ), - # Parent/child tree endpoints (must be before / pattern) + # Parent/child tree endpoints (must be before / pattern) path( - "/tree/", + "/tree/", parent_views.CaseTreeView.as_view(), name="case_tree", ), path( - "/link/", + "/link/", parent_views.CaseLinkParentView.as_view(), name="case_link_parent", ), path( - "/close-with-children/", + "/close-with-children/", parent_views.CaseCloseWithChildrenView.as_view(), name="case_close_with_children", ), - # Time-tracking endpoints (must be before / catchall) + # Time-tracking endpoints (must be before / catchall) path( - "/time-entries/", + "/time-entries/", time_views.TimeEntryListCreateView.as_view(), name="case_time_entries", ), path( - "/time-entries/start/", + "/time-entries/start/", time_views.TimeEntryStartView.as_view(), name="case_time_entry_start", ), path( - "/time-summary/", + "/time-summary/", time_views.TimeSummaryView.as_view(), name="case_time_summary", ), - # Approval workflows (must be before / catchall), Tier 3 approvals. + # Approval workflows (must be before / catchall), Tier 3 approvals. path( "approval-rules/", approval_views.ApprovalRuleListCreateView.as_view(), name="approval_rule_list_create", ), path( - "approval-rules//", + "approval-rules//", approval_views.ApprovalRuleDetailView.as_view(), name="approval_rule_detail", ), @@ -291,28 +291,28 @@ urlpatterns = [ name="approval_inbox", ), path( - "approvals//approve/", + "approvals//approve/", approval_views.ApprovalApproveView.as_view(), name="approval_approve", ), path( - "approvals//reject/", + "approvals//reject/", approval_views.ApprovalRejectView.as_view(), name="approval_reject", ), path( - "approvals//cancel/", + "approvals//cancel/", approval_views.ApprovalCancelView.as_view(), name="approval_cancel", ), path( - "/request-approval/", + "/request-approval/", approval_views.CaseRequestApprovalView.as_view(), name="case_request_approval", ), - # Case detail routes (must be after all named routes due to pattern) - path("/", views.CaseDetailView.as_view()), - path("/move/", kanban_views.CaseMoveView.as_view(), name="case_move"), - path("comment//", views.CaseCommentView.as_view()), - path("attachment//", views.CaseAttachmentView.as_view()), + # Case detail routes (must be after all named routes due to pattern) + path("/", views.CaseDetailView.as_view()), + path("/move/", kanban_views.CaseMoveView.as_view(), name="case_move"), + path("comment//", views.CaseCommentView.as_view()), + path("attachment//", views.CaseAttachmentView.as_view()), ] diff --git a/backend/cases/views.py b/backend/cases/views.py index 4bcb28ea..9afd1b66 100644 --- a/backend/cases/views.py +++ b/backend/cases/views.py @@ -1110,7 +1110,16 @@ class CaseSolutionLinkView(APIView): fields={"solution_id": serializers.CharField()}, ), ) - def post(self, request, pk): + def post(self, request, pk, solution_pk=None): + # This class is routed twice: `/solutions/` for POST and + # `/solutions//` for DELETE. Django hands every route's + # captured kwargs to whichever method it dispatches, so POST to the + # second route arrived with an argument it had no parameter for and + # DELETE to the first arrived missing one. Both were an uncaught + # TypeError, which is a 500 on a route the client is simply using the + # wrong verb on. 405 is the answer to a wrong verb. + if solution_pk is not None: + self.http_method_not_allowed(request) case = get_case_or_404(request.profile, pk) # Attaching an article to a ticket changes the ticket, so it takes the # same permission as any other change to it. Org alone was the whole @@ -1144,7 +1153,11 @@ class CaseSolutionLinkView(APIView): ) @extend_schema(tags=["Cases"]) - def delete(self, request, pk, solution_pk): + def delete(self, request, pk, solution_pk=None): + # See `post` above: reached without a solution id, this is the + # collection route being deleted, which is a wrong verb, not a crash. + if solution_pk is None: + self.http_method_not_allowed(request) case = get_case_or_404(request.profile, pk) assert_case_write_access(request.profile, case) sol = self._get_solution(solution_pk, request.profile.org) diff --git a/backend/common/converters.py b/backend/common/converters.py new file mode 100644 index 00000000..a71ec4fb --- /dev/null +++ b/backend/common/converters.py @@ -0,0 +1,46 @@ +"""A URL path converter for id segments, so a malformed id 404s at the router. + +Every model here has a UUID primary key, but the routes captured those ids with +````. The string reached the view unexamined and went straight into an +``id`` lookup, where Django parses it while *building* the query and raises +``django.core.exceptions.ValidationError``. DRF's exception handler does not +translate that class, so the request answered 500. A stale bookmark, a +hand-edited URL, or a crawler hitting ``/api/leads/undefined/`` took the +endpoint down, and it did so on 114 route/method combinations across roughly 50 +views: every detail, move, comment, attachment, watch, merge and approval route +in the product. + +Fixing it per view means auditing 50 view bodies and remembering the guard in +every future one. Fixing it at the router is one line per route and cannot be +forgotten, because a view can no longer be reached with an unparseable id. + +Django ships a ``uuid`` converter already, and it is deliberately not used here: +its regex accepts only the canonical lowercase hyphenated form, while the ORM +accepts bare hex, braced and URN forms and any case. Swapping in ```` +would turn requests that work today into 404s. ``uuid.UUID`` is what +``UUIDField.to_python`` itself calls, so parsing with it accepts exactly what +the ORM accepts and nothing that used to work can break. ``common.validators`` +made the same choice for query parameters and says so for the same reason. + +Raising ``ValueError`` from ``to_python`` is the documented way to tell the +resolver "this pattern does not match": resolution continues to the next +pattern and ends in a normal 404, rather than the pattern matching and the view +failing later. +""" + +import uuid as uuid_module + + +class UUIDLikeConverter: + """Match a path segment that ``uuid.UUID`` can parse, in any of its forms.""" + + regex = "[^/]+" + + def to_python(self, value): + try: + return str(uuid_module.UUID(value)) + except (AttributeError, TypeError, ValueError): + raise ValueError(f"{value!r} is not a usable id.") from None + + def to_url(self, value): + return str(value) diff --git a/backend/common/tests/test_malformed_id_routes.py b/backend/common/tests/test_malformed_id_routes.py new file mode 100644 index 00000000..4910980a --- /dev/null +++ b/backend/common/tests/test_malformed_id_routes.py @@ -0,0 +1,163 @@ +"""No route may answer 500 because an id in the path was not a UUID. + +Every model here has a UUID primary key, and the routes used to capture those +ids with ````. The raw string reached the view and went into an ``id`` +lookup, where Django parses it while building the query and raises +``django.core.exceptions.ValidationError``. DRF does not translate that class, +so the request answered 500: 114 route/method combinations across roughly 50 +views, reachable by anything as ordinary as a stale bookmark or a crawler +following ``/api/leads/undefined/``. + +The fix is the ``uid`` path converter (``common/converters.py``), so this is a +sweep rather than a list: it walks the live URLconf, so a route added later +with ```` fails here instead of shipping the old bug back. That is the +point of driving it from the resolver and not from a fixed list of paths. + +``pack_id`` and the public ``token`` are deliberately still ````: one +is a registry key and the other a ``secrets.token_urlsafe`` string. Neither is +a UUID, and neither reaches a UUID column. +""" + +import re + +import pytest +from django.urls import get_resolver + +MALFORMED = "not-a-uuid" + +# Path params that are legitimately not UUIDs. Anything else must be ``. +NON_UUID_PARAMS = {"token", "pack_id"} + + +def _routes(): + """Every leaf route in the live URLconf, with its full pattern and view.""" + found = [] + + def walk(patterns, prefix=""): + for entry in patterns: + if hasattr(entry, "url_patterns"): + walk(entry.url_patterns, prefix + str(entry.pattern)) + else: + found.append((prefix + str(entry.pattern), entry.callback)) + + walk(get_resolver().url_patterns) + return found + + +def _id_routes(): + """Routes carrying at least one id segment, excluding the public portal.""" + out = [] + for route, callback in _routes(): + if "api/public/" in route: + continue + if "", route) + if name not in NON_UUID_PARAMS + ] + assert offenders == [], ( + "these routes capture an id as a bare string, so a malformed id " + f"reaches the ORM and answers 500: {offenders}" + ) + + +@pytest.mark.django_db +def test_no_route_answers_500_for_a_malformed_id(admin_client): + """Drive every id-bearing route with an unparseable id.""" + failures = [] + for route, callback in _id_routes(): + url = "/" + re.sub(r"<\w+:(\w+)>", MALFORMED, route) + view = getattr(callback, "cls", None) + if view is None: + continue + for method in ("get", "post", "put", "patch", "delete"): + if not hasattr(view, method): + continue + try: + code = getattr(admin_client, method)(url, {}, format="json").status_code + except Exception as exc: # noqa: BLE001 + failures.append(f"{method.upper()} {url} raised {type(exc).__name__}: {exc}") + continue + if code >= 500: + failures.append(f"{method.upper()} {url} answered {code}") + assert failures == [], "routes that fail on a malformed id:\n" + "\n".join(failures) + + +@pytest.mark.django_db +class TestTheConverterAcceptsEveryFormTheORMAccepts: + """A guard that rejects too much is the same defect facing the other way. + + ``uuid.UUID`` accepts bare hex, braced and URN forms and any case, and so + does the ORM. Django's built-in ``uuid`` converter accepts only canonical + lowercase, which is why it is not used here. A 404 in these cases would + mean the fix broke requests that used to work. + """ + + @pytest.fixture + def account(self, org_a): + from accounts.models import Account + + return Account.objects.create(name="Converter Account", org=org_a) + + @pytest.mark.parametrize( + "form", + [ + "canonical", + "uppercase", + "bare_hex", + "braced", + "urn", + ], + ) + def test_form_resolves(self, admin_client, account, form): + raw = account.id + rendered = { + "canonical": str(raw), + "uppercase": str(raw).upper(), + "bare_hex": raw.hex, + "braced": f"{{{raw}}}", + "urn": raw.urn, + }[form] + response = admin_client.get(f"/api/accounts/{rendered}/") + assert response.status_code != 404, ( + f"the {form} form of a real id no longer resolves; the converter " + "is stricter than the ORM" + ) + + +@pytest.mark.django_db +class TestWrongVerbOnTheCaseSolutionRoutes: + """One view class, two routes, different captured kwargs. + + Django passes a route's kwargs to whichever method it dispatches, so + ``DELETE`` on the collection route arrived missing ``solution_pk`` and + ``POST`` on the member route arrived with one. Both were an uncaught + ``TypeError``, so a client using the wrong verb got a 500 instead of a 405. + A malformed id now 404s before reaching either, which would have hidden + this: these use a well-formed id on purpose. + """ + + @pytest.fixture + def case(self, org_a): + from cases.models import Case + + return Case.objects.create(name="Converter Case", status="New", org=org_a) + + def test_delete_on_the_collection_route(self, admin_client, case): + response = admin_client.delete(f"/api/cases/{case.id}/solutions/") + assert response.status_code == 405, response.status_code + + def test_post_on_the_member_route(self, admin_client, case): + response = admin_client.post( + f"/api/cases/{case.id}/solutions/{case.id}/", {}, format="json" + ) + assert response.status_code == 405, response.status_code diff --git a/backend/common/tests/test_tags.py b/backend/common/tests/test_tags.py index ae9c31d9..fb992e9f 100644 --- a/backend/common/tests/test_tags.py +++ b/backend/common/tests/test_tags.py @@ -9,15 +9,22 @@ from rest_framework import status from accounts.models import Account from cases.models import Case -from common.models import Tags +from common.models import APISettings, Tags +from contacts.models import Contact from leads.models import Lead from opportunity.models import Opportunity +from tasks.models import Task def _tag_row(response, tag_id): return next(r for r in response.data["tags"] if r["id"] == str(tag_id)) +def _tag_row_by_id(body, tag_id): + """Same lookup against an already-unwrapped response body.""" + return next(r for r in body["tags"] if r["id"] == str(tag_id)) + + @pytest.mark.django_db class TestTagsListView: """Tests for GET/POST /api/tags/""" @@ -168,6 +175,12 @@ class TestTagsUsageAndTotals: Case.objects.create( org=org_a, name="Ticket", status="New", priority="Normal" ).tags.add(tag) + Contact.objects.create( + org=org_a, first_name="Dana", last_name="Client" + ).tags.add(tag) + Task.objects.create( + org=org_a, title="Follow up", status="New", priority="Medium" + ).tags.add(tag) response = admin_client.get(self.url) assert response.status_code == status.HTTP_200_OK @@ -177,6 +190,9 @@ class TestTagsUsageAndTotals: "leads": 2, "opportunities": 1, "cases": 1, + "contacts": 1, + "tasks": 1, + "api_settings": 0, } def test_unused_tag_reports_zero_usage(self, admin_client, org_a): @@ -187,8 +203,67 @@ class TestTagsUsageAndTotals: "leads": 0, "opportunities": 0, "cases": 0, + "contacts": 0, + "tasks": 0, + "api_settings": 0, } + @pytest.mark.parametrize("kind", ["contacts", "tasks", "api_settings"]) + def test_a_tag_used_only_off_the_headline_models_is_not_unused( + self, admin_client, org_a, kind + ): + """The bug: `_TAGGABLE` held four of the seven taggable models. + + A tag applied only to a contact, a task or an API setting reported zero + usage everywhere and was counted in the "unused" stat, telling an admin + it was safe to archive while it was in active use. + """ + tag = Tags.objects.create(name=f"Only {kind}", slug=f"only-{kind}", org=org_a) + if kind == "contacts": + Contact.objects.create( + org=org_a, first_name="Dana", last_name="Client" + ).tags.add(tag) + elif kind == "tasks": + Task.objects.create( + org=org_a, title="Follow up", status="New", priority="Medium" + ).tags.add(tag) + else: + APISettings.objects.create( + org=org_a, title="Site", website="https://example.com" + ).tags.add(tag) + + body = admin_client.get(self.url).data + assert _tag_row_by_id(body, tag.id)["usage"][kind] == 1 + assert body["totals"]["unused"] == 0, ( + f"a tag in use on {kind} was reported as unused" + ) + + def test_taggable_covers_every_model_with_a_tags_m2m(self): + """Guard the hand-written constant with the live model registry. + + `_TAGGABLE` is written out by hand so a change shows up in a diff, which + is only safe if something notices when a new taggable model appears. + Without this, the next model to grow a `tags` M2M silently repeats the + bug above. + """ + from django.apps import apps + + from common.views.tags_views import _TAGGABLE + + declared = { + model + for model in apps.get_models() + for field in model._meta.get_fields() + if field.many_to_many + and not field.auto_created + and getattr(field, "related_model", None) is Tags + } + counted = {model for _, model in _TAGGABLE} + assert declared == counted, ( + "these models carry a tags M2M but their usage is never counted, so " + f"a tag used only on them reads as unused: {declared - counted}" + ) + def test_totals_count_active_unused(self, admin_client, org_a): used = Tags.objects.create(name="Used", slug="used", org=org_a) Account.objects.create(org=org_a, name="Acme").tags.add(used) diff --git a/backend/common/urls.py b/backend/common/urls.py index 2b354a93..6b8ff7eb 100644 --- a/backend/common/urls.py +++ b/backend/common/urls.py @@ -85,7 +85,7 @@ urlpatterns = [ # Organization and profile management path("org/", OrgProfileCreateView.as_view()), path("org/settings/", OrgSettingsView.as_view(), name="org_settings"), - # These literal org/… paths must precede org// so they are not + # These literal org/… paths must precede org// so they are not # captured as a pk (org/tokens/ would otherwise resolve to OrgUpdateView # with pk="tokens"). org/tokens/ is ADMIN-only token oversight, separate # from profile/tokens/ (self-scoped) so the self guard is never widened; an @@ -98,7 +98,7 @@ urlpatterns = [ OrgAccessTokenDetailView.as_view(), name="org_pat_detail", ), - path("org//", OrgUpdateView.as_view()), + path("org//", OrgUpdateView.as_view()), path("profile/", ProfileView.as_view()), # Personal Access Tokens (REST API), a user manages ONLY their own path( @@ -114,23 +114,23 @@ urlpatterns = [ # User management path("users/get-teams-and-users/", GetTeamsAndUsersView.as_view()), path("users/", UsersListView.as_view()), - path("user//", UserDetailView.as_view()), - path("user//status/", UserStatusView.as_view()), + path("user//", UserDetailView.as_view()), + path("user//status/", UserStatusView.as_view()), # Documents path("documents/", DocumentListView.as_view()), - path("documents//", DocumentDetailView.as_view()), + path("documents//", DocumentDetailView.as_view()), # API Settings path("api-settings/", DomainList.as_view()), - path("api-settings//", DomainDetailView.as_view()), + path("api-settings//", DomainDetailView.as_view()), # Activities (for dashboard recent activities) path("activities/", ActivityListView.as_view(), name="activities"), # Teams (merged from teams app) path("teams/", TeamsListView.as_view()), - path("teams//", TeamsDetailView.as_view()), + path("teams//", TeamsDetailView.as_view()), # Tags path("tags/", TagsListView.as_view()), - path("tags//", TagsDetailView.as_view()), - path("tags//restore/", TagsRestoreView.as_view()), + path("tags//", TagsDetailView.as_view()), + path("tags//restore/", TagsRestoreView.as_view()), # Custom fields (per-org schema extension; cross-entity) path( "custom-fields/", @@ -138,7 +138,7 @@ urlpatterns = [ name="custom_fields_list_create", ), path( - "custom-fields//", + "custom-fields//", CustomFieldDefinitionDetailView.as_view(), name="custom_field_detail", ), @@ -150,12 +150,12 @@ urlpatterns = [ name="notifications_read_all", ), path( - "notifications//read/", + "notifications//read/", NotificationReadView.as_view(), name="notifications_read", ), path( - "notifications//", + "notifications//", NotificationDetailView.as_view(), name="notifications_detail", ), diff --git a/backend/common/views/tags_views.py b/backend/common/views/tags_views.py index 19628434..c342b8bb 100644 --- a/backend/common/views/tags_views.py +++ b/backend/common/views/tags_views.py @@ -12,19 +12,36 @@ from accounts.models import Account from cases.models import Case from common import swagger_params from common.lookups import get_scoped_or_404 -from common.models import Tags +from common.models import APISettings, Tags from common.permissions import HasOrgContext from common.serializer import TagsSerializer +from contacts.models import Contact from leads.models import Lead from opportunity.models import Opportunity +from tasks.models import Task # The models a tag can be applied to, paired with the usage key the settings # page shows. Each is org-scoped and carries a `tags` M2M back to Tags. +# +# This must list EVERY model with that M2M, not just the prominent ones. It +# held four of the seven, and the three it left out (Contact, Task, +# APISettings) all accept tag writes through the API, contacts through the CSV +# importer too. A tag applied only to contacts therefore reported zero usage +# and was counted as "unused" on the settings page, which is the one number an +# admin uses to decide whether a tag is safe to archive. +# +# Hand-written on purpose, so a change shows up in a diff, and guarded by +# `test_taggable_covers_every_model_with_a_tags_m2m`, which walks the live +# model registry: a new taggable model fails that test instead of silently +# under-reporting usage forever. _TAGGABLE = ( ("accounts", Account), ("leads", Lead), ("opportunities", Opportunity), ("cases", Case), + ("contacts", Contact), + ("tasks", Task), + ("api_settings", APISettings), ) diff --git a/backend/contacts/urls.py b/backend/contacts/urls.py index af4c7189..70acae1d 100644 --- a/backend/contacts/urls.py +++ b/backend/contacts/urls.py @@ -6,7 +6,7 @@ app_name = "api_contacts" urlpatterns = [ path("", views.ContactsListView.as_view()), - # CSV import (must be before / to avoid being captured as an ID) + # CSV import (must be before / to avoid being captured as an ID) path( "import/preview/", import_views.ContactImportPreviewView.as_view(), @@ -17,7 +17,7 @@ urlpatterns = [ import_views.ContactImportCommitView.as_view(), name="contacts_import_commit", ), - path("/", views.ContactDetailView.as_view()), - path("comment//", views.ContactCommentView.as_view()), - path("attachment//", views.ContactAttachmentView.as_view()), + path("/", views.ContactDetailView.as_view()), + path("comment//", views.ContactCommentView.as_view()), + path("attachment//", views.ContactAttachmentView.as_view()), ] diff --git a/backend/crm/urls.py b/backend/crm/urls.py index 41a1efd0..0f5b4d5f 100644 --- a/backend/crm/urls.py +++ b/backend/crm/urls.py @@ -1,15 +1,29 @@ from django.conf import settings from django.contrib import admin from django.contrib.auth import views -from django.urls import include, path +from django.urls import include, path, register_converter from django.urls import re_path as url from django.views.generic import TemplateView +from drf_spectacular.plumbing import DJANGO_PATH_CONVERTER_MAPPING +from drf_spectacular.types import OpenApiTypes from drf_spectacular.views import ( SpectacularAPIView, SpectacularRedocView, SpectacularSwaggerView, ) +from common.converters import UUIDLikeConverter + +# Registered on the root URLconf so every app's `` segment resolves. +# See common/converters.py for why this exists and why it is not ``. +register_converter(UUIDLikeConverter, "uid") + +# drf-spectacular maps a converter name to an OpenAPI type; an unknown one +# falls back to a bare string. Without this the published schema would describe +# 165 id path parameters as untyped strings, which is what `` gave us +# and is less than the route now guarantees. +DJANGO_PATH_CONVERTER_MAPPING["uid"] = OpenApiTypes.UUID + app_name = "crm" urlpatterns = [ diff --git a/backend/invoices/api_urls.py b/backend/invoices/api_urls.py index b827312b..7781f25e 100644 --- a/backend/invoices/api_urls.py +++ b/backend/invoices/api_urls.py @@ -139,6 +139,11 @@ urlpatterns = [ api_views.InvoiceTemplateDetailView.as_view(), name="template_detail", ), + path( + "templates//editor/", + api_views.InvoiceTemplateEditorView.as_view(), + name="template_editor", + ), # ========================================================================== # REPORTS # ========================================================================== diff --git a/backend/invoices/api_views.py b/backend/invoices/api_views.py index 88b61a65..1212eaf7 100644 --- a/backend/invoices/api_views.py +++ b/backend/invoices/api_views.py @@ -59,6 +59,7 @@ from invoices.serializer import ( InvoiceSerializer, InvoiceTemplateCreateSerializer, InvoiceTemplateListSerializer, + InvoiceTemplateSerializer, PaymentCreateSerializer, PaymentSerializer, ProductCreateSerializer, @@ -653,7 +654,9 @@ class InvoiceLineItemListView(APIView): if error: return error - serializer = InvoiceLineItemCreateSerializer(data=request.data) + serializer = InvoiceLineItemCreateSerializer( + data=request.data, request_obj=request + ) if serializer.is_valid(): line_item = serializer.save(invoice=invoice, org=request.profile.org) @@ -700,7 +703,7 @@ class InvoiceLineItemDetailView(APIView): ) serializer = InvoiceLineItemCreateSerializer( - line_item, data=request.data, partial=True + line_item, data=request.data, partial=True, request_obj=request ) if serializer.is_valid(): line_item = serializer.save() @@ -1528,22 +1531,26 @@ class RecurringInvoicePauseView(APIView): # ============================================================================= -def _forbid_non_admin_template_write(request): +def _forbid_non_admin_template( + request, message="Only an administrator can change invoice templates." +): """Return a 403 Response for non-admins, or None to allow. Invoice templates are org-wide shared config: every member reads the catalogue (to see how their invoices look), but changing a template: its colours, its default flag, its raw HTML/CSS, affects everyone in the org and drives the server-side PDF render. So writes (create/update/delete) are - admin-only, while reads stay open. Role is derived from ``request.profile``, - never the request body. + admin-only, while the catalogue read stays open. Role is derived from + ``request.profile``, never the request body. + + ``message`` is a parameter because one read is admin-only too: fetching a + template's raw markup for the editor. That is the same rule for the same + reason, so it is the same gate rather than a second copy of the role check + that could drift away from this one. """ if request.profile.role != "ADMIN" and not request.user.is_superuser: return Response( - { - "error": True, - "message": "Only an administrator can change invoice templates.", - }, + {"error": True, "message": message}, status=status.HTTP_403_FORBIDDEN, ) return None @@ -1574,7 +1581,7 @@ class InvoiceTemplateListView(APIView, LimitOffsetPagination): @extend_schema(tags=["Invoice Templates"], operation_id="templates_create") def post(self, request): - denied = _forbid_non_admin_template_write(request) + denied = _forbid_non_admin_template(request) if denied: return denied @@ -1621,7 +1628,7 @@ class InvoiceTemplateDetailView(APIView): @extend_schema(tags=["Invoice Templates"], operation_id="templates_update") def put(self, request, pk): - denied = _forbid_non_admin_template_write(request) + denied = _forbid_non_admin_template(request) if denied: return denied @@ -1652,7 +1659,7 @@ class InvoiceTemplateDetailView(APIView): @extend_schema(tags=["Invoice Templates"], operation_id="templates_destroy") def delete(self, request, pk): - denied = _forbid_non_admin_template_write(request) + denied = _forbid_non_admin_template(request) if denied: return denied @@ -1670,6 +1677,47 @@ class InvoiceTemplateDetailView(APIView): ) +class InvoiceTemplateEditorView(APIView): + """The only endpoint that returns a template's raw ``template_html``/``css``. + + Every other read path uses ``InvoiceTemplateListSerializer``, which reports + booleans and a byte count in place of the markup, because that markup is + org-authored HTML/CSS that WeasyPrint renders into a PDF server-side: the + moment either field lands in a browser DOM, a PDF setting becomes stored + XSS. + + An editor still has to load what it is editing, and it could not: the edit + form had no way to pre-fill the two fields it exists to change, so opening + it and saving silently blanked them. Serving the markup from its own + admin-only route keeps that fix out of the detail response every member + reads. A client must bind these two fields to a `` + + + + + + +
+ +
+ Custom layout +
+ Filling these in replaces the whole printed document, the line-item table and the totals + included. Nothing checks that what you write still prints an amount due. Leave both empty + to use the built-in layout. Emptying a field that has markup in it now clears it. +
+
+
+ + + + + + {#if data.template?.is_default} +

+ This is the org default, so these changes apply to every new invoice. Which template is the + default is changed from the templates page, not here. +

+ {/if} + +
+ + Cancel +
+ + {/if} + + + diff --git a/frontend/src/routes/(app)/invoices/templates/new/+page.server.js b/frontend/src/routes/(app)/invoices/templates/new/+page.server.js index 78083701..95fd743b 100644 --- a/frontend/src/routes/(app)/invoices/templates/new/+page.server.js +++ b/frontend/src/routes/(app)/invoices/templates/new/+page.server.js @@ -6,7 +6,7 @@ import { readableError } from '$lib/server/v2/form-errors.js'; * Creating a template. * * Admin-only: `POST /api/invoices/templates/` refuses a non-admin via - * `_forbid_non_admin_template_write`, so this mirrors the products/new and + * `_forbid_non_admin_template`, so this mirrors the products/new and * goals/new pattern: `load` computes `can_manage` from `locals.profile.role` * (same source the list page's `load` already uses) so a non-admin who * navigates here directly sees an "admins only" state instead of a form the diff --git a/frontend/src/routes/(app)/invoices/templates/new/+page.svelte b/frontend/src/routes/(app)/invoices/templates/new/+page.svelte index e4c04af1..3f972130 100644 --- a/frontend/src/routes/(app)/invoices/templates/new/+page.svelte +++ b/frontend/src/routes/(app)/invoices/templates/new/+page.svelte @@ -12,7 +12,7 @@ * full reasoning. * * VALIDATION HERE IS A UX HINT, NOT A RULE. `POST /api/invoices/templates/` - * is admin-gated (`_forbid_non_admin_template_write`) and enforces that + * is admin-gated (`_forbid_non_admin_template`) and enforces that * regardless of what this page shows; curl and the mobile client reach the * API without passing through here. The two colour inputs use * `type="color"` so the browser can only ever submit a valid six-digit hex diff --git a/frontend/src/routes/(app)/settings/tags/+page.svelte b/frontend/src/routes/(app)/settings/tags/+page.svelte index 7fce47be..86bdee50 100644 --- a/frontend/src/routes/(app)/settings/tags/+page.svelte +++ b/frontend/src/routes/(app)/settings/tags/+page.svelte @@ -38,16 +38,17 @@ let totals = $derived(data.totals); /** - * Sums the four models the backend's `_TAGGABLE` list - * (`backend/common/views/tags_views.py:22-27`) counts: accounts, leads, - * opportunities (deals) and cases (tickets). `Contact` and `Task` also - * declare `tags = ManyToManyField(Tags, ...)` and accept tag writes through - * their own APIs, plus the contacts CSV importer, but `_TAGGABLE` does not - * include them, so this total, and the "unused" totals card below, are - * partial: a tag can carry real usage on contacts or tasks and still show - * as unused here. + * Total usage across every model a tag can be applied to. + * + * Summed over the keys the backend sends rather than a list written out + * here, so it cannot drift from `_TAGGABLE` + * (`backend/common/views/tags_views.py`) the way the old hard-coded four + * did: contacts, tasks and API settings were all missing, so a tag in real + * use on contacts read as unused both here and on the totals card. A test + * on the backend now walks the model registry to keep that list complete; + * this sums whatever it sends. */ - const used = (t) => t.usage.accounts + t.usage.leads + t.usage.opportunities + t.usage.cases; + const used = (t) => Object.values(t.usage ?? {}).reduce((sum, n) => sum + (n || 0), 0); let tags = $derived( [...data.tags].sort((a, b) => used(b) - used(a) || a.name.localeCompare(b.name)) @@ -178,9 +179,11 @@ Tag Accounts + Contacts Leads Deals Tickets + Tasks Total {#if data.can_edit}{/if} @@ -198,9 +201,11 @@ {t.color} {t.usage.accounts || '—'} + {t.usage.contacts || '—'} {t.usage.leads || '—'} {t.usage.opportunities || '—'} {t.usage.cases || '—'} + {t.usage.tasks || '—'} {used(t) || '—'} {#if !t.is_active} @@ -223,8 +228,8 @@ label="Turn off" confirmLabel="Turn off" explain={used(t) > 0 - ? `${used(t)} ${used(t) === 1 ? 'account, lead, deal or ticket keeps' : 'accounts, leads, deals and tickets keep'} this tag (contacts and tasks can carry it too, but are not counted here). Turning it off stops it being offered on new records. You can turn it back on.` - : 'No account, lead, deal or ticket carries this tag. Contacts and tasks can also carry tags but are not counted here, so this is not the full picture. Turning it off stops it being offered on new records. You can turn it back on.'} + ? `${used(t)} ${used(t) === 1 ? 'record keeps' : 'records keep'} this tag. Turning it off stops it being offered on new records, and keeps it on the ones that have it. You can turn it back on.` + : 'Nothing carries this tag. Turning it off stops it being offered on new records. You can turn it back on.'} hidden={{ id: t.id }} /> {:else}