Refactor ticket and contact handling to improve visibility and access control

- Updated task views to utilize new access functions for accounts and contacts, ensuring users only see what they can access.
- Enhanced contact editing to maintain linked accounts even when not visible in the picker, preventing accidental unlinking.
- Introduced tests for contact form to verify linked account behavior.
- Implemented parent ticket handling to correctly display restricted tickets, ensuring hidden parents are represented accurately in the UI.
- Added tests for ticket parent visibility and interaction, confirming that restricted parents do not allow navigation.
- Improved ticket tree structure to handle restricted nodes, ensuring proper functionality during close operations.
This commit is contained in:
Ashwin 2026-09-26 12:17:45 +05:30
parent 41d4148edb
commit 794ff2dbd4
83 changed files with 5802 additions and 705 deletions

View file

@ -5,8 +5,10 @@ download view asks the same question rather than carrying a second copy of the
answer. The detail view still calls it.
"""
from django.db.models import Q
from rest_framework.exceptions import PermissionDenied
from accounts.models import Account
from common.permissions import is_org_admin
_DENIED = "You do not have Permission to perform this action"
@ -33,6 +35,18 @@ def has_account_access(profile, user, account):
return profile.id in {assignee.id for assignee in account.assigned_to.all()}
def visible_accounts_qs(profile, user):
"""Accounts ``profile`` may open, the queryset form of `has_account_access`.
Scoped to ``profile.org`` here, unlike the predicate, whose callers fetch
the account with that filter first.
"""
qs = Account.objects.filter(org=profile.org)
if is_org_admin(profile) or user.is_superuser:
return qs
return qs.filter(Q(created_by=profile.user) | Q(assigned_to=profile)).distinct()
def assert_account_access(profile, user, account):
"""Raise 403 unless ``profile`` may open ``account``."""
if not has_account_access(profile, user, account):

View file

@ -0,0 +1,91 @@
"""`visible_accounts_qs` is `has_account_access` as a queryset.
The ticket form's account picker, the ticket CSV import and the ticket API's
`validate_account` all ask "which accounts may this member open". The first
two ask through the queryset, the last through the predicate, so the two must
give the same answer for every kind of caller or a picker offers an account
the save path refuses (or the reverse).
"""
import pytest
from accounts.access import has_account_access, visible_accounts_qs
from accounts.models import Account
from common.models import Profile
from conftest import rls_org
def _account(org, name, created_by=None):
account = Account.objects.create(name=name, org=org)
# `update` rather than `create(created_by=...)`, so a request user left in
# the thread local cannot restamp it.
Account.objects.filter(pk=account.pk).update(created_by=created_by)
account.refresh_from_db()
return account
@pytest.fixture
def accounts(org_a, admin_user, regular_user, user_profile):
created = _account(org_a, "Created by member", created_by=regular_user)
assigned = _account(org_a, "Assigned to member", created_by=admin_user)
assigned.assigned_to.add(user_profile)
hidden = _account(org_a, "Hidden from member", created_by=admin_user)
ownerless = _account(org_a, "Nobody's")
return [created, assigned, hidden, ownerless]
@pytest.fixture
def superuser(regular_user):
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
def _by_predicate(profile, user, accounts):
return {a.id for a in accounts if has_account_access(profile, user, a)}
def _by_queryset(profile, user):
return set(visible_accounts_qs(profile, user).values_list("id", flat=True))
@pytest.mark.django_db
class TestVisibleAccountsAgreesWithThePredicate:
def test_plain_member(self, accounts, user_profile, regular_user):
expected = _by_predicate(user_profile, regular_user, accounts)
assert _by_queryset(user_profile, regular_user) == expected
# Both halves of the rule fire, and both refusals do too.
assert expected == {accounts[0].id, accounts[1].id}
def test_admin(self, accounts, admin_profile, admin_user):
expected = _by_predicate(admin_profile, admin_user, accounts)
assert _by_queryset(admin_profile, admin_user) == expected
assert expected == {a.id for a in accounts}
def test_superuser_with_a_plain_profile(self, accounts, user_profile, superuser):
assert user_profile.role != "ADMIN"
expected = _by_predicate(user_profile, superuser, accounts)
assert _by_queryset(user_profile, superuser) == expected
assert expected == {a.id for a in accounts}
def test_member_with_nothing(self, accounts, org_a, user_b):
# A second org_a member who created nothing and is assigned nothing.
profile = Profile.objects.create(
user=user_b, org=org_a, role="USER", is_active=True
)
assert _by_predicate(profile, user_b, accounts) == set()
assert _by_queryset(profile, user_b) == set()
def test_two_assignees_do_not_duplicate_the_row(
self, accounts, user_profile, regular_user, admin_profile
):
accounts[1].assigned_to.add(admin_profile)
rows = list(
visible_accounts_qs(user_profile, regular_user).values_list("id", flat=True)
)
assert len(rows) == len(set(rows))
def test_never_leaves_the_org(self, accounts, org_b, user_b, admin_profile):
with rls_org(org_b):
other = _account(org_b, "Someone else's", created_by=user_b)
assert other.id not in _by_queryset(admin_profile, admin_profile.user)

View file

@ -41,7 +41,7 @@ from accounts.serializer import (
from accounts.tasks import send_email, send_email_to_assigned_user
from cases.access import visible_cases_qs
from cases.models import Case
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from cases.workflow import TERMINAL_STATUSES
from common.custom_fields import validate_payload as validate_custom_fields_payload
from common.lookups import get_scoped_or_404
@ -256,16 +256,12 @@ class AccountsListView(APIView, LimitOffsetPagination):
def get_context_data(self, **kwargs):
params = self.request.query_params
# The read rule itself, so the list holds exactly what the detail view
# opens for this caller.
queryset = annotate_rollups(
self.model.objects.filter(org=self.request.profile.org),
access.visible_accounts_qs(self.request.profile, self.request.user),
self.request.profile,
).order_by("-id")
# The list form of `accounts.access.has_account_access`.
if not (is_org_admin(self.request.profile) or self.request.user.is_superuser):
queryset = queryset.filter(
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
if params:
if params.get("name"):
@ -348,15 +344,10 @@ class AccountsListView(APIView, LimitOffsetPagination):
# full, including leads whose own detail route answers 403 for them.
# `/api/cases/` and `/api/opportunities/` already narrow their
# equivalents; these two were missed.
member_scope = Q(created_by=self.request.profile.user) | Q(
assigned_to=self.request.profile
)
narrow_to_member = not is_org_admin(self.request.profile)
contact_qs = Contact.objects.filter(org=self.request.profile.org)
if narrow_to_member:
contact_qs = contact_qs.filter(member_scope).distinct()
contacts = contact_qs.values("id", "first_name")
#
# Contacts use their own read rule, which is what the save path
# accepts: it adds superusers and account assignment.
contacts = visible_contacts_qs(self.request.profile).values("id", "first_name")
context["contacts"] = contacts
context["closed_accounts"] = {
"offset": offset,
@ -719,6 +710,7 @@ class AccountDetailView(APIView):
object_id=self.account.id,
org=self.request.profile.org,
).order_by("-id")
account_cases = visible_cases_qs(profile).filter(account=self.account)
context.update(
{
"attachments": AttachmentsSerializer(attachments, many=True).data,
@ -742,7 +734,9 @@ class AccountDetailView(APIView):
many=True,
).data,
"cases": CaseSerializer(
visible_cases_qs(profile).filter(account=self.account), many=True
account_cases,
many=True,
context=parent_access_context(profile, account_cases),
).data,
"teams": TeamsSerializer(
Teams.objects.filter(org=self.request.profile.org), many=True

View file

@ -56,6 +56,19 @@ def visible_cases_qs(profile):
).distinct()
def writable_cases_qs(profile):
"""Cases ``profile`` may change or reply on. The queryset form of `write`.
`visible_cases_qs` without the watcher clause, for any list that offers an
action on each row: a watcher can open the ticket but is refused the reply,
so offering them one is a button that answers 403.
"""
qs = Case.objects.filter(org=profile.org)
if is_org_admin(profile):
return qs
return qs.filter(Q(created_by=profile.user) | Q(assigned_to=profile)).distinct()
def get_case_or_404(profile, pk):
"""Fetch a case in the requester's org, or raise ``Http404``.

View file

@ -28,7 +28,7 @@ from cases import analytics
from cases.access import is_org_admin
from cases.analytics import DEFAULT_SERVICE_DAYS
from cases.models import Case
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from common.permissions import HasOrgContext
from common.renderers import CSV_RENDERERS
from common.validators import uuid_param
@ -243,7 +243,9 @@ class AnalyticsDrilldownView(_AnalyticsBaseView):
cases = Case.objects.filter(org=request.profile.org, id__in=ids).order_by(
"-created_at"
)
data = CaseSerializer(cases, many=True).data
data = CaseSerializer(
cases, many=True, context=parent_access_context(request.profile, cases)
).data
return Response({"count": len(data), "results": data})

View file

@ -133,6 +133,49 @@ def find_matching_rule(case, trigger_event: str = "pre_close"):
return candidates[0]
def close_refusal(case, *, status, closed_on, priority, case_type):
"""Why a write may not close ``case``, as ``{field: message}``, or ``None``.
The close gate, one rule for both API paths that close a single ticket:
the detail PUT/PATCH (`CaseCreateSerializer.validate`) and the board move
(`CaseMoveView.patch`). The move used to set ``status="Closed"`` itself and
skip the gate, so a drag closed a ticket that PATCH refused for want of an
approval.
``case`` is the stored record, ``None`` on create; ``status``,
``closed_on``, ``priority`` and ``case_type`` are the values the write
would leave on it. Only the transition into Closed is judged, so a case
that is already Closed can be edited without re-approving. A rule matches
on priority, case_type and team, so it is evaluated against the incoming
values, or a caller could re-target the case out of the rule and close it
in the same request. ``case`` is restored either way; nothing is saved.
"""
if status != "Closed":
return None
if case is not None and case.status == "Closed":
return None
if not closed_on:
return {"closed_on": "Closed date is required when closing a case"}
if case is None:
return None
saved = (case.priority, case.case_type)
case.priority, case.case_type = priority, case_type
try:
rule = find_matching_rule(case, trigger_event="pre_close")
finally:
case.priority, case.case_type = saved
if rule is None:
return None
if Approval.objects.filter(case_id=case.pk, rule=rule, state="approved").exists():
return None
return {
"status": (
f"An approval is required before this case can be closed (rule: {rule.name})."
)
}
class Approval(BaseModel):
"""A single approval request bound to one Case + ApprovalRule."""

View file

@ -2,7 +2,8 @@
Preview reads the uploaded file and returns row-level validation results
without touching the DB. Commit re-runs validation and writes inside a
single transaction. Both endpoints are gated to ADMIN or sales-access users
single transaction. Both endpoints are gated by
`common.permissions.can_mass_import` (admins, superusers, sales access)
so non-privileged members cannot mass-create cases through this surface.
"""
@ -14,22 +15,11 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from cases.services.csv_import import commit_rows, parse_and_validate
from common.permissions import HasOrgContext
from common.permissions import HasOrgContext, can_mass_import
MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint
def _can_import(profile) -> bool:
"""Mass-create requires admin or explicit sales-access permission."""
if profile is None:
return False
if getattr(profile, "role", None) == "ADMIN":
return True
if getattr(profile, "is_admin", False):
return True
return bool(getattr(profile, "has_sales_access", False))
def _read_upload(request):
"""Return (file_bytes, error_response). One of the two will be None.
@ -83,7 +73,7 @@ class CaseImportPreviewView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,
@ -117,7 +107,7 @@ class CaseImportCommitView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,

View file

@ -6,12 +6,15 @@ Supports both status-based (default) and custom pipeline-based kanban boards.
from django.db import transaction
from django.db.models import Q
from django.shortcuts import get_object_or_404
from django.utils import timezone
from drf_spectacular.utils import OpenApiParameter, extend_schema, inline_serializer
from rest_framework import serializers, status
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import assert_case_write_access, visible_cases_qs
from cases.approvals import close_refusal
from cases.models import Case, CasePipeline, CaseStage
from cases.serializer import (
CaseKanbanCardSerializer,
@ -97,17 +100,18 @@ class CaseKanbanView(APIView):
status="Duplicate"
)
# Apply permission filtering
if not is_org_admin(request.profile) and not request.user.is_superuser:
queryset = queryset.filter(
Q(assigned_to=request.profile) | Q(created_by=request.profile.user)
)
# The ticket read rule itself, as the list and the detail view apply
# it: watchers included, and no superuser clause, because the detail
# view has none.
queryset = queryset.filter(
pk__in=visible_cases_qs(request.profile).values("pk")
)
# Apply search/filters
queryset = self._apply_filters(queryset, request.query_params)
if pipeline_id:
return self._get_pipeline_kanban(queryset, pipeline_id, org)
return self._get_pipeline_kanban(queryset, pipeline_id, request)
return self._get_status_kanban(queryset)
def _apply_filters(self, queryset, params):
@ -184,10 +188,15 @@ class CaseKanbanView(APIView):
}
)
def _get_pipeline_kanban(self, queryset, pipeline_id, org):
def _get_pipeline_kanban(self, queryset, pipeline_id, request):
"""Build kanban data using CasePipeline stages as columns."""
pipeline = get_object_or_404(
CasePipeline, pk=pipeline_id, org=org, is_active=True
CasePipelineListSerializer.with_counts(
CasePipeline.objects.all(), request.profile
),
pk=pipeline_id,
org=request.profile.org,
is_active=True,
)
queryset = queryset.filter(stage__pipeline=pipeline)
@ -239,15 +248,11 @@ class CaseMoveView(APIView):
# edit committing between this read and that save would be lost.
case = get_object_or_404(Case.objects.select_for_update(), pk=pk, org=org)
# Permission check
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not (
request.profile.user == case.created_by
or request.profile in case.assigned_to.all()
):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
# A move rewrites the ticket's status, stage and order, so it takes the
# ticket's own write rule and nothing wider. This used to add a Django
# superuser clause, which let a superuser who is a plain member of the
# org move any ticket here that `CaseDetailView.patch` refuses them.
assert_case_write_access(request.profile, case)
serializer = CaseMoveSerializer(data=request.data)
if not serializer.is_valid():
@ -257,6 +262,8 @@ class CaseMoveView(APIView):
)
data = serializer.validated_data
new_stage = case.stage
new_status = case.status
# Handle stage change
if "stage_id" in data:
@ -274,17 +281,45 @@ class CaseMoveView(APIView):
status=status.HTTP_400_BAD_REQUEST,
)
case.stage = stage
new_stage = stage
# Auto-update status if stage has maps_to_status
if stage.maps_to_status:
case.status = stage.maps_to_status
new_status = stage.maps_to_status
else:
case.stage = None
new_stage = None
# Handle status change (for status-based kanban)
if "status" in data:
case.status = data["status"]
new_status = data["status"]
# A drag into Closed is a close, so it takes the gate PATCH takes and
# answers a refusal the same way, before anything is written. The board
# has no date field, so the closing date is today, as it is for the
# ticket page's quick status change. Moving back out of Closed needs
# nothing here: the pre_save signal clears `closed_on` and
# `resolved_at` on every save that leaves Closed, this one included.
closed_on = case.closed_on
if new_status == "Closed" and case.status != "Closed":
closed_on = timezone.localdate()
refusal = close_refusal(
case,
status=new_status,
closed_on=closed_on,
priority=case.priority,
case_type=case.case_type,
)
if refusal:
return Response(
{
"error": True,
"errors": {field: [msg] for field, msg in refusal.items()},
},
status=status.HTTP_400_BAD_REQUEST,
)
case.stage = new_stage
case.status = new_status
case.closed_on = closed_on
# Calculate new order
# `case.stage`/`case.status` are already the destination by this
@ -332,7 +367,9 @@ class CasePipelineListCreateView(APIView):
def get(self, request):
"""List all pipelines for the organization."""
org = request.profile.org
pipelines = CasePipeline.objects.filter(org=org, is_active=True)
pipelines = CasePipelineListSerializer.with_counts(
CasePipeline.objects.filter(org=org, is_active=True), request.profile
)
serializer = CasePipelineListSerializer(pipelines, many=True)
return Response({"pipelines": serializer.data})
@ -407,7 +444,8 @@ class CasePipelineListCreateView(APIView):
# Refresh to include created stages
pipeline.refresh_from_db()
return Response(
CasePipelineSerializer(pipeline).data, status=status.HTTP_201_CREATED
CasePipelineSerializer(pipeline, context={"request": request}).data,
status=status.HTTP_201_CREATED,
)
@ -422,7 +460,9 @@ class CasePipelineDetailView(APIView):
@extend_schema(tags=["Case Pipelines"], responses={200: CasePipelineSerializer})
def get(self, request, pk):
pipeline = self.get_object(pk, request.profile.org)
return Response(CasePipelineSerializer(pipeline).data)
return Response(
CasePipelineSerializer(pipeline, context={"request": request}).data
)
@extend_schema(
tags=["Case Pipelines"],
@ -445,7 +485,9 @@ class CasePipelineDetailView(APIView):
)
pipeline = serializer.save(updated_by=request.user)
return Response(CasePipelineSerializer(pipeline).data)
return Response(
CasePipelineSerializer(pipeline, context={"request": request}).data
)
@extend_schema(tags=["Case Pipelines"], responses={204: None})
def delete(self, request, pk):
@ -497,7 +539,10 @@ class CaseStageCreateView(APIView):
)
stage = serializer.save(pipeline=pipeline, org=org, created_by=request.user)
return Response(CaseStageSerializer(stage).data, status=status.HTTP_201_CREATED)
return Response(
CaseStageSerializer(stage, context={"request": request}).data,
status=status.HTTP_201_CREATED,
)
class CaseStageDetailView(APIView):
@ -526,7 +571,7 @@ class CaseStageDetailView(APIView):
)
stage = serializer.save(updated_by=request.user)
return Response(CaseStageSerializer(stage).data)
return Response(CaseStageSerializer(stage, context={"request": request}).data)
@extend_schema(tags=["Case Stages"], responses={204: None})
def delete(self, request, pk):

View file

@ -20,7 +20,7 @@ from rest_framework.views import APIView
from cases.models import Case, EmailMessage
from cases.notifications import case_link
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from cases.signals import _create_activity
from common.models import Attachments, Comment
from common.permissions import HasOrgContext, is_org_admin
@ -100,7 +100,12 @@ class CaseMergeView(APIView):
"error": False,
"message": "Tickets already merged.",
"already_merged": True,
"target_case": CaseSerializer(target).data,
"target_case": CaseSerializer(
target,
context=parent_access_context(
request.profile, [target]
),
).data,
"source_case_id": str(source.id),
"redirected_url": case_link(target.id),
},
@ -219,7 +224,9 @@ class CaseMergeView(APIView):
{
"error": False,
"message": "Tickets merged",
"target_case": CaseSerializer(target).data,
"target_case": CaseSerializer(
target, context=parent_access_context(request.profile, [target])
).data,
"source_case_id": str(source.id),
"redirected_url": case_link(target.id),
},

View file

@ -199,36 +199,23 @@ class Case(AssignableMixin, BaseModel):
if self.status == "Closed" and not self.closed_on:
errors["closed_on"] = _("Closed date is required when closing a case")
# Tier 3 approvals: when transitioning *into* Closed and an active
# rule matches, require a recorded `approved` Approval. Skip the
# check when callers explicitly opt out (e.g. internal auto-flows
# that have already verified the gate or migration data fixtures).
if (
self.status == "Closed"
and self.org_id
and not getattr(self, "_approval_gate_skip", False)
):
old_status = None
if self.pk:
try:
old_status = (
type(self).objects.only("status").get(pk=self.pk).status
)
except type(self).DoesNotExist:
old_status = None
if old_status != "Closed":
from cases.approvals import Approval, find_matching_rule
# The close gate is `cases.approvals.close_refusal`, the rule the
# detail PUT/PATCH and the board move call too, so the bulk path that
# calls this method cannot drift from them. It judges the transition
# against the stored row: `self` already carries the incoming values.
# An unsaved case has no stored row, so it is judged as a create.
from cases.approvals import close_refusal
rule = find_matching_rule(self, trigger_event="pre_close")
if rule is not None:
has_approved = Approval.objects.filter(
case_id=self.pk, rule=rule, state="approved"
).exists()
if not has_approved:
errors["status"] = _(
"An approval is required before this case can be "
"closed (rule: %(rule)s)."
) % {"rule": rule.name}
stored = type(self).objects.filter(pk=self.pk).first() if self.pk else None
refusal = close_refusal(
stored,
status=self.status,
closed_on=self.closed_on,
priority=self.priority,
case_type=self.case_type,
)
if refusal:
errors.update(refusal)
# Parent/child guards.
if self.parent_id is not None:

View file

