feat(P3.6): milestone edit restrictions — block PATCH in terminal states, restrict scope fields in freeze/undergoing, protect delete

app/api/routers/milestones.py

@@ -82,7 +82,36 @@ def update_milestone(project_id: int, milestone_id: int, milestone: schemas.Mile
     if not db_milestone:
         raise HTTPException(status_code=404, detail="Milestone not found")
     ensure_can_edit_milestone(db, current_user.id, db_milestone)
+
+    # --- P3.6 Milestone edit restrictions based on status ---
+    ms_status = db_milestone.status.value if hasattr(db_milestone.status, 'value') else db_milestone.status
+
+    # Terminal states: no edits allowed
+    if ms_status in ("completed", "closed"):
+        raise HTTPException(
+            status_code=400,
+            detail=f"Cannot edit a milestone that is '{ms_status}'. No modifications are allowed in terminal state."
+        )
+
     data = milestone.model_dump(exclude_unset=True)
+
+    # Never allow status changes via PATCH — use action endpoints instead
+    if "status" in data:
+        raise HTTPException(
+            status_code=400,
+            detail="Milestone status cannot be changed via PATCH. Use the action endpoints (freeze/start/close) instead."
+        )
+
+    # Freeze / undergoing: restrict scope-changing fields
+    SCOPE_FIELDS = {"title", "description", "due_date", "planned_release_date", "depend_on_milestones", "depend_on_tasks"}
+    if ms_status in ("freeze", "undergoing"):
+        blocked = SCOPE_FIELDS & set(data.keys())
+        if blocked:
+            raise HTTPException(
+                status_code=400,
+                detail=f"Cannot modify scope fields {sorted(blocked)} when milestone is '{ms_status}'. Scope changes are only allowed in 'open' status."
+            )
+
     if "depend_on_milestones" in data:
         data["depend_on_milestones"] = json.dumps(data["depend_on_milestones"]) if data["depend_on_milestones"] else None
     if "depend_on_tasks" in data:
@@ -100,6 +129,9 @@ def delete_milestone(project_id: int, milestone_id: int, db: Session = Depends(g
     db_milestone = db.query(Milestone).filter(Milestone.id == milestone_id, Milestone.project_id == project_id).first()
     if not db_milestone:
         raise HTTPException(status_code=404, detail="Milestone not found")
+    ms_status = db_milestone.status.value if hasattr(db_milestone.status, 'value') else db_milestone.status
+    if ms_status in ("undergoing", "completed"):
+        raise HTTPException(status_code=400, detail=f"Cannot delete a milestone that is '{ms_status}'")
     db.delete(db_milestone)
     db.commit()
     return None

app/api/routers/tasks.py

@@ -19,6 +19,25 @@ from app.services.activity import log_activity
 
 router = APIRouter(tags=["Tasks"])
 
+# ---- State-machine: valid transitions (P5.1-P5.6) ----
+VALID_TRANSITIONS: dict[str, set[str]] = {
+    "pending":    {"open", "closed"},
+    "open":       {"undergoing", "closed"},
+    "undergoing": {"completed", "closed"},
+    "completed":  {"open"},       # reopen
+    "closed":     {"open"},       # reopen
+}
+
+def _check_transition(old_status: str, new_status: str) -> None:
+    """Raise 400 if the transition is not allowed by the state machine."""
+    allowed = VALID_TRANSITIONS.get(old_status, set())
+    if new_status not in allowed:
+        raise HTTPException(
+            status_code=400,
+            detail=f"Cannot transition from '{old_status}' to '{new_status}'. "
+                   f"Allowed targets from '{old_status}': {sorted(allowed) if allowed else 'none'}",
+        )
+
 # ---- Type / Subtype validation ----
 TASK_SUBTYPE_MAP = {
     'issue': {'infrastructure', 'performance', 'regression', 'security', 'user_experience', 'defect'},
@@ -167,6 +186,11 @@ def update_task(task_id: int, task_update: schemas.TaskUpdate, db: Session = Dep
     update_data = task_update.model_dump(exclude_unset=True)
     if "status" in update_data:
         new_status = update_data["status"]
+        old_status = task.status.value if hasattr(task.status, 'value') else task.status
+        # P5.1: enforce state-machine even through PATCH
+        _check_transition(old_status, new_status)
+        if new_status == "open" and old_status in ("completed", "closed"):
+            task.finished_on = None
         if new_status == "undergoing" and not task.started_on:
             task.started_on = datetime.utcnow()
         if new_status in ("closed", "completed") and not task.finished_on:
@@ -208,6 +232,31 @@ def transition_task(task_id: int, new_status: str, bg: BackgroundTasks, db: Sess
     if not task:
         raise HTTPException(status_code=404, detail="Task not found")
     old_status = task.status.value if hasattr(task.status, 'value') else task.status
+
+    # P5.1: enforce state-machine
+    _check_transition(old_status, new_status)
+
+    # P5.2: pending -> open requires milestone to be undergoing (dependencies checked later)
+    if old_status == "pending" and new_status == "open":
+        milestone = db.query(Milestone).filter(Milestone.id == task.milestone_id).first()
+        if milestone:
+            ms_status = milestone.status.value if hasattr(milestone.status, 'value') else milestone.status
+            if ms_status != "undergoing":
+                raise HTTPException(
+                    status_code=400,
+                    detail=f"Cannot open task: milestone is '{ms_status}', must be 'undergoing'",
+                )
+
+    # P5.3: open -> undergoing requires assignee
+    if old_status == "open" and new_status == "undergoing":
+        if not task.assignee_id:
+            raise HTTPException(status_code=400, detail="Cannot start task: assignee must be set first")
+
+    # P5.6: reopen from completed/closed -> open
+    if new_status == "open" and old_status in ("completed", "closed"):
+        # Clear finished_on on reopen so lifecycle timestamps are accurate
+        task.finished_on = None
+
     if new_status == "undergoing" and not task.started_on:
         task.started_on = datetime.utcnow()
     if new_status in ("closed", "completed") and not task.finished_on:
@@ -306,17 +355,32 @@ def batch_transition(data: BatchTransition, bg: BackgroundTasks, db: Session = D
     if data.new_status not in valid_statuses:
         raise HTTPException(status_code=400, detail="Invalid status")
     updated = []
+    skipped = []
     for task_id in data.task_ids:
         task = db.query(Task).filter(Task.id == task_id).first()
         if task:
             old_status = task.status.value if hasattr(task.status, 'value') else task.status
+            allowed = VALID_TRANSITIONS.get(old_status, set())
+            if data.new_status not in allowed:
+                skipped.append({"id": task.id, "title": task.title, "old": old_status,
+                                "reason": f"Cannot transition from '{old_status}' to '{data.new_status}'"})
+                continue
+            if data.new_status == "undergoing" and not task.started_on:
+                task.started_on = datetime.utcnow()
+            if data.new_status in ("closed", "completed") and not task.finished_on:
+                task.finished_on = datetime.utcnow()
+            if data.new_status == "open" and old_status in ("completed", "closed"):
+                task.finished_on = None
             task.status = data.new_status
             updated.append({"id": task.id, "title": task.title, "old": old_status, "new": data.new_status})
     db.commit()
     for u in updated:
         event = "task.closed" if data.new_status == "closed" else "task.updated"
         bg.add_task(fire_webhooks_sync, event, u, None, db)
-    return {"updated": len(updated), "tasks": updated}
+    result = {"updated": len(updated), "tasks": updated}
+    if skipped:
+        result["skipped"] = skipped
+    return result
 
 
 @router.post("/tasks/batch/assign")