From 6b92c9098c366c1b38d329cced020ba3e4c8a656 Mon Sep 17 00:00:00 2001 From: root Date: Sun, 2 Aug 2026 14:26:48 +0000 Subject: [PATCH] no-mistakes(review): Guard photo cleanup; add transitions helper and assign auto-advance --- app/routers/tickets.py | 25 +++++++++- app/services/ticket.py | 19 +++++++- app/templates/tickets/detail.html | 32 ++++++------ tests/test_workflow_hardening.py | 81 +++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 17 deletions(-) diff --git a/app/routers/tickets.py b/app/routers/tickets.py index 4bdfaa8..c6405aa 100644 --- a/app/routers/tickets.py +++ b/app/routers/tickets.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging import uuid from datetime import datetime from pathlib import Path @@ -32,6 +33,8 @@ from app.schemas.ticket import ( from app.services import ticket as ticket_service from app.services.sla import get_sla_status +logger = logging.getLogger(__name__) + router = APIRouter(prefix="/api/tickets", tags=["tickets"]) # Ensure uploads directory exists @@ -224,6 +227,19 @@ async def list_tickets( return TicketListResponse(items=items, total=total, page=page, page_size=page_size) +@router.get("/{ticket_id}/transitions") +async def get_ticket_transitions( + ticket_id: int, + db: Annotated[AsyncSession, Depends(get_db)], +) -> dict: + """Return the valid next statuses for a ticket's current status.""" + ticket = await ticket_service.get_ticket(db, ticket_id) + return { + "current_status": ticket.status, + "transitions": ticket_service.VALID_TRANSITIONS.get(ticket.status, []), + } + + @router.get("/{ticket_id}", response_model=TicketOut) async def get_ticket( ticket_id: int, @@ -266,7 +282,14 @@ async def delete_ticket( for photo in ticket.photos: if photo.photo_url: name = photo.photo_url.rsplit("/", 1)[-1] - (UPLOADS_DIR / name).unlink(missing_ok=True) + try: + (UPLOADS_DIR / name).unlink(missing_ok=True) + except OSError: + logger.warning( + "Could not remove orphaned photo file %s for ticket %s", + name, + ticket_id, + ) # ── Status Transitions (convenience endpoints) ─────────────────────── diff --git a/app/services/ticket.py b/app/services/ticket.py index 0c642f1..a8cf8da 100644 --- a/app/services/ticket.py +++ b/app/services/ticket.py @@ -236,6 +236,7 @@ async def update_ticket( # Handle status transitions separately new_status = data.get("status") old_status = ticket.status + status_changed = new_status is not None and old_status != new_status if new_status is not None: if old_status != new_status: valid_targets = VALID_TRANSITIONS.get(old_status, []) @@ -296,9 +297,25 @@ async def update_ticket( ticket.status = new_status + # Assigning a technician advances pre-Assigned tickets to Assigned with a + # timeline entry; tickets already past Assigned keep their current status. + advanced_to_assigned = False + if "assigned_to" in data and data["assigned_to"] != ticket.assigned_to: + if ticket.status in {"New", "Logged", "Triage"}: + await _log_status_change( + db, + ticket.id, + from_status=ticket.status, + to_status="Assigned", + note=data.get("note") if not status_changed else None, + user_id=user.id if user else None, + ) + ticket.status = "Assigned" + advanced_to_assigned = True + # Handle standalone note (no status change) note_only = data.get("note") - if note_only and not (new_status is not None and old_status != new_status): + if note_only and not status_changed and not advanced_to_assigned: await _log_status_change( db, ticket.id, diff --git a/app/templates/tickets/detail.html b/app/templates/tickets/detail.html index 9f51cd8..ffdce9d 100644 --- a/app/templates/tickets/detail.html +++ b/app/templates/tickets/detail.html @@ -204,21 +204,9 @@
@@ -303,6 +291,7 @@ statusForm: { newStatus: '', note: '' }, statusSubmitting: false, statusError: '', + availableTransitions: [], // Assign form assignForm: { technicianId: '' }, @@ -316,6 +305,7 @@ async init() { await this.loadTicket(); + await this.loadTransitions(); if (app().isFM || app().isAdmin) { await this.loadTechnicians(); } @@ -332,6 +322,16 @@ } }, + async loadTransitions() { + try { + const data = await app().apiGet(`/api/tickets/${this.ticketId}/transitions`); + this.availableTransitions = (data && data.transitions) || []; + } catch (e) { + console.error('Transitions load error', e); + this.availableTransitions = []; + } + }, + async loadTechnicians() { try { const users = await app().apiGet('/api/auth/users'); @@ -356,6 +356,7 @@ const updated = await app().apiPatch(`/api/tickets/${this.ticketId}`, payload); this.ticket = updated; + await this.loadTransitions(); this.showStatusModal = false; this.statusForm = { newStatus: '', note: '' }; app().showToast('Status updated', 'success'); @@ -378,6 +379,7 @@ assigned_to: parseInt(this.assignForm.technicianId) }); this.ticket = updated; + await this.loadTransitions(); this.showAssignModal = false; app().showToast('Technician assigned', 'success'); } catch (e) { diff --git a/tests/test_workflow_hardening.py b/tests/test_workflow_hardening.py index f879cbb..fad9a85 100644 --- a/tests/test_workflow_hardening.py +++ b/tests/test_workflow_hardening.py @@ -181,6 +181,87 @@ async def test_users_endpoint_lists_technicians(client): assert {"id", "full_name", "role"} <= set(users[0].keys()) +# ── Transitions helper (status dropdown) ───────────────────────────── +async def test_transitions_helper_returns_valid_targets(client): + """GET /api/tickets/{id}/transitions returns the valid next statuses.""" + from app.services.ticket import VALID_TRANSITIONS + + token = await _login(client) + ticket = await _create_ticket(client, token, description="transitions helper") # status: Logged + + resp = await client.get(f"/api/tickets/{ticket['id']}/transitions") + assert resp.status_code == 200, resp.text + data = resp.json() + assert data["current_status"] == "Logged" + assert set(data["transitions"]) == set(VALID_TRANSITIONS["Logged"]) + + +async def test_transitions_helper_terminal_is_empty(client): + """A cancelled ticket exposes no selectable next statuses.""" + token = await _login(client) + ticket = await _create_ticket(client, token, description="terminal transitions") + resp = await client.patch( + f"/api/tickets/{ticket['id']}", + json={"status": "Cancelled"}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert resp.status_code == 200 + + data = (await client.get(f"/api/tickets/{ticket['id']}/transitions")).json() + assert data["current_status"] == "Cancelled" + assert data["transitions"] == [] + + +# ── Assign auto-advance ─────────────────────────────────────────────── +async def _first_tech_id(client, token: str) -> int: + users = (await client.get("/api/auth/users", headers={"Authorization": f"Bearer {token}"})).json() + return next(u["id"] for u in users if u["role"] == "Tech") + + +async def test_assign_auto_advances_to_assigned(client): + """Assigning a pre-Assigned ticket advances it to Assigned with a timeline entry.""" + token = await _login(client) + ticket = await _create_ticket(client, token, description="assign advance") # status: Logged + tech_id = await _first_tech_id(client, token) + + resp = await client.patch( + f"/api/tickets/{ticket['id']}", + json={"assigned_to": tech_id}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert resp.status_code == 200, resp.text + data = resp.json() + assert data["status"] == "Assigned" + assert data["assigned_to"] == tech_id + assert data["timeline"][-1]["to_status"] == "Assigned" + + +async def test_assign_does_not_regress_past_assigned(client): + """Re-assigning a ticket already past Assigned leaves its status untouched.""" + token = await _login(client) + ticket = await _create_ticket(client, token, description="no regression") + + for step in ("Triage", "Assigned", "Accepted"): + resp = await client.patch( + f"/api/tickets/{ticket['id']}", + json={"status": step}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert resp.status_code == 200, resp.text + + users = (await client.get("/api/auth/users", headers={"Authorization": f"Bearer {token}"})).json() + techs = [u["id"] for u in users if u["role"] == "Tech"] + other_tech = techs[1] if len(techs) > 1 else techs[0] + + resp = await client.patch( + f"/api/tickets/{ticket['id']}", + json={"assigned_to": other_tech}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["status"] == "Accepted" + + # ── Ticket numbering survives deletions ─────────────────────────────── async def test_ticket_number_uses_max_plus_one(client, seed_tickets): """After deleting a middle ticket, numbering must skip over surviving numbers."""