@ -10,8 +10,19 @@ Three endpoints over and above what `CaseDetailView` already exposes:
The link endpoint takes a row lock on both rows so two concurrent agents cannot
build a cycle. Cascade close honours ``Org.auto_close_children_on_parent_close``
as the default, and accepts ``cascade`` in the body to override.
Who may call them follows `cases.access`, the same as the ticket detail view:
* the tree needs read on the ticket asked about, and a node the caller cannot
read comes back redacted (see `_redacted`);
* linking needs write on the child and read on the new parent, and a parent
the caller cannot read answers exactly like one that does not exist;
* closing needs write on the ticket and on every descendant the cascade would
close, and each of them must pass `close_refusal`. Nothing is written unless
every one of them does.
"""
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db import transaction
from django.shortcuts import get_object_or_404
from django.utils import timezone
@ -20,6 +31,13 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import (
assert_case_read_access,
assert_case_write_access,
has_case_read_access,
has_case_write_access,
)
from cases.approvals import close_refusal
from cases.models import Case
from cases.parent_guards import check_parent_link
from common.models import Activity
@ -41,15 +59,41 @@ def _summary(case):
}
def _build_tree(case, depth=0):
"""Recurse to ``TREE_MAX_DEPTH``. Returns ``{...summary, children: [...]}``."""
node = _summary(case)
def _redacted(case):
"""Node payload for a ticket the caller may not open.
Kept in the tree rather than dropped, so a readable grandchild under a
hidden child still sits at its real depth instead of being lost or
re-parented. It carries the id (both clients key rows on it), ``status``
and ``is_active`` (both clients count the open descendants a cascading
close would take, and the close refuses when one of those is not the
caller's), and nothing else: no name, priority or assignees.
"""
return {
"id": str(case.id),
"name": None,
"restricted": True,
"status": case.status,
"is_active": case.is_active,
}
def _build_tree(case, profile, depth=0):
"""Recurse to ``TREE_MAX_DEPTH``. Returns ``{...summary, children: [...]}``.
Each node is the full summary when ``profile`` passes the read rule on it,
and `_redacted` otherwise.
"""
if has_case_read_access(profile, case):
node = _summary(case)
else:
node = _redacted(case)
if depth >= TREE_MAX_DEPTH:
node["children"] = []
node["truncated"] = True
return node
children = list(case.children.all().prefetch_related("assigned_to"))
node["children"] = [_build_tree(c, depth + 1) for c in children]
node["children"] = [_build_tree(c, profile, depth + 1) for c in children]
return node
@ -73,6 +117,9 @@ class CaseTreeView(APIView):
case = get_object_or_404(
Case.objects.prefetch_related("assigned_to"), id=pk, org=org
)
# The same answer the ticket detail GET gives: 404 outside the org,
# 403 for a ticket in it that the caller may not open.
assert_case_read_access(request.profile, case)
# Return the root of the visible tree: walk up to the highest ancestor
# in the same org so a child URL still shows the full incident.
root = case
@ -83,7 +130,7 @@ class CaseTreeView(APIView):
seen.add(root.parent_id)
root = root.parent
return Response(
{"root": _build_tree(root), "focus_id": str(case.id)},
{"root": _build_tree(root, request.profile), "focus_id": str(case.id)},
status=status.HTTP_200_OK,
)
@ -100,6 +147,9 @@ class CaseLinkParentView(APIView):
case = Case.objects.select_for_update().filter(id=pk, org=org).first()
if case is None:
return Response({"detail": "Not found."}, status=status.HTTP_404_NOT_FOUND)
# Moving a ticket in or out of a tree is a change to it, so it takes
# the write rule, as the detail PUT does. Unlinking included.
assert_case_write_access(request.profile, case)
former_parent_id = case.parent_id
@ -124,8 +174,17 @@ class CaseLinkParentView(APIView):
status=status.HTTP_200_OK,
)
parent = Case.objects.select_for_update().filter(id=parent_id, org=org).first()
if parent is None:
try:
parent = (
Case.objects.select_for_update().filter(id=parent_id, org=org).first()
)
except (DjangoValidationError, ValueError, TypeError):
# Not a UUID, so it names no case: the same answer as a missing one.
parent = None
# A parent the caller may not open answers exactly like a missing one.
# Otherwise the link would confirm the ticket exists, return its name,
# and put the caller's ticket under somebody else's.
if parent is None or not has_case_read_access(request.profile, parent):
return Response(
{"parent_id": "Parent case not found in this organization."},
status=status.HTTP_400_BAD_REQUEST,
@ -169,17 +228,59 @@ class CaseLinkParentView(APIView):
)
def _open_descendants(case, out=None):
"""Collect all open (status != Closed) descendants, breadth-first."""
def _open_descendants(case, out=None, seen=None):
"""Collect all open (status != Closed) active descendants, depth-first.
Recursion goes through closed children too, so an open grandchild under a
closed child is still collected. ``seen`` stops the walk on a stored
cycle, which would otherwise recurse until the worker fell over. Rows are
locked for the length of the caller's transaction.
"""
if out is None:
out = []
for child in case.children.all():
if seen is None:
seen = {case.id}
for child in case.children.select_for_update().prefetch_related("assigned_to"):
if child.id in seen:
continue
seen.add(child.id)
if child.status != "Closed" and child.is_active:
out.append(child)
_open_descendants(child, out)
_open_descendants(child, out, seen)
return out
def _refused(errors):
"""The 400 a refused close answers.
``errors`` is `close_refusal`'s ``{field: message}`` for the ticket itself,
answered in `CaseMoveView`'s per-field shape, or a sentence about the
cascade, which is about no one field and so rides as a plain string. Both
clients render either shape.
"""
if isinstance(errors, dict):
errors = {field: [msg] for field, msg in errors.items()}
return Response(
{"error": True, "errors": errors}, status=status.HTTP_400_BAD_REQUEST
)
def _close(case, today):
"""Close one ticket the way the ordinary close path leaves it.
``closed_on`` is today. ``resolved_at`` is stamped by the
``case_pre_save_stamp_resolved_at`` signal on the transition into Closed,
and is listed in ``update_fields`` so that stamp is saved.
"""
case.status = "Closed"
case.closed_on = today
case.save(update_fields=["status", "closed_on", "resolved_at", "updated_at"])
def _ticket_count(n):
return f"{n} linked ticket{'' if n == 1 else 's'}"
class CaseCloseWithChildrenView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
@ -189,6 +290,7 @@ class CaseCloseWithChildrenView(APIView):
case = Case.objects.select_for_update().filter(id=pk, org=org).first()
if case is None:
return Response({"detail": "Not found."}, status=status.HTTP_404_NOT_FOUND)
assert_case_write_access(request.profile, case)
if case.status == "Duplicate":
return Response(
{"detail": "Cannot close a merged case."},
@ -202,52 +304,72 @@ class CaseCloseWithChildrenView(APIView):
cascade = bool(cascade_override)
today = timezone.localdate()
# Close the parent first.
if case.status != "Closed":
case.status = "Closed"
case.closed_on = today
if not case.resolved_at:
from django.utils import timezone as _tz
case.resolved_at = _tz.now()
case.save(
update_fields=[
"status",
"closed_on",
"resolved_at",
"updated_at",
]
# Every check runs before anything is written, so a refusal leaves the
# whole tree as it was. The ticket itself takes the same close gate
# as a PATCH; `close_refusal` passes it when it is already Closed.
refusal = close_refusal(
case,
status="Closed",
closed_on=today,
priority=case.priority,
case_type=case.case_type,
)
if refusal:
return _refused(refusal)
descendants = _open_descendants(case) if cascade else []
# A cascade closes each descendant, so each one needs the write rule.
# Counted, never named: some of them the caller cannot even open.
not_writable = [
d for d in descendants if not has_case_write_access(request.profile, d)
]
if not_writable:
n = len(not_writable)
return _refused(
f"{_ticket_count(n)} under this one {'is' if n == 1 else 'are'} "
"not yours to close, so nothing was closed. Close this ticket "
"on its own, or ask an admin to close them all.",
)
cascaded = []
if cascade:
descendants = _open_descendants(case)
for child in descendants:
child.status = "Closed"
child.closed_on = today
if not child.resolved_at:
from django.utils import timezone as _tz
# Named: every descendant left here is one the caller may write, and
# so may open.
gated = [
d
for d in descendants
if close_refusal(
d,
status="Closed",
closed_on=today,
priority=d.priority,
case_type=d.case_type,
)
]
if gated:
names = ", ".join(f'"{d.name}"' for d in gated)
return _refused(
f"{_ticket_count(len(gated))} under this one need an approval "
f"before closing ({names}), so nothing was closed.",
)
child.resolved_at = _tz.now()
child.save(
update_fields=[
"status",
"closed_on",
"resolved_at",
"updated_at",
]
)
_record(
child,
"PARENT_CLOSED_CASCADE",
{
"parent_id": str(case.id),
"acted_via_cascade": True,
"resolution_comment": resolution_comment[:1000],
},
actor=request.profile,
)
cascaded.append(str(child.id))
if case.status != "Closed":
_close(case, today)
cascaded = []
for child in descendants:
_close(child, today)
_record(
child,
"PARENT_CLOSED_CASCADE",
{
"parent_id": str(case.id),
"acted_via_cascade": True,
"resolution_comment": resolution_comment[:1000],
},
actor=request.profile,
)
cascaded.append(str(child.id))
return Response(
{

View file

@ -1,8 +1,10 @@
from django.db.models import Sum
from django.db.models import Count, Q, Sum
from rest_framework import serializers
from accounts.access import has_account_access
from accounts.serializer import AccountPickerSerializer
from cases.approvals import Approval, ApprovalRule
from cases.access import has_case_read_access, visible_cases_qs
from cases.approvals import Approval, ApprovalRule, close_refusal
from cases.models import (
Case,
CasePipeline,
@ -31,6 +33,29 @@ from contacts.serializer import ContactLinkSerializer
# - created_on_arrow (frontend computes its own humanized timestamps)
# The refusal `/link/` gives, so the two doors into a tree answer alike.
PARENT_NOT_FOUND = "Parent case not found in this organization."
def parent_access_context(profile, cases):
"""`CaseSerializer` context: which of these cases' parents ``profile`` may open.
One query for the whole batch, so a page of tickets costs the same whether
it has one parent or twenty-five. Pass the same list or queryset object the
serializer is then given: iterating a queryset here fills its result cache,
which the serializer reads instead of querying again.
"""
parent_ids = {c.parent_id for c in cases if c.parent_id}
readable = set()
if parent_ids:
readable = set(
visible_cases_qs(profile)
.filter(id__in=parent_ids)
.values_list("id", flat=True)
)
return {"readable_parent_ids": readable}
class CaseSerializer(serializers.ModelSerializer):
# `{id, name}` only. Seeing this record is not access to its account, and
# both clients read only these two; the rest is on `/api/accounts/<id>/`.
@ -58,10 +83,33 @@ class CaseSerializer(serializers.ModelSerializer):
time_summary = serializers.SerializerMethodField()
def get_parent_summary(self, obj):
"""The parent's name and status, or only its id when the viewer may
not open it.
Reading a child is not reading its parent. This used to hand anyone
who could open the child the parent's name and status, so a member
assigned one sub-ticket learned the subject of a ticket the detail view
refuses them. Which parents are readable comes from
`parent_access_context`; a serializer built without it redacts every
parent, since not knowing who is asking is not permission. The redacted
shape matches the one `/tree/` returns for a hidden node.
"""
if not obj.parent_id:
return None
if obj.parent_id not in self.context.get("readable_parent_ids", ()):
return {
"id": str(obj.parent_id),
"name": None,
"status": None,
"restricted": True,
}
p = obj.parent
return {"id": str(p.id), "name": p.name, "status": p.status}
return {
"id": str(p.id),
"name": p.name,
"status": p.status,
"restricted": False,
}
def get_child_count(self, obj):
# Prefer prefetched count when callers annotated it.
@ -152,10 +200,19 @@ class CaseCreateSerializer(serializers.ModelSerializer):
def __init__(self, *args, **kwargs):
request_obj = kwargs.pop("request_obj", None)
super().__init__(*args, **kwargs)
self.profile = request_obj.profile
self.user = request_obj.user
self.org = request_obj.profile.org
# Make account read-only on updates (can only be set on creation)
if self.instance:
self.fields["account"].read_only = True
else:
# An id that matches no account at all reads the same as one the
# caller may not open; see `validate_account`.
self.fields["account"].error_messages["does_not_exist"] = "No such account."
# An id that matches no case reads the same as one in another org or
# one the caller may not open; see `validate_parent`.
self.fields["parent"].error_messages["does_not_exist"] = PARENT_NOT_FOUND
def validate_account(self, account):
"""An account belonging to another org is not a valid link.
@ -168,8 +225,20 @@ class CaseCreateSerializer(serializers.ModelSerializer):
was both a cross-tenant write and a cross-tenant read. `validate_parent`
below already guarded the other FK on this serializer; this one had
been missed.
Nor is an account in this org that the caller may not open. Linking
one attached the ticket to a company the member cannot see, so the
rule is `has_account_access`, the one the account detail view applies.
An unknown id, another org's account and a hidden one all get the same
message, so a response never confirms that a hidden account exists.
`account` is read-only once the ticket exists (see `__init__`), so this
runs on create only.
"""
if account is not None and account.org_id != self.org.id:
if account is None:
return account
if account.org_id != self.org.id or not has_account_access(
self.profile, self.user, account
):
raise serializers.ValidationError("No such account.")
return account
@ -189,6 +258,9 @@ class CaseCreateSerializer(serializers.ModelSerializer):
case that is already Closed can be edited without re-approving, which
is why this compares against the stored status instead of just looking
at the incoming one.
The rule itself is `cases.approvals.close_refusal`, which the board
move (`CaseMoveView.patch`) calls too, so the two cannot drift apart.
"""
attrs = super().validate(attrs)
@ -205,80 +277,58 @@ class CaseCreateSerializer(serializers.ModelSerializer):
if refusal:
raise serializers.ValidationError({"parent": refusal})
if new_status != "Closed":
return attrs
old_status = getattr(self.instance, "status", None)
if old_status == "Closed":
return attrs
closed_on = attrs.get("closed_on", getattr(self.instance, "closed_on", None))
if not closed_on:
raise serializers.ValidationError(
{"closed_on": "Closed date is required when closing a case"}
)
if self.instance is not None:
from cases.approvals import Approval, find_matching_rule
# A rule matches on priority, case_type and team, so evaluate it
# against the values this request is setting, not the stored ones,
# or a caller could re-target the case out of the rule and close it
# in the same PATCH. The instance is restored either way; nothing
# here is saved.
probe = self.instance
saved = (probe.priority, probe.case_type)
probe.priority = attrs.get("priority", probe.priority)
probe.case_type = attrs.get("case_type", probe.case_type)
try:
rule = find_matching_rule(probe, trigger_event="pre_close")
finally:
probe.priority, probe.case_type = saved
if (
rule is not None
and not Approval.objects.filter(
case_id=self.instance.pk, rule=rule, state="approved"
).exists()
):
raise serializers.ValidationError(
{
"status": (
"An approval is required before this case can be "
f"closed (rule: {rule.name})."
)
}
)
refusal = close_refusal(
self.instance,
status=new_status,
closed_on=attrs.get("closed_on", getattr(self.instance, "closed_on", None)),
priority=attrs.get("priority", getattr(self.instance, "priority", None)),
case_type=attrs.get("case_type", getattr(self.instance, "case_type", None)),
)
if refusal:
raise serializers.ValidationError(refusal)
return attrs
def validate_name(self, name):
if self.instance:
if (
Case.objects.filter(name__iexact=name, org=self.org)
.exclude(id=self.instance.id)
.exists()
):
raise serializers.ValidationError("Case already exists with this name")
"""Refuse a name one of the caller's own tickets already carries.
else:
if Case.objects.filter(name__iexact=name, org=self.org).exists():
raise serializers.ValidationError("Case already exists with this name")
Ticket names carry no DB constraint, so this is a courtesy against
filing the same ticket twice, and it looks only at tickets the caller
may open (`visible_cases_qs`), as the CSV import does. Checked across
the whole org it was an oracle: the refusal for a name held only by a
ticket the caller cannot open confirmed that ticket existed.
"""
clash = visible_cases_qs(self.profile).filter(name__iexact=name)
if self.instance:
clash = clash.exclude(id=self.instance.id)
if clash.exists():
raise serializers.ValidationError("Case already exists with this name")
return name
def validate_parent(self, parent):
"""Cross-org link prevention only.
"""A parent the caller may name: in this org and readable by them.
The rest of the linking rules moved to `validate()`, which is the
first place with both the incoming `status` and the record being
moved. This one stays a field validator because it is the check that
decides whether `parent` may be used at all, and because the caller
should get it against the `parent` field either way.
The read rule is the one `/link/` applies: a parent the caller cannot
open answers exactly like one that does not exist. Checked by org
alone, this was the way round the link endpoint: it put the caller's
ticket under somebody else's and, since a missing id is refused and a
hidden one was not, confirmed the hidden ticket existed. Re-sending the
parent a ticket already has is not a new link, so it is let through
even when that parent is hidden from the caller.
"""
if parent is None:
return parent
if parent.org_id != self.org.id:
raise serializers.ValidationError(
"Parent case must belong to the same organization."
)
if self.instance is not None and parent.id == self.instance.parent_id:
return parent
if parent.org_id != self.org.id or not has_case_read_access(
self.profile, parent
):
raise serializers.ValidationError(PARENT_NOT_FOUND)
return parent
class Meta:
@ -332,8 +382,21 @@ class CaseCommentEditSwaggerSerializer(serializers.Serializer):
# ============================================
def _visible_case_count(serializer, **lookup):
"""How many cases matching ``lookup`` the requester may open.
``case_count`` once counted every case in the org, so a member whose board
showed three cards read a count that included tickets they cannot open. It
follows ``visible_cases_qs`` now, the rule the board and the list use. The
request has to be in the serializer context: a missing one is a KeyError,
never an unscoped count.
"""
profile = serializer.context["request"].profile
return visible_cases_qs(profile).filter(**lookup).count()
class CaseStageSerializer(serializers.ModelSerializer):
"""Serializer for case stages."""
"""Serializer for case stages. Needs ``request`` in its context."""
case_count = serializers.SerializerMethodField()
@ -354,11 +417,12 @@ class CaseStageSerializer(serializers.ModelSerializer):
read_only_fields = ("id", "created_at", "updated_at", "org")
def get_case_count(self, obj):
return obj.cases.count()
return _visible_case_count(self, stage=obj)
class CasePipelineSerializer(serializers.ModelSerializer):
"""Serializer for case pipelines with nested stages."""
"""Serializer for case pipelines with nested stages. Needs ``request`` in
its context."""
stages = CaseStageSerializer(many=True, read_only=True)
stage_count = serializers.SerializerMethodField()
@ -384,11 +448,17 @@ class CasePipelineSerializer(serializers.ModelSerializer):
return obj.stages.count()
def get_case_count(self, obj):
return Case.objects.filter(stage__pipeline=obj).count()
return _visible_case_count(self, stage__pipeline=obj)
class CasePipelineListSerializer(serializers.ModelSerializer):
"""Simplified pipeline serializer for lists."""
"""Simplified pipeline serializer for lists.
Both counts are read from annotations, so every queryset serialized here
must come through ``with_counts``: one query for the whole list, however
many pipelines. A pipeline without them raises AttributeError, which is
louder than a count that silently ignores who is asking.
"""
stage_count = serializers.SerializerMethodField()
case_count = serializers.SerializerMethodField()
@ -406,11 +476,23 @@ class CasePipelineListSerializer(serializers.ModelSerializer):
"created_at",
]
@staticmethod
def with_counts(pipelines, profile):
# The case join repeats a stage once per case in it, hence distinct on
# the stage count. A case sits in one stage, so it is counted once.
return pipelines.annotate(
stage_count=Count("stages", distinct=True),
case_count=Count(
"stages__cases",
filter=Q(stages__cases__in=visible_cases_qs(profile).values("pk")),
),
)
def get_stage_count(self, obj):
return obj.stages.count()
return obj.stage_count
def get_case_count(self, obj):
return Case.objects.filter(stage__pipeline=obj).count()
return obj.case_count
class CaseKanbanCardSerializer(serializers.ModelSerializer):

View file

@ -5,10 +5,10 @@ beyond read-only lookups; `commit_rows` writes inside a transaction. Both phases
re-run validation so the commit endpoint is safe even if called directly.
Reference fields (`account_name`, `contact_emails`, `assigned_emails`,
`team_names`) must resolve within the caller's org, and `contact_emails` only
among the contacts the importer may open; missing references are reported as
row errors rather than silently dropped. Tags are auto-created at
commit if not present.
`team_names`) must resolve within the caller's org, and `account_name` and
`contact_emails` only among the records the importer may open; missing
references are reported as row errors rather than silently dropped. Tags are
auto-created at commit if not present.
All reference lookups are bulk-prefetched once per call (one SELECT per
reference type), not per-row, a 5000-row file with five reference columns
@ -27,7 +27,8 @@ from typing import Any, Iterable
from django.db import transaction
from django.db.models.functions import Lower
from accounts.models import Account
from accounts.access import visible_accounts_qs
from cases.access import visible_cases_qs
from cases.models import Case
from common.models import Profile, Tags, Teams
from common.utils import CASE_TYPE, PRIORITY_CHOICE, STATUS_CHOICE
@ -280,19 +281,27 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org, profile) -> _
for team in _split_multi(record.get("team_names", "")):
team_names.add(team.lower())
# Only tickets the importer may open. Ticket names carry no DB constraint,
# so this check is a courtesy against importing the same ticket twice, not
# a rule the database would enforce. Checked across the whole org it was
# an oracle: "A ticket with this name already exists" for a name held only
# by a ticket the importer cannot open confirmed that ticket existed.
existing_case_names: set[str] = set()
if candidate_names:
existing_case_names = set(
Case.objects.filter(org=org)
visible_cases_qs(profile)
.annotate(name_lower=Lower("name"))
.filter(name_lower__in=candidate_names)
.values_list("name_lower", flat=True)
)
# Only accounts the importer may open, for the reason given for contacts
# below: "No account named ..." for a name nobody holds and a valid row
# for one held by a hidden account told the importer the hidden one existed.
accounts: dict[str, str] = {}
if account_names:
for pk, name_lower in (
Account.objects.filter(org=org)
visible_accounts_qs(profile, profile.user)
.annotate(name_lower=Lower("name"))
.filter(name_lower__in=account_names)
.values_list("id", "name_lower")
@ -416,7 +425,11 @@ def _validate_and_build(
account_id = refs.accounts.get(account_name.lower())
if account_id is None:
errors.append(
RowError(idx, "account_name", f"No account named '{account_name}'")
RowError(
idx,
"account_name",
f"No account you can open is named '{account_name}'",
)
)
contact_ids: list[str] = []

View file

@ -0,0 +1,249 @@
"""A member may link a ticket only to an account they can open (D38).
`validate_account` refused another org's account and nothing else, so a member
could attach a ticket to any account in the org, including one the account
detail view would answer 403 for. The ticket CSV import went further: it
resolved `account_name` across the whole org, so its "No account named ..."
row error said which hidden accounts existed.
Both now ask `accounts.access`: the API through `has_account_access`, the
import and the ticket form's picker through `visible_accounts_qs`. Every
refusal reads the same as an account that does not exist.
"""
import csv
import io
import uuid
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
from accounts.models import Account
from cases.models import Case
from conftest import rls_org
CASES_URL = "/api/cases/"
PREVIEW = "/api/cases/import/preview/"
COMMIT = "/api/cases/import/commit/"
def _account(org, name, created_by=None):
account = Account.objects.create(name=name, org=org)
Account.objects.filter(pk=account.pk).update(created_by=created_by)
return account
@pytest.fixture
def hidden(org_a, admin_user):
return _account(org_a, "Hidden Holdings", created_by=admin_user)
@pytest.fixture
def assigned(org_a, admin_user, user_profile):
account = _account(org_a, "Assigned Industries", created_by=admin_user)
account.assigned_to.add(user_profile)
return account
@pytest.fixture
def foreign(org_b, user_b):
with rls_org(org_b):
return _account(org_b, "Other Org Ltd", created_by=user_b)
@pytest.fixture
def superuser(regular_user):
"""The `user_client` caller, with a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
@pytest.fixture
def importer(user_profile):
user_profile.has_sales_access = True
user_profile.save(update_fields=["has_sales_access"])
return user_profile
def _create(client, name, account_id):
return client.post(
CASES_URL,
{
"name": name,
"status": "New",
"priority": "Normal",
"account": str(account_id),
},
format="json",
)
def _refusal(response):
assert response.status_code == 400, response.content
return response.json()["errors"]["account"]
@pytest.mark.django_db
class TestTicketApiAccountLink:
def test_member_may_link_an_account_they_are_assigned_to(
self, user_client, assigned
):
response = _create(user_client, "Assigned link", assigned.id)
assert response.status_code == 200, response.content
assert Case.objects.get(name="Assigned link").account_id == assigned.id
def test_member_may_link_an_account_they_created(
self, user_client, org_a, regular_user
):
mine = _account(org_a, "Mine Co", created_by=regular_user)
response = _create(user_client, "Own link", mine.id)
assert response.status_code == 200, response.content
assert Case.objects.get(name="Own link").account_id == mine.id
def test_hidden_account_reads_like_an_unknown_id(self, user_client, hidden):
hidden_refusal = _refusal(_create(user_client, "Hidden link", hidden.id))
unknown_refusal = _refusal(_create(user_client, "Unknown link", uuid.uuid4()))
assert hidden_refusal == unknown_refusal == ["No such account."]
assert not Case.objects.filter(
name__in=["Hidden link", "Unknown link"]
).exists()
def test_admin_may_link_any_account_in_the_org(self, admin_client, hidden):
response = _create(admin_client, "Admin link", hidden.id)
assert response.status_code == 200, response.content
assert Case.objects.get(name="Admin link").account_id == hidden.id
def test_superuser_may_link_any_account_in_the_org(
self, superuser, user_client, hidden
):
response = _create(user_client, "Superuser link", hidden.id)
assert response.status_code == 200, response.content
assert Case.objects.get(name="Superuser link").account_id == hidden.id
@pytest.mark.parametrize("caller", ["member", "admin", "superuser"])
def test_other_org_account_is_refused_for_everyone(
self, caller, request, user_client, admin_client, foreign
):
if caller == "superuser":
request.getfixturevalue("superuser")
client = admin_client if caller == "admin" else user_client
assert _refusal(_create(client, "Foreign link", foreign.id)) == [
"No such account."
]
assert not Case.objects.filter(name="Foreign link").exists()
def test_account_cannot_be_changed_after_creation(
self, user_client, org_a, regular_user, user_profile, assigned, hidden
):
case = Case.objects.create(
name="Existing", status="New", priority="Normal", org=org_a
)
Case.objects.filter(pk=case.pk).update(created_by=regular_user)
case.assigned_to.add(user_profile)
for target in (assigned, hidden):
response = user_client.patch(
f"{CASES_URL}{case.id}/", {"account": str(target.id)}, format="json"
)
assert response.status_code == 200, response.content
case.refresh_from_db()
assert case.account_id is None
def test_picker_offers_exactly_what_the_save_path_accepts(
self, user_client, assigned, hidden
):
offered = {
row["id"] for row in user_client.get(CASES_URL).json()["accounts_list"]
}
assert str(assigned.id) in offered
assert str(hidden.id) not in offered
def test_picker_offers_a_superuser_every_account(
self, superuser, user_client, assigned, hidden
):
offered = {
row["id"] for row in user_client.get(CASES_URL).json()["accounts_list"]
}
assert {str(assigned.id), str(hidden.id)} <= offered
def _csv(rows):
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(["name", "status", "priority", "account_name"])
writer.writerows(rows)
return SimpleUploadedFile(
"tickets.csv", buf.getvalue().encode("utf-8"), content_type="text/csv"
)
def _messages(body):
return {e["row"]: e["message"] for e in body["errors"]}
@pytest.mark.django_db
class TestTicketImportAccountVisibility:
def test_hidden_and_missing_accounts_get_the_same_error(
self, user_client, importer, hidden
):
response = user_client.post(
PREVIEW,
{
"file": _csv(
[
["Hidden", "New", "High", "Hidden Holdings"],
["Missing", "New", "High", "Nobody Inc"],
]
)
},
format="multipart",
)
assert response.status_code == 200, response.content
body = response.json()
assert body["summary"]["valid"] == 0
messages = _messages(body)
assert messages[1].replace("Hidden Holdings", "NAME") == messages[2].replace(
"Nobody Inc", "NAME"
)
assert {e["field"] for e in body["errors"]} == {"account_name"}
def test_visible_account_resolves_case_insensitively(
self, user_client, importer, assigned
):
response = user_client.post(
COMMIT,
{"file": _csv([["Imported", "New", "High", "assigned INDUSTRIES"]])},
format="multipart",
)
assert response.status_code == 200, response.content
assert response.json()["created"] == 1
assert Case.objects.get(name="Imported").account_id == assigned.id
def test_commit_refuses_a_hidden_account(self, user_client, importer, hidden):
response = user_client.post(
COMMIT,
{"file": _csv([["Sneaky", "New", "High", "Hidden Holdings"]])},
format="multipart",
)
assert response.json()["created"] == 0
assert not Case.objects.filter(name="Sneaky").exists()
def test_admin_resolves_an_account_a_member_cannot(self, admin_client, hidden):
response = admin_client.post(
COMMIT,
{"file": _csv([["By admin", "New", "High", "Hidden Holdings"]])},
format="multipart",
)
assert response.json()["created"] == 1, response.content
assert Case.objects.get(name="By admin").account_id == hidden.id
def test_superuser_resolves_an_account_a_member_cannot(
self, superuser, user_client, importer, hidden
):
response = user_client.post(
COMMIT,
{"file": _csv([["By superuser", "New", "High", "Hidden Holdings"]])},
format="multipart",
)
assert response.json()["created"] == 1, response.content
assert Case.objects.get(name="By superuser").account_id == hidden.id

View file

@ -0,0 +1,134 @@
"""A ticket-name clash is judged among tickets the caller can open (D48).
Ticket names carry no DB constraint (``Case.name`` is a plain ``CharField``
with no unique index or ``UniqueConstraint``), so refusing a duplicate name is
a courtesy against filing the same ticket twice. Both places that offered it
checked the whole org: the CSV import answered "A ticket with this name
already exists" and ``POST /api/cases/`` answered "Case already exists with
this name" for a name held only by a ticket the caller cannot open, which
confirmed that ticket existed. Both now look only at ``visible_cases_qs``.
"""
import csv
import io
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
from cases.models import Case
PREVIEW = "/api/cases/import/preview/"
IMPORT_CLASH = "A ticket with this name already exists"
CREATE_CLASH = "Case already exists with this name"
def _csv(names):
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(["name", "status", "priority"])
for name in names:
writer.writerow([name, "New", "High"])
return SimpleUploadedFile(
"tickets.csv", buf.getvalue().encode("utf-8"), content_type="text/csv"
)
@pytest.fixture
def importer(user_profile):
"""A member with sales access, which `can_mass_import` admits."""
user_profile.has_sales_access = True
user_profile.save(update_fields=["has_sales_access"])
return user_profile
@pytest.fixture
def hidden(org_a, admin_user):
return Case.objects.create(
name="Hidden outage",
status="New",
priority="High",
org=org_a,
created_by=admin_user,
)
@pytest.fixture
def mine(org_a, regular_user):
return Case.objects.create(
name="My outage",
status="New",
priority="High",
org=org_a,
created_by=regular_user,
)
def _preview_errors(client, names):
response = client.post(PREVIEW, {"file": _csv(names)}, format="multipart")
assert response.status_code == 200, response.content
return response.json()
@pytest.mark.django_db
class TestImportNameClash:
def test_hidden_ticket_name_is_not_reported(self, user_client, importer, hidden):
body = _preview_errors(user_client, ["hidden OUTAGE"])
assert body["errors"] == []
assert body["summary"]["valid"] == 1
def test_visible_ticket_name_is_reported(self, user_client, importer, mine):
body = _preview_errors(user_client, ["my outage"])
assert body["summary"]["valid"] == 0
assert [(e["field"], e["message"]) for e in body["errors"]] == [
("name", IMPORT_CLASH)
]
def test_admin_sees_every_clash_in_the_org(self, admin_client, hidden):
body = _preview_errors(admin_client, ["Hidden outage"])
assert [e["message"] for e in body["errors"]] == [IMPORT_CLASH]
def _create(client, name):
return client.post(
"/api/cases/",
{"name": name, "status": "New", "priority": "High"},
format="json",
)
@pytest.mark.django_db
class TestCreateNameClash:
def test_hidden_ticket_name_does_not_refuse_the_create(
self, user_client, org_a, hidden
):
response = _create(user_client, "Hidden outage")
assert response.status_code == 200, response.content
assert Case.objects.filter(org=org_a, name="Hidden outage").count() == 2
def test_visible_ticket_name_refuses_the_create(self, user_client, org_a, mine):
response = _create(user_client, "MY OUTAGE")
assert response.status_code == 400, response.content
assert response.json()["errors"]["name"] == [CREATE_CLASH]
assert Case.objects.filter(org=org_a, name__iexact="my outage").count() == 1
def test_rename_onto_a_visible_name_is_refused(
self, user_client, org_a, regular_user, mine
):
other = Case.objects.create(
name="Second",
status="New",
priority="High",
org=org_a,
created_by=regular_user,
)
response = user_client.patch(
f"/api/cases/{other.id}/", {"name": "My outage"}, format="json"
)
assert response.status_code == 400, response.content
assert response.json()["errors"]["name"] == [CREATE_CLASH]
def test_keeping_its_own_name_is_not_a_clash(self, user_client, mine):
response = user_client.patch(
f"/api/cases/{mine.id}/", {"name": "My outage"}, format="json"
)
assert response.status_code == 200, response.content

View file

@ -1,6 +1,6 @@
"""The ticket import preview does not reveal whether a contact exists.
`_can_import` admits members with `has_sales_access`, not only admins. The
`can_mass_import` admits members with `has_sales_access`, not only admins. The
preview resolved `contact_emails` across the whole org, so such a member got
"No contact with email ..." for an address nobody holds and a valid row for
one held by a contact they cannot open: an existence oracle for any address.

View file

@ -0,0 +1,183 @@
"""A board move into Closed takes the close gate PATCH takes (D49).
`CaseMoveView.patch` set ``status="Closed"`` itself, directly or through a
stage's ``maps_to_status``, and skipped the gate `CaseCreateSerializer.validate`
applies on the edit path. With a ``pre_close`` approval rule armed, PATCH
refused the close and a drag on the board closed the ticket anyway, with no
approval and no ``closed_on``. Both paths now call
``cases.approvals.close_refusal``: a refused move answers what PATCH answers
and writes nothing, an allowed one stamps ``closed_on``, and moving back out of
Closed clears what PATCH clears.
"""
import datetime
import pytest
from django.utils import timezone
from cases.approvals import Approval, ApprovalRule
from cases.models import Case, CasePipeline, CaseStage
def _move(client, case, **body):
return client.patch(f"/api/cases/{case.id}/move/", body, format="json")
def _patch(client, case, **body):
return client.patch(f"/api/cases/{case.id}/", body, format="json")
def _case(org, user, name="Printer on fire", **fields):
fields.setdefault("status", "New")
return Case.objects.create(
name=name, priority="High", org=org, created_by=user, **fields
)
@pytest.fixture
def rule(org_a):
return ApprovalRule.objects.create(
org=org_a, name="High needs sign-off", is_active=True, match_priority="High"
)
@pytest.fixture
def closing_stage(org_a, admin_user):
pipeline = CasePipeline.objects.create(
name="Support", org=org_a, created_by=admin_user
)
return CaseStage.objects.create(
pipeline=pipeline,
name="Resolved",
order=1,
stage_type="closed",
maps_to_status="Closed",
org=org_a,
created_by=admin_user,
)
def _snapshot(case):
case.refresh_from_db()
return (
case.status,
case.stage_id,
case.closed_on,
case.resolved_at,
case.kanban_order,
)
@pytest.mark.django_db
class TestRefusedLikePatch:
def test_status_move_is_refused_with_the_patch_answer(
self, admin_client, admin_profile, admin_user, org_a, rule
):
case = _case(org_a, admin_user)
before = _snapshot(case)
patched = _patch(
admin_client,
case,
status="Closed",
closed_on=timezone.localdate().isoformat(),
)
moved = _move(admin_client, case, status="Closed")
assert patched.status_code == 400, patched.content
assert moved.status_code == patched.status_code, moved.content
assert moved.json() == patched.json()
assert moved.json()["errors"] == {
"status": [
"An approval is required before this case can be closed "
"(rule: High needs sign-off)."
]
}
assert _snapshot(case) == before
def test_stage_move_that_maps_to_closed_is_refused(
self, admin_client, admin_profile, admin_user, org_a, rule, closing_stage
):
case = _case(org_a, admin_user)
before = _snapshot(case)
moved = _move(admin_client, case, stage_id=str(closing_stage.id))
assert moved.status_code == 400, moved.content
assert list(moved.json()["errors"]) == ["status"]
assert _snapshot(case) == before
@pytest.mark.django_db
class TestAllowedClose:
def test_approved_close_goes_through_and_stamps_closed_on(
self, admin_client, admin_profile, admin_user, org_a, rule
):
case = _case(org_a, admin_user)
Approval.objects.create(
org=org_a,
case=case,
rule=rule,
requested_by=admin_profile,
state="approved",
)
moved = _move(admin_client, case, status="Closed")
assert moved.status_code == 200, moved.content
case.refresh_from_db()
assert case.status == "Closed"
assert case.closed_on == timezone.localdate()
assert case.resolved_at is not None
def test_close_without_a_rule_stamps_closed_on(
self, admin_client, admin_profile, admin_user, org_a, closing_stage
):
case = _case(org_a, admin_user)
moved = _move(admin_client, case, stage_id=str(closing_stage.id))
assert moved.status_code == 200, moved.content
case.refresh_from_db()
assert (case.status, case.stage_id) == ("Closed", closing_stage.id)
assert case.closed_on == timezone.localdate()
def test_a_case_already_closed_is_not_re_approved(
self, admin_client, admin_profile, admin_user, org_a, rule
):
closed_on = timezone.localdate() - datetime.timedelta(days=3)
case = _case(org_a, admin_user, status="Closed", closed_on=closed_on)
moved = _move(admin_client, case, status="Closed")
assert moved.status_code == 200, moved.content
case.refresh_from_db()
assert case.closed_on == closed_on
@pytest.mark.django_db
def test_reopen_by_move_clears_what_patch_clears(
admin_client, admin_profile, admin_user, org_a
):
closed_on = timezone.localdate() - datetime.timedelta(days=2)
by_move = _case(org_a, admin_user, "A", status="Closed", closed_on=closed_on)
by_patch = _case(org_a, admin_user, "B", status="Closed", closed_on=closed_on)
for case in (by_move, by_patch):
case.refresh_from_db()
assert case.resolved_at is not None
assert _move(admin_client, by_move, status="Pending").status_code == 200
assert _patch(admin_client, by_patch, status="Pending").status_code == 200
for case in (by_move, by_patch):
case.refresh_from_db()
assert (by_move.status, by_move.closed_on, by_move.resolved_at) == (
by_patch.status,
by_patch.closed_on,
by_patch.resolved_at,
)
assert (by_move.status, by_move.closed_on, by_move.resolved_at) == (
"Pending",
None,
None,
)

View file

@ -0,0 +1,144 @@
"""The board move takes the ticket's write rule, exactly (D42).
`CaseMoveView.patch` rewrites a ticket's status, stage and kanban order. It used
to gate on ``is_org_admin(...) or request.user.is_superuser`` plus creator or
assignee, while `cases.access` gives a Django superuser nothing and
`CaseDetailView.patch` refuses them. So a superuser who is a plain member could
move, through the board, any ticket they could not edit through the detail
endpoint. These tests pin the move to `assert_case_write_access`.
"""
import pytest
from cases.models import Case, CaseWatcher
from conftest import rls_org
def _move(client, case, status="Assigned"):
return client.patch(
f"/api/cases/{case.id}/move/", {"status": status}, format="json"
)
@pytest.fixture
def superuser(regular_user):
"""The `user_client` caller, still a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
@pytest.fixture
def others_case(admin_user, org_a):
"""A ticket the `user_client` caller neither created nor was handed."""
return Case.objects.create(
name="Someone else's ticket",
status="New",
priority="Normal",
org=org_a,
created_by=admin_user,
)
def _assert_unchanged(case):
case.refresh_from_db()
assert case.status == "New"
class TestSuperuserPlainMember:
def test_refused_on_a_ticket_they_cannot_edit(
self, superuser, user_client, others_case
):
assert (
user_client.patch(
f"/api/cases/{others_case.id}/", {"status": "Assigned"}, format="json"
).status_code
== 403
)
response = _move(user_client, others_case)
assert response.status_code == 403, response.content
_assert_unchanged(others_case)
def test_allowed_on_a_ticket_they_created(
self, superuser, user_client, regular_user, org_a
):
case = Case.objects.create(
name="Mine",
status="New",
priority="Normal",
org=org_a,
created_by=regular_user,
)
response = _move(user_client, case)
assert response.status_code == 200, response.content
case.refresh_from_db()
assert case.status == "Assigned"
def test_allowed_on_a_ticket_they_are_assigned(
self, superuser, user_client, user_profile, others_case
):
others_case.assigned_to.add(user_profile)
response = _move(user_client, others_case)
assert response.status_code == 200, response.content
others_case.refresh_from_db()
assert others_case.status == "Assigned"
class TestOrdinaryRoles:
def test_admin_may_move_any_ticket(self, admin_client, regular_user, org_a):
case = Case.objects.create(
name="A member's ticket",
status="New",
priority="Normal",
org=org_a,
created_by=regular_user,
)
response = _move(admin_client, case)
assert response.status_code == 200, response.content
case.refresh_from_db()
assert case.status == "Assigned"
def test_member_with_write_access_may_move(
self, user_client, user_profile, others_case
):
others_case.assigned_to.add(user_profile)
response = _move(user_client, others_case)
assert response.status_code == 200, response.content
others_case.refresh_from_db()
assert others_case.status == "Assigned"
def test_member_without_write_access_is_refused(self, user_client, others_case):
response = _move(user_client, others_case)
assert response.status_code == 403, response.content
_assert_unchanged(others_case)
def test_watcher_reads_but_may_not_move(
self, user_client, user_profile, others_case, org_a
):
"""Watching is read access, not write access, on the board as elsewhere."""
CaseWatcher.objects.create(case=others_case, profile=user_profile, org=org_a)
assert user_client.get(f"/api/cases/{others_case.id}/").status_code == 200
response = _move(user_client, others_case)
assert response.status_code == 403, response.content
_assert_unchanged(others_case)
@pytest.mark.parametrize("caller", ["member", "admin", "superuser"])
def test_another_orgs_ticket_is_404_for_everyone(
caller, request, user_client, admin_client, user_b, org_b
):
if caller == "superuser":
request.getfixturevalue("superuser")
client = admin_client if caller == "admin" else user_client
with rls_org(org_b):
foreign = Case.objects.create(
name="Foreign",
status="New",
priority="Normal",
org=org_b,
created_by=user_b,
)
response = _move(client, foreign)
assert response.status_code == 404, response.content
with rls_org(org_b):
_assert_unchanged(foreign)

View file

@ -0,0 +1,46 @@
"""A merged ticket's redirect is behind the read rule (D45).
The ticket detail GET answered a merged duplicate's redirect before checking
read access, so any member got the duplicate's name and the id of the ticket
it was merged into.
"""
import pytest
from cases.models import Case
@pytest.fixture
def merged_pair(admin_user, org_a):
target = Case.objects.create(
name="Kept ticket",
status="New",
priority="Normal",
org=org_a,
created_by=admin_user,
)
duplicate = Case.objects.create(
name="Secret duplicate",
status="Closed",
priority="Normal",
org=org_a,
created_by=admin_user,
merged_into=target,
)
return duplicate, target
class TestMergedRedirect:
def test_member_who_cannot_open_it_learns_nothing(self, user_client, merged_pair):
duplicate, target = merged_pair
response = user_client.get(f"/api/cases/{duplicate.id}/")
assert response.status_code == 403
body = response.content.decode()
assert "Secret duplicate" not in body
assert str(target.id) not in body
def test_reader_gets_the_redirect(self, admin_client, merged_pair):
duplicate, target = merged_pair
response = admin_client.get(f"/api/cases/{duplicate.id}/")
assert response.status_code == 200
assert response.json()["redirect_to"] == str(target.id)

View file

@ -89,7 +89,7 @@ class TestCleanGuards:
@pytest.mark.django_db
class TestSerializer:
def test_parent_summary_and_child_count(self, admin_user, admin_profile, org_a):
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
root = _make_case(org_a, admin_user, name="Root", is_problem=True)
_make_case(org_a, admin_user, name="C1", parent=root)
@ -103,11 +103,14 @@ class TestSerializer:
assert data["is_problem"] is True
c1 = root.children.first()
c1_data = CaseSerializer(c1).data
c1_data = CaseSerializer(
c1, context=parent_access_context(admin_profile, [c1])
).data
assert c1_data["parent_summary"] == {
"id": str(root.id),
"name": root.name,
"status": root.status,
"restricted": False,
}
assert c1_data["child_count"] == 0

View file

@ -0,0 +1,230 @@
"""`parent_summary` follows the ticket read rule on the parent (D51).
`CaseSerializer.get_parent_summary` used to return the parent's id, name and
status to anybody who could read the child. Reading a child is not reading its
parent: a member assigned one sub-ticket learned the subject of a ticket the
detail view answers 403 for. A parent the viewer cannot open now comes back as
`{id, name: None, status: None, restricted: True}`, the shape `/tree/` uses for
a hidden node.
The create/update serializer had the matching write-side gap: `parent` was
checked by org alone, so a member could file a ticket under a parent they
cannot open, the same link `/link/` refuses, and learn from the difference
between a 400 and a 200 that the hidden ticket exists.
`user_client` is a plain member (role USER) whose user is `regular_user`.
"""
from __future__ import annotations
import uuid
import pytest
from crum import impersonate
from django.db import connection
from django.test.utils import CaptureQueriesContext
from cases.models import Case, CaseWatcher
from cases.serializer import PARENT_NOT_FOUND, CaseSerializer
from conftest import rls_org
SECRET = "Secret payroll ticket"
REDACTED = {"name": None, "status": None, "restricted": True}
def _case(org, creator, name, *, parent=None):
with impersonate(creator):
with rls_org(org):
return Case.objects.create(
name=name, status="New", priority="Normal", org=org, parent=parent
)
def _row(resp, case):
return next(r for r in resp.json()["cases"] if r["id"] == str(case.id))
@pytest.mark.django_db
class TestDetail:
def test_member_who_cannot_read_the_parent_gets_it_redacted(
self, user_client, admin_user, regular_user, org_a
):
parent = _case(org_a, admin_user, SECRET)
child = _case(org_a, regular_user, "My sub-ticket", parent=parent)
# The parent really is hidden from this member.
assert user_client.get(f"/api/cases/{parent.id}/").status_code == 403
resp = user_client.get(f"/api/cases/{child.id}/")
assert resp.status_code == 200, resp.content
assert resp.json()["cases_obj"]["parent_summary"] == {
"id": str(parent.id),
**REDACTED,
}
assert SECRET.encode() not in resp.content
def test_member_who_can_read_the_parent_gets_it_in_full(
self, user_client, admin_user, regular_user, user_profile, org_a
):
parent = _case(org_a, admin_user, "Watched parent")
CaseWatcher.objects.create(case=parent, profile=user_profile, org=org_a)
child = _case(org_a, regular_user, "My sub-ticket", parent=parent)
resp = user_client.get(f"/api/cases/{child.id}/")
assert resp.status_code == 200, resp.content
assert resp.json()["cases_obj"]["parent_summary"] == {
"id": str(parent.id),
"name": "Watched parent",
"status": "New",
"restricted": False,
}
def test_admin_gets_every_parent_in_full(
self, admin_client, admin_user, regular_user, org_a
):
parent = _case(org_a, regular_user, "Member's parent")
child = _case(org_a, admin_user, "Child", parent=parent)
summary = admin_client.get(f"/api/cases/{child.id}/").json()["cases_obj"][
"parent_summary"
]
assert summary["name"] == "Member's parent"
assert summary["restricted"] is False
def test_a_serializer_given_no_viewer_redacts(self, admin_user, org_a):
# Fail closed: a call site that forgets the context leaks nothing.
parent = _case(org_a, admin_user, SECRET)
child = _case(org_a, admin_user, "Child", parent=parent)
assert CaseSerializer(child).data["parent_summary"] == {
"id": str(parent.id),
**REDACTED,
}
@pytest.mark.django_db
class TestList:
def test_each_row_is_judged_on_its_own_parent(
self, user_client, admin_user, regular_user, user_profile, org_a
):
hidden = _case(org_a, admin_user, SECRET)
mine = _case(org_a, regular_user, "My own parent")
under_hidden = _case(org_a, regular_user, "Under hidden", parent=hidden)
under_mine = _case(org_a, regular_user, "Under mine", parent=mine)
resp = user_client.get("/api/cases/")
assert resp.status_code == 200, resp.content
assert _row(resp, under_hidden)["parent_summary"] == {
"id": str(hidden.id),
**REDACTED,
}
assert _row(resp, under_mine)["parent_summary"]["name"] == "My own parent"
assert _row(resp, under_mine)["parent_summary"]["restricted"] is False
assert SECRET.encode() not in resp.content
def test_parents_cost_one_query_per_page_not_one_per_row(
self, user_client, admin_user, regular_user, org_a
):
# Every case exists before the first request, so both requests list
# the same rows; the readable parents are the member's own tickets and
# are on the list either way. Only the links change in between.
children = [_case(org_a, regular_user, f"Child {i}") for i in range(6)]
parents = [
_case(org_a, admin_user if i % 2 else regular_user, f"Parent {i}")
for i in range(6)
]
with CaptureQueriesContext(connection) as unlinked:
assert user_client.get("/api/cases/").status_code == 200
for child, parent in zip(children, parents):
Case.objects.filter(pk=child.pk).update(parent=parent)
with CaptureQueriesContext(connection) as linked:
resp = user_client.get("/api/cases/")
assert resp.status_code == 200
rows = [_row(resp, c)["parent_summary"] for c in children]
assert [r["restricted"] for r in rows] == [False, True] * 3
# Six parents, half of them hidden, and one extra query for all of
# them: the batch lookup of which parent ids the member may read.
assert len(linked) - len(unlinked) <= 1
@pytest.mark.django_db
class TestLinkingThroughTheTicketForm:
"""`parent` on create and PATCH takes the rule `/link/` takes."""
def test_a_hidden_parent_is_refused_like_a_missing_one(
self, user_client, admin_user, regular_user, org_a
):
hidden = _case(org_a, admin_user, SECRET)
mine = _case(org_a, regular_user, "Mine")
refused = user_client.patch(
f"/api/cases/{mine.id}/", {"parent": str(hidden.id)}, format="json"
)
missing = user_client.patch(
f"/api/cases/{mine.id}/", {"parent": str(uuid.uuid4())}, format="json"
)
assert refused.status_code == missing.status_code == 400
assert refused.json()["errors"]["parent"] == [PARENT_NOT_FOUND]
assert missing.json()["errors"]["parent"] == [PARENT_NOT_FOUND]
assert Case.objects.get(pk=mine.pk).parent_id is None
def test_a_hidden_parent_is_refused_on_create(self, user_client, admin_user, org_a):
hidden = _case(org_a, admin_user, SECRET)
resp = user_client.post(
"/api/cases/",
{
"name": "New one",
"status": "New",
"priority": "Normal",
"parent": str(hidden.id),
},
format="json",
)
assert resp.status_code == 400, resp.content
assert SECRET.encode() not in resp.content
assert not Case.objects.filter(name="New one").exists()
def test_a_readable_parent_is_accepted(self, user_client, regular_user, org_a):
parent = _case(org_a, regular_user, "My parent")
mine = _case(org_a, regular_user, "Mine")
resp = user_client.patch(
f"/api/cases/{mine.id}/", {"parent": str(parent.id)}, format="json"
)
assert resp.status_code == 200, resp.content
assert Case.objects.get(pk=mine.pk).parent_id == parent.id
def test_resending_the_current_hidden_parent_is_not_a_new_link(
self, user_client, admin_user, regular_user, org_a
):
hidden = _case(org_a, admin_user, SECRET)
mine = _case(org_a, regular_user, "Mine", parent=hidden)
resp = user_client.patch(
f"/api/cases/{mine.id}/",
{"parent": str(hidden.id), "priority": "High"},
format="json",
)
assert resp.status_code == 200, resp.content
assert SECRET.encode() not in resp.content
assert Case.objects.get(pk=mine.pk).priority == "High"
def test_admin_may_link_under_any_parent(
self, admin_client, admin_user, regular_user, org_a
):
parent = _case(org_a, regular_user, "Member's")
child = _case(org_a, admin_user, "Admin's")
resp = admin_client.patch(
f"/api/cases/{child.id}/", {"parent": str(parent.id)}, format="json"
)
assert resp.status_code == 200, resp.content
@pytest.mark.django_db
def test_another_orgs_case_is_refused_like_a_missing_one(
user_client, regular_user, case_b, org_a
):
mine = _case(org_a, regular_user, "Mine")
resp = user_client.patch(
f"/api/cases/{mine.id}/", {"parent": str(case_b.id)}, format="json"
)
assert resp.status_code == 400, resp.content
assert resp.json()["errors"]["parent"] == [PARENT_NOT_FOUND]

View file

@ -0,0 +1,348 @@
"""Object-level authorization on the parent/child endpoints (D50).
`CaseTreeView`, `CaseLinkParentView` and `CaseCloseWithChildrenView` used to
filter by org and nothing else. Any member could read the names of tickets they
cannot open through the tree, link or unlink any ticket, and close any ticket
together with its whole subtree, skipping the approval gate on the way.
The rules they follow now are the ones in `cases.access`:
* tree: read on the ticket asked about; unreadable nodes come back redacted;
* link: write on the child, read on the new parent (a hidden parent answers
like a missing one); unlink: write on the child;
* close with children: write on the ticket and on every descendant the
cascade would close, and `close_refusal` passes on each. All or nothing.
`user_client` is a plain member (role USER). `admin_user` creates the tickets
that member is not supposed to reach.
"""
from __future__ import annotations
import uuid
import pytest
from crum import impersonate
from django.utils import timezone
from cases.approvals import ApprovalRule
from cases.models import Case, CaseWatcher
from common.models import Activity
from conftest import rls_org
def _case(org, creator, name, *, parent=None, priority="Normal", status="New"):
with impersonate(creator):
with rls_org(org):
return Case.objects.create(
name=name,
status=status,
priority=priority,
org=org,
parent=parent,
closed_on=timezone.localdate() if status == "Closed" else None,
)
def _watch(case, profile):
CaseWatcher.objects.create(case=case, profile=profile, org=case.org)
def _statuses(*cases):
return [Case.objects.get(pk=c.pk).status for c in cases]
# --------------------------------------------------------------------------- #
# GET /api/cases/<pk>/tree/
# --------------------------------------------------------------------------- #
@pytest.mark.django_db
class TestTreeAccess:
def test_member_without_read_is_refused(
self, user_client, admin_user, user_profile, org_a
):
root = _case(org_a, admin_user, "Admin root")
_case(org_a, admin_user, "Admin child", parent=root)
resp = user_client.get(f"/api/cases/{root.id}/tree/")
# The ticket detail GET answers a hidden in-org ticket 403 too.
assert resp.status_code == 403
assert b"Admin child" not in resp.content
assert b"Admin root" not in resp.content
def test_detail_get_answers_the_same(self, user_client, admin_user, org_a):
root = _case(org_a, admin_user, "Admin root")
assert user_client.get(f"/api/cases/{root.id}/").status_code == 403
def test_watcher_may_read_the_tree(
self, user_client, admin_user, user_profile, org_a
):
root = _case(org_a, admin_user, "Watched root")
_watch(root, user_profile)
resp = user_client.get(f"/api/cases/{root.id}/tree/")
assert resp.status_code == 200, resp.content
assert resp.json()["root"]["name"] == "Watched root"
def test_hidden_descendant_is_redacted(
self, user_client, admin_user, regular_user, org_a
):
root = _case(org_a, regular_user, "My root")
hidden = _case(org_a, admin_user, "Secret payroll ticket", parent=root)
mine = _case(org_a, regular_user, "My grandchild", parent=hidden)
resp = user_client.get(f"/api/cases/{mine.id}/tree/")
assert resp.status_code == 200, resp.content
assert b"Secret payroll ticket" not in resp.content
body = resp.json()
assert body["root"]["name"] == "My root"
node = body["root"]["children"][0]
assert node["id"] == str(hidden.id)
assert node["restricted"] is True
assert node["name"] is None
assert "priority" not in node and "assigned_to" not in node
# Shape kept: the readable grandchild still sits under it.
assert node["children"][0]["name"] == "My grandchild"
assert "restricted" not in node["children"][0]
def test_admin_sees_every_name(self, admin_client, regular_user, org_a):
root = _case(org_a, regular_user, "Member root")
_case(org_a, regular_user, "Member child", parent=root)
resp = admin_client.get(f"/api/cases/{root.id}/tree/")
assert resp.status_code == 200
child = resp.json()["root"]["children"][0]
assert child["name"] == "Member child"
assert "restricted" not in child
def test_other_org_is_404(self, org_b_client, admin_user, org_a):
root = _case(org_a, admin_user, "Org A root")
assert org_b_client.get(f"/api/cases/{root.id}/tree/").status_code == 404
# --------------------------------------------------------------------------- #
# POST /api/cases/<pk>/link/
# --------------------------------------------------------------------------- #
@pytest.mark.django_db
class TestLinkAccess:
def test_member_cannot_link_a_ticket_they_cannot_write(
self, user_client, admin_user, regular_user, user_profile, org_a
):
child = _case(org_a, admin_user, "Admin child")
# Watching grants read, not write.
_watch(child, user_profile)
parent = _case(org_a, regular_user, "My parent")
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json"
)
assert resp.status_code == 403
child.refresh_from_db()
assert child.parent_id is None
assert (
not Activity.objects.filter(entity_id=child.id)
.filter(action="LINKED_PARENT")
.exists()
)
def test_member_cannot_unlink_a_ticket_they_cannot_write(
self, user_client, admin_user, org_a
):
parent = _case(org_a, admin_user, "Admin parent")
child = _case(org_a, admin_user, "Admin child", parent=parent)
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json"
)
assert resp.status_code == 403
child.refresh_from_db()
assert child.parent_id == parent.id
def test_hidden_parent_answers_like_a_missing_one(
self, user_client, admin_user, regular_user, org_a
):
child = _case(org_a, regular_user, "My child")
hidden = _case(org_a, admin_user, "Secret parent")
hidden_resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": str(hidden.id)}, format="json"
)
missing_resp = user_client.post(
f"/api/cases/{child.id}/link/",
{"parent_id": str(uuid.uuid4())},
format="json",
)
assert hidden_resp.status_code == missing_resp.status_code == 400
assert hidden_resp.json() == missing_resp.json()
assert b"Secret parent" not in hidden_resp.content
child.refresh_from_db()
assert child.parent_id is None
def test_malformed_parent_id_is_a_missing_parent(
self, user_client, regular_user, org_a
):
child = _case(org_a, regular_user, "My child")
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": "not-a-uuid"}, format="json"
)
assert resp.status_code == 400
assert "parent_id" in resp.json()
def test_writer_links_under_a_readable_parent(
self, user_client, admin_user, regular_user, user_profile, org_a
):
child = _case(org_a, regular_user, "My child")
parent = _case(org_a, admin_user, "Watched parent")
_watch(parent, user_profile)
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json"
)
assert resp.status_code == 200, resp.content
child.refresh_from_db()
assert child.parent_id == parent.id
def test_writer_unlinks(self, user_client, admin_user, regular_user, org_a):
parent = _case(org_a, admin_user, "Admin parent")
child = _case(org_a, regular_user, "My child", parent=parent)
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json"
)
assert resp.status_code == 200, resp.content
child.refresh_from_db()
assert child.parent_id is None
def test_admin_links_any_ticket(self, admin_client, regular_user, org_a):
child = _case(org_a, regular_user, "Member child")
parent = _case(org_a, regular_user, "Member parent")
resp = admin_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json"
)
assert resp.status_code == 200, resp.content
child.refresh_from_db()
assert child.parent_id == parent.id
def test_other_org_is_404(self, org_b_client, admin_user, org_a):
child = _case(org_a, admin_user, "Org A child")
resp = org_b_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json"
)
assert resp.status_code == 404
# --------------------------------------------------------------------------- #
# POST /api/cases/<pk>/close-with-children/
# --------------------------------------------------------------------------- #
def _close(client, case, cascade=True):
return client.post(
f"/api/cases/{case.id}/close-with-children/",
{"cascade": cascade, "resolution_comment": "done"},
format="json",
)
@pytest.mark.django_db
class TestCloseWithChildrenAccess:
def test_member_cannot_close_a_ticket_they_cannot_write(
self, user_client, admin_user, user_profile, org_a
):
root = _case(org_a, admin_user, "Admin root")
_watch(root, user_profile)
child = _case(org_a, admin_user, "Admin child", parent=root)
resp = _close(user_client, root)
assert resp.status_code == 403
assert _statuses(root, child) == ["New", "New"]
def test_one_unwritable_descendant_refuses_the_whole_close(
self, user_client, admin_user, regular_user, org_a
):
root = _case(org_a, regular_user, "My root")
mine = _case(org_a, regular_user, "My child", parent=root)
hidden = _case(org_a, admin_user, "Secret sibling", parent=root)
resp = _close(user_client, root)
assert resp.status_code == 400, resp.content
body = resp.json()
assert body["error"] is True
assert "not yours to close" in body["errors"]
assert b"Secret sibling" not in resp.content
assert _statuses(root, mine, hidden) == ["New", "New", "New"]
assert not Activity.objects.filter(action="PARENT_CLOSED_CASCADE").exists()
def test_member_may_close_their_ticket_alone(
self, user_client, admin_user, regular_user, org_a
):
# Without the cascade, the unwritable child is not touched and does
# not stand in the way.
root = _case(org_a, regular_user, "My root")
hidden = _case(org_a, admin_user, "Admin child", parent=root)
resp = _close(user_client, root, cascade=False)
assert resp.status_code == 200, resp.content
assert _statuses(root, hidden) == ["Closed", "New"]
def test_approval_required_on_a_descendant_refuses(
self, admin_client, admin_user, org_a
):
root = _case(org_a, admin_user, "Root", priority="Low")
gated = _case(org_a, admin_user, "Urgent child", parent=root, priority="Urgent")
ApprovalRule.objects.create(
name="Urgent needs sign-off",
org=org_a,
trigger_event="pre_close",
match_priority="Urgent",
)
resp = _close(admin_client, root)
assert resp.status_code == 400, resp.content
message = resp.json()["errors"]
assert "approval" in message and "Urgent child" in message
assert _statuses(root, gated) == ["New", "New"]
def test_approval_required_on_the_ticket_itself_refuses(
self, admin_client, admin_user, org_a
):
root = _case(org_a, admin_user, "Urgent root", priority="Urgent")
ApprovalRule.objects.create(
name="Urgent needs sign-off",
org=org_a,
trigger_event="pre_close",
match_priority="Urgent",
)
resp = _close(admin_client, root, cascade=False)
assert resp.status_code == 400, resp.content
assert "approval" in resp.json()["errors"]["status"][0]
assert _statuses(root) == ["New"]
def test_allowed_close_stamps_every_ticket(self, user_client, regular_user, org_a):
root = _case(org_a, regular_user, "My root")
child = _case(org_a, regular_user, "My child", parent=root)
grandchild = _case(org_a, regular_user, "My grandchild", parent=child)
resp = _close(user_client, root)
assert resp.status_code == 200, resp.content
assert set(resp.json()["cascaded_case_ids"]) == {
str(child.id),
str(grandchild.id),
}
today = timezone.localdate()
for c in (root, child, grandchild):
c.refresh_from_db()
assert c.status == "Closed"
assert c.closed_on == today
assert c.resolved_at is not None
def test_admin_closes_everybodys_subtree(self, admin_client, regular_user, org_a):
root = _case(org_a, regular_user, "Member root")
child = _case(org_a, regular_user, "Member child", parent=root)
resp = _close(admin_client, root)
assert resp.status_code == 200, resp.content
assert _statuses(root, child) == ["Closed", "Closed"]
def test_stored_cycle_does_not_hang(self, admin_client, admin_user, org_a):
a = _case(org_a, admin_user, "A")
b = _case(org_a, admin_user, "B", parent=a)
Case.objects.filter(pk=a.pk).update(parent=b)
resp = _close(admin_client, a)
assert resp.status_code == 200, resp.content
assert resp.json()["cascaded_case_ids"] == [str(b.id)]
def test_other_org_is_404(self, org_b_client, admin_user, org_a):
root = _case(org_a, admin_user, "Org A root")
assert _close(org_b_client, root).status_code == 404
assert _statuses(root) == ["New"]

