no-mistakes(review): Guard photo cleanup; add transitions helper and assign auto-advance

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) ───────────────────────

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,

app/templates/tickets/detail.html

@@ -204,21 +204,9 @@
                     <label class="block text-sm font-medium text-gray-700 mb-1">Current: <span class="font-bold" x-text="ticket.status"></span></label>
                     <select x-model="statusForm.newStatus" class="w-full px-4 py-2.5 rounded-lg border border-gray-300 focus:ring-2 focus:ring-denya-500 outline-none">
                         <option value="">Select new status</option>
-                        <option value="Logged">Logged</option>
+                        <template x-for="target in availableTransitions" :key="target">
-                        <option value="Triage">Triage</option>
+                            <option :value="target" x-text="target"></option>
-                        <option value="Assigned">Assigned</option>
+                        </template>
-                        <option value="Accepted">Accepted</option>
-                        <option value="Travelling">Travelling</option>
-                        <option value="On Site">On Site</option>
-                        <option value="In Progress">In Progress</option>
-                        <option value="Waiting Parts">Waiting Parts</option>
-                        <option value="Escalated">Escalated</option>
-                        <option value="Completed">Completed</option>
-                        <option value="On-Field Verification">On-Field Verification</option>
-                        <option value="Wahab Review">Wahab Review</option>
-                        <option value="Closed">Closed</option>
-                        <option value="Reopened">Reopened</option>
-                        <option value="Cancelled">Cancelled</option>
                     </select>
                 </div>
                 <div>
@@ -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) {

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."""