View file

@ -0,0 +1,127 @@
"""``case_count`` on a ticket pipeline counts only tickets the caller can open (D47).
It used to count every case in the org, so a member whose board showed one
card read ``case_count: 2``, a number that included a ticket they cannot open.
It follows ``cases.access.visible_cases_qs`` now, the rule the board and the
list use, in all four places it appears: the pipeline list, the pipeline
detail, each of its nested stages, and the pipeline block on the board. The
same fix D9 made for lead pipelines.
"""
import pytest
from django.db import connection
from django.test.utils import CaptureQueriesContext
from cases.models import Case, CasePipeline, CaseStage, CaseWatcher
from conftest import rls_org
def _pipeline(org, user, name="Support"):
pipeline = CasePipeline.objects.create(name=name, org=org, created_by=user)
first = CaseStage.objects.create(
pipeline=pipeline, name="Triage", order=1, org=org, created_by=user
)
second = CaseStage.objects.create(
pipeline=pipeline, name="Working", order=2, org=org, created_by=user
)
return pipeline, first, second
def _case(org, user, name, stage):
return Case.objects.create(
name=name,
status="New",
priority="Normal",
org=org,
created_by=user,
stage=stage,
)
@pytest.fixture
def seeded(admin_user, admin_profile, user_profile, org_a, org_b, user_b):
pipeline, first, _second = _pipeline(org_a, admin_user)
_case(org_a, admin_user, "Hidden from the member", first)
mine = _case(org_a, admin_user, "Handed to the member", first)
# Two assignees: a join on the M2M would count this ticket twice.
mine.assigned_to.add(user_profile, admin_profile)
# Another org's ticket, in another org's pipeline, never counted here.
with rls_org(org_b):
_b_pipeline, b_stage, _ = _pipeline(org_b, user_b, "Org B")
_case(org_b, user_b, "Org B ticket", b_stage)
return pipeline
def _counts(client, pipeline):
"""case_count from the list, the detail, its first stage and the board."""
(row,) = client.get("/api/cases/pipelines/").json()["pipelines"]
assert row["id"] == str(pipeline.id)
detail = client.get(f"/api/cases/pipelines/{pipeline.id}/").json()
board = client.get("/api/cases/kanban/", {"pipeline_id": str(pipeline.id)}).json()
assert [s["case_count"] for s in detail["stages"][1:]] == [0]
return (
row["case_count"],
detail["case_count"],
detail["stages"][0]["case_count"],
board["pipeline"]["case_count"],
)
@pytest.mark.django_db
class TestCasePipelineCountsFollowVisibility:
def test_member_counts_only_visible_cases(self, user_client, seeded):
assert _counts(user_client, seeded) == (1, 1, 1, 1)
def test_admin_counts_every_case_in_the_org(self, admin_client, seeded):
assert _counts(admin_client, seeded) == (2, 2, 2, 2)
board = admin_client.get(
"/api/cases/kanban/", {"pipeline_id": str(seeded.id)}
).json()
assert board["columns"][0]["case_count"] == 2
assert board["pipeline"]["stage_count"] == 2
def test_member_board_column_and_pipeline_agree(self, user_client, seeded):
board = user_client.get(
"/api/cases/kanban/", {"pipeline_id": str(seeded.id)}
).json()
assert board["columns"][0]["case_count"] == 1
assert len(board["columns"][0]["cases"]) == 1
assert board["pipeline"]["case_count"] == 1
def test_watcher_counts_the_ticket_they_follow(
self, user_client, user_profile, admin_user, org_a
):
pipeline, first, _second = _pipeline(org_a, admin_user, "Watched")
watched = _case(org_a, admin_user, "Watched ticket", first)
CaseWatcher.objects.create(case=watched, profile=user_profile, org=org_a)
detail = user_client.get(f"/api/cases/pipelines/{pipeline.id}/").json()
assert detail["case_count"] == 1
def test_write_responses_carry_case_count(self, admin_client, admin_user, org_a):
created = admin_client.post(
"/api/cases/pipelines/", {"name": "Fresh"}, format="json"
).json()
assert created["case_count"] == 0
assert {s["case_count"] for s in created["stages"]} == {0}
_pipeline_obj, first, _second = _pipeline(org_a, admin_user, "Other")
_case(org_a, admin_user, "Staged", first)
updated = admin_client.put(
f"/api/cases/stages/{first.id}/", {"name": "Renamed"}, format="json"
).json()
assert updated["case_count"] == 1
def test_pipeline_list_query_count_does_not_grow_per_pipeline(
self, admin_client, admin_user, org_a, seeded
):
with CaptureQueriesContext(connection) as one:
admin_client.get("/api/cases/pipelines/")
for name in ("Second", "Third"):
_p, stage, _s = _pipeline(org_a, admin_user, name)
_case(org_a, admin_user, f"{name} ticket", stage)
with CaptureQueriesContext(connection) as three:
response = admin_client.get("/api/cases/pipelines/")
assert len(response.json()["pipelines"]) == 3
assert len(three.captured_queries) == len(one.captured_queries)

View file

@ -39,13 +39,14 @@ def auto_stop_stale_timers(*args, **kwargs):
restore_rls_context()
def _make_case(org, creator, name="Sample case"):
def _make_case(org, creator, name="Sample case", account=None):
with impersonate(creator):
return Case.objects.create(
name=name,
status="New",
priority="Normal",
org=org,
account=account,
)
@ -490,8 +491,8 @@ class TestInvoiceFromTimeEntries:
def test_creates_draft_invoice(
self, admin_client, admin_user, admin_profile, org_a
):
case = _make_case(org_a, admin_user)
account = Account.objects.create(name="ACME", org=org_a)
case = _make_case(org_a, admin_user, account=account)
e1 = _entry(
case,
admin_profile,
@ -530,8 +531,8 @@ class TestInvoiceFromTimeEntries:
def test_mixed_currency_rejected(
self, admin_client, admin_user, admin_profile, org_a
):
case = _make_case(org_a, admin_user)
account = Account.objects.create(name="ACME", org=org_a)
case = _make_case(org_a, admin_user, account=account)
e1 = _entry(
case,
admin_profile,
@ -561,8 +562,8 @@ class TestInvoiceFromTimeEntries:
def test_non_billable_rejected(
self, admin_client, admin_user, admin_profile, org_a
):
case = _make_case(org_a, admin_user)
account = Account.objects.create(name="ACME", org=org_a)
case = _make_case(org_a, admin_user, account=account)
e1 = _entry(
case,
admin_profile,

View file

@ -23,7 +23,7 @@ from rest_framework.views import APIView
from cases.merge_views import _can_merge
from cases.models import Case, EmailMessage
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from cases.signals import _create_activity
from common.models import Attachments, Comment
from common.permissions import HasOrgContext
@ -187,7 +187,9 @@ class CaseUnmergeView(APIView):
{
"error": False,
"message": "Tickets unmerged",
"source_case": CaseSerializer(source).data,
"source_case": CaseSerializer(
source, context=parent_access_context(request.profile, [source])
).data,
},
status=status.HTTP_200_OK,
)

View file

@ -17,7 +17,7 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from accounts.models import Account
from accounts.access import visible_accounts_qs
from accounts.serializer import AccountPickerSerializer
from cases import swagger_params
from cases.access import (
@ -39,6 +39,7 @@ from cases.serializer import (
CaseSerializer,
EmailMessageSerializer,
ReopenPolicySerializer,
parent_access_context,
)
from cases.solution_serializers import SolutionSerializer
from cases.tasks import send_email_to_assigned_user
@ -67,8 +68,7 @@ from common.utils import (
validate_attachment,
)
from common.validators import date_param, payload_id_list, uuid_list_param, uuid_param
from contacts.access import replace_visible_contacts
from contacts.models import Contact
from contacts.access import replace_visible_contacts, visible_contacts_qs
from contacts.serializer import ContactLinkSerializer, ContactPickerSerializer
# A ticket is "open" while somebody still owes the customer something. The
@ -202,8 +202,12 @@ class CaseListView(APIView, LimitOffsetPagination):
queryset = queryset.filter(merged_into__isnull=True).exclude(
status="Duplicate"
)
accounts = Account.objects.filter(org=self.request.profile.org).order_by("-id")
contacts = Contact.objects.filter(org=self.request.profile.org).order_by("-id")
# The account read rule itself, which is what the save path accepts.
accounts = visible_accounts_qs(
self.request.profile, self.request.user
).order_by("-id")
# The contact read rule itself, which is what the save path accepts.
contacts = visible_contacts_qs(self.request.profile).order_by("-id")
profiles = Profile.objects.filter(is_active=True, org=self.request.profile.org)
if not is_org_admin(self.request.profile):
# Watcher allowance: a non-admin who is a watcher must still be
@ -214,14 +218,6 @@ class CaseListView(APIView, LimitOffsetPagination):
queryset = queryset.filter(
pk__in=visible_cases_qs(self.request.profile).values("pk")
)
accounts = accounts.filter(
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
contacts = contacts.filter(
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
profiles = profiles.filter(role="ADMIN")
queryset = apply_case_list_filters(queryset, params)
@ -245,7 +241,11 @@ class CaseListView(APIView, LimitOffsetPagination):
).count()
results_cases = self.paginate_queryset(queryset, self.request, view=self)
cases = CaseSerializer(results_cases, many=True).data
cases = CaseSerializer(
results_cases,
many=True,
context=parent_access_context(self.request.profile, results_cases),
).data
if results_cases:
offset = queryset.filter(id__gte=results_cases[-1].id).count()
@ -392,7 +392,10 @@ class CaseListView(APIView, LimitOffsetPagination):
"error": False,
"message": "Case Created Successfully",
"id": str(cases_obj.id),
"cases_obj": CaseSerializer(cases_obj).data,
"cases_obj": CaseSerializer(
cases_obj,
context=parent_access_context(request.profile, [cases_obj]),
).data,
},
status=status.HTTP_200_OK,
)
@ -579,7 +582,11 @@ class CaseDetailView(APIView):
)
def get(self, request, pk, format=None):
self.cases = self.get_object(pk=pk)
# Merged duplicate → tell the client to redirect. JSON form (200) keeps
# Authorise before anything about the case leaves: the merge redirect
# below used to answer first, handing any member the name of a ticket
# they cannot open and the id of the one it was merged into.
assert_case_read_access(request.profile, self.cases)
# Merged duplicate: tell the client to redirect. JSON form (200) keeps
# the SvelteKit route's error handling simple. The query param
# `?show_merged=true` lets agents view the duplicate directly via
# bookmark / list-view escape hatch.
@ -596,12 +603,11 @@ class CaseDetailView(APIView):
},
status=status.HTTP_200_OK,
)
# Authorise before serialising: the old order built the response body
# for a case the requester was about to be refused.
assert_case_read_access(request.profile, self.cases)
context = {}
context["cases_obj"] = CaseSerializer(self.cases).data
context["cases_obj"] = CaseSerializer(
self.cases, context=parent_access_context(request.profile, [self.cases])
).data
# `comment_permission` used to be creator-or-admin while `post` below
# accepted creator, admin *or assignee*. The flag told an assignee they
@ -762,7 +768,10 @@ class CaseDetailView(APIView):
context.update(
{
"cases_obj": CaseSerializer(self.cases_obj).data,
"cases_obj": CaseSerializer(
self.cases_obj,
context=parent_access_context(request.profile, [self.cases_obj]),
).data,
"attachments": AttachmentsSerializer(attachments, many=True).data,
"comments": CommentSerializer(
comments_qs.filter(is_internal=False), many=True

View file

@ -95,7 +95,7 @@ class WatchingListView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get(self, request, *args, **kwargs):
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from cases.views import apply_case_list_filters
cases = (
@ -106,7 +106,11 @@ class WatchingListView(APIView):
cases = apply_case_list_filters(cases, request.query_params)
return Response(
{
"cases": CaseSerializer(cases, many=True).data,
"cases": CaseSerializer(
cases,
many=True,
context=parent_access_context(request.profile, cases),
).data,
"count": cases.count(),
}
)

View file

@ -240,6 +240,15 @@ def _validate_stage(
raise PackValidationError(
f"{where}: maps_to_status {maps_to_status!r} is not valid"
)
if "win_probability" in stage:
# Copied onto Lead.probability by the board move, which has a 0-100
# CheckConstraint, and LeadStage carries the same one. bool is an int
# subclass, so it is refused by name.
wp = stage["win_probability"]
if isinstance(wp, bool) or not isinstance(wp, int) or not 0 <= wp <= 100:
raise PackValidationError(
f"{where}: win_probability {wp!r} must be an integer from 0 to 100"
)
def _validate_pipeline(

View file

@ -43,6 +43,22 @@ def is_org_admin(profile):
return getattr(profile, "role", None) == "ADMIN"
def can_mass_import(profile):
"""Whether ``profile`` may bulk-create records through a CSV import.
Org admins, Django superusers, and members granted ``has_sales_access``.
Everyone else can still create records one at a time; the import is the
mass-create surface, so it is gated more narrowly. One rule for the
contact, ticket and lead importers, which used to carry three copies of
it, none of which admitted a superuser.
"""
if profile is None:
return False
if is_org_admin(profile) or profile.user.is_superuser:
return True
return bool(profile.has_sales_access)
class HasOrgContext(permissions.BasePermission):
"""
Permission class that requires valid organization context.

View file

@ -4,15 +4,18 @@ ONE endpoint, deliberately. The org comes from the JWT (`request.profile.org`)
exactly once and every queryset is filtered by it; a per-model fan-out from the
browser would be as many chances to read another tenant's rows.
Each type also honours the SAME read visibility as its own list view, so search
can never surface a record the caller could not open from the list:
Each type also honours the SAME read rule as its own detail view, by calling
that module's visibility helper rather than restating it, so search can never
surface a record the caller could not open, or hide one they could:
* leads / opportunities / invoices, admin (role ``ADMIN`` or a Django
superuser) sees the whole org; everyone else sees only what they created or
were assigned.
* accounts / contacts, admin (role ``ADMIN`` or the org-admin flag) sees the
whole org; everyone else sees created-or-assigned.
* tickets. Reuse ``visible_cases_qs`` (adds the watcher clause).
* leads ``visible_leads_qs``, deals ``visible_deals_qs``, accounts
``visible_accounts_qs``, invoices ``visible_invoices_qs``: admins and Django
superusers see the whole org, everyone else what they created or were
assigned.
* contacts ``visible_contacts_qs``: the same, plus contacts at an account the
caller is assigned to.
* tickets ``visible_cases_qs``: admins, creator, assignees and watchers. No
superuser clause, because the ticket detail view has none.
* knowledge-base articles. Org-wide, every member reads them.
Matching stays on the server; the whole record set never reaches the browser.
@ -23,14 +26,14 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from accounts.models import Account
from cases.access import is_org_admin, visible_cases_qs
from accounts.access import visible_accounts_qs
from cases.access import visible_cases_qs
from cases.models import Solution
from common.permissions import HasOrgContext
from contacts.models import Contact
from invoices.models import Invoice
from leads.models import Lead
from opportunity.models import Opportunity
from contacts.access import visible_contacts_qs
from invoices.permissions import visible_invoices_qs
from leads.access import visible_leads_qs
from opportunity.access import visible_deals_qs
# Rows per type. Small on purpose. The palette shows a handful per group and
# the point is the fastest match, not an exhaustive report.
@ -39,30 +42,6 @@ PER_TYPE = 6
MIN_QUERY = 2
def _own_filter(profile):
"""The created-or-assigned clause every assignable list view uses.
``created_by`` is a FK to ``User`` (not Profile), so it is compared against
``profile.user``: comparing it to the Profile is the silent always-False
bug this codebase has hit before.
"""
return Q(created_by=profile.user) | Q(assigned_to=profile)
def _scope_orgadmin(qs, profile):
"""accounts / contacts rule: admin = role ADMIN or the org-admin flag."""
if is_org_admin(profile):
return qs
return qs.filter(_own_filter(profile)).distinct()
def _scope_superuser(qs, profile, user):
"""leads / opportunities / invoices rule: admin = role ADMIN or superuser."""
if is_org_admin(profile) or user.is_superuser:
return qs
return qs.filter(_own_filter(profile)).distinct()
class GlobalSearchView(APIView):
"""``GET /api/search/?q=<query>``. A handful of matches per record type."""
@ -79,7 +58,7 @@ class GlobalSearchView(APIView):
results = []
# Leads
leads = _scope_superuser(Lead.objects.filter(org=org), profile, user).filter(
leads = visible_leads_qs(profile, user).filter(
Q(title__icontains=q)
| Q(first_name__icontains=q)
| Q(last_name__icontains=q)
@ -98,15 +77,15 @@ class GlobalSearchView(APIView):
)
# Deals (Opportunity)
deals = _scope_superuser(
Opportunity.objects.filter(org=org).select_related("account"),
profile,
user,
).filter(
Q(name__icontains=q)
| Q(description__icontains=q)
| Q(account__name__icontains=q)
)[:PER_TYPE]
deals = (
visible_deals_qs(profile, user)
.select_related("account")
.filter(
Q(name__icontains=q)
| Q(description__icontains=q)
| Q(account__name__icontains=q)
)[:PER_TYPE]
)
for deal in deals:
results.append(
{
@ -120,7 +99,7 @@ class GlobalSearchView(APIView):
)
# Accounts
accounts = _scope_orgadmin(Account.objects.filter(org=org), profile).filter(
accounts = visible_accounts_qs(profile, user).filter(
Q(name__icontains=q)
| Q(email__icontains=q)
| Q(website__icontains=q)
@ -137,7 +116,7 @@ class GlobalSearchView(APIView):
)
# Contacts
contacts = _scope_orgadmin(Contact.objects.filter(org=org), profile).filter(
contacts = visible_contacts_qs(profile).filter(
Q(first_name__icontains=q)
| Q(last_name__icontains=q)
| Q(email__icontains=q)
@ -177,16 +156,16 @@ class GlobalSearchView(APIView):
)
# Invoices
invoices = _scope_superuser(
Invoice.objects.filter(org=org).select_related("account"),
profile,
user,
).filter(
Q(invoice_number__icontains=q)
| Q(invoice_title__icontains=q)
| Q(client_name__icontains=q)
| Q(account__name__icontains=q)
)[:PER_TYPE]
invoices = (
visible_invoices_qs(profile, user)
.select_related("account")
.filter(
Q(invoice_number__icontains=q)
| Q(invoice_title__icontains=q)
| Q(client_name__icontains=q)
| Q(account__name__icontains=q)
)[:PER_TYPE]
)
for invoice in invoices:
results.append(
{

View file

@ -0,0 +1,197 @@
"""D43: the recent-activity feed answers with each record's own read rule.
Both feeds (``activities`` on ``GET /api/dashboard/`` and ``GET
/api/activities/``) returned every activity in the org to every member. An
activity row carries the record's name and what was done to it, so a member
read the names of accounts, deals and tickets hidden from them everywhere else.
The rule now: an admin sees the org's feed; anyone else sees an activity only
while its record passes that module's read rule. An entity type with no read
rule, and an activity whose record is gone, fail closed for a member.
``ActivityListView`` and ``ApiTodayView`` also lacked ``HasOrgContext``, so the
without-org-claim case is pinned here too.
"""
import uuid
import pytest
from django.db import connection
from django.test.utils import CaptureQueriesContext
from rest_framework import status
from rest_framework.test import APIClient
from rest_framework_simplejwt.tokens import RefreshToken
from accounts.models import Account
from cases.models import Case, CaseWatcher
from common.models import Activity
from conftest import rls_org
pytestmark = pytest.mark.django_db
FEED = "/api/activities/"
HOME = "/api/dashboard/"
def _stamp(model, obj, user):
"""Set ``created_by`` without the thread-local request user restamping it."""
model.objects.filter(pk=obj.pk).update(created_by=user)
obj.refresh_from_db()
return obj
def _activity(org, entity_type, entity_id, name, actor=None):
return Activity.objects.create(
user=actor,
action="UPDATE",
entity_type=entity_type,
entity_id=entity_id,
entity_name=name,
org=org,
)
def _feed_names(client, url=FEED):
response = client.get(url)
assert response.status_code == status.HTTP_200_OK, response.content
return {row["entity_name"] for row in response.data["activities"]}
@pytest.fixture
def my_account(org_a, regular_user):
return _stamp(
Account, Account.objects.create(name="Mine Co", org=org_a), regular_user
)
@pytest.fixture
def hidden_account(org_a, admin_user):
return _stamp(
Account, Account.objects.create(name="Hidden Co", org=org_a), admin_user
)
@pytest.fixture
def feed(org_a, org_b, admin_profile, my_account, hidden_account, profile_b):
"""One activity on a visible record, one on a hidden one, one abroad."""
_activity(org_a, "Account", my_account.id, "Mine Co", admin_profile)
_activity(org_a, "Account", hidden_account.id, "Hidden Co", admin_profile)
with rls_org(org_b):
foreign = Account.objects.create(name="Foreign Co", org=org_b)
_activity(org_b, "Account", foreign.id, "Foreign Co", profile_b)
@pytest.mark.parametrize("url", [FEED, HOME])
class TestBothFeedsFollowTheReadRule:
def test_member_sees_activity_on_a_record_they_can_open(
self, url, user_client, feed
):
assert "Mine Co" in _feed_names(user_client, url)
def test_member_does_not_see_activity_on_a_hidden_record(
self, url, user_client, feed
):
assert "Hidden Co" not in _feed_names(user_client, url)
def test_admin_sees_both(self, url, admin_client, feed):
names = _feed_names(admin_client, url)
assert {"Mine Co", "Hidden Co"} <= names
def test_another_org_never_appears(self, url, admin_client, user_client, feed):
assert "Foreign Co" not in _feed_names(admin_client, url)
assert "Foreign Co" not in _feed_names(user_client, url)
class TestFailClosedForMembers:
def test_activity_on_a_deleted_record_is_hidden_from_a_member(
self, org_a, user_profile, user_client, admin_client
):
# Same author and org as a visible row; only the record is missing.
_activity(org_a, "Account", uuid.uuid4(), "Gone Co", user_profile)
assert "Gone Co" not in _feed_names(user_client)
assert "Gone Co" in _feed_names(admin_client)
def test_entity_type_without_a_read_rule_is_hidden_from_a_member(
self, org_a, user_profile, user_client, admin_client
):
_activity(org_a, "Team", uuid.uuid4(), "Some Team", user_profile)
assert "Some Team" not in _feed_names(user_client)
assert "Some Team" in _feed_names(admin_client)
def test_the_rule_is_each_modules_own_a_watcher_sees_the_ticket(
self, org_a, admin_user, admin_profile, user_profile, user_client
):
"""Proves the per-type rule is the module's, not a generic owner check:
tickets admit watchers, so their activity follows."""
case = _stamp(
Case,
Case.objects.create(name="Watched", status="New", org=org_a),
admin_user,
)
_activity(org_a, "Case", case.id, "Watched", admin_profile)
assert "Watched" not in _feed_names(user_client)
CaseWatcher.objects.create(case=case, profile=user_profile, org=org_a)
assert "Watched" in _feed_names(user_client)
def test_entity_type_filter_still_narrows_within_what_is_visible(
self, org_a, admin_profile, user_client, my_account, hidden_account, feed
):
response = user_client.get(FEED, {"entity_type": "Account"})
assert {r["entity_name"] for r in response.data["activities"]} == {"Mine Co"}
def test_the_query_count_does_not_grow_with_the_rows(
self, org_a, admin_profile, user_client, my_account
):
def queries():
with CaptureQueriesContext(connection) as ctx:
assert user_client.get(FEED).status_code == status.HTTP_200_OK
return len(ctx.captured_queries)
_activity(org_a, "Account", my_account.id, "Mine Co", admin_profile)
one = queries()
for _ in range(5):
_activity(org_a, "Account", my_account.id, "Mine Co", admin_profile)
assert queries() == one
class TestActivityListLimit:
@pytest.mark.parametrize("limit", ["abc", "-1", "0", ""])
def test_an_unusable_limit_is_the_default_not_a_500(
self, limit, admin_client, feed
):
response = admin_client.get(FEED, {"limit": limit})
assert response.status_code == status.HTTP_200_OK
assert response.data["count"] == 2
def test_a_usable_limit_is_honoured(self, admin_client, feed):
response = admin_client.get(FEED, {"limit": "1"})
assert response.data["count"] == 1
def _client_without_org_claim(user):
"""A JWT that authenticates but names no org, as between login and org pick."""
client = APIClient()
token = RefreshToken.for_user(user)
client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.access_token}")
return client
@pytest.mark.parametrize("url", [FEED, "/api/dashboard/today/", HOME])
class TestOrgContextIsRequired:
def test_without_org_context_it_is_refused_not_crashed(
self, url, admin_user, admin_profile
):
response = _client_without_org_claim(admin_user).get(url)
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_with_org_context_it_still_answers(self, url, admin_client):
assert admin_client.get(url).status_code == status.HTTP_200_OK
def test_the_permission_class_is_declared(self, url):
"""The middleware answers 403 first today, so the pair above passes
without the class. The class is the contract; this pins it."""
from django.urls import resolve
from common.permissions import HasOrgContext
assert HasOrgContext in resolve(url).func.view_class.permission_classes

View file

@ -0,0 +1,66 @@
"""`can_mass_import`: one gate for the contact, ticket and lead CSV importers.
The three importers each carried their own copy of the rule, and none of them
admitted a Django superuser, while every read and write rule next to them
(accounts, contacts, deals, invoices) does.
"""
import csv
import io
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
from common.permissions import can_mass_import
PREVIEWS = [
"/api/contacts/import/preview/",
"/api/cases/import/preview/",
"/api/leads/import/preview/",
]
def _upload():
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(["first_name", "last_name", "email", "name"])
writer.writerow(["Ada", "Byte", "ada@example.com", "Printer on fire"])
return SimpleUploadedFile("rows.csv", buf.getvalue().encode("utf-8"), "text/csv")
@pytest.mark.django_db
class TestPredicate:
def test_no_profile_is_refused(self):
assert can_mass_import(None) is False
def test_plain_member_is_refused(self, user_profile):
assert can_mass_import(user_profile) is False
def test_admin_is_admitted(self, admin_profile):
assert can_mass_import(admin_profile) is True
def test_superuser_member_is_admitted(self, user_profile, regular_user):
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
user_profile.refresh_from_db()
assert can_mass_import(user_profile) is True
def test_sales_access_member_is_admitted(self, user_profile):
user_profile.has_sales_access = True
user_profile.save(update_fields=["has_sales_access"])
assert can_mass_import(user_profile) is True
@pytest.mark.django_db
class TestEndpoints:
@pytest.mark.parametrize("url", PREVIEWS)
def test_plain_member_gets_403(self, user_client, url):
response = user_client.post(url, {"file": _upload()}, format="multipart")
assert response.status_code == 403
@pytest.mark.parametrize("url", PREVIEWS)
def test_superuser_member_is_let_through(self, user_client, regular_user, url):
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
response = user_client.post(url, {"file": _upload()}, format="multipart")
assert response.status_code == 200, response.content

View file

@ -13,16 +13,10 @@ from contacts.services.csv_import import parse_and_validate as parse_contacts
pytestmark = pytest.mark.django_db
def _parse_contacts(file_bytes, org, profile):
# The ticket parser also takes the importer, to resolve contact emails
# among the contacts they may open; the contact parser does not.
return parse_contacts(file_bytes, org)
@pytest.mark.parametrize(
"parse, csv_bytes",
[
(_parse_contacts, b"first_name,last_name,email\nAda,Love\x00lace,a@x.com\n"),
(parse_contacts, b"first_name,last_name,email\nAda,Love\x00lace,a@x.com\n"),
(parse_tickets, b"name,status,priority\nBro\x00ken,New,High\n"),
],
ids=["contacts", "tickets"],
@ -36,7 +30,7 @@ def test_nul_character_is_a_header_error(parse, csv_bytes, org_a, admin_profile)
@pytest.mark.parametrize(
"parse, csv_bytes",
[
(_parse_contacts, b"first_name,last_name,email\nAda,Lovelace,a@x.com\n"),
(parse_contacts, b"first_name,last_name,email\nAda,Lovelace,a@x.com\n"),
(parse_tickets, b"name,status,priority\nBroken,New,High\n"),
],
ids=["contacts", "tickets"],

View file

@ -18,16 +18,10 @@ from contacts.services.csv_import import parse_and_validate as parse_contacts
pytestmark = pytest.mark.django_db
def _parse_contacts(file_bytes, org, profile):
# The ticket parser also takes the importer, to resolve contact emails
# among the contacts they may open; the contact parser does not.
return parse_contacts(file_bytes, org)
CONTACTS = b"first_name,last_name,email,tags\nAda,Lovelace,%s@x.com,%s\n"
TICKETS = b"name,status,priority,tags\nBroken %s,New,High,%s\n"
IMPORTERS = [
(_parse_contacts, commit_contacts, CONTACTS),
(parse_contacts, commit_contacts, CONTACTS),
(parse_tickets, commit_tickets, TICKETS),
]
IDS = ["contacts", "tickets"]

View file

@ -0,0 +1,354 @@
"""D40: every list, search, picker and count answers with the detail read rule.
Several paths carried their own copy of "who may see this record" and had
drifted from the detail view's, so a user could open a record their list,
search or dashboard hid, or be shown one that answered 403:
* global search scoped accounts without the superuser clause;
* the account list restated the account rule inline (it agreed; it now calls
``visible_accounts_qs`` so it cannot drift);
* the task list's account picker had no superuser clause;
* the dashboard counted contacts without account assignment, and counted
tasks for a superuser that the task detail view refuses them;
* the Today queue showed a superuser tickets and tasks they cannot open (it
also leaves out tickets a watcher can open, on purpose since D44: its ticket
action is a reply, which only the write rule allows);
* the ticket and task boards admitted superusers the detail views refuse,
the ticket board left watchers out, and the task board repeated a card
once per assignee on a task its caller created.
Rules differ on purpose. Accounts, contacts, deals, leads and invoices admit a
Django superuser; tickets and tasks do not, because their detail views do not.
Each test here pins one path against the detail answer for the same caller.
"""
import pytest
from django.utils import timezone
from rest_framework import status
from accounts.models import Account
from cases.models import Case, CaseWatcher
from conftest import rls_org
from contacts.models import Contact
from tasks.models import Task
pytestmark = pytest.mark.django_db
def _stamp(model, obj, user):
"""Set ``created_by`` without the thread-local request user restamping it."""
model.objects.filter(pk=obj.pk).update(created_by=user)
obj.refresh_from_db()
return obj
@pytest.fixture
def superuser(regular_user):
"""The ``user_client`` caller, still a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
@pytest.fixture
def others_account(org_a, admin_user):
return _stamp(
Account, Account.objects.create(name="Zyx Others", org=org_a), admin_user
)
@pytest.fixture
def foreign_account(org_b, user_b):
with rls_org(org_b):
return _stamp(
Account, Account.objects.create(name="Zyx Foreign", org=org_b), user_b
)
@pytest.fixture
def others_case(org_a, admin_user):
case = Case.objects.create(
name="Zyx ticket", status="New", priority="Normal", org=org_a
)
return _stamp(Case, case, admin_user)
@pytest.fixture
def foreign_case(org_b, user_b):
with rls_org(org_b):
case = Case.objects.create(
name="Zyx foreign ticket", status="New", priority="Normal", org=org_b
)
return _stamp(Case, case, user_b)
def _watch(case, profile):
CaseWatcher.objects.create(case=case, profile=profile, org=case.org)
def _task(org, user, title, **extra):
task = Task.objects.create(
title=title, status="New", priority="Low", org=org, **extra
)
return _stamp(Task, task, user)
@pytest.fixture
def others_task(org_a, admin_user):
return _task(org_a, admin_user, "Zyx task", due_date=timezone.localdate())
@pytest.fixture
def foreign_task(org_b, user_b):
with rls_org(org_b):
return _task(org_b, user_b, "Zyx foreign task", due_date=timezone.localdate())
# ── accounts: search, list, task picker ───────────────────────────────────
def _search_ids(client, kind):
response = client.get("/api/search/", {"q": "Zyx"})
assert response.status_code == status.HTTP_200_OK
return {r["id"] for r in response.data["results"] if r["type"] == kind}
def _account_list_ids(client):
response = client.get("/api/accounts/")
assert response.status_code == status.HTTP_200_OK
return {str(row["id"]) for row in response.data["active_accounts"]["open_accounts"]}
def _task_picker_account_ids(client):
response = client.get("/api/tasks/")
assert response.status_code == status.HTTP_200_OK, response.content
return {str(row["id"]) for row in response.data["accounts_list"]}
ACCOUNT_PATHS = [_search_ids, _account_list_ids, _task_picker_account_ids]
def _account_ids(path, client):
if path is _search_ids:
return _search_ids(client, "account")
return path(client)
class TestAccountsFollowTheDetailRule:
@pytest.mark.parametrize("path", ACCOUNT_PATHS)
def test_plain_member_is_not_shown_it(self, path, user_client, others_account):
response = user_client.get(f"/api/accounts/{others_account.pk}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
assert str(others_account.id) not in _account_ids(path, user_client)
@pytest.mark.parametrize("path", ACCOUNT_PATHS)
def test_superuser_is_shown_it(self, path, superuser, user_client, others_account):
response = user_client.get(f"/api/accounts/{others_account.pk}/")
assert response.status_code == status.HTTP_200_OK
assert str(others_account.id) in _account_ids(path, user_client)
@pytest.mark.parametrize("path", ACCOUNT_PATHS)
def test_superuser_never_sees_another_orgs(
self, path, superuser, user_client, others_account, foreign_account
):
ids = _account_ids(path, user_client)
assert str(others_account.id) in ids
assert str(foreign_account.id) not in ids
# ── dashboard counts ──────────────────────────────────────────────────────
def _home(client):
response = client.get("/api/dashboard/")
assert response.status_code == status.HTTP_200_OK
return response.data
class TestDashboardContactCount:
@pytest.fixture
def others_contact(self, org_a, admin_user):
return _stamp(
Contact,
Contact.objects.create(first_name="Zyx", last_name="C", org=org_a),
admin_user,
)
@pytest.fixture
def foreign_contact(self, org_b, user_b):
with rls_org(org_b):
return _stamp(
Contact,
Contact.objects.create(first_name="Zyx", last_name="F", org=org_b),
user_b,
)
def test_plain_member_does_not_count_it(self, user_client, others_contact):
assert _home(user_client)["contacts_count"] == 0
def test_member_assigned_to_its_account_counts_it(
self, org_a, user_profile, user_client, others_contact
):
account = Account.objects.create(name="Via account", org=org_a)
account.assigned_to.add(user_profile)
account.contacts.add(others_contact)
# The detail view and the list already admit this member.
response = user_client.get(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_200_OK
assert _home(user_client)["contacts_count"] == 1
def test_another_orgs_contact_is_never_counted(
self, superuser, user_client, others_contact, foreign_contact
):
assert _home(user_client)["contacts_count"] == 1
class TestDashboardAccountCount:
def test_superuser_counts_it_and_a_member_does_not(
self, regular_user, user_client, others_account, foreign_account
):
assert _home(user_client)["accounts_count"] == 0
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
assert _home(user_client)["accounts_count"] == 1
class TestDashboardTasks:
"""Tasks have no superuser clause, so the dashboard must not invent one."""
def test_superuser_does_not_get_a_task_they_cannot_open(
self, superuser, user_client, others_task
):
response = user_client.get(f"/api/tasks/{others_task.pk}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
data = _home(user_client)
assert str(others_task.id) not in {str(t["id"]) for t in data["tasks"]}
assert data["urgent_counts"]["tasks_due_today"] == 0
def test_assignee_gets_it(self, user_profile, user_client, others_task):
others_task.assigned_to.add(user_profile)
data = _home(user_client)
assert str(others_task.id) in {str(t["id"]) for t in data["tasks"]}
assert data["urgent_counts"]["tasks_due_today"] == 1
def test_another_orgs_task_is_never_counted(
self, user_profile, user_client, others_task, foreign_task
):
others_task.assigned_to.add(user_profile)
data = _home(user_client)
assert str(foreign_task.id) not in {str(t["id"]) for t in data["tasks"]}
assert data["urgent_counts"]["tasks_due_today"] == 1
# ── Today queue ───────────────────────────────────────────────────────────
def _queue_ids(client):
response = client.get("/api/dashboard/today/")
assert response.status_code == status.HTTP_200_OK
return {row["id"] for row in response.data["queue"]}
class TestTodayFollowsTheDetailRule:
def test_watcher_can_open_the_ticket_but_is_not_asked_to_reply(
self, user_profile, user_client, others_case
):
"""D44: every queue row carries an action, and a ticket's is "Reply",
which the write rule refuses a watcher. The read rule still lets them
open it; the queue just does not offer them a button that 403s. The
full pair lives in ``test_today_actionable.py``."""
_watch(others_case, user_profile)
response = user_client.get(f"/api/cases/{others_case.pk}/")
assert response.status_code == status.HTTP_200_OK
assert f"case-{others_case.id}" not in _queue_ids(user_client)
def test_plain_member_does_not_see_it(self, user_client, others_case):
assert f"case-{others_case.id}" not in _queue_ids(user_client)
def test_superuser_does_not_get_rows_they_cannot_open(
self, superuser, user_client, others_case, others_task
):
assert (
user_client.get(f"/api/cases/{others_case.pk}/").status_code
== status.HTTP_403_FORBIDDEN
)
ids = _queue_ids(user_client)
assert f"case-{others_case.id}" not in ids
assert f"task-{others_task.id}" not in ids
def test_another_orgs_rows_never_appear(
self, user_profile, user_client, others_case, foreign_case, foreign_task
):
others_case.assigned_to.add(user_profile)
ids = _queue_ids(user_client)
assert f"case-{others_case.id}" in ids
assert f"case-{foreign_case.id}" not in ids
assert f"task-{foreign_task.id}" not in ids
# ── boards ────────────────────────────────────────────────────────────────
def _board_ids(client, url, key):
response = client.get(url)
assert response.status_code == status.HTTP_200_OK, response.content
return [str(card["id"]) for col in response.data["columns"] for card in col[key]]
def _case_board(client):
return _board_ids(client, "/api/cases/kanban/", "cases")
def _task_board(client):
return _board_ids(client, "/api/tasks/kanban/", "tasks")
class TestCaseBoard:
def test_watcher_sees_it(self, user_profile, user_client, others_case):
_watch(others_case, user_profile)
assert str(others_case.id) in _case_board(user_client)
def test_plain_member_does_not(self, user_client, others_case):
assert str(others_case.id) not in _case_board(user_client)
def test_superuser_does_not_see_a_ticket_they_cannot_open(
self, superuser, user_client, others_case
):
assert str(others_case.id) not in _case_board(user_client)
def test_another_orgs_ticket_never_appears(
self, admin_client, others_case, foreign_case
):
ids = _case_board(admin_client)
assert str(others_case.id) in ids
assert str(foreign_case.id) not in ids
class TestTaskBoard:
def test_creator_with_two_assignees_sees_one_card(
self, regular_user, user_profile, admin_profile, user_client, others_task
):
_stamp(Task, others_task, regular_user)
others_task.assigned_to.add(user_profile, admin_profile)
assert _task_board(user_client).count(str(others_task.id)) == 1
def test_assignee_sees_it_once(
self, user_profile, admin_profile, user_client, others_task
):
others_task.assigned_to.add(user_profile, admin_profile)
assert _task_board(user_client).count(str(others_task.id)) == 1
def test_plain_member_does_not(self, user_client, others_task):
assert str(others_task.id) not in _task_board(user_client)
def test_superuser_does_not_see_a_task_they_cannot_open(
self, superuser, user_client, others_task
):
assert str(others_task.id) not in _task_board(user_client)
def test_another_orgs_task_never_appears(
self, admin_client, others_task, foreign_task
):
ids = _task_board(admin_client)
assert str(others_task.id) in ids
assert str(foreign_task.id) not in ids

View file

@ -53,6 +53,34 @@ def test_win_probability_rejected_on_non_lead_stage():
validate_manifest(raw)
def _lead_stage(**stage):
return _minimal() | {
"lead_pipeline": {"name": "Sales", "stages": [{"name": "New", **stage}]}
}
# A lead stage's win_probability is copied onto Lead.probability by the board
# move, and both columns carry a 0-100 check constraint (D11).
@pytest.mark.parametrize("value", [150, 101, -1, "50", True, False, 50.0, None])
def test_lead_stage_win_probability_out_of_range_or_not_an_int_is_rejected(value):
with pytest.raises(
PackValidationError,
match=r"stages\[0\]: win_probability .* must be an integer from 0 to 100",
):
validate_manifest(_lead_stage(win_probability=value))
@pytest.mark.parametrize("value", [0, 50, 100])
def test_lead_stage_win_probability_in_range_is_accepted(value):
raw = _lead_stage(win_probability=value)
assert validate_manifest(raw) is raw
def test_lead_stage_without_win_probability_is_accepted():
raw = _lead_stage()
assert validate_manifest(raw) is raw
def test_unknown_tag_color_is_rejected():
raw = _minimal() | {"tags": [{"name": "Hot", "color": "chartreuse"}]}
with pytest.raises(PackValidationError, match="chartreuse"):

View file

@ -0,0 +1,132 @@
"""D44: every row in the Today queue offers an action its caller can take.
D40 narrowed each Today source by its read rule. For tickets that admits
watchers, so a ticket a member only watches landed in "awaiting first reply"
with a "Reply" button, and the reply endpoint (``CaseDetailView.post``, the
write rule in ``cases/access.py``) refused it. The ticket source now uses the
write rule. Deals and tasks offer "Open", invoices "Send a reminder", and each
of those endpoints checks the same rule the source is narrowed by, so those
three were already actionable; the last test pins the invoice one.
"""
import datetime
import pytest
from django.utils import timezone
from rest_framework import status
from cases.models import Case, CaseWatcher
from invoices.models import Invoice
pytestmark = pytest.mark.django_db
URL = "/api/dashboard/today/"
def _stamp(model, obj, user):
model.objects.filter(pk=obj.pk).update(created_by=user)
obj.refresh_from_db()
return obj
@pytest.fixture
def unanswered_case(org_a, admin_user):
"""A colleague's ticket awaiting a first reply."""
case = Case.objects.create(
name="Awaiting reply",
status="New",
priority="High",
org=org_a,
first_response_at=None,
)
return _stamp(Case, case, admin_user)
def _today(client):
response = client.get(URL)
assert response.status_code == status.HTTP_200_OK, response.content
return response.data
def _row(data, case):
return next((r for r in data["queue"] if r["id"] == f"case-{case.id}"), None)
def _ticket_count(data):
return sum(
s["count"] for s in data["summary"]["sources"] if s["href"] == "/tickets"
)
class TestTicketsAreOfferedOnlyToThoseWhoCanReply:
def test_watcher_only_ticket_is_not_offered(
self, user_profile, user_client, unanswered_case
):
CaseWatcher.objects.create(
case=unanswered_case, profile=user_profile, org=unanswered_case.org
)
data = _today(user_client)
assert _row(data, unanswered_case) is None
# The header count is of rows that want the caller, so it agrees.
assert _ticket_count(data) == 0
assert data["summary"]["count"] == 0
def test_assignees_ticket_is_offered_with_reply(
self, user_profile, user_client, unanswered_case
):
unanswered_case.assigned_to.add(user_profile)
data = _today(user_client)
row = _row(data, unanswered_case)
assert row is not None
assert row["action"] == "Reply"
assert _ticket_count(data) == 1
def test_the_reply_it_offers_is_accepted_and_the_one_it_hides_is_not(
self, user_profile, user_client, unanswered_case
):
"""The queue's choice is the reply endpoint's own answer."""
CaseWatcher.objects.create(
case=unanswered_case, profile=user_profile, org=unanswered_case.org
)
url = f"/api/cases/{unanswered_case.pk}/"
body = {"comment": "On it"}
assert (
user_client.post(url, body, format="json").status_code
== status.HTTP_403_FORBIDDEN
)
unanswered_case.assigned_to.add(user_profile)
assert (
user_client.post(url, body, format="json").status_code
!= status.HTTP_403_FORBIDDEN
)
def test_admin_is_offered_a_ticket_they_neither_opened_nor_hold(
self, admin_client, regular_user, unanswered_case
):
_stamp(Case, unanswered_case, regular_user)
assert _row(_today(admin_client), unanswered_case) is not None
class TestInvoicesAreActionableByWhoeverSeesThem:
def test_assigned_invoice_is_offered_and_its_send_is_accepted(
self, org_a, admin_user, user_profile, user_client
):
inv = Invoice.objects.create(
invoice_title="Overdue",
org=org_a,
currency="USD",
total_amount=100,
status="Sent",
due_date=timezone.localdate() - datetime.timedelta(days=3),
)
_stamp(Invoice, inv, admin_user)
assert f"invoice-{inv.id}" not in {
r["id"] for r in _today(user_client)["queue"]
}
inv.assigned_to.add(user_profile)
row = next(
r for r in _today(user_client)["queue"] if r["id"] == f"invoice-{inv.id}"
)
assert row["action"] == "Send a reminder"
send = user_client.post(f"/api/invoices/{inv.id}/send/", {}, format="json")
assert send.status_code == status.HTTP_200_OK

View file

@ -9,17 +9,24 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from accounts.access import visible_accounts_qs
from accounts.models import Account
from cases.access import visible_cases_qs, writable_cases_qs
from cases.models import Case
from common import serializer, swagger_params
from common.models import Activity
from common.permissions import HasOrgContext, is_org_admin
from common.utils import STAGES
from contacts.access import visible_contacts_qs
from contacts.models import Contact
from invoices.models import UNPAID_STATUSES, Invoice
from invoices.permissions import visible_invoices_qs
from leads.access import visible_leads_qs
from leads.models import Lead
from opportunity.access import visible_deals_qs
from opportunity.models import Opportunity, StageAgingConfig
from opportunity.workflow import DEFAULT_STAGE_EXPECTED_DAYS, ROTTEN_MULTIPLIER
from tasks.access import visible_tasks_qs
from tasks.models import Task
from tasks.serializer import TaskSerializer
@ -64,23 +71,56 @@ def _fmt_date(d):
return f"{d:%b} {d.day}"
def _owned_or_assigned(queryset, profile):
"""Narrow a queryset to what a non-admin may see, without joining.
def _readable(queryset, visible):
"""Narrow ``queryset`` to the rows in ``visible``, without joining.
Filtering straight onto the ``assigned_to`` M2M multiplies rows: a record
the member created that also carries three assignees comes back three
times, because the OR forces a LEFT JOIN and every joined row satisfies the
``created_by`` half. That inflates ``.count()`` and, worse, ``Sum()``, so a
member's pipeline total could read three times its real value.
Resolving the ids in a subquery keeps the outer query at one row per
record, which is what the counts and the aggregates below assume.
``visible`` is the module's own read rule (``visible_accounts_qs`` and its
siblings), so every count and total here matches what the caller's list
shows and what the detail view opens. It is applied by id: filtering
straight onto the ``assigned_to`` M2M multiplies rows, a record carrying
three assignees comes back three times, which inflates ``.count()`` and,
worse, ``Sum()``. Resolving the ids in a subquery keeps the outer query at
one row per record, which is what the counts and the aggregates assume.
"""
return queryset.filter(
pk__in=queryset.filter(
Q(assigned_to=profile) | Q(created_by=profile.user)
).values("pk")
)
return queryset.filter(pk__in=visible.values("pk"))
# The read rule for each `Activity.entity_type` a feed may name. An activity
# carries the record's name and what was done to it, so showing one is showing
# a slice of that record: it has to pass the same rule the record's own detail
# view applies. Types without an entry here (Event, Document, Team: nothing
# writes them today) have no rule to pass, so a member never sees them.
_ACTIVITY_READ_RULES = {
"Account": lambda profile, user: visible_accounts_qs(profile, user),
"Lead": lambda profile, user: visible_leads_qs(profile, user),
"Contact": lambda profile, user: visible_contacts_qs(profile),
"Opportunity": lambda profile, user: visible_deals_qs(profile, user),
"Case": lambda profile, user: visible_cases_qs(profile),
"Task": lambda profile, user: visible_tasks_qs(profile),
"Invoice": lambda profile, user: visible_invoices_qs(profile, user),
}
def _readable_activities(profile, user):
"""The org's activities that ``profile`` may see: one query, whatever the size.
An admin sees the whole org's feed, as before. Anyone else sees an activity
only while its record is one they can open, resolved as one id subquery per
entity type, so the cost does not grow with the number of rows returned.
That fails closed twice: an entity type with no read rule is hidden, and so
is an activity whose record has since been deleted, because no read rule
can match a row that no longer exists.
"""
qs = Activity.objects.filter(org=profile.org)
if is_org_admin(profile):
return qs
readable = Q()
for entity_type, rule in _ACTIVITY_READ_RULES.items():
readable |= Q(
entity_type=entity_type,
entity_id__in=rule(profile, user).values("pk"),
)
return qs.filter(readable)
class ApiHomeView(APIView):
@ -113,24 +153,31 @@ class ApiHomeView(APIView):
profile = request.profile
today = timezone.localdate()
accounts = Account.objects.filter(is_active=True, org=org)
contacts = Contact.objects.filter(org=org)
user = request.user
# Each one is its module's read rule, so every figure below counts the
# rows the matching list shows. They differ on purpose: contacts add
# account assignment, and tasks have no superuser clause.
accounts = _readable(
Account.objects.filter(is_active=True, org=org),
visible_accounts_qs(profile, user),
)
contacts = _readable(
Contact.objects.filter(org=org), visible_contacts_qs(profile)
)
# Kept separate from `leads` because the conversion rate below needs
# converted leads, which `leads` deliberately excludes.
all_leads = Lead.objects.filter(org=org)
all_leads = _readable(
Lead.objects.filter(org=org), visible_leads_qs(profile, user)
)
leads = all_leads.exclude(Q(status="converted") | Q(status="closed"))
opportunities = Opportunity.objects.filter(org=org)
tasks = Task.objects.filter(org=org)
opportunities = _readable(
Opportunity.objects.filter(org=org), visible_deals_qs(profile, user)
)
tasks = _readable(Task.objects.filter(org=org), visible_tasks_qs(profile))
is_admin = is_org_admin(profile) or request.user.is_superuser
if not is_admin:
accounts = _owned_or_assigned(accounts, profile)
contacts = _owned_or_assigned(contacts, profile)
all_leads = _owned_or_assigned(all_leads, profile)
leads = _owned_or_assigned(leads, profile).exclude(status="closed")
opportunities = _owned_or_assigned(opportunities, profile)
tasks = _owned_or_assigned(tasks, profile)
# Decides only whether the goal summary below includes org-wide goals.
is_admin = is_org_admin(profile) or user.is_superuser
# Counts only. This used to serialize every account, contact, lead and
# opportunity in the org in full beside them: 372 KB of a 384 KB
@ -314,9 +361,10 @@ class ApiHomeView(APIView):
for g in active_goals
]
# Include recent activities (avoid separate API call)
# Recent activities, narrowed like every figure above: a member sees
# activity only on records they can open.
activities = (
Activity.objects.filter(org=org)
_readable_activities(profile, user)
.select_related("user", "user__user")
.order_by("-created_at")[:10]
)
@ -340,13 +388,18 @@ class ApiTodayView(APIView):
* tasks overdue or due today.
Security: every query is org-scoped. The org comes from the JWT via
middleware, never the client, and a member sees only rows assigned to or
created by them, the same visibility ``ApiHomeView`` applies. Admins see the
whole org. ``HasOrgContext`` guarantees ``request.profile``/``org`` are set,
so this never dereferences a ``None`` profile.
middleware, never the client, and each source is narrowed by its own
module's read rule, as ``ApiHomeView`` does, so the queue never offers a
row the caller cannot open. Every row also carries an action, so each
source is one the caller can act on: opening a deal or a task, and sending
an invoice, need nothing beyond reading it, but replying to a ticket needs
the write rule, which leaves watchers out. A ticket someone only watches
is therefore not in the queue, nor in its count. ``HasOrgContext``
guarantees ``request.profile``/``org`` are set, so this never dereferences
a ``None`` profile.
"""
permission_classes = (IsAuthenticated,)
permission_classes = (IsAuthenticated, HasOrgContext)
@extend_schema(
tags=["home"],
@ -383,31 +436,39 @@ class ApiTodayView(APIView):
today = timezone.localdate()
yesterday = today - timedelta(days=1)
week_end = today + timedelta(days=7)
is_admin = is_org_admin(profile) or request.user.is_superuser
org_currency = org.default_currency or "USD"
def mine(qs):
"""Restrict a queryset to rows a member may see. ``created_by`` is a
User FK (compared to ``profile.user``); ``assigned_to`` is an M2M of
Profile: the OR-over-a-join can duplicate rows, so distinct()."""
return qs.filter(
Q(assigned_to__id__in=[profile.id]) | Q(created_by=profile.user)
).distinct()
user = request.user
# Each source is narrowed by its module's read rule, so a row here is
# one the caller can open and a count here matches the list it links
# to. Resolved once, reused by the queue, the summary and "later".
visible_deals = visible_deals_qs(profile, user)
visible_cases = visible_cases_qs(profile)
visible_invoices = visible_invoices_qs(profile, user)
visible_tasks = visible_tasks_qs(profile)
# ── base, org-scoped querysets ──────────────────────────────────────
opportunities = Opportunity.objects.filter(org=org, stage__in=OPEN_STAGES)
cases = Case.objects.filter(org=org, status__in=OPEN_CASE_STATUSES)
invoices = Invoice.objects.filter(
org=org, status__in=UNPAID_STATUSES, due_date__lt=today
opportunities = _readable(
Opportunity.objects.filter(org=org, stage__in=OPEN_STAGES), visible_deals
)
tasks = Task.objects.filter(
org=org, status__in=["New", "In Progress"], due_date__lte=today
# The one source whose action needs more than reading: "Reply" is a
# write, so a ticket the caller only watches would be a dead button.
cases = _readable(
Case.objects.filter(org=org, status__in=OPEN_CASE_STATUSES),
writable_cases_qs(profile),
)
invoices = _readable(
Invoice.objects.filter(
org=org, status__in=UNPAID_STATUSES, due_date__lt=today
),
visible_invoices,
)
tasks = _readable(
Task.objects.filter(
org=org, status__in=["New", "In Progress"], due_date__lte=today
),
visible_tasks,
)
if not is_admin:
opportunities = mine(opportunities)
cases = mine(cases)
invoices = mine(invoices)
tasks = mine(tasks)
# ── deal aging as DB date cutoffs (no per-row Python) ───────────────
# For each open stage: "quiet" (yellow+) once the deal has sat past its
@ -562,15 +623,16 @@ class ApiTodayView(APIView):
# ── "cleared yesterday" (a morale line) ─────────────────────────────
# Tasks have no completed_at, so proxy with "marked Completed and last
# touched yesterday"; cases carry a real closed_on date.
cleared_tasks = Task.objects.filter(
org=org, status="Completed", updated_at__date=yesterday
cleared_tasks = _readable(
Task.objects.filter(
org=org, status="Completed", updated_at__date=yesterday
),
visible_tasks,
)
cleared_cases = Case.objects.filter(
org=org, status="Closed", closed_on=yesterday
cleared_cases = _readable(
Case.objects.filter(org=org, status="Closed", closed_on=yesterday),
visible_cases,
)
if not is_admin:
cleared_tasks = mine(cleared_tasks)
cleared_cases = mine(cleared_cases)
summary = {
"count": total_urgent,
@ -583,22 +645,30 @@ class ApiTodayView(APIView):
# ── "later this week" (due tomorrow … +7 days) ──────────────────────
soon = Q(due_date__gt=today, due_date__lte=week_end)
later_tasks = Task.objects.filter(
org=org, status__in=["New", "In Progress"]
).filter(soon)
later_opps = Opportunity.objects.filter(
org=org, stage__in=OPEN_STAGES, closed_on__gt=today, closed_on__lte=week_end
later_tasks = _readable(
Task.objects.filter(org=org, status__in=["New", "In Progress"]).filter(
soon
),
visible_tasks,
)
later_invoices = Invoice.objects.filter(
org=org,
status__in=UNPAID_STATUSES,
due_date__gt=today,
due_date__lte=week_end,
later_opps = _readable(
Opportunity.objects.filter(
org=org,
stage__in=OPEN_STAGES,
closed_on__gt=today,
closed_on__lte=week_end,
),
visible_deals,
)
later_invoices = _readable(
Invoice.objects.filter(
org=org,
status__in=UNPAID_STATUSES,
due_date__gt=today,
due_date__lte=week_end,
),
visible_invoices,
)
if not is_admin:
later_tasks = mine(later_tasks)
later_opps = mine(later_opps)
later_invoices = mine(later_invoices)
later_rows = []
for t in later_tasks.order_by("due_date")[:10]:
@ -647,12 +717,13 @@ class ApiTodayView(APIView):
class ActivityListView(APIView):
"""
Get recent activities for the organization
Returns the last 10 activities by default
"""Recent activities the caller may see, newest first, 10 by default.
Scoped the way the dashboard's feed is (`_readable_activities`): an admin
sees the org, a member sees activity on records they can open.
"""
permission_classes = (IsAuthenticated,)
permission_classes = (IsAuthenticated, HasOrgContext)
@extend_schema(
tags=["activities"],
@ -674,18 +745,18 @@ class ActivityListView(APIView):
responses={200: serializer.DashboardActivitySerializer(many=True)},
)
def get(self, request, *args, **kwargs):
if not request.profile:
return Response(
{"error": True, "errors": "Organization context required"},
status=status.HTTP_400_BAD_REQUEST,
)
# Get query params
limit = min(int(request.query_params.get("limit", 10)), 50)
# A malformed or negative `limit` used to reach `int()` and the slice
# unguarded, a 500 either way. Anything unusable is the default.
try:
limit = int(request.query_params.get("limit", 10))
except (TypeError, ValueError):
limit = 10
if limit < 1:
limit = 10
limit = min(limit, 50)
entity_type = request.query_params.get("entity_type", None)
# Query activities for this organization
queryset = Activity.objects.filter(org=request.profile.org)
queryset = _readable_activities(request.profile, request.user)
# Filter by entity type if specified
if entity_type:

View file

@ -11,7 +11,7 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from cases.models import Case
from cases.serializer import CaseSerializer
from cases.serializer import CaseSerializer, parent_access_context
from common import swagger_params
from common.models import Comment, PersonalAccessToken, Profile, Teams, User
from common.permissions import HasOrgContext, is_org_admin
@ -341,7 +341,9 @@ class UserDetailView(APIView):
)
context["contacts"] = ContactSerializer(contacts, many=True).data
cases = Case.objects.filter(assigned_to=profile_obj, org=request.profile.org)
context["cases"] = CaseSerializer(cases, many=True).data
context["cases"] = CaseSerializer(
cases, many=True, context=parent_access_context(request.profile, cases)
).data
context["assigned_data"] = assigned_data
comments = Comment.objects.filter(
commented_by=profile_obj, org=request.profile.org

View file

@ -37,8 +37,15 @@ def has_contact_access(profile, contact):
Assignment to the account the person belongs to counts as access. Whoever
owns the company owns the conversation with the people at it, and there is
no reading under which that is true for viewing but false for editing.
A Django superuser sees every contact in their own org, as they do every
account, deal and invoice. The flag is read from ``profile.user``, the
identity the request authenticated as, so a caller without a request (the
case CSV import, through `visible_contacts_qs`) gets the same answer. Like
the admin branch, it does not check the org itself: every caller fetches
``contact`` with ``org=profile.org`` first.
"""
if is_org_admin(profile):
if is_org_admin(profile) or profile.user.is_superuser:
return True
if profile.user_id == contact.created_by_id:
return True
@ -51,7 +58,7 @@ def has_contact_access(profile, contact):
def visible_contacts_qs(profile):
"""Contacts ``profile`` may open, the queryset form of `has_contact_access`."""
qs = Contact.objects.filter(org=profile.org)
if is_org_admin(profile):
if is_org_admin(profile) or profile.user.is_superuser:
return qs
my_accounts = profile.account_assigned_users.values_list("id", flat=True)
return qs.filter(

View file

@ -2,7 +2,8 @@
Preview reads the uploaded file and returns row-level validation results
without touching the DB. Commit re-runs validation and writes inside a single
transaction. Both endpoints are gated to ADMIN or sales-access users so
transaction. Both endpoints are gated by
`common.permissions.can_mass_import` (admins, superusers, sales access) so
non-privileged members cannot mass-create contacts through this surface.
"""
@ -13,23 +14,12 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from common.permissions import HasOrgContext
from common.permissions import HasOrgContext, can_mass_import
from contacts.services.csv_import import commit_rows, parse_and_validate
MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint
def _can_import(profile) -> bool:
"""Mass-create requires admin or explicit sales-access permission."""
if profile is None:
return False
if getattr(profile, "role", None) == "ADMIN":
return True
if getattr(profile, "is_admin", False):
return True
return bool(getattr(profile, "has_sales_access", False))
def _read_upload(request):
"""Return (file_bytes, error_response). One of the two will be None.
@ -83,7 +73,7 @@ class ContactImportPreviewView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,
@ -91,7 +81,7 @@ class ContactImportPreviewView(APIView):
file_bytes, err = _read_upload(request)
if err is not None:
return err
result = parse_and_validate(file_bytes, request.profile.org)
result = parse_and_validate(file_bytes, request.profile.org, request.profile)
return Response(result.to_dict(), status=status.HTTP_200_OK)
@ -117,7 +107,7 @@ class ContactImportCommitView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,

View file

@ -1,6 +1,7 @@
from drf_spectacular.utils import extend_schema_field
from rest_framework import serializers
from accounts.access import has_account_access
from common.serializer import (
AttachmentsSerializer,
OrganizationSerializer,
@ -138,9 +139,14 @@ class CreateContactSerializer(serializers.ModelSerializer):
# or, worse, a check that quietly passes because there was nothing to
# compare against.
self.org = request_obj.profile.org if request_obj else None
self.profile = request_obj.profile if request_obj else None
self.user = request_obj.user if request_obj else None
# An id that matches no account at all reads the same as one the
# caller may not open; see `validate_account`.
self.fields["account"].error_messages["does_not_exist"] = "No such account."
def validate_account(self, account):
"""An account from somebody else's org is not a valid link.
"""An account the caller may not open is not a valid link.
`account` is a plain ModelSerializer field, so DRF resolved it against
`Account.objects.all()` -- every account in the database, not the ones
@ -149,8 +155,28 @@ class CreateContactSerializer(serializers.ModelSerializer):
that in a correctly configured deployment, because the lookup runs
under the tenant policy; it did not stop it here, where the dev role is
a superuser. The org filter is the contract either way.
Nor is an account in this org that the caller may not open. Linking a
contact to one hands the contact to that account's assignees (see
`contacts.access.has_contact_access`), so a member could push a person
into a company they cannot see. The rule is `has_account_access`, the
one the account detail view applies, and an unknown id, another org's
account and a hidden one all get the same message, so a response never
confirms that a hidden account exists.
The account the contact is already linked to is kept whoever saves.
The caller's form never had a choice about it, and re-sending it (the
mobile form always does) is not a new link.
"""
if account is not None and (self.org is None or account.org_id != self.org.id):
if account is None:
return account
if self.instance is not None and account.id == self.instance.account_id:
return account
if (
self.org is None
or account.org_id != self.org.id
or not has_account_access(self.profile, self.user, account)
):
raise serializers.ValidationError("No such account.")
return account

View file

@ -17,9 +17,15 @@ Duplicate policy (per org):
disambiguate from an existing same-named contact. Two people
can legitimately share a name when other identifiers differ.
Phone and full name are matched only against contacts the importer may open
(`visible_contacts_qs`): nothing in the database forces them unique, so a
refusal that named a hidden contact would only confirm it exists. Email is
checked across the org because the constraint forces it.
All reference lookups (account, assignees, teams) are bulk-prefetched once per
call (one SELECT per reference type), scoped to the caller's org so a
malicious CSV cannot reach across tenants.
malicious CSV cannot reach across tenants. `account_name` resolves only among
the accounts the importer may open, so a hidden account reads as a missing one.
"""
from __future__ import annotations
@ -35,10 +41,11 @@ from django.core.validators import URLValidator
from django.db import IntegrityError, transaction
from django.db.models.functions import Lower
from accounts.models import Account
from accounts.access import visible_accounts_qs
from common.models import Profile, Tags, Teams
from common.utils import COUNTRIES
from common.validators import normalize_phone
from contacts.access import visible_contacts_qs
from contacts.models import Contact
from contacts.services.account_link import link_primary_account
@ -212,11 +219,12 @@ def _parse_bool(raw: str) -> bool | None:
return None # caller treats None as "invalid input"
def parse_and_validate(file_bytes: bytes, org) -> ImportResult:
def parse_and_validate(file_bytes: bytes, org, profile) -> ImportResult:
"""Parse a CSV byte string and validate every row against the given org.
All reference and duplicate lookups are scoped to `org` so a malicious CSV
cannot reach across tenants.
cannot reach across tenants. `profile` is the importer, whose account
access bounds `account_name`.
"""
text = _decode(file_bytes)
if text is None:
@ -279,7 +287,7 @@ def parse_and_validate(file_bytes: bytes, org) -> ImportResult:
}
parsed.append((idx, record))
ref_maps = _build_ref_maps(parsed, org)
ref_maps = _build_ref_maps(parsed, org, profile)
valid: list[ValidatedRow] = []
errors: list[RowError] = []
@ -311,7 +319,7 @@ def parse_and_validate(file_bytes: bytes, org) -> ImportResult:
return ImportResult(valid=valid, errors=errors)
def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps:
def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org, profile) -> _RefMaps:
"""Bulk-prefetch every reference value referenced anywhere in the file.
One query per reference type, scoped to `org`. Keys are lowercased so
@ -348,10 +356,14 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps:
if first and last and not email and not phone:
candidate_full_names.add(f"{first.lower()}|{last.lower()}")
# Only accounts the importer may open. Resolving across the whole org told
# the importer a hidden account existed ("No account named ..." for a name
# nobody holds, a valid row for a hidden one), and linking a contact to an
# account hands it to that account's assignees (`has_contact_access`).
accounts: dict[str, str] = {}
if account_names:
for pk, name_lower in (
Account.objects.filter(org=org)
visible_accounts_qs(profile, profile.user)
.annotate(name_lower=Lower("name"))
.filter(name_lower__in=account_names)
.values_list("id", "name_lower")
@ -387,13 +399,18 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps:
.values_list("email_lower", flat=True)
)
# Phone and full name carry no DB constraint, so unlike email these two
# checks are a courtesy, and they look only at contacts the importer may
# open. Across the whole org they were an oracle: "A contact with this
# phone number already exists" for a number held only by a hidden contact
# confirmed that contact existed.
existing_phones: set[str] = set()
if candidate_phones:
# Phone has no DB constraint and no normalized column; we have to scan
# all contacts that have a phone in this org and normalize in Python.
# Bounded by org size, not file size, fine for any reasonable tenant.
# No normalized column, so scan the visible contacts that have a phone
# and normalize in Python. Bounded by org size, not file size, fine for
# any reasonable tenant.
for raw_phone in (
Contact.objects.filter(org=org)
visible_contacts_qs(profile)
.exclude(phone__isnull=True)
.exclude(phone="")
.values_list("phone", flat=True)
@ -407,7 +424,7 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps:
# Only run this query if any row has a full name but no email and no
# phone. That's the only case where full-name dedup applies.
for first, last in (
Contact.objects.filter(org=org)
visible_contacts_qs(profile)
.annotate(
first_lower=Lower("first_name"),
last_lower=Lower("last_name"),
@ -583,7 +600,11 @@ def _validate_and_build(
account_id = refs.accounts.get(account_name.lower())
if account_id is None:
errors.append(
RowError(idx, "account_name", f"No account named '{account_name}'")
RowError(
idx,
"account_name",
f"No account you can open is named '{account_name}'",
)
)
assigned_ids: list[str] = []
@ -677,7 +698,7 @@ def commit_rows(file_bytes: bytes, org, profile) -> dict[str, Any]:
by another request) are caught and reported as a 400-equivalent payload
so the user sees an actionable message instead of a generic 500.
"""
result = parse_and_validate(file_bytes, org)
result = parse_and_validate(file_bytes, org, profile)
if result.header_error:
return {
"error": True,

View file

@ -0,0 +1,188 @@
"""D37: the contact read rule ignored Django ``is_superuser``.
``has_contact_access`` and ``visible_contacts_qs`` let an org admin through
and nobody else by role, while the account, deal, lead and invoice rules all
admit a superuser too. They now read ``profile.user.is_superuser``. The org
filter stays: a superuser sees every contact in their own org, never another
org's.
The contact list, the global search and the contact pickers on the lead,
account, deal, ticket and task lists each carried an inline copy of the rule
that knew neither superusers nor account assignment. They now call
``visible_contacts_qs``, so these tests pin them too.
"""
import pytest
from rest_framework import status
from accounts.models import Account
from common.models import Profile
from contacts.access import has_contact_access, visible_contacts_qs
from contacts.models import Contact
pytestmark = pytest.mark.django_db
@pytest.fixture
def others_contact(org_a, admin_user):
"""A contact the ``user_client`` caller neither created nor is assigned to."""
return Contact.objects.create(
first_name="Grace", last_name="Hopper", org=org_a, created_by=admin_user
)
@pytest.fixture
def foreign_contact(org_b, user_b):
"""A contact in the other tenant."""
return Contact.objects.create(
first_name="Gracie", last_name="Elsewhere", org=org_b, created_by=user_b
)
@pytest.fixture
def superuser(regular_user):
"""The ``user_client`` caller, with a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
def _fresh(profile):
"""Reload so ``profile.user`` reflects the flag as the database holds it."""
return Profile.objects.select_related("user", "org").get(pk=profile.pk)
class TestPredicate:
def test_plain_member_is_refused(self, user_profile, others_contact):
assert has_contact_access(_fresh(user_profile), others_contact) is False
def test_superuser_without_an_admin_profile_is_admitted(
self, superuser, user_profile, others_contact
):
profile = _fresh(user_profile)
assert profile.role != "ADMIN"
assert has_contact_access(profile, others_contact) is True
class TestQueryset:
def test_plain_member_does_not_see_it(self, user_profile, others_contact):
assert not visible_contacts_qs(_fresh(user_profile)).filter(
pk=others_contact.pk
)
def test_superuser_sees_every_contact_in_their_org(
self, superuser, user_profile, others_contact
):
assert visible_contacts_qs(_fresh(user_profile)).filter(pk=others_contact.pk)
def test_superuser_does_not_see_another_orgs_contact(
self, superuser, user_profile, others_contact, foreign_contact
):
ids = set(
visible_contacts_qs(_fresh(user_profile)).values_list("id", flat=True)
)
assert others_contact.id in ids
assert foreign_contact.id not in ids
class TestDetail:
def test_plain_member_is_refused(self, user_client, others_contact):
response = user_client.get(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_superuser_may_open_it(self, superuser, user_client, others_contact):
response = user_client.get(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_200_OK, response.content
def test_superuser_may_not_open_another_orgs_contact(
self, superuser, user_client, foreign_contact
):
response = user_client.get(f"/api/contacts/{foreign_contact.pk}/")
assert response.status_code == status.HTTP_404_NOT_FOUND
class TestList:
def _ids(self, client):
response = client.get("/api/contacts/")
assert response.status_code == status.HTTP_200_OK
return {row["id"] for row in response.data["results"]}
def test_plain_member_does_not_see_it(self, user_client, others_contact):
assert str(others_contact.id) not in self._ids(user_client)
def test_superuser_sees_it_but_not_another_orgs(
self, superuser, user_client, others_contact, foreign_contact
):
ids = self._ids(user_client)
assert str(others_contact.id) in ids
assert str(foreign_contact.id) not in ids
def test_member_sees_a_contact_at_an_account_they_are_assigned(
self, org_a, user_profile, user_client, others_contact
):
# The detail page already opened this one; the list used to hide it.
account = Account.objects.create(name="Navy", org=org_a)
account.assigned_to.add(user_profile)
account.contacts.add(others_contact)
response = user_client.get(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_200_OK
assert str(others_contact.id) in self._ids(user_client)
class TestDelete:
def test_plain_member_is_refused(self, user_client, others_contact):
response = user_client.delete(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
assert Contact.objects.filter(pk=others_contact.pk).exists()
def test_superuser_may_delete_it(self, superuser, user_client, others_contact):
response = user_client.delete(f"/api/contacts/{others_contact.pk}/")
assert response.status_code == status.HTTP_200_OK, response.content
assert not Contact.objects.filter(pk=others_contact.pk).exists()
class TestSearch:
def _contact_ids(self, client):
response = client.get("/api/search/", {"q": "Grac"})
assert response.status_code == status.HTTP_200_OK
return {r["id"] for r in response.data["results"] if r["type"] == "contact"}
def test_plain_member_does_not_find_it(self, user_client, others_contact):
assert str(others_contact.id) not in self._contact_ids(user_client)
def test_superuser_finds_it_but_not_another_orgs(
self, superuser, user_client, others_contact, foreign_contact
):
ids = self._contact_ids(user_client)
assert str(others_contact.id) in ids
assert str(foreign_contact.id) not in ids
class TestPickers:
"""Each list endpoint's contact catalogue offers what its save path accepts."""
@pytest.mark.parametrize(
"path, key",
[
("/api/tasks/", "contacts_list"),
("/api/cases/", "contacts_list"),
("/api/accounts/", "contacts"),
("/api/leads/", "contacts"),
("/api/opportunities/", "contacts_list"),
],
)
def test_superuser_is_offered_it_and_a_member_is_not(
self, path, key, regular_user, user_client, others_contact, foreign_contact
):
def offered():
response = user_client.get(path)
assert response.status_code == status.HTTP_200_OK, response.content
return {str(row["id"]) for row in response.data[key]}
assert str(others_contact.id) not in offered()
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
ids = offered()
assert str(others_contact.id) in ids
assert str(foreign_contact.id) not in ids

View file

@ -0,0 +1,322 @@
"""A member may link a contact only to an account they can open (D39).
`CreateContactSerializer.validate_account` refused another org's account and
nothing else, so a member could attach a contact to any account in the org.
Linking one hands the contact to that account's assignees
(`has_contact_access`), so a member could push a person into a company they
cannot see. The contact CSV import resolved `account_name` across the whole
org, so its "No account named ..." row error also said which hidden accounts
existed.
Both now ask `accounts.access`, as tickets do since D38: the API through
`has_account_access`, the import through `visible_accounts_qs`. Every refusal
reads the same as an account that does not exist. On edit, the account the
contact is already linked to is kept, since the caller's form never chose it.
"""
import csv
import io
import uuid
from unittest.mock import patch
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
from accounts.models import Account
from conftest import rls_org
from contacts.models import Contact
CONTACTS_URL = "/api/contacts/"
PREVIEW = "/api/contacts/import/preview/"
COMMIT = "/api/contacts/import/commit/"
pytestmark = pytest.mark.django_db
def _account(org, name, created_by=None):
account = Account.objects.create(name=name, org=org)
Account.objects.filter(pk=account.pk).update(created_by=created_by)
return account
@pytest.fixture(autouse=True)
def _no_mail():
with patch("contacts.views.send_email_to_assigned_user.delay"):
yield
@pytest.fixture
def hidden(org_a, admin_user):
return _account(org_a, "Hidden Holdings", created_by=admin_user)
@pytest.fixture
def assigned(org_a, admin_user, user_profile):
account = _account(org_a, "Assigned Industries", created_by=admin_user)
account.assigned_to.add(user_profile)
return account
@pytest.fixture
def foreign(org_b, user_b):
with rls_org(org_b):
return _account(org_b, "Other Org Ltd", created_by=user_b)
@pytest.fixture
def superuser(regular_user):
"""The `user_client` caller, with a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
@pytest.fixture
def importer(user_profile):
user_profile.has_sales_access = True
user_profile.save(update_fields=["has_sales_access"])
return user_profile
@pytest.fixture
def mine_at_hidden(org_a, regular_user, hidden):
"""The member's own contact, linked by an admin to an account they cannot open."""
contact = Contact.objects.create(
first_name="Ada", last_name="Lovelace", org=org_a, account=hidden
)
Contact.objects.filter(pk=contact.pk).update(created_by=regular_user)
hidden.contacts.add(contact)
return contact
def _create(client, first_name, account_id):
return client.post(
CONTACTS_URL,
{"first_name": first_name, "last_name": "Person", "account": str(account_id)},
format="json",
)
def _refusal(response):
assert response.status_code == 400, response.content
return response.json()["errors"]["account"]
class TestContactApiAccountLink:
def test_member_may_link_an_account_they_are_assigned_to(
self, user_client, assigned
):
response = _create(user_client, "Assigned", assigned.id)
assert response.status_code == 200, response.content
contact = Contact.objects.get(first_name="Assigned")
assert contact.account_id == assigned.id
assert contact in assigned.contacts.all()
def test_member_may_link_an_account_they_created(
self, user_client, org_a, regular_user
):
mine = _account(org_a, "Mine Co", created_by=regular_user)
response = _create(user_client, "Own", mine.id)
assert response.status_code == 200, response.content
assert Contact.objects.get(first_name="Own").account_id == mine.id
def test_hidden_account_reads_like_an_unknown_id(self, user_client, hidden):
hidden_refusal = _refusal(_create(user_client, "Hidden", hidden.id))
unknown_refusal = _refusal(_create(user_client, "Unknown", uuid.uuid4()))
assert hidden_refusal == unknown_refusal == ["No such account."]
assert not Contact.objects.filter(first_name__in=["Hidden", "Unknown"])
assert not hidden.contacts.exists()
def test_admin_may_link_any_account_in_the_org(self, admin_client, hidden):
response = _create(admin_client, "Admin", hidden.id)
assert response.status_code == 200, response.content
assert Contact.objects.get(first_name="Admin").account_id == hidden.id
def test_superuser_may_link_any_account_in_the_org(
self, superuser, user_client, hidden
):
response = _create(user_client, "Superuser", hidden.id)
assert response.status_code == 200, response.content
assert Contact.objects.get(first_name="Superuser").account_id == hidden.id
@pytest.mark.parametrize("caller", ["member", "admin", "superuser"])
def test_other_org_account_is_refused_for_everyone(
self, caller, request, user_client, admin_client, foreign
):
if caller == "superuser":
request.getfixturevalue("superuser")
client = admin_client if caller == "admin" else user_client
assert _refusal(_create(client, "Foreign", foreign.id)) == ["No such account."]
assert not Contact.objects.filter(first_name="Foreign").exists()
def test_patch_may_not_move_a_contact_to_a_hidden_account(
self, user_client, org_a, regular_user, hidden
):
contact = Contact.objects.create(first_name="Mover", last_name="P", org=org_a)
Contact.objects.filter(pk=contact.pk).update(created_by=regular_user)
response = user_client.patch(
f"{CONTACTS_URL}{contact.id}/", {"account": str(hidden.id)}, format="json"
)
assert _refusal(response) == ["No such account."]
contact.refresh_from_db()
assert contact.account_id is None
assert not hidden.contacts.exists()
def test_put_may_not_move_a_contact_to_a_hidden_account(
self, user_client, org_a, regular_user, hidden
):
contact = Contact.objects.create(first_name="Mover", last_name="P", org=org_a)
Contact.objects.filter(pk=contact.pk).update(created_by=regular_user)
response = user_client.put(
f"{CONTACTS_URL}{contact.id}/",
{"first_name": "Mover", "last_name": "P", "account": str(hidden.id)},
format="json",
)
assert _refusal(response) == ["No such account."]
contact.refresh_from_db()
assert contact.account_id is None
def test_patch_resending_a_stored_hidden_account_keeps_it(
self, user_client, hidden, mine_at_hidden
):
# What the mobile form does: it sends `account` on every save.
response = user_client.patch(
f"{CONTACTS_URL}{mine_at_hidden.id}/",
{"title": "Countess", "account": str(hidden.id)},
format="json",
)
assert response.status_code == 200, response.content
mine_at_hidden.refresh_from_db()
assert mine_at_hidden.title == "Countess"
assert mine_at_hidden.account_id == hidden.id
assert mine_at_hidden in hidden.contacts.all()
def test_patch_without_account_keeps_a_stored_hidden_account(
self, user_client, hidden, mine_at_hidden
):
response = user_client.patch(
f"{CONTACTS_URL}{mine_at_hidden.id}/", {"title": "Countess"}, format="json"
)
assert response.status_code == 200, response.content
mine_at_hidden.refresh_from_db()
assert mine_at_hidden.account_id == hidden.id
def test_put_resending_a_stored_hidden_account_keeps_it(
self, user_client, hidden, mine_at_hidden
):
response = user_client.put(
f"{CONTACTS_URL}{mine_at_hidden.id}/",
{"first_name": "Ada", "last_name": "King", "account": str(hidden.id)},
format="json",
)
assert response.status_code == 200, response.content
mine_at_hidden.refresh_from_db()
assert mine_at_hidden.last_name == "King"
assert mine_at_hidden.account_id == hidden.id
def test_picker_offers_exactly_what_the_save_path_accepts(
self, user_client, assigned, hidden
):
# Both clients build the contact form's account picker from this list.
body = user_client.get("/api/accounts/?limit=200").json()
offered = {row["id"] for row in body["active_accounts"]["open_accounts"]}
assert str(assigned.id) in offered
assert str(hidden.id) not in offered
def _csv(rows):
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(["first_name", "last_name", "account_name"])
writer.writerows(rows)
return SimpleUploadedFile(
"contacts.csv", buf.getvalue().encode("utf-8"), content_type="text/csv"
)
def _messages(body):
return {e["row"]: e["message"] for e in body["errors"]}
class TestContactImportAccountVisibility:
def test_hidden_and_missing_accounts_get_the_same_error(
self, user_client, importer, hidden
):
response = user_client.post(
PREVIEW,
{
"file": _csv(
[
["Hidden", "Row", "Hidden Holdings"],
["Missing", "Row", "Nobody Inc"],
]
)
},
format="multipart",
)
assert response.status_code == 200, response.content
body = response.json()
assert body["summary"]["valid"] == 0
messages = _messages(body)
assert messages[1] == "No account you can open is named 'Hidden Holdings'"
assert messages[1].replace("Hidden Holdings", "NAME") == messages[2].replace(
"Nobody Inc", "NAME"
)
assert {e["field"] for e in body["errors"]} == {"account_name"}
def test_visible_account_resolves_case_insensitively(
self, user_client, importer, assigned
):
response = user_client.post(
COMMIT,
{"file": _csv([["Imported", "Row", "assigned INDUSTRIES"]])},
format="multipart",
)
assert response.status_code == 200, response.content
assert response.json()["created"] == 1
contact = Contact.objects.get(first_name="Imported")
assert contact.account_id == assigned.id
assert contact in assigned.contacts.all()
def test_commit_refuses_a_hidden_account(self, user_client, importer, hidden):
response = user_client.post(
COMMIT,
{"file": _csv([["Sneaky", "Row", "Hidden Holdings"]])},
format="multipart",
)
assert response.status_code == 400, response.content
assert response.json()["created"] == 0
assert not Contact.objects.filter(first_name="Sneaky").exists()
assert not hidden.contacts.exists()
def test_admin_resolves_an_account_a_member_cannot(self, admin_client, hidden):
response = admin_client.post(
COMMIT,
{"file": _csv([["ByAdmin", "Row", "Hidden Holdings"]])},
format="multipart",
)
assert response.json()["created"] == 1, response.content
assert Contact.objects.get(first_name="ByAdmin").account_id == hidden.id
def test_superuser_resolves_an_account_a_member_cannot(
self, superuser, user_client, importer, hidden
):
response = user_client.post(
COMMIT,
{"file": _csv([["BySuperuser", "Row", "Hidden Holdings"]])},
format="multipart",
)
assert response.json()["created"] == 1, response.content
assert Contact.objects.get(first_name="BySuperuser").account_id == hidden.id
def test_other_org_account_is_missing_even_for_an_admin(
self, admin_client, foreign
):
response = admin_client.post(
PREVIEW,
{"file": _csv([["Abroad", "Row", "Other Org Ltd"]])},
format="multipart",
)
assert _messages(response.json()) == {
1: "No account you can open is named 'Other Org Ltd'"
}

View file

@ -0,0 +1,128 @@
"""The contact import judges phone and name clashes among visible contacts (D48).
`can_mass_import` admits a member with sales access. The preview checked a
row's phone and full name against every contact in the org, so such a member
got "A contact with this phone number already exists" for a number held only
by a contact they cannot open, which confirmed that contact existed. Neither
field has a DB constraint (the only one on ``Contact`` is the per-org email
index), so both checks now look only at ``visible_contacts_qs``. Email stays
org-wide, because the database refuses the duplicate either way.
"""
import csv
import io
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
from contacts.models import Contact
PREVIEW = "/api/contacts/import/preview/"
PHONE_CLASH = "A contact with this phone number already exists in your organization"
NAME_CLASH = (
"A contact with this name already exists; add an email or phone to confirm "
"it's a different person"
)
def _csv(headers, rows):
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(headers)
writer.writerows(rows)
return SimpleUploadedFile(
"contacts.csv", buf.getvalue().encode("utf-8"), content_type="text/csv"
)
@pytest.fixture
def importer(user_profile):
user_profile.has_sales_access = True
user_profile.save(update_fields=["has_sales_access"])
return user_profile
def _contact(org, creator, first, last, phone, email):
return Contact.objects.create(
first_name=first,
last_name=last,
phone=phone,
email=email,
org=org,
created_by=creator,
)
@pytest.fixture
def hidden(org_a, admin_user):
return _contact(
org_a, admin_user, "Hidden", "Person", "+1 (202) 555-0101", "hid@x.test"
)
@pytest.fixture
def mine(org_a, regular_user):
return _contact(
org_a, regular_user, "Mine", "Visible", "+1 (202) 555-0202", "mine@x.test"
)
def _preview(client, headers, rows):
response = client.post(PREVIEW, {"file": _csv(headers, rows)}, format="multipart")
assert response.status_code == 200, response.content
body = response.json()
return body, [(e["field"], e["message"]) for e in body["errors"]]
@pytest.mark.django_db
class TestPhoneClash:
HEADERS = ["first_name", "last_name", "phone"]
def test_hidden_contact_phone_is_not_reported(self, user_client, importer, hidden):
body, errors = _preview(
user_client, self.HEADERS, [["New", "Person", "202-555-0101"]]
)
assert errors == []
assert body["summary"]["valid"] == 1
def test_visible_contact_phone_is_reported(self, user_client, importer, mine):
body, errors = _preview(
user_client, self.HEADERS, [["New", "Person", "202-555-0202"]]
)
assert errors == [("phone", PHONE_CLASH)]
assert body["summary"]["valid"] == 0
def test_admin_sees_every_phone_clash(self, admin_client, admin_profile, hidden):
_body, errors = _preview(
admin_client, self.HEADERS, [["New", "Person", "202-555-0101"]]
)
assert errors == [("phone", PHONE_CLASH)]
@pytest.mark.django_db
class TestFullNameClash:
HEADERS = ["first_name", "last_name"]
def test_hidden_contact_name_is_not_reported(self, user_client, importer, hidden):
body, errors = _preview(user_client, self.HEADERS, [["hidden", "PERSON"]])
assert errors == []
assert body["summary"]["valid"] == 1
def test_visible_contact_name_is_reported(self, user_client, importer, mine):
body, errors = _preview(user_client, self.HEADERS, [["mine", "VISIBLE"]])
assert errors == [("first_name", NAME_CLASH)]
assert body["summary"]["valid"] == 0
def test_admin_sees_every_name_clash(self, admin_client, admin_profile, hidden):
_body, errors = _preview(admin_client, self.HEADERS, [["Hidden", "Person"]])
assert errors == [("first_name", NAME_CLASH)]
@pytest.mark.django_db
def test_email_clash_stays_org_wide(user_client, importer, hidden):
"""The per-org email index refuses the row at commit anyway, so the
preview reports it whoever holds the address."""
_body, errors = _preview(
user_client, ["first_name", "last_name", "email"], [["A", "B", "HID@x.test"]]
)
assert [field for field, _msg in errors] == ["email"]

View file

@ -57,8 +57,12 @@ class ContactsListView(APIView, LimitOffsetPagination):
def get_context_data(self, **kwargs):
params = self.request.query_params
# The list form of `access.has_contact_access`. This filter was a
# second, inline copy of the rule that knew neither superusers nor
# account assignment, so the list hid contacts their detail page
# would open.
queryset = (
self.model.objects.filter(org=self.request.profile.org)
access.visible_contacts_qs(self.request.profile)
# `-id` is a random UUID, so "the list" was in no order at all --
# a page that says "most recent first" was shuffling people. The
# model's own Meta.ordering is `-created_at`; this now agrees.
@ -66,11 +70,6 @@ class ContactsListView(APIView, LimitOffsetPagination):
.select_related("account")
.prefetch_related("account_contacts", "assigned_to__user", "teams", "tags")
)
if not is_org_admin(self.request.profile):
queryset = queryset.filter(
Q(assigned_to__in=[self.request.profile])
| Q(created_by=self.request.profile.user)
).distinct()
if params:
if params.get("name"):
@ -641,10 +640,12 @@ class ContactDetailView(APIView):
def delete(self, request, pk, format=None):
self.object = self.get_object(pk)
# Deliberately narrower than `assert_contact_access`: an assignee may
# work on a contact, only an admin or the person who entered it may
# destroy the record. This comparison was already the right one.
# work on a contact, only an admin, a superuser or the person who
# entered it may destroy the record. The account and deal delete
# rules let superusers through; this one alone refused them.
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
and self.request.profile.user_id != self.object.created_by_id
):
return Response(

View file

@ -1,6 +1,7 @@
import datetime
import json
import logging
import uuid
from datetime import timedelta
from decimal import Decimal
@ -27,7 +28,7 @@ from common.serializer import (
CustomFieldDefinitionSerializer,
)
from common.utils import create_attachment
from common.validators import uuid_param
from common.validators import payload_id_list, uuid_param, validate_uuid
from contacts.access import visible_contacts_qs
from invoices.models import (
UNPAID_STATUSES,
@ -2465,6 +2466,17 @@ class InvoiceFromTimeEntriesView(APIView):
into a draft invoice (Tier 3 time-tracking).
Body: ``{"account_id": "<uuid>", "entry_ids": ["<uuid>", ...]}``.
Org admins and Django superusers only; anyone else gets a 403 before the
body is read. Billing time is a finance action over the whole org's
logged hours, while a member may only see their own entries
(``cases.time_views._visible_entry_qs``), so a member could otherwise
bill a colleague's time and then read it back as the invoice's creator.
Every entry must be on a ticket whose account is ``account_id``. An entry
on a ticket with no account, or with another account, is a 400 and
nothing is written. The account and the entries are looked up inside the
caller's org, so another org's ids answer 404.
"""
permission_classes = (IsAuthenticated, HasOrgContext)
@ -2478,24 +2490,37 @@ class InvoiceFromTimeEntriesView(APIView):
build_invoice_lines_from_entries,
)
if not is_org_admin(request.profile) and not request.user.is_superuser:
return Response(
{
"error": True,
"message": "Only admins can create invoices from time entries.",
},
status=status.HTTP_403_FORBIDDEN,
)
org = request.profile.org
account_id = request.data.get("account_id")
entry_ids = request.data.get("entry_ids") or []
if not account_id:
return Response(
{"error": True, "message": "account_id is required."},
status=status.HTTP_400_BAD_REQUEST,
)
if not isinstance(entry_ids, list) or not entry_ids:
validate_uuid(account_id, "account_id")
# Deduplicated as UUIDs, not strings, so the same id sent twice (or
# once upper-case, once lower) is one entry rather than a false 404.
entry_ids = {
uuid.UUID(str(i))
for i in payload_id_list(request.data.get("entry_ids"), "entry_ids")
}
if not entry_ids:
return Response(
{"error": True, "message": "entry_ids must be a non-empty list."},
status=status.HTTP_400_BAD_REQUEST,
)
try:
account = Account.objects.get(id=account_id, org=org)
except (Account.DoesNotExist, ValueError):
account = Account.objects.filter(id=account_id, org=org).first()
if account is None:
return Response(
{"error": True, "message": "Account not found."},
status=status.HTTP_404_NOT_FOUND,
@ -2507,7 +2532,7 @@ class InvoiceFromTimeEntriesView(APIView):
.select_related("case")
.filter(id__in=entry_ids, org=org)
)
if len(entries) != len(set(str(i) for i in entry_ids)):
if len(entries) != len(entry_ids):
return Response(
{
"error": True,
@ -2516,6 +2541,17 @@ class InvoiceFromTimeEntriesView(APIView):
status=status.HTTP_404_NOT_FOUND,
)
for entry in entries:
if entry.case.account_id != account.id:
return Response(
{
"error": True,
"message": f"Time entry {entry.id} is not on a "
"ticket for this account.",
},
status=status.HTTP_400_BAD_REQUEST,
)
try:
currency, lines = build_invoice_lines_from_entries(entries)
except TimeEntryInvoicingError as exc:

View file

@ -293,6 +293,10 @@ class TestInvoiceFromDealPicksAVisibleContact:
@pytest.mark.django_db
class TestInvoiceFromTimeEntriesPicksAVisibleContact:
"""Only admins and superusers may bill time, so a member never reaches the
contact pick here: their refusal is pinned in
``test_invoice_from_time_entries_authz.py``."""
URL = "/api/invoices/from-time-entries/"
def _post(self, client, org, account, profile, creator):
@ -301,6 +305,7 @@ class TestInvoiceFromTimeEntriesPicksAVisibleContact:
status="New",
priority="Normal",
org=org,
account=account,
created_by=creator,
)
entry = TimeEntry.objects.create(
@ -319,27 +324,6 @@ class TestInvoiceFromTimeEntriesPicksAVisibleContact:
format="json",
)
def test_member_gets_the_contact_they_can_open(
self, user_client, org_a, account, mine, hidden, user_profile, regular_user
):
_newer(hidden)
account.contacts.add(mine, hidden)
response = self._post(user_client, org_a, account, user_profile, regular_user)
assert response.status_code == 201, response.content
assert Invoice.objects.get().contact == mine
def test_member_with_only_hidden_contacts_gets_none(
self, user_client, org_a, account, hidden, user_profile, regular_user
):
account.contacts.add(hidden)
response = self._post(user_client, org_a, account, user_profile, regular_user)
assert response.status_code == 201, response.content
assert Invoice.objects.get().contact is None
def test_admin_gets_the_newest_contact(
self, admin_client, org_a, account, mine, hidden, admin_profile, admin_user
):

View file

@ -0,0 +1,238 @@
"""Who may bill time, and which time may be billed to which account (D36).
``POST /api/invoices/from-time-entries/`` checked only for an org, so any
member could invoice any account from any entry in the org, a colleague's
included, and then read the invoice as its creator. It also never checked
that an entry's ticket belonged to the account being billed.
The rule now: org admins and Django superusers only, and every entry must be
on a ticket for the chosen account. ``user_client`` is a plain member (role
USER); the ``superuser`` fixture turns that same caller into a superuser
without giving the profile an admin role.
"""
import datetime
from decimal import Decimal
import pytest
from django.utils import timezone
from accounts.models import Account
from cases.models import Case, TimeEntry
from invoices.models import Invoice, InvoiceLineItem
URL = "/api/invoices/from-time-entries/"
@pytest.fixture
def account(org_a):
return Account.objects.create(name="Billed Co", org=org_a)
@pytest.fixture
def other_account(org_a):
return Account.objects.create(name="Someone Else", org=org_a)
@pytest.fixture
def superuser(regular_user):
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
def _entry(org, profile, account):
case = Case.objects.create(
name="Billable work", status="New", priority="Normal", org=org, account=account
)
return TimeEntry.objects.create(
org=org,
case=case,
profile=profile,
started_at=timezone.now() - datetime.timedelta(hours=2),
ended_at=timezone.now() - datetime.timedelta(hours=1),
billable=True,
hourly_rate=Decimal("100.00"),
currency="USD",
)
def _post(client, account_id, entry_ids):
return client.post(
URL, {"account_id": account_id, "entry_ids": entry_ids}, format="json"
)
def _assert_nothing_written(*entries):
assert not Invoice.objects.exists()
assert not InvoiceLineItem.objects.exists()
for entry in entries:
entry.refresh_from_db()
assert entry.invoice_id is None
@pytest.mark.django_db
class TestWhoMayBillTime:
def test_admin_may_bill(self, admin_client, org_a, account, admin_profile):
entry = _entry(org_a, admin_profile, account)
response = _post(admin_client, str(account.id), [str(entry.id)])
assert response.status_code == 201, response.content
invoice = Invoice.objects.get(id=response.json()["invoice_id"])
assert invoice.account == account
entry.refresh_from_db()
assert entry.invoice_id == invoice.id
def test_admin_may_bill_a_members_time(
self, admin_client, org_a, account, user_profile
):
entry = _entry(org_a, user_profile, account)
response = _post(admin_client, str(account.id), [str(entry.id)])
assert response.status_code == 201, response.content
def test_superuser_without_an_admin_profile_may_bill(
self, superuser, user_client, org_a, account, user_profile
):
assert user_profile.role != "ADMIN"
entry = _entry(org_a, user_profile, account)
response = _post(user_client, str(account.id), [str(entry.id)])
assert response.status_code == 201, response.content
entry.refresh_from_db()
assert entry.invoice_id is not None
def test_member_is_refused_even_for_their_own_time(
self, user_client, org_a, account, user_profile
):
entry = _entry(org_a, user_profile, account)
response = _post(user_client, str(account.id), [str(entry.id)])
assert response.status_code == 403, response.content
_assert_nothing_written(entry)
def test_member_is_refused_a_colleagues_time(
self, user_client, org_a, account, admin_profile
):
entry = _entry(org_a, admin_profile, account)
response = _post(user_client, str(account.id), [str(entry.id)])
assert response.status_code == 403, response.content
_assert_nothing_written(entry)
def test_member_is_refused_before_the_body_is_read(self, user_client):
"""A 400 here would tell a member which ids parse, so the role check
comes first."""
response = _post(user_client, "not-a-uuid", ["nope"])
assert response.status_code == 403, response.content
@pytest.mark.django_db
class TestEntriesMustBeOnTheAccountsTickets:
def test_entry_on_another_accounts_ticket_is_refused(
self, admin_client, org_a, account, other_account, admin_profile
):
entry = _entry(org_a, admin_profile, other_account)
response = _post(admin_client, str(account.id), [str(entry.id)])
assert response.status_code == 400, response.content
assert "not on a ticket for this account" in response.json()["message"]
_assert_nothing_written(entry)
def test_entry_on_a_ticket_with_no_account_is_refused(
self, admin_client, org_a, account, admin_profile
):
entry = _entry(org_a, admin_profile, None)
response = _post(admin_client, str(account.id), [str(entry.id)])
assert response.status_code == 400, response.content
_assert_nothing_written(entry)
def test_one_foreign_entry_refuses_the_whole_batch(
self, admin_client, org_a, account, other_account, admin_profile
):
good = _entry(org_a, admin_profile, account)
foreign = _entry(org_a, admin_profile, other_account)
response = _post(admin_client, str(account.id), [str(good.id), str(foreign.id)])
assert response.status_code == 400, response.content
_assert_nothing_written(good, foreign)
@pytest.mark.django_db
class TestMalformedIds:
@pytest.mark.parametrize("account_id", ["not-a-uuid", 123, {"id": "x"}])
def test_malformed_account_id_is_a_400(
self, admin_client, org_a, account, admin_profile, account_id
):
entry = _entry(org_a, admin_profile, account)
response = _post(admin_client, account_id, [str(entry.id)])
assert response.status_code == 400, response.content
_assert_nothing_written(entry)
@pytest.mark.parametrize(
"entry_ids", [["not-a-uuid"], [123], [["nested"]], {"id": "x"}, 7]
)
def test_malformed_entry_ids_are_a_400(self, admin_client, account, entry_ids):
response = _post(admin_client, str(account.id), entry_ids)
assert response.status_code == 400, response.content
assert not Invoice.objects.exists()
@pytest.mark.parametrize("entry_ids", [[], None, ""])
def test_missing_entry_ids_are_a_400(self, admin_client, account, entry_ids):
response = _post(admin_client, str(account.id), entry_ids)
assert response.status_code == 400, response.content
def test_missing_account_id_is_a_400(self, admin_client):
response = _post(admin_client, None, ["00000000-0000-0000-0000-000000000000"])
assert response.status_code == 400, response.content
def test_a_repeated_id_is_billed_once(
self, admin_client, org_a, account, admin_profile
):
entry = _entry(org_a, admin_profile, account)
ids = [str(entry.id), str(entry.id), str(entry.id).upper()]
response = _post(admin_client, str(account.id), ids)
assert response.status_code == 201, response.content
assert response.json()["line_count"] == 1
@pytest.mark.django_db
class TestAnotherOrgStaysInvisible:
def test_another_orgs_entry_is_a_404(
self, admin_client, org_a, org_b, account, profile_b
):
other_account = Account.objects.create(name="Other org", org=org_b)
entry = _entry(org_b, profile_b, other_account)
response = _post(admin_client, str(account.id), [str(entry.id)])
assert response.status_code == 404, response.content
_assert_nothing_written(entry)
def test_another_orgs_account_is_a_404(
self, admin_client, org_a, org_b, account, admin_profile
):
other_account = Account.objects.create(name="Other org", org=org_b)
entry = _entry(org_a, admin_profile, account)
response = _post(admin_client, str(other_account.id), [str(entry.id)])
assert response.status_code == 404, response.content
_assert_nothing_written(entry)

View file

@ -0,0 +1,74 @@
"""Bound `LeadStage.win_probability` to 0-100 at the database (tracker D11).
The board move copies a stage's win probability onto `Lead.probability`, which
has the `lead_probability_range` check. A stage stored at 150 or -5 therefore
turned every later move into it into an IntegrityError 500. The API now refuses
such a value, and this adds the matching check so no other writer can store one.
CLAMP FIRST
A row already out of range would make `AddConstraint` fail, since Postgres
validates every existing row when the check is added. So the data step runs
first and clamps: below 0 becomes 0, above 100 becomes 100. Clamping keeps the
stage and its nearest meaningful value; nothing is deleted.
THE ORG LOOP IS NOT DECORATION
`lead_stage` is in ORG_SCOPED_TABLES, so it has `FORCE ROW LEVEL SECURITY` and
an unscoped update during a migration (where `app.current_org` is empty)
matches no row in production. The loop sets the context per org, the shape
`common/0042_recompute_tag_slugs` documents. Constraint validation is not
subject to RLS, which is exactly why every org has to be clamped.
REVERSE
The constraint is dropped. The clamp is a no-op: the original out-of-range
values were the defect, and nothing needs them back.
"""
from django.db import migrations, models
def clamp_win_probability(apps, schema_editor):
connection = schema_editor.connection
# RLS and `set_config` are Postgres-only; on SQLite the loop still runs
# and only the context call is skipped.
is_postgres = connection.vendor == "postgresql"
Org = apps.get_model("common", "Org")
LeadStage = apps.get_model("leads", "LeadStage")
def set_context(value):
if not is_postgres:
return
with connection.cursor() as cursor:
cursor.execute("SELECT set_config('app.current_org', %s, false)", [value])
try:
for org_id in Org.objects.values_list("id", flat=True).iterator():
set_context(str(org_id))
stages = LeadStage.objects.filter(org_id=org_id)
stages.filter(win_probability__lt=0).update(win_probability=0)
stages.filter(win_probability__gt=100).update(win_probability=100)
finally:
set_context("")
class Migration(migrations.Migration):
dependencies = [
("common", "0042_recompute_tag_slugs"),
("leads", "0016_alter_leadpipeline_is_default"),
]
operations = [
migrations.RunPython(clamp_win_probability, migrations.RunPython.noop),
migrations.AddConstraint(
model_name="leadstage",
constraint=models.CheckConstraint(
condition=models.Q(
("win_probability__gte", 0), ("win_probability__lte", 100)
),
name="lead_stage_win_probability_range",
),
),
]

View file

@ -325,6 +325,14 @@ class LeadStage(BaseModel):
models.Index(fields=["org", "order"]),
models.Index(fields=["pipeline", "order"]),
]
constraints = [
# Copied onto Lead.probability by the board move, so it has to fit
# lead_probability_range or every move into this stage is a 500.
models.CheckConstraint(
condition=Q(win_probability__gte=0) & Q(win_probability__lte=100),
name="lead_stage_win_probability_range",
),
]
def __str__(self):
return f"{self.pipeline.name} - {self.name}"

View file

@ -0,0 +1,189 @@
"""`LeadStage.win_probability` is bounded to 0-100 at the database (D11).
The board move copies it onto `Lead.probability`, whose `lead_probability_range`
check made an out-of-range stage a 500 on every move into it. The serializer
bound is pinned in `test_leads_kanban.py::TestLeadStageWinProbabilityBounds`;
this file pins the check constraint and `leads/0017`'s clamp.
"""
import importlib
from contextlib import contextmanager
from types import SimpleNamespace
import pytest
from django.apps import apps as global_apps
from django.db import IntegrityError, connection, transaction
from leads.models import Lead, LeadPipeline, LeadStage
CONSTRAINT = "lead_stage_win_probability_range"
def _set_rls(org):
if connection.vendor != "postgresql":
return
with connection.cursor() as cursor:
cursor.execute("SELECT set_config('app.current_org', %s, false)", [str(org.id)])
def _stage(org, created_by, name, win_probability=0):
pipeline, _ = LeadPipeline.objects.get_or_create(
name="Sales", org=org, defaults={"created_by": created_by}
)
return LeadStage.objects.create(
pipeline=pipeline,
name=name,
org=org,
win_probability=win_probability,
)
@contextmanager
def _check_suspended():
"""Let a test store a row as production may hold one from before the check.
SQLite cannot drop a constraint inside the test transaction, but it can be
told to skip CHECKs. Postgres drops it; the test transaction's rollback
restores it, and re-adding it here also proves the rows now satisfy it.
"""
with connection.cursor() as cursor:
if connection.vendor == "sqlite":
cursor.execute("PRAGMA ignore_check_constraints = ON")
try:
yield
finally:
cursor.execute("PRAGMA ignore_check_constraints = OFF")
else:
cursor.execute(f"ALTER TABLE lead_stage DROP CONSTRAINT {CONSTRAINT}")
yield
# Rows inserted and updated in this transaction leave deferred FK
# checks queued, and Postgres refuses ALTER TABLE while they are.
cursor.execute("SET CONSTRAINTS ALL IMMEDIATE")
cursor.execute(
f"ALTER TABLE lead_stage ADD CONSTRAINT {CONSTRAINT} "
"CHECK (win_probability >= 0 AND win_probability <= 100)"
)
@pytest.mark.django_db
class TestConstraint:
@pytest.mark.parametrize("value", [-1, 101, 150])
def test_out_of_range_is_refused_by_the_database(self, admin_user, org_a, value):
_set_rls(org_a)
stage = _stage(org_a, admin_user, "New")
# .update() skips every Python-side check, so only the DB can refuse it.
with pytest.raises(IntegrityError), transaction.atomic():
LeadStage.objects.filter(pk=stage.pk).update(win_probability=value)
stage.refresh_from_db()
assert stage.win_probability == 0
@pytest.mark.parametrize("value", [0, 100])
def test_the_bounds_save(self, admin_user, org_a, value):
_set_rls(org_a)
stage = _stage(org_a, admin_user, "Edge", win_probability=value)
stage.refresh_from_db()
assert stage.win_probability == value
@pytest.mark.django_db
class TestClampMigration:
"""`leads/0017`'s data step, called directly as `common/tests/test_tag_slugs.py`
calls `common/0042`. The live models stand in for the historical ones,
which is sound while `LeadStage` has the same fields as at 0017."""
def _clamp(self):
module = importlib.import_module(
"leads.migrations.0017_leadstage_win_probability_range"
)
module.clamp_win_probability(
global_apps, SimpleNamespace(connection=connection)
)
def test_out_of_range_rows_are_clamped_in_every_org(self, admin_user, org_a, org_b):
with _check_suspended():
_set_rls(org_a)
low = _stage(org_a, admin_user, "Low", -5)
high = _stage(org_a, admin_user, "High", 150)
zero = _stage(org_a, admin_user, "Zero", 0)
mid = _stage(org_a, admin_user, "Mid", 50)
hundred = _stage(org_a, admin_user, "Hundred", 100)
_set_rls(org_b)
other = _stage(org_b, admin_user, "Other org", 150)
self._clamp()
got = {}
for org, stage in [
(org_a, low),
(org_a, high),
(org_a, zero),
(org_a, mid),
(org_a, hundred),
(org_b, other),
]:
_set_rls(org)
got[stage.name] = LeadStage.objects.get(pk=stage.pk).win_probability
assert got == {
"Low": 0,
"High": 100,
"Zero": 0,
"Mid": 50,
"Hundred": 100,
"Other org": 100,
}
def test_a_move_into_a_clamped_stage_succeeds(
self, admin_client, admin_user, org_a
):
with _check_suspended():
_set_rls(org_a)
stage = _stage(org_a, admin_user, "Was 150", 150)
self._clamp()
_set_rls(org_a)
lead = Lead.objects.create(
first_name="Board",
last_name="Lead",
email="clamped@example.com",
status="assigned",
created_by=admin_user,
org=org_a,
)
response = admin_client.patch(
f"/api/leads/{lead.id}/move/", {"stage_id": str(stage.id)}, format="json"
)
assert response.status_code == 200
lead.refresh_from_db()
assert lead.stage_id == stage.id
assert lead.probability == 100
@pytest.mark.django_db
class TestMoveAtTheBounds:
@pytest.mark.parametrize("value", [0, 1, 99, 100])
def test_a_move_into_any_storable_stage_succeeds(
self, admin_client, admin_user, org_a, value
):
_set_rls(org_a)
stage = _stage(org_a, admin_user, "Target", value)
lead = Lead.objects.create(
first_name="Board",
last_name="Lead",
email="bounds@example.com",
status="assigned",
created_by=admin_user,
org=org_a,
)
response = admin_client.patch(
f"/api/leads/{lead.id}/move/", {"stage_id": str(stage.id)}, format="json"
)
assert response.status_code == 200
lead.refresh_from_db()
assert lead.stage_id == stage.id
assert lead.probability == value

View file

@ -3,8 +3,9 @@ single-step `upload/` alias over the same commit path.
The same contract as `contacts.import_views` and `cases.import_views`, which
is what the web import drawers speak. All three endpoints are gated by
`_can_import`, so a member without admin or sales access cannot mass-create
leads here even though they can create them one at a time.
`common.permissions.can_mass_import`, so a member without admin or sales
access cannot mass-create leads here even though they can create them one at
a time.
"""
from drf_spectacular.utils import extend_schema, inline_serializer
@ -14,26 +15,12 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from common.permissions import HasOrgContext
from common.permissions import HasOrgContext, can_mass_import
from leads.csv_import import commit_rows, parse_and_validate
MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint
def _can_import(profile) -> bool:
"""Mass-create requires admin or explicit sales-access permission.
Same rule as `contacts.import_views._can_import` and its cases twin.
"""
if profile is None:
return False
if getattr(profile, "role", None) == "ADMIN":
return True
if getattr(profile, "is_admin", False):
return True
return bool(getattr(profile, "has_sales_access", False))
def _read_upload(request, field="file", csv_name=True):
"""Return (file_bytes, problem); exactly one is None.
@ -100,7 +87,7 @@ class LeadImportPreviewView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return _forbidden()
file_bytes, problem = _read_upload(request)
if problem:
@ -131,7 +118,7 @@ class LeadImportCommitView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return _forbidden()
file_bytes, problem = _read_upload(request)
if problem:
@ -182,7 +169,7 @@ class LeadUploadView(APIView):
},
)
def post(self, request, *args, **kwargs):
if not _can_import(request.profile):
if not can_mass_import(request.profile):
return Response(
{"error": True, "errors": "Admin access required"},
status=status.HTTP_403_FORBIDDEN,

View file

@ -46,8 +46,7 @@ from common.validators import (
payload_id_list,
uuid_list_param,
)
from contacts.access import replace_visible_contacts
from contacts.models import Contact
from contacts.access import replace_visible_contacts, visible_contacts_qs
from leads import access, swagger_params
from leads.models import Lead
from leads.serializer import (
@ -237,20 +236,12 @@ class LeadListView(APIView, LimitOffsetPagination):
"close_leads": close_leads,
"offset": offset,
}
# Narrowed for a non-admin the same way the lead queryset above is.
# This catalogue feeds the lead form's contact picker, and org scope
# alone would let a member read back the first name of every contact
# in the org, including ones `/api/contacts/` refuses them.
contact_qs = Contact.objects.filter(org=self.request.profile.org)
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
contact_qs = contact_qs.filter(
Q(assigned_to__in=[self.request.profile])
| Q(created_by=self.request.profile.user)
).distinct()
contacts = contact_qs.values("id", "first_name")
# The contact read rule itself. This catalogue feeds the lead form's
# contact picker, and org scope alone would let a member read back the
# first name of every contact in the org, including ones
# `/api/contacts/` refuses them. It is also exactly what
# `replace_visible_contacts` accepts on save.
contacts = visible_contacts_qs(self.request.profile).values("id", "first_name")
context["contacts"] = contacts
context["status"] = LEAD_STATUS

View file

@ -47,7 +47,6 @@ from common.validators import (
uuid_param,
)
from contacts.access import replace_visible_contacts, visible_contacts_qs
from contacts.models import Contact
from contacts.serializer import ContactPickerSerializer
from opportunity import access, swagger_params
from opportunity.models import Opportunity, StageAgingConfig
@ -151,7 +150,8 @@ class OpportunityListView(APIView, LimitOffsetPagination):
"-id"
)
accounts = Account.objects.filter(org=self.request.profile.org)
contacts = Contact.objects.filter(org=self.request.profile.org)
# The contact read rule itself, which is what the save path accepts.
contacts = visible_contacts_qs(self.request.profile)
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
@ -164,10 +164,6 @@ class OpportunityListView(APIView, LimitOffsetPagination):
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
contacts = contacts.filter(
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
if params:
if params.get("name"):

View file

@ -1,3 +1,4 @@
from django.db.models import Count, Q
from drf_spectacular.utils import extend_schema_field
from rest_framework import serializers
@ -10,6 +11,7 @@ from common.serializer import (
UserSerializer,
)
from contacts.serializer import ContactLinkSerializer
from tasks.access import visible_tasks_qs
from tasks.models import (
Board,
BoardColumn,
@ -427,8 +429,20 @@ class TaskCreateSwaggerSerializer(serializers.ModelSerializer):
# ============================================================================
def _visible_task_count(serializer, **lookup):
"""How many tasks matching ``lookup`` the requester may open.
``task_count`` once counted every task in the org, so a member read a
count that included tasks they cannot open. It follows ``visible_tasks_qs``
now, the rule the board and the list use. The request has to be in the
serializer context: a missing one is a KeyError, never an unscoped count.
"""
profile = serializer.context["request"].profile
return visible_tasks_qs(profile).filter(**lookup).count()
class TaskStageSerializer(serializers.ModelSerializer):
"""Serializer for task stages."""
"""Serializer for task stages. Needs ``request`` in its context."""
task_count = serializers.SerializerMethodField()
@ -450,11 +464,28 @@ class TaskStageSerializer(serializers.ModelSerializer):
@extend_schema_field(int)
def get_task_count(self, obj):
return obj.tasks.count()
return _visible_task_count(self, stage=obj)
def validate_maps_to_status(self, value):
"""Only a real task status, or nothing.
The column is free text, and a stage mapped to anything else is one no
task can move into: `Task.save()` runs `full_clean()` and refuses the
status the move would set.
"""
if value in (None, ""):
return value
allowed = [choice for choice, _ in Task.STATUS_CHOICES]
if value not in allowed:
raise serializers.ValidationError(
f"maps_to_status must be one of {', '.join(allowed)}, or empty."
)
return value
class TaskPipelineSerializer(serializers.ModelSerializer):
"""Serializer for task pipelines with nested stages."""
"""Serializer for task pipelines with nested stages. Needs ``request`` in
its context."""
stages = TaskStageSerializer(many=True, read_only=True)
stage_count = serializers.SerializerMethodField()
@ -482,11 +513,17 @@ class TaskPipelineSerializer(serializers.ModelSerializer):
@extend_schema_field(int)
def get_task_count(self, obj):
return Task.objects.filter(stage__pipeline=obj).count()
return _visible_task_count(self, stage__pipeline=obj)
class TaskPipelineListSerializer(serializers.ModelSerializer):
"""Simplified pipeline serializer for lists."""
"""Simplified pipeline serializer for lists.
Both counts are read from annotations, so every queryset serialized here
must come through ``with_counts``: one query for the whole list, however
many pipelines. A pipeline without them raises AttributeError, which is
louder than a count that silently ignores who is asking.
"""
stage_count = serializers.SerializerMethodField()
task_count = serializers.SerializerMethodField()
@ -504,13 +541,25 @@ class TaskPipelineListSerializer(serializers.ModelSerializer):
"created_at",
]
@staticmethod
def with_counts(pipelines, profile):
# The task join repeats a stage once per task in it, hence distinct on
# the stage count. A task sits in one stage, so it is counted once.
return pipelines.annotate(
stage_count=Count("stages", distinct=True),
task_count=Count(
"stages__tasks",
filter=Q(stages__tasks__in=visible_tasks_qs(profile).values("pk")),
),
)
@extend_schema_field(int)
def get_stage_count(self, obj):
return obj.stages.count()
return obj.stage_count
@extend_schema_field(int)
def get_task_count(self, obj):
return Task.objects.filter(stage__pipeline=obj).count()
return obj.task_count
class RelatedEntitySerializer(serializers.Serializer):

View file

@ -0,0 +1,163 @@
"""Board write verbs answer a stranger the way GET does (D46).
Board GETs answered a non-member 404, but column create, card create, card
update and card delete answered 403, so anyone in the org could tell which
board and card ids exist. A 404 with a different body from a missing id's
told them the same thing, so the answers are compared whole.
"""
import uuid
import pytest
from tasks.models import Board, BoardColumn, BoardMember, BoardTask
@pytest.fixture
def board(admin_profile, admin_user, org_a):
board = Board.objects.create(
name="Private", owner=admin_profile, org=org_a, created_by=admin_user
)
BoardMember.objects.create(board=board, profile=admin_profile, role="owner")
return board
@pytest.fixture
def column(board, org_a):
return BoardColumn.objects.create(board=board, name="Col", order=1, org=org_a)
@pytest.fixture
def card(column, org_a, admin_user):
return BoardTask.objects.create(
column=column, title="Card", priority="low", org=org_a, created_by=admin_user
)
def _join(board, profile, role="member"):
BoardMember.objects.create(board=board, profile=profile, role=role)
class TestStrangerGets404:
def test_column_create(self, user_client, board):
response = user_client.post(
f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json"
)
assert response.status_code == 404
assert board.columns.count() == 0
def test_card_create(self, user_client, column):
response = user_client.post(
f"/api/boards/columns/{column.id}/tasks/",
{"title": "New", "priority": "low"},
format="json",
)
assert response.status_code == 404
assert column.tasks.count() == 0
def test_card_update(self, user_client, card):
response = user_client.put(
f"/api/boards/tasks/{card.id}/", {"title": "Changed"}, format="json"
)
assert response.status_code == 404
card.refresh_from_db()
assert card.title == "Card"
def test_card_delete(self, user_client, card):
response = user_client.delete(f"/api/boards/tasks/{card.id}/")
assert response.status_code == 404
assert BoardTask.objects.filter(id=card.id).exists()
class TestMembersStillAllowed:
def test_member_updates_and_deletes_a_card(self, user_client, user_profile, card):
_join(card.column.board, user_profile)
response = user_client.put(
f"/api/boards/tasks/{card.id}/", {"title": "Changed"}, format="json"
)
assert response.status_code == 200, response.content
assert user_client.delete(f"/api/boards/tasks/{card.id}/").status_code == 204
def test_member_creates_a_card(self, user_client, user_profile, column):
_join(column.board, user_profile)
response = user_client.post(
f"/api/boards/columns/{column.id}/tasks/",
{"title": "New", "priority": "low"},
format="json",
)
assert response.status_code == 201, response.content
def test_plain_member_still_gets_403_on_column_create(
self, user_client, user_profile, board
):
"""A member can see the board, so a refusal here reveals nothing."""
_join(board, user_profile)
response = user_client.post(
f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json"
)
assert response.status_code == 403
def test_board_admin_creates_a_column(self, user_client, user_profile, board):
_join(board, user_profile, role="admin")
response = user_client.post(
f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json"
)
assert response.status_code == 201, response.content
def _calls(board, column, card):
"""(method, url for the real id, url for a missing id, body) per verb."""
missing = uuid.uuid4()
return [
("get", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", None),
("put", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", {"name": "x"}),
("delete", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", None),
(
"get",
f"/api/boards/{board.id}/columns/",
f"/api/boards/{missing}/columns/",
None,
),
(
"post",
f"/api/boards/{board.id}/columns/",
f"/api/boards/{missing}/columns/",
{"name": "x"},
),
(
"get",
f"/api/boards/columns/{column.id}/tasks/",
f"/api/boards/columns/{missing}/tasks/",
None,
),
(
"post",
f"/api/boards/columns/{column.id}/tasks/",
f"/api/boards/columns/{missing}/tasks/",
{"title": "x", "priority": "low"},
),
(
"put",
f"/api/boards/tasks/{card.id}/",
f"/api/boards/tasks/{missing}/",
{"title": "x"},
),
(
"delete",
f"/api/boards/tasks/{card.id}/",
f"/api/boards/tasks/{missing}/",
None,
),
]
def test_hidden_and_missing_ids_get_the_same_answer(user_client, board, column, card):
for method, real, missing, body in _calls(board, column, card):
call = getattr(user_client, method)
kwargs = {"format": "json"} if body is not None else {}
hidden = call(real, body, **kwargs) if body is not None else call(real)
absent = call(missing, body, **kwargs) if body is not None else call(missing)
assert hidden.status_code == absent.status_code == 404, (method, real)
assert hidden.json() == absent.json(), (method, real)
assert Board.objects.filter(id=board.id).exists()
assert BoardTask.objects.filter(id=card.id, title="Card").exists()

View file

@ -464,7 +464,7 @@ class TestBoardTasks:
def test_update_board_task_non_member_forbidden(
self, user_client, admin_profile, admin_user, org_a
):
"""Non-member should get 403 when updating a board task."""
"""Non-member should get 404, as GET answers them, when updating a board task."""
board = Board.objects.create(
name="Private Task Board",
owner=admin_profile,
@ -485,7 +485,7 @@ class TestBoardTasks:
{"title": "Hacked Card"},
format="json",
)
assert response.status_code == 403
assert response.status_code == 404
def test_get_tasks_non_member_forbidden(
self, user_client, admin_profile, admin_user, org_a
@ -505,7 +505,7 @@ class TestBoardTasks:
def test_create_task_non_member_forbidden(
self, user_client, admin_profile, admin_user, org_a
):
"""Non-member should get 403 when creating a task in a column."""
"""Non-member should get 404, as GET answers them, when creating a task in a column."""
board = Board.objects.create(
name="No Access Task Board",
owner=admin_profile,
@ -519,7 +519,7 @@ class TestBoardTasks:
{"title": "Blocked Card", "priority": "low"},
format="json",
)
assert response.status_code == 403
assert response.status_code == 404
def test_update_board_task_with_assigned_to(
self, admin_client, admin_profile, admin_user, org_a

View file

@ -0,0 +1,70 @@
"""A task move the model refuses answers 400, as PATCH does, not 500.
``Task.save()`` calls ``full_clean()``. The PATCH and PUT paths catch the
Django ``ValidationError`` that raises and answer 400 (`_model_errors`); the
board move did not, so any move the model refused was a 500. It is reachable
through configuration alone: ``TaskStage.maps_to_status`` is free text, so a
stage mapped to a status ``Task`` does not have made every move into it crash.
Task moves have no completion side effect to mirror: ``Task`` has no
``completed_at`` and no save signal, so status is the whole of it.
"""
import pytest
from tasks.models import Task, TaskPipeline, TaskStage
@pytest.mark.django_db
def test_move_into_a_stage_the_model_refuses_is_a_400(
admin_client, admin_profile, admin_user, org_a
):
pipeline = TaskPipeline.objects.create(
name="Delivery", org=org_a, created_by=admin_user
)
stage = TaskStage.objects.create(
pipeline=pipeline,
name="Shipped",
order=1,
maps_to_status="Shipped",
org=org_a,
created_by=admin_user,
)
task = Task.objects.create(
title="Pack the box",
status="New",
priority="Low",
org=org_a,
created_by=admin_user,
)
response = admin_client.patch(
f"/api/tasks/{task.id}/move/", {"stage_id": str(stage.id)}, format="json"
)
assert response.status_code == 400, response.content
body = response.json()
assert body["error"] is True
assert "status" in body["errors"]
task.refresh_from_db()
assert (task.status, task.stage_id) == ("New", None)
@pytest.mark.django_db
def test_move_to_completed_still_goes_through(
admin_client, admin_profile, admin_user, org_a
):
task = Task.objects.create(
title="Pack the box",
status="New",
priority="Low",
org=org_a,
created_by=admin_user,
)
response = admin_client.patch(
f"/api/tasks/{task.id}/move/", {"status": "Completed"}, format="json"
)
assert response.status_code == 200, response.content
task.refresh_from_db()
assert task.status == "Completed"

View file

@ -0,0 +1,117 @@
"""The board move takes the task's access rule, exactly (D42).
`TaskMoveView.patch` rewrites a task's status, stage and kanban order. It used
to gate on ``is_org_admin(...) or request.user.is_superuser`` plus creator or
assignee, while `tasks.access` gives a Django superuser nothing and
`TaskDetailView.patch` refuses them. So a superuser who is a plain member could
move, through the board, any task they could not edit through the detail
endpoint. These tests pin the move to `assert_task_access`.
"""
import pytest
from conftest import rls_org
from tasks.models import Task
def _move(client, task, status="In Progress"):
return client.patch(
f"/api/tasks/{task.id}/move/", {"status": status}, format="json"
)
@pytest.fixture
def superuser(regular_user):
"""The `user_client` caller, still a plain USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
def _task(org, created_by, title="A task"):
return Task.objects.create(
title=title, status="New", priority="Low", org=org, created_by=created_by
)
@pytest.fixture
def others_task(admin_user, org_a):
"""A task the `user_client` caller neither created nor was handed."""
return _task(org_a, admin_user, "Someone else's task")
def _assert_moved(task):
task.refresh_from_db()
assert task.status == "In Progress"
def _assert_unchanged(task):
task.refresh_from_db()
assert task.status == "New"
class TestSuperuserPlainMember:
def test_refused_on_a_task_they_cannot_edit(
self, superuser, user_client, others_task
):
assert (
user_client.patch(
f"/api/tasks/{others_task.id}/",
{"status": "In Progress"},
format="json",
).status_code
== 403
)
response = _move(user_client, others_task)
assert response.status_code == 403, response.content
_assert_unchanged(others_task)
def test_allowed_on_a_task_they_created(
self, superuser, user_client, regular_user, org_a
):
task = _task(org_a, regular_user, "Mine")
response = _move(user_client, task)
assert response.status_code == 200, response.content
_assert_moved(task)
def test_allowed_on_a_task_they_are_assigned(
self, superuser, user_client, user_profile, others_task
):
others_task.assigned_to.add(user_profile)
response = _move(user_client, others_task)
assert response.status_code == 200, response.content
_assert_moved(others_task)
class TestOrdinaryRoles:
def test_admin_may_move_any_task(self, admin_client, regular_user, org_a):
task = _task(org_a, regular_user, "A member's task")
response = _move(admin_client, task)
assert response.status_code == 200, response.content
_assert_moved(task)
def test_member_with_access_may_move(self, user_client, user_profile, others_task):
others_task.assigned_to.add(user_profile)
response = _move(user_client, others_task)
assert response.status_code == 200, response.content
_assert_moved(others_task)
def test_member_without_access_is_refused(self, user_client, others_task):
response = _move(user_client, others_task)
assert response.status_code == 403, response.content
_assert_unchanged(others_task)
@pytest.mark.parametrize("caller", ["member", "admin", "superuser"])
def test_another_orgs_task_is_404_for_everyone(
caller, request, user_client, admin_client, user_b, org_b
):
if caller == "superuser":
request.getfixturevalue("superuser")
client = admin_client if caller == "admin" else user_client
with rls_org(org_b):
foreign = _task(org_b, user_b, "Foreign")
response = _move(client, foreign)
assert response.status_code == 404, response.content
with rls_org(org_b):
_assert_unchanged(foreign)

View file

@ -0,0 +1,118 @@
"""``task_count`` on a task pipeline counts only tasks the caller can open (D47).
It used to count every task in the org, so a member whose board showed one
card read ``task_count: 2``, a number that included a task they cannot open.
It follows ``tasks.access.visible_tasks_qs`` now, the rule the board and the
list use, in all four places it appears: the pipeline list, the pipeline
detail, each of its nested stages, and the pipeline block on the board. The
same fix D9 made for lead pipelines.
"""
import pytest
from django.db import connection
from django.test.utils import CaptureQueriesContext
from conftest import rls_org
from tasks.models import Task, TaskPipeline, TaskStage
def _pipeline(org, user, name="Delivery"):
pipeline = TaskPipeline.objects.create(name=name, org=org, created_by=user)
first = TaskStage.objects.create(
pipeline=pipeline, name="To do", order=1, org=org, created_by=user
)
second = TaskStage.objects.create(
pipeline=pipeline, name="Doing", order=2, org=org, created_by=user
)
return pipeline, first, second
def _task(org, user, title, stage):
return Task.objects.create(
title=title,
status="New",
priority="Low",
org=org,
created_by=user,
stage=stage,
)
@pytest.fixture
def seeded(admin_user, admin_profile, user_profile, org_a, org_b, user_b):
pipeline, first, _second = _pipeline(org_a, admin_user)
_task(org_a, admin_user, "Hidden from the member", first)
mine = _task(org_a, admin_user, "Handed to the member", first)
# Two assignees: a join on the M2M would count this task twice.
mine.assigned_to.add(user_profile, admin_profile)
# Another org's task, in another org's pipeline, never counted here.
with rls_org(org_b):
_b_pipeline, b_stage, _ = _pipeline(org_b, user_b, "Org B")
_task(org_b, user_b, "Org B task", b_stage)
return pipeline
def _counts(client, pipeline):
"""task_count from the list, the detail, its first stage and the board."""
(row,) = client.get("/api/tasks/pipelines/").json()["pipelines"]
assert row["id"] == str(pipeline.id)
detail = client.get(f"/api/tasks/pipelines/{pipeline.id}/").json()
board = client.get("/api/tasks/kanban/", {"pipeline_id": str(pipeline.id)}).json()
assert [s["task_count"] for s in detail["stages"][1:]] == [0]
return (
row["task_count"],
detail["task_count"],
detail["stages"][0]["task_count"],
board["pipeline"]["task_count"],
)
@pytest.mark.django_db
class TestTaskPipelineCountsFollowVisibility:
def test_member_counts_only_visible_tasks(self, user_client, seeded):
assert _counts(user_client, seeded) == (1, 1, 1, 1)
def test_admin_counts_every_task_in_the_org(self, admin_client, seeded):
assert _counts(admin_client, seeded) == (2, 2, 2, 2)
board = admin_client.get(
"/api/tasks/kanban/", {"pipeline_id": str(seeded.id)}
).json()
assert board["columns"][0]["task_count"] == 2
assert board["pipeline"]["stage_count"] == 2
def test_member_board_column_and_pipeline_agree(self, user_client, seeded):
board = user_client.get(
"/api/tasks/kanban/", {"pipeline_id": str(seeded.id)}
).json()
assert board["columns"][0]["task_count"] == 1
assert len(board["columns"][0]["tasks"]) == 1
assert board["pipeline"]["task_count"] == 1
def test_write_responses_carry_task_count(self, admin_client, admin_user, org_a):
created = admin_client.post(
"/api/tasks/pipelines/", {"name": "Fresh"}, format="json"
).json()
assert created["task_count"] == 0
assert {s["task_count"] for s in created["stages"]} == {0}
_pipeline_obj, first, _second = _pipeline(org_a, admin_user, "Other")
_task(org_a, admin_user, "Staged", first)
updated = admin_client.put(
f"/api/tasks/stages/{first.id}/", {"name": "Renamed"}, format="json"
).json()
assert updated["task_count"] == 1
def test_pipeline_list_query_count_does_not_grow_per_pipeline(
self, admin_client, admin_user, org_a, seeded
):
with CaptureQueriesContext(connection) as one:
admin_client.get("/api/tasks/pipelines/")
for name in ("Second", "Third"):
_p, stage, _s = _pipeline(org_a, admin_user, name)
_task(org_a, admin_user, f"{name} task", stage)
with CaptureQueriesContext(connection) as three:
response = admin_client.get("/api/tasks/pipelines/")
assert len(response.json()["pipelines"]) == 3
assert len(three.captured_queries) == len(one.captured_queries)

View file

@ -0,0 +1,61 @@
"""`TaskStage.maps_to_status` must be a task status or empty.
The column is free text. A stage mapped to anything else is one no task can
move into, because `Task.save()` runs `full_clean()` and refuses the status
the move would set. The serializer now refuses it at the door with a 400.
"""
import pytest
from tasks.models import TaskPipeline, TaskStage
@pytest.fixture
def pipeline(admin_user, org_a):
return TaskPipeline.objects.create(
name="Stage status pipeline", org=org_a, created_by=admin_user
)
def _create(client, pipeline, **extra):
return client.post(
f"/api/tasks/pipelines/{pipeline.id}/stages/",
{"name": "Stage", "order": 1, "stage_type": "open", **extra},
format="json",
)
@pytest.mark.django_db
class TestMapsToStatus:
def test_unknown_status_is_400(self, admin_client, pipeline):
resp = _create(admin_client, pipeline, maps_to_status="Shipped")
assert resp.status_code == 400, resp.content
body = resp.json()
assert body["error"] is True
assert "maps_to_status" in body["errors"]
assert not TaskStage.objects.filter(pipeline=pipeline).exists()
def test_real_status_is_201(self, admin_client, pipeline):
resp = _create(admin_client, pipeline, maps_to_status="In Progress")
assert resp.status_code == 201, resp.content
assert resp.json()["maps_to_status"] == "In Progress"
@pytest.mark.parametrize("value", ["", None])
def test_empty_is_allowed(self, admin_client, pipeline, value):
resp = _create(admin_client, pipeline, maps_to_status=value)
assert resp.status_code == 201, resp.content
def test_update_to_unknown_status_is_400(
self, admin_client, admin_user, org_a, pipeline
):
stage = TaskStage.objects.create(
pipeline=pipeline, name="Existing", order=1, org=org_a
)
resp = admin_client.put(
f"/api/tasks/stages/{stage.id}/",
{"maps_to_status": "completed"},
format="json",
)
assert resp.status_code == 400, resp.content
stage.refresh_from_db()
assert stage.maps_to_status is None

View file

@ -66,6 +66,23 @@ def _pop_assignee_ids(data):
return payload_id_list(value, "assigned_to_ids")
def _readable_boards(profile):
"""Boards ``profile`` may open: its owner or a member, with no org-admin
exception, inside their org.
Every board, column and card lookup goes through this queryset, so an id
the caller may not open raises the same 404 as an id that does not exist.
Checking access after an org-wide lookup gave the two different answers:
first a 403 on the write verbs, then a 404 with a different body, and
either one told anyone in the org which board and card ids exist.
"""
return (
Board.objects.filter(org=profile.org)
.filter(Q(owner=profile) | Q(members=profile))
.distinct()
)
def _set_card_assignees(card, org, ids):
"""Replace the card's assignees with the profiles in ``ids`` who can open
its board, and email the ones this write added.
@ -117,16 +134,12 @@ class BoardListCreateView(APIView, LimitOffsetPagination):
)
def get(self, request):
"""List all boards for the user's organization"""
org = request.profile.org
user_profile = request.profile
# Get boards where user is owner or member
queryset = (
Board.objects.filter(
Q(org=org) & (Q(owner=user_profile) | Q(members=user_profile))
)
_readable_boards(user_profile)
.prefetch_related("memberships")
.distinct()
.order_by("-created_at")
)
@ -204,16 +217,9 @@ class BoardDetailView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get_object(self, pk, org, user_profile):
"""Get board if user has access"""
board = get_object_or_404(Board, pk=pk, org=org)
# Check if user is owner or member
if (
board.owner != user_profile
and not board.members.filter(id=user_profile.id).exists()
):
return None
return board
def get_object(self, pk, user_profile):
"""The board, or 404 when it is missing or the caller may not open it."""
return get_object_or_404(_readable_boards(user_profile), pk=pk)
@extend_schema(
tags=["Boards"],
@ -226,12 +232,7 @@ class BoardDetailView(APIView):
)
def get(self, request, pk):
"""Get board details with columns and tasks"""
board = self.get_object(pk, request.profile.org, request.profile)
if not board:
return Response(
{"error": "Board not found or access denied"},
status=status.HTTP_404_NOT_FOUND,
)
board = self.get_object(pk, request.profile)
serializer = BoardSerializer(board)
return Response(serializer.data)
@ -247,12 +248,7 @@ class BoardDetailView(APIView):
)
def put(self, request, pk):
"""Update board"""
board = self.get_object(pk, request.profile.org, request.profile)
if not board:
return Response(
{"error": "Board not found or access denied"},
status=status.HTTP_404_NOT_FOUND,
)
board = self.get_object(pk, request.profile)
# Only owner or admin can update board
membership = BoardMember.objects.filter(
@ -286,12 +282,7 @@ class BoardDetailView(APIView):
)
def patch(self, request, pk):
"""Handle partial updates to a board."""
board = self.get_object(pk, request.profile.org, request.profile)
if not board:
return Response(
{"error": "Board not found or access denied"},
status=status.HTTP_404_NOT_FOUND,
)
board = self.get_object(pk, request.profile)
# Only owner or admin can update board
membership = BoardMember.objects.filter(
@ -323,12 +314,7 @@ class BoardDetailView(APIView):
)
def delete(self, request, pk):
"""Delete board (owner only)"""
board = self.get_object(pk, request.profile.org, request.profile)
if not board:
return Response(
{"error": "Board not found or access denied"},
status=status.HTTP_404_NOT_FOUND,
)
board = self.get_object(pk, request.profile)
# Only owner can delete
if board.owner != request.profile:
@ -357,18 +343,7 @@ class BoardColumnListCreateView(APIView):
)
def get(self, request, board_pk):
"""List all columns for a board"""
org = request.profile.org
board = get_object_or_404(Board, pk=board_pk, org=org)
# Check access
if (
board.owner != request.profile
and not board.members.filter(id=request.profile.id).exists()
):
return Response(
{"error": "Board not found or access denied"},
status=status.HTTP_404_NOT_FOUND,
)
board = get_object_or_404(_readable_boards(request.profile), pk=board_pk)
# Prefetch the nested cards with their account + assignees so rendering a
# full board (this is the endpoint the kanban reads) stays a handful of
@ -397,7 +372,7 @@ class BoardColumnListCreateView(APIView):
def post(self, request, board_pk):
"""Create a new column"""
org = request.profile.org
board = get_object_or_404(Board, pk=board_pk, org=org)
board = get_object_or_404(_readable_boards(request.profile), pk=board_pk)
# Check permission
membership = BoardMember.objects.filter(
@ -454,18 +429,9 @@ class BoardTaskListCreateView(APIView):
)
def get(self, request, column_pk):
"""List all tasks for a column"""
org = request.profile.org
column = get_object_or_404(BoardColumn, pk=column_pk, board__org=org)
# Check access
board = column.board
if (
board.owner != request.profile
and not board.members.filter(id=request.profile.id).exists()
):
return Response(
{"error": "Access denied"}, status=status.HTTP_404_NOT_FOUND
)
column = get_object_or_404(
BoardColumn, pk=column_pk, board__in=_readable_boards(request.profile)
)
tasks = column.tasks.all()
serializer = BoardTaskSerializer(tasks, many=True)
@ -484,17 +450,9 @@ class BoardTaskListCreateView(APIView):
def post(self, request, column_pk):
"""Create a new task"""
org = request.profile.org
column = get_object_or_404(BoardColumn, pk=column_pk, board__org=org)
# Check access
board = column.board
membership = BoardMember.objects.filter(
board=board, profile=request.profile
).first()
if not membership:
return Response(
{"error": "Access denied"}, status=status.HTTP_403_FORBIDDEN
)
column = get_object_or_404(
BoardColumn, pk=column_pk, board__in=_readable_boards(request.profile)
)
data = request.data.copy()
try:
@ -536,17 +494,10 @@ class BoardTaskDetailView(APIView):
def put(self, request, pk):
"""Update task (including moving to a different column)."""
org = request.profile.org
task = get_object_or_404(BoardTask, pk=pk, column__board__org=org)
# Check access
task = get_object_or_404(
BoardTask, pk=pk, column__board__in=_readable_boards(request.profile)
)
board = task.column.board
membership = BoardMember.objects.filter(
board=board, profile=request.profile
).first()
if not membership:
return Response(
{"error": "Access denied"}, status=status.HTTP_403_FORBIDDEN
)
data = request.data.copy()
try:
@ -619,18 +570,9 @@ class BoardTaskDetailView(APIView):
)
def delete(self, request, pk):
"""Delete task"""
org = request.profile.org
task = get_object_or_404(BoardTask, pk=pk, column__board__org=org)
# Check permission
board = task.column.board
membership = BoardMember.objects.filter(
board=board, profile=request.profile
).first()
if not membership or membership.role not in ["owner", "admin", "member"]:
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
task = get_object_or_404(
BoardTask, pk=pk, column__board__in=_readable_boards(request.profile)
)
task.delete()
return Response(status=status.HTTP_204_NO_CONTENT)

View file

@ -3,6 +3,7 @@ Kanban views for task management.
Supports both status-based (default) and custom pipeline-based kanban boards.
"""
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db import transaction
from django.db.models import Q
from django.shortcuts import get_object_or_404
@ -15,6 +16,7 @@ from rest_framework.views import APIView
from common.kanban import place_in_column
from common.permissions import HasOrgContext, is_org_admin
from common.validators import date_param, uuid_param
from tasks.access import assert_task_access, visible_tasks_qs
from tasks.models import Task, TaskPipeline, TaskStage
from tasks.serializer import (
TaskKanbanCardSerializer,
@ -23,6 +25,7 @@ from tasks.serializer import (
TaskPipelineSerializer,
TaskStageSerializer,
)
from tasks.views.task_views import _model_errors
class TaskKanbanView(APIView):
@ -117,18 +120,19 @@ class TaskKanbanView(APIView):
.prefetch_related("assigned_to", "tags")
)
# Apply permission filtering
if not is_org_admin(request.profile) and not request.user.is_superuser:
queryset = queryset.filter(
Q(assigned_to=request.profile) | Q(created_by=request.profile.user)
)
# The task read rule itself, as the list and the detail view apply it:
# no superuser clause, because the detail view has none. By id, so a
# card with several assignees appears once.
queryset = queryset.filter(
pk__in=visible_tasks_qs(request.profile).values("pk")
)
# Apply search/filters
queryset = self._apply_filters(queryset, request.query_params)
if pipeline_id:
# Pipeline-based kanban
return self._get_pipeline_kanban(queryset, pipeline_id, org)
return self._get_pipeline_kanban(queryset, pipeline_id, request)
# Status-based kanban
return self._get_status_kanban(queryset)
@ -202,10 +206,15 @@ class TaskKanbanView(APIView):
}
)
def _get_pipeline_kanban(self, queryset, pipeline_id, org):
def _get_pipeline_kanban(self, queryset, pipeline_id, request):
"""Build kanban data using TaskPipeline stages as columns."""
pipeline = get_object_or_404(
TaskPipeline, pk=pipeline_id, org=org, is_active=True
TaskPipelineListSerializer.with_counts(
TaskPipeline.objects.all(), request.profile
),
pk=pipeline_id,
org=request.profile.org,
is_active=True,
)
# Filter tasks to this pipeline
@ -258,15 +267,11 @@ class TaskMoveView(APIView):
# edit committing between this read and that save would be lost.
task = get_object_or_404(Task.objects.select_for_update(), pk=pk, org=org)
# Permission check
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not (
request.profile.user == task.created_by
or request.profile in task.assigned_to.all()
):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
# A move rewrites the task's status, stage and order, so it takes the
# task's own access rule and nothing wider. This used to add a Django
# superuser clause, which let a superuser who is a plain member of the
# org move any task here that `TaskDetailView.patch` refuses them.
assert_task_access(request.profile, task)
serializer = TaskMoveSerializer(data=request.data)
if not serializer.is_valid():
@ -316,7 +321,16 @@ class TaskMoveView(APIView):
exclude_pk=task.pk,
)
task.save()
# `Task.save()` runs `full_clean()`, which refuses, for one, a stage
# whose free-text `maps_to_status` is not a task status. PATCH answers
# that refusal 400 with the model's own errors; so does the move.
try:
task.save()
except DjangoValidationError as exc:
return Response(
{"error": True, "errors": _model_errors(exc)},
status=status.HTTP_400_BAD_REQUEST,
)
return Response(
{
@ -351,7 +365,9 @@ class TaskPipelineListCreateView(APIView):
def get(self, request):
"""List all pipelines for the organization."""
org = request.profile.org
pipelines = TaskPipeline.objects.filter(org=org, is_active=True)
pipelines = TaskPipelineListSerializer.with_counts(
TaskPipeline.objects.filter(org=org, is_active=True), request.profile
)
serializer = TaskPipelineListSerializer(pipelines, many=True)
return Response({"pipelines": serializer.data})
@ -416,7 +432,8 @@ class TaskPipelineListCreateView(APIView):
TaskStage.objects.create(pipeline=pipeline, org=org, **stage_data)
return Response(
TaskPipelineSerializer(pipeline).data, status=status.HTTP_201_CREATED
TaskPipelineSerializer(pipeline, context={"request": request}).data,
status=status.HTTP_201_CREATED,
)
@ -432,7 +449,9 @@ class TaskPipelineDetailView(APIView):
def get(self, request, pk):
"""Get pipeline details with all stages."""
pipeline = self.get_object(pk, request.profile.org)
return Response(TaskPipelineSerializer(pipeline).data)
return Response(
TaskPipelineSerializer(pipeline, context={"request": request}).data
)
@extend_schema(
tags=["Task Pipelines"],
@ -456,7 +475,9 @@ class TaskPipelineDetailView(APIView):
)
pipeline = serializer.save(updated_by=request.user)
return Response(TaskPipelineSerializer(pipeline).data)
return Response(
TaskPipelineSerializer(pipeline, context={"request": request}).data
)
@extend_schema(tags=["Task Pipelines"], responses={204: None})
def delete(self, request, pk):
@ -511,7 +532,10 @@ class TaskStageCreateView(APIView):
)
stage = serializer.save(pipeline=pipeline, org=org, created_by=request.user)
return Response(TaskStageSerializer(stage).data, status=status.HTTP_201_CREATED)
return Response(
TaskStageSerializer(stage, context={"request": request}).data,
status=status.HTTP_201_CREATED,
)
class TaskStageDetailView(APIView):
@ -541,7 +565,7 @@ class TaskStageDetailView(APIView):
)
stage = serializer.save(updated_by=request.user)
return Response(TaskStageSerializer(stage).data)
return Response(TaskStageSerializer(stage, context={"request": request}).data)
@extend_schema(tags=["Task Stages"], responses={204: None})
def delete(self, request, pk):

View file

@ -13,7 +13,7 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from accounts.models import Account
from accounts.access import visible_accounts_qs
from accounts.serializer import AccountPickerSerializer
from common.custom_fields import validate_payload as validate_custom_fields_payload
from common.models import (
@ -40,8 +40,7 @@ from common.validators import (
uuid_list_param,
uuid_param,
)
from contacts.access import replace_visible_contacts
from contacts.models import Contact
from contacts.access import replace_visible_contacts, visible_contacts_qs
from contacts.serializer import ContactPickerSerializer
from tasks import swagger_params
from tasks.access import (
@ -225,19 +224,15 @@ class TaskListView(APIView, LimitOffsetPagination):
context["users"] = list(profiles.values("id", "user__email"))
# The catalogues are for the parent picker and grow with the org, not
# with the page, `?slim=true` omits them for callers that only want
# the list. A non-admin is offered only the accounts and contacts they
# created or are assigned to, as `/api/cases/` and
# `/api/opportunities/` already did: this endpoint used to hand every
# member the whole org's accounts and contacts in full.
# the list. Each offers exactly the records its own detail view would
# open for this caller, by calling that module's read rule: accounts
# to admins, superusers, the creator and assignees; contacts to the
# same, plus anyone assigned to one of the contact's accounts. This
# endpoint used to hand every member the whole org's accounts and
# contacts in full.
if params.get("slim") != "true":
accounts = Account.objects.filter(org=self.request.profile.org)
contacts = Contact.objects.filter(org=self.request.profile.org)
if not is_org_admin(self.request.profile):
member_scope = Q(created_by=self.request.profile.user) | Q(
assigned_to=self.request.profile
)
accounts = accounts.filter(member_scope).distinct()
contacts = contacts.filter(member_scope).distinct()
accounts = visible_accounts_qs(self.request.profile, self.request.user)
contacts = visible_contacts_qs(self.request.profile)
context["accounts_list"] = AccountPickerSerializer(accounts, many=True).data
context["contacts_list"] = ContactPickerSerializer(contacts, many=True).data
return context

View file

@ -386,9 +386,24 @@ export async function getContactForEdit({ cookies }, id) {
const raw = response.contact_obj;
const contact = toRow(raw);
/*
* The linked account is always an option, even when the picker does not
* carry it: an account the caller cannot open (the list holds only the ones
* they can), a closed one, or one past the 200 cap. A select whose value
* matches no option shows nothing and submits nothing or its first entry, so
* a save would unlink it. The server keeps an unchanged link whoever saves,
* and refuses a newly chosen account the caller cannot open.
*/
const stored = raw.account_detail;
const accounts =
stored && !choices.accounts.some((/** @type {any} */ row) => row.id === stored.id)
? [{ id: stored.id, name: stored.name }, ...choices.accounts]
: choices.accounts;
return {
contact,
...choices,
accounts,
form: {
first_name: contact.first_name,
last_name: contact.last_name,
@ -477,7 +492,7 @@ export async function updateContact({ cookies }, id, values) {
*
* No `org` and no `created_by` in the body: `ContactsListView.post` sets both
* from `request.profile`, and `CreateContactSerializer` lists neither as a
* field. `account` is checked against the caller's org before it is accepted.
* field. `account` must be one the caller can open, or it is refused.
*
* The response now carries the new `id`, which it did not before this change.
* Without it a client cannot open what it just made.

View file

@ -87,3 +87,58 @@ describe('getContactForEdit deal count', () => {
expect(server.deal_count).toBe(10);
});
});
describe('getContactForEdit account picker', () => {
beforeEach(() => {
apiRequest.mockReset();
});
/** @param {any} contactObj */
function respond(contactObj) {
apiRequest.mockImplementation(async (/** @type {string} */ url) => {
if (url.startsWith('/contacts/c1/'))
return { ...detail({ count: 0, by_currency: [] }), contact_obj: contactObj };
if (url.startsWith('/accounts/'))
return {
active_accounts: {
open_accounts: [{ id: 'a-visible', name: 'Visible Co' }],
open_accounts_count: 1
}
};
return {};
});
}
const base = { id: 'c1', first_name: 'Ada', last_name: 'Lovelace', assigned_to: [], teams: [] };
it('offers a linked account the picker list does not carry, so a save keeps it', async () => {
respond({
...base,
account: 'a-hidden',
account_detail: { id: 'a-hidden', name: 'Hidden Holdings' }
});
const { accounts, form } = await getContactForEdit(event, 'c1');
expect(form.account).toBe('a-hidden');
expect(accounts).toEqual([
{ id: 'a-hidden', name: 'Hidden Holdings' },
{ id: 'a-visible', name: 'Visible Co' }
]);
});
it('does not repeat a linked account the list already carries', async () => {
respond({
...base,
account: 'a-visible',
account_detail: { id: 'a-visible', name: 'Visible Co' }
});
const { accounts } = await getContactForEdit(event, 'c1');
expect(accounts).toEqual([{ id: 'a-visible', name: 'Visible Co' }]);
});
it('offers only the list when the contact has no account', async () => {
respond({ ...base, account: null, account_detail: null });
const { accounts, form } = await getContactForEdit(event, 'c1');
expect(form.account).toBe('');
expect(accounts).toEqual([{ id: 'a-visible', name: 'Visible Co' }]);
});
});

View file

@ -0,0 +1,60 @@
/**
* The parent a ticket sits under, as the list and the detail load read it.
*
* `parent_summary` names the parent only when the viewer may open it. A hidden
* parent arrives as `{ id, name: null, status: null, restricted: true }` (D51,
* the same redaction `/tree/` applies to a hidden node), and must read as the
* same phrase the tree uses rather than as an empty or null name.
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js';
const apiRequest = vi.fn();
vi.mock('$lib/api-helpers.js', () => ({ apiRequest: (...a) => apiRequest(...a) }));
const { getTicket, listTickets } = await import('./tickets.js');
const event = /** @type {any} */ ({ cookies: { get: () => 'token' } });
const HIDDEN = { id: 'p-hidden', name: null, status: null, restricted: true };
const READABLE = { id: 'p-open', name: 'Outage umbrella', status: 'Pending', restricted: false };
/** @param {any} parent_summary */
function row(id, parent_summary) {
return { id, name: `Ticket ${id}`, status: 'New', priority: 'Low', parent_summary };
}
describe('a ticket parent', () => {
beforeEach(() => {
apiRequest.mockReset();
});
it('maps each list row on its own parent', async () => {
apiRequest.mockResolvedValue({
cases: [row('a', HIDDEN), row('b', READABLE), row('c', null)]
});
const { results } = await listTickets(event);
expect(results.map((t) => t.parent)).toEqual([
{ id: 'p-hidden', name: RESTRICTED_TICKET_NAME, status: null, restricted: true },
{ id: 'p-open', name: 'Outage umbrella', status: 'Pending', restricted: false },
null
]);
});
it('never renders a hidden parent as a blank name on the detail', async () => {
apiRequest.mockResolvedValue({ cases_obj: row('a', HIDDEN), comment_permission: false });
const { ticket } = await getTicket(event, 'a');
expect(ticket.parent).toEqual({
id: 'p-hidden',
name: RESTRICTED_TICKET_NAME,
status: null,
restricted: true
});
});
it('is the phrase the tree uses for a hidden node', () => {
expect(RESTRICTED_TICKET_NAME).toBe('A ticket you cannot open');
});
});

View file

@ -46,6 +46,7 @@
import { error, redirect } from '@sveltejs/kit';
import { apiRequest } from '$lib/api-helpers.js';
import { attachmentHref } from '$lib/server/v2/files.js';
import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js';
/** Statuses where somebody still owes the customer something. Mirrors `cases.views.OPEN_STATUSES`. */
export const OPEN_STATUSES = ['New', 'Assigned', 'Pending'];
@ -64,6 +65,29 @@ function accountLink(account) {
return account ? { id: account.id, name: account.name ?? '' } : null;
}
/**
* The ticket this one sits under, or null.
*
* A parent the viewer may not open arrives as `{ id, name: null, status: null,
* restricted: true }`. It keeps its id (the viewer may still detach from it,
* since unlinking needs write on the child alone) and reads as
* `RESTRICTED_TICKET_NAME`, never as a link: opening it would only answer 403.
*
* @param {any} parent
*/
function parentLink(parent) {
if (!parent) return null;
if (parent.restricted) {
return { id: parent.id, name: RESTRICTED_TICKET_NAME, status: null, restricted: true };
}
return {
id: parent.id,
name: parent.name ?? '',
status: parent.status ?? null,
restricted: false
};
}
/**
* The subset of a Django case the v2 pages read.
*
@ -110,7 +134,7 @@ function toRow(row) {
escalation_count: row.escalation_count ?? 0,
paused_at: row.sla_paused_at ?? null,
child_count: row.child_count ?? 0,
parent: row.parent_summary ?? null,
parent: parentLink(row.parent_summary),
// Everyone's logged time on this ticket, which is not what
// `/cases/<id>/time-entries/` returns to an agent: that list is narrowed
// to their own rows, so a panel adding it up would tell a team of three

View file

@ -5,8 +5,9 @@
* sources into a single prioritised action list: support cases still awaiting a
* first reply, invoices past due, deals that have gone quiet (stage-aging), and
* tasks overdue or due today. The endpoint is org-scoped from the JWT and, for a
* member, restricted to rows assigned to or created by them, so nothing here
* decides visibility; the API does.
* member, narrowed to rows whose action they can take (a ticket they only watch
* is left out, since they cannot reply to it), so nothing here decides
* visibility; the API does.
*
* This is read-only: the home page links out to each item's own module, where
* the writes already live. There is no write path to wire here.

View file

@ -176,6 +176,15 @@ export const CASE_STATUS_TONE = {
Duplicate: 'slate'
};
/**
* What a ticket the viewer may not open is called wherever it still appears.
*
* The API keeps such a ticket in place, by id, with `restricted: true` and no
* name: a node in `/tree/`, or a ticket's `parent_summary`. Shared so the tree,
* the close confirm step and the parent link all say the same thing.
*/
export const RESTRICTED_TICKET_NAME = 'A ticket you cannot open';
export const INVOICE_STATUS_TONE = {
Draft: 'slate',
Sent: 'ink',

View file

@ -166,8 +166,10 @@ export const actions = {
* PATCHes the case; this posts to `close-with-children/`, which closes the
* subtree in one transaction and writes a `PARENT_CLOSED_CASCADE` activity
* row on each child. Folding the two together would mean a ticket with no
* children took the heavier path for no reason, and the approval gate on the
* ordinary close lives on the PATCH.
* children took the heavier path for no reason. Both take the same approval
* gate; this one also refuses the whole cascade (nothing closed) when any
* ticket it would take is not the viewer's to close, and the refusal lands
* in `form.error` like any other.
*
* `cascade` is read from the checkbox, so an unticked box sends `false` and
* closes the parent alone. It is never omitted: the API reads the org

View file

@ -25,9 +25,21 @@
* closed child is still cascaded.
*/
import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js';
/** A ticket closes to any status but this one. */
const CLOSED = 'Closed';
/**
* What a ticket the viewer may not open is called in the list.
*
* `/tree/` redacts such a node to `{ id, name: null, restricted: true, status,
* is_active }`: kept, so the count and the shape stay true, but nameless. The
* close refuses the whole cascade when one of these would be closed, so the
* list should say plainly that it is there.
*/
export const RESTRICTED_NAME = RESTRICTED_TICKET_NAME;
/**
* The node for `id` inside the tree, or null.
*
@ -49,13 +61,13 @@ export function findNode(root, id) {
*
* @param {any} root the tree response's `root`
* @param {string} id the ticket being closed
* @returns {Array<{id: string, name: string, status: string}>}
* @returns {Array<{id: string, name: string, status: string, restricted: boolean}>}
*/
export function openDescendants(root, id) {
const focus = findNode(root, id);
if (!focus) return [];
/** @type {Array<{id: string, name: string, status: string}>} */
/** @type {Array<{id: string, name: string, status: string, restricted: boolean}>} */
const out = [];
/** @param {any} node */
const walk = (node) => {
@ -63,7 +75,12 @@ export function openDescendants(root, id) {
// Open AND active, matching `_open_descendants`. An inactive row is a
// merged duplicate and the backend skips it.
if (child.status !== CLOSED && child.is_active !== false) {
out.push({ id: child.id, name: child.name, status: child.status });
out.push({
id: child.id,
name: child.restricted ? RESTRICTED_NAME : child.name,
status: child.status,
restricted: Boolean(child.restricted)
});
}
// Through closed children regardless: an open grandchild under a closed
// child still cascades.

View file

@ -5,7 +5,8 @@ import {
subtreeTruncated,
cascadeSummary,
closeResultMessage,
cascadedCount
cascadedCount,
RESTRICTED_NAME
} from './close.js';
/** @param {any} over */
@ -93,6 +94,20 @@ describe('openDescendants', () => {
expect(openDescendants(tree, 'p').map((d) => d.id)).toEqual(['real']);
});
it('lists a ticket the viewer cannot open, without a name', () => {
// `/tree/` redacts it to an id, a status and `restricted`. It still
// counts, because the close refuses a cascade that would take it.
const tree = node({
id: 'p',
children: [
{ id: 'hidden', name: null, restricted: true, status: 'New', is_active: true, children: [] }
]
});
expect(openDescendants(tree, 'p')).toEqual([
{ id: 'hidden', name: RESTRICTED_NAME, status: 'New', restricted: true }
]);
});
it('is empty for a leaf, and for a ticket not in the tree', () => {
expect(openDescendants(TREE, 'kidOpen')).toEqual([]);
expect(openDescendants(TREE, 'nope')).toEqual([]);

View file

@ -20,6 +20,7 @@ On 2026-09-26 the counts were 90 `+page.svelte` files under `frontend/src/routes
| ID | Gap | Missing on | Backend | Opened | Notes |
|---|---|---|---|---|---|
| B1 | CSV import for contacts, tickets and leads | Mobile | `import/preview/` and `import/commit/` exist | 2026-09-26 | Web mounted `ContactImportDrawer` and `TicketImportDrawer` on 2026-09-26, and `LeadImportDrawer` (`/api/leads/import/`) the same day. Tracked as G1 in `enterprise-crm/docs/gap-analysis/TRACKER.md`. |
| B3 | Parent and child tickets: parent banner, link parent, detach, tree | Web | `/api/cases/<id>/tree/`, `link/`, `close-with-children/` and `parent_summary` exist (authz fixed as D50 and D51) | 2026-09-26 | Mobile has all of it in `ticket_detail_screen.dart`. Web has `TicketTreePanel.svelte` and `LinkParentDialog.svelte` exported from the components index but imported nowhere; only the close-with-children cascade (`tickets/[id]/close.js`) is wired. |
## Web-only by design

View file

@ -461,22 +461,33 @@ class Ticket {
}
/// Lightweight ref to a parent ticket, used to render the "linked to" tile.
///
/// A parent the viewer may not open arrives as `restricted: true` with no name
/// or status, the redaction `/tree/` applies to a hidden node. [name] then
/// reads as [restrictedName] and the screen does not offer to open it. The id
/// stays, since detaching from it needs write on the child alone.
class TicketParentSummary {
static const restrictedName = 'A ticket you cannot open';
final String id;
final String name;
final String? status;
final bool restricted;
const TicketParentSummary({
required this.id,
required this.name,
this.status,
this.restricted = false,
});
factory TicketParentSummary.fromJson(Map<String, dynamic> json) {
final restricted = json['restricted'] == true;
return TicketParentSummary(
id: json['id']?.toString() ?? '',
name: json['name']?.toString() ?? '',
status: json['status'] as String?,
name: restricted ? restrictedName : json['name']?.toString() ?? '',
status: restricted ? null : json['status'] as String?,
restricted: restricted,
);
}
}

View file

@ -661,7 +661,14 @@ class TicketDetailResult {
}
/// Recursive tree node returned by `/api/cases/{id}/tree/`.
///
/// A ticket the viewer may not open comes back redacted: `restricted: true`,
/// `name: null`, and only its id, status and active flag. It stays in the tree
/// so the shape and the cascade count are true; [name] reads as
/// [restrictedName] for it, and the screen does not offer to open it.
class TicketTreeNode {
static const restrictedName = TicketParentSummary.restrictedName;
final String id;
final String name;
final String? status;
@ -669,6 +676,7 @@ class TicketTreeNode {
final bool isProblem;
final bool isActive;
final bool truncated;
final bool restricted;
final List<TicketTreeNode> children;
final String? focusId;
@ -680,6 +688,7 @@ class TicketTreeNode {
this.isProblem = false,
this.isActive = true,
this.truncated = false,
this.restricted = false,
this.children = const [],
this.focusId,
});
@ -690,14 +699,16 @@ class TicketTreeNode {
Map<String, dynamic> json, {
String? focusId,
}) {
final restricted = json['restricted'] == true;
return TicketTreeNode(
id: json['id']?.toString() ?? '',
name: json['name']?.toString() ?? '',
name: restricted ? restrictedName : json['name']?.toString() ?? '',
status: json['status'] as String?,
priority: json['priority'] as String?,
isProblem: json['is_problem'] as bool? ?? false,
isActive: json['is_active'] as bool? ?? true,
truncated: json['truncated'] as bool? ?? false,
restricted: restricted,
children: ((json['children'] as List<dynamic>?) ?? const [])
.whereType<Map<String, dynamic>>()
.map((c) => TicketTreeNode.fromJson(c, focusId: focusId))

View file

@ -334,7 +334,24 @@ class _ContactFormScreenState extends ConsumerState<ContactFormScreen> {
/// choice, because a select whose value matches no option submits its first
/// entry, which would attach the contact to whichever company happens to
/// sort first.
Widget _accountPicker(List<AccountLookup> accounts) {
///
/// The account the contact is already linked to is always an item, even
/// when the lookup does not carry it: one the user cannot open (the lookup
/// holds only those they can), a closed one, or one past the lookup cap.
/// Otherwise the picker reads "No account" while the save keeps the link.
/// The server keeps an unchanged link whoever saves, and refuses a newly
/// chosen account the user cannot open.
Widget _accountPicker(List<AccountLookup> lookup) {
final storedId = _existing?.accountId;
final accounts = storedId == null || lookup.any((a) => a.id == storedId)
? lookup
: [
AccountLookup(
id: storedId,
name: _existing?.accountName ?? 'Current account',
),
...lookup,
];
final ids = accounts.map((a) => a.id).toSet();
final value = ids.contains(_accountId) ? _accountId : null;
return Padding(

View file

@ -1199,11 +1199,16 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
children: [
Icon(icon, size: 20, color: iconColor ?? AppColors.textSecondary),
const SizedBox(width: 16),
Text(
label,
style: AppTypography.body.copyWith(
color: labelColor ?? AppColors.textPrimary,
fontWeight: FontWeight.w500,
// Expanded so a long label wraps instead of overflowing: the
// detach row names the parent, and a ticket subject has no limit
// a 390px sheet can hold.
Expanded(
child: Text(
label,
style: AppTypography.body.copyWith(
color: labelColor ?? AppColors.textPrimary,
fontWeight: FontWeight.w500,
),
),
),
],
@ -1636,7 +1641,10 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
const SizedBox(height: 12),
if (c.parentSummary != null)
InkWell(
onTap: () => context.push('/tickets/${c.parentSummary!.id}'),
// A parent the viewer cannot open would only answer 403.
onTap: c.parentSummary!.restricted
? null
: () => context.push('/tickets/${c.parentSummary!.id}'),
child: Padding(
padding: const EdgeInsets.symmetric(vertical: 6),
child: Row(
@ -1661,6 +1669,12 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
c.parentSummary!.name,
style: AppTypography.body.copyWith(
fontWeight: FontWeight.w500,
fontStyle: c.parentSummary!.restricted
? FontStyle.italic
: FontStyle.normal,
color: c.parentSummary!.restricted
? AppColors.textTertiary
: null,
),
),
],
@ -1711,7 +1725,10 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
if (depth > 0) {
rows.add(
InkWell(
onTap: () => context.push('/tickets/${node.id}'),
// A ticket the viewer cannot open would only answer 403.
onTap: node.restricted
? null
: () => context.push('/tickets/${node.id}'),
child: Padding(
padding: EdgeInsets.fromLTRB((depth - 1) * 16.0, 6, 0, 6),
child: Row(
@ -1729,8 +1746,13 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
fontWeight: node.isFocused
? FontWeight.w700
: FontWeight.w500,
fontStyle: node.restricted
? FontStyle.italic
: FontStyle.normal,
color: node.isFocused
? AppColors.primary600
: node.restricted
? AppColors.textTertiary
: AppColors.textPrimary,
),
),
@ -1785,7 +1807,11 @@ class _TicketDetailScreenState extends ConsumerState<TicketDetailScreen>
_actionRow(
icon: LucideIcons.unlink,
iconColor: AppColors.danger600,
label: 'Detach from "${c.parentSummary!.name}"',
// Unlinking needs write on this ticket only, so a hidden
// parent can still be left; it just is not named.
label: c.parentSummary!.restricted
? 'Detach from parent ticket'
: 'Detach from "${c.parentSummary!.name}"',
labelColor: AppColors.danger600,
onTap: () => Navigator.pop(context, 'detach'),
),

View file

@ -0,0 +1,131 @@
import 'package:bottle_crm/core/theme/theme.dart';
import 'package:bottle_crm/data/models/contact.dart';
import 'package:bottle_crm/data/models/lookup_models.dart';
import 'package:bottle_crm/providers/contacts_provider.dart';
import 'package:bottle_crm/providers/lookup_provider.dart';
import 'package:bottle_crm/screens/contacts/contact_form_screen.dart';
import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:go_router/go_router.dart';
/// The contact's linked account stays on the picker and in the save.
///
/// The accounts lookup carries only the accounts the user can open, so a
/// contact an admin linked to some other company has an `account` the picker
/// does not list. The picker used to read "No account" for it. The save still
/// sent the id, which the server keeps as an unchanged link (D39), so the
/// screen said one thing and the record another.
void main() {
TestWidgetsFlutterBinding.ensureInitialized();
const visible = AccountLookup(id: 'a-visible', name: 'Visible Co');
Contact contact({String? accountId, String? accountName}) =>
Contact.fromJson({
'id': 'c1',
'first_name': 'Ada',
'last_name': 'Lovelace',
'account': accountId,
'account_detail': accountId == null
? null
: {'id': accountId, 'name': accountName},
});
Future<_FakeContacts> open(WidgetTester tester, Contact stored) async {
final fake = _FakeContacts(stored);
await tester.pumpWidget(
ProviderScope(
overrides: [
contactsProvider.overrideWith(() => fake),
accountOptionsProvider.overrideWithValue(const [visible]),
],
child: MaterialApp.router(
theme: AppTheme.light,
routerConfig: GoRouter(
initialLocation: '/contacts/edit',
routes: [
GoRoute(
path: '/contacts',
builder: (_, _) => const Text('list'),
routes: [
GoRoute(
path: 'edit',
builder: (_, _) => const ContactFormScreen(contactId: 'c1'),
),
],
),
],
),
),
),
);
await tester.pumpAndSettle();
return fake;
}
Future<void> save(WidgetTester tester) async {
final button = find.text('Save changes');
await tester.ensureVisible(button);
await tester.tap(button);
await tester.pumpAndSettle();
}
testWidgets('shows and keeps a linked account the lookup does not carry', (
tester,
) async {
final fake = await open(
tester,
contact(accountId: 'a-hidden', accountName: 'Hidden Holdings'),
);
expect(find.text('Hidden Holdings'), findsOneWidget);
expect(find.text('No account'), findsNothing);
await save(tester);
expect(fake.updates.single['account'], 'a-hidden');
});
testWidgets('a linked account the lookup carries is listed once', (
tester,
) async {
final fake = await open(
tester,
contact(accountId: 'a-visible', accountName: 'Visible Co'),
);
expect(find.text('Visible Co'), findsOneWidget);
await save(tester);
expect(fake.updates.single['account'], 'a-visible');
});
testWidgets('no linked account reads as No account and sends none', (
tester,
) async {
final fake = await open(tester, contact());
expect(find.text('No account'), findsOneWidget);
await save(tester);
expect(fake.updates.single['account'], isNull);
});
}
class _FakeContacts extends ContactsNotifier {
_FakeContacts(this.stored);
final Contact stored;
/// Every update body, in order.
final List<Map<String, dynamic>> updates = [];
@override
Future<ContactsListData> build() async => const ContactsListData();
@override
Future<Contact?> getContact(String id) async => stored;
@override
Future<String?> updateContact(String id, Map<String, dynamic> payload) async {
updates.add(payload);
return null;
}
}

View file

@ -0,0 +1,227 @@
import 'package:bottle_crm/core/theme/theme.dart';
import 'package:bottle_crm/data/models/ticket.dart';
import 'package:bottle_crm/providers/lookup_provider.dart';
import 'package:bottle_crm/providers/tickets_provider.dart';
import 'package:bottle_crm/screens/tickets/ticket_detail_screen.dart';
import 'package:bottle_crm/widgets/cards/ticket_card.dart';
import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:go_router/go_router.dart';
import 'package:lucide_icons_flutter/lucide_icons.dart';
/// A ticket whose parent the viewer may not open (D51).
///
/// `parent_summary` used to carry the parent's name to anyone who could read
/// the child. The API now sends `{id, name: null, status: null, restricted:
/// true}` for a hidden parent, the redaction `/tree/` uses for a hidden node,
/// and the phone must say "A ticket you cannot open", not offer to open it
/// (that would only answer 403), and still let the viewer detach from it,
/// since unlinking needs write on the child alone.
void main() {
TestWidgetsFlutterBinding.ensureInitialized();
const hiddenJson = {
'id': 'p-hidden',
'name': null,
'status': null,
'restricted': true,
};
const readableJson = {
'id': 'p-open',
'name': 'Outage umbrella',
'status': 'Pending',
'restricted': false,
};
group('TicketParentSummary.fromJson', () {
test('a hidden parent reads as the tree phrase, with no status', () {
final p = TicketParentSummary.fromJson(hiddenJson);
expect(p.id, 'p-hidden');
expect(p.restricted, isTrue);
expect(p.name, 'A ticket you cannot open');
expect(p.name, TicketTreeNode.restrictedName);
expect(p.status, isNull);
});
test('a readable parent keeps its name and status', () {
final p = TicketParentSummary.fromJson(readableJson);
expect(p.restricted, isFalse);
expect(p.name, 'Outage umbrella');
expect(p.status, 'Pending');
});
});
void usePhone(WidgetTester tester, {double textScale = 1.0}) {
tester.view.devicePixelRatio = 3.0;
tester.view.physicalSize = const Size(390 * 3, 844 * 3);
tester.platformDispatcher.textScaleFactorTestValue = textScale;
addTearDown(tester.view.resetPhysicalSize);
addTearDown(tester.view.resetDevicePixelRatio);
addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
}
Ticket child(Map<String, dynamic> parent) => Ticket.fromJson({
'id': 'c1',
'name': 'My sub-ticket',
'status': 'New',
'priority': 'Normal',
'created_at': '2026-09-01T09:00:00Z',
'parent': parent['id'],
'parent_summary': parent,
});
group('TicketCard at 390px', () {
testWidgets('a hidden parent shows the sub-ticket pill and no name', (
tester,
) async {
// At 1.0: the card overflows at 1.3 with or without a parent, which is
// older than this change and not what it tests.
usePhone(tester);
await tester.pumpWidget(
MaterialApp(
theme: AppTheme.light,
home: Scaffold(body: TicketCard(ticketItem: child(hiddenJson))),
),
);
expect(tester.takeException(), isNull);
expect(find.text('Sub-ticket'), findsOneWidget);
});
});
group('TicketDetailScreen at 390px', () {
Widget app(Ticket ticket) => ProviderScope(
overrides: [
ticketsProvider.overrideWith(() => _FakeTicketsNotifier(ticket)),
usersProvider.overrideWithValue(const []),
tagsProvider.overrideWithValue(const []),
accountOptionsProvider.overrideWithValue(const []),
],
child: MaterialApp.router(
theme: AppTheme.light,
routerConfig: GoRouter(
initialLocation: '/here',
routes: [
GoRoute(
path: '/here',
builder: (_, _) => TicketDetailScreen(ticketId: ticket.id),
),
GoRoute(
path: '/tickets/:id',
builder: (_, s) =>
Scaffold(body: Text('opened ${s.pathParameters['id']}')),
),
],
),
),
);
testWidgets('a hidden parent is named as one and does not open', (
tester,
) async {
usePhone(tester);
await tester.pumpWidget(app(child(hiddenJson)));
await tester.pumpAndSettle();
expect(tester.takeException(), isNull);
final row = find.text('A ticket you cannot open');
await tester.ensureVisible(row);
await tester.pumpAndSettle();
expect(row, findsOneWidget);
await tester.tap(row);
await tester.pumpAndSettle();
expect(find.text('opened p-hidden'), findsNothing);
});
testWidgets('a readable parent is named and opens', (tester) async {
usePhone(tester);
await tester.pumpWidget(app(child(readableJson)));
await tester.pumpAndSettle();
final row = find.text('Outage umbrella');
await tester.ensureVisible(row);
await tester.pumpAndSettle();
await tester.tap(row);
await tester.pumpAndSettle();
expect(find.text('opened p-open'), findsOneWidget);
});
Future<void> openParentSheet(WidgetTester tester) async {
await tester.tap(find.byIcon(LucideIcons.moreVertical));
await tester.pumpAndSettle();
await tester.tap(find.text('Change / detach parent'));
await tester.pumpAndSettle();
}
testWidgets('detaching from a hidden parent is offered, unnamed', (
tester,
) async {
usePhone(tester);
await tester.pumpWidget(app(child(hiddenJson)));
await tester.pumpAndSettle();
await openParentSheet(tester);
expect(find.text('Detach from parent ticket'), findsOneWidget);
expect(find.textContaining('Detach from "'), findsNothing);
expect(tester.takeException(), isNull);
});
testWidgets('detaching from a readable parent names it', (tester) async {
usePhone(tester);
await tester.pumpWidget(app(child(readableJson)));
await tester.pumpAndSettle();
await openParentSheet(tester);
expect(find.text('Detach from "Outage umbrella"'), findsOneWidget);
});
testWidgets('a long parent name wraps in the sheet at large text', (
tester,
) async {
usePhone(tester, textScale: 1.3);
await tester.pumpWidget(
app(
child({
...readableJson,
'name': 'Every printer on the third floor stopped at once',
}),
),
);
await tester.pumpAndSettle();
await openParentSheet(tester);
expect(find.textContaining('Every printer'), findsWidgets);
expect(tester.takeException(), isNull);
});
});
}
class _FakeTicketsNotifier extends TicketsNotifier {
_FakeTicketsNotifier(this.ticket);
final Ticket ticket;
@override
Future<TicketsListData> build() async =>
TicketsListData(tickets: const [], totalCount: 0, hasMore: false);
@override
Future<TicketDetailResult?> getTicketDetail(String id) async =>
TicketDetailResult(
ticketObj: ticket,
activities: const [],
commentPermission: true,
internalCommentIds: const {},
);
@override
Future<TicketWatchers?> getWatchers(String id) async => const TicketWatchers(
watchers: [],
count: 0,
isCurrentUserWatching: false,
);
@override
Future<TicketTreeNode?> fetchTree(String id) async => null;
}

View file

@ -100,6 +100,31 @@ void main() {
expect(merged.openDescendantsOf('p').map((n) => n.id), ['real']);
});
test('lists a ticket the viewer cannot open, without a name', () {
// `/tree/` redacts it to an id, a status and `restricted`. It still
// counts, because the close refuses a cascade that would take it.
final redacted = TicketTreeNode.fromJson(
treeJson(
id: 'p',
children: [
{
'id': 'hidden',
'name': null,
'restricted': true,
'status': 'New',
'is_active': true,
'children': const [],
},
],
),
);
final open = redacted.openDescendantsOf('p');
expect(open.map((n) => n.id), ['hidden']);
expect(open.single.restricted, isTrue);
expect(open.single.name, TicketTreeNode.restrictedName);
expect(redacted.restricted, isFalse);
});
test('is empty for a leaf, and for a ticket not in the tree', () {
expect(tree.openDescendantsOf('kidOpen'), isEmpty);
expect(tree.openDescendantsOf('nope'), isEmpty);