StarPunk/docs/reports/delete-route-implementation-spec.md

# Delete Route Implementation Specification

**Date**: 2025-11-18
**Component**: Admin Routes - Delete Note
**File**: `/home/phil/Projects/starpunk/starpunk/routes/admin.py`
**Function**: `delete_note_submit()` (lines 173-206)
**ADR**: ADR-012 (HTTP Error Handling Policy)

## Implementation Requirements

### Objective

Modify the delete route to check note existence before deletion and return HTTP 404 with an error message when attempting to delete a nonexistent note.

## Exact Code Change

### Current Implementation (Lines 173-206)

```python
@bp.route("/delete/<int:note_id>", methods=["POST"])
@require_auth
def delete_note_submit(note_id: int):
    """
    Handle note deletion

    Deletes a note after confirmation.
    Requires authentication.

    Args:
        note_id: Database ID of note to delete

    Form data:
        confirm: Must be 'yes' to proceed with deletion

    Returns:
        Redirect to dashboard with success/error message

    Decorator: @require_auth
    """
    # Check for confirmation
    if request.form.get("confirm") != "yes":
        flash("Deletion cancelled", "info")
        return redirect(url_for("admin.dashboard"))

    try:
        delete_note(id=note_id, soft=False)
        flash("Note deleted successfully", "success")
    except ValueError as e:
        flash(f"Error deleting note: {e}", "error")
    except Exception as e:
        flash(f"Unexpected error deleting note: {e}", "error")

    return redirect(url_for("admin.dashboard"))
```

### Required Implementation (Lines 173-206)

```python
@bp.route("/delete/<int:note_id>", methods=["POST"])
@require_auth
def delete_note_submit(note_id: int):
    """
    Handle note deletion

    Deletes a note after confirmation.
    Requires authentication.

    Args:
        note_id: Database ID of note to delete

    Form data:
        confirm: Must be 'yes' to proceed with deletion

    Returns:
        Redirect to dashboard with success/error message

    Decorator: @require_auth
    """
    # Check if note exists first (per ADR-012)
    existing_note = get_note(id=note_id, load_content=False)
    if not existing_note:
        flash("Note not found", "error")
        return redirect(url_for("admin.dashboard")), 404

    # Check for confirmation
    if request.form.get("confirm") != "yes":
        flash("Deletion cancelled", "info")
        return redirect(url_for("admin.dashboard"))

    try:
        delete_note(id=note_id, soft=False)
        flash("Note deleted successfully", "success")
    except ValueError as e:
        flash(f"Error deleting note: {e}", "error")
    except Exception as e:
        flash(f"Unexpected error deleting note: {e}", "error")

    return redirect(url_for("admin.dashboard"))
```

## Line-by-Line Changes

### Insert After Line 192 (after docstring, before confirmation check)

**Add these 4 lines**:

```python
    # Check if note exists first (per ADR-012)
    existing_note = get_note(id=note_id, load_content=False)
    if not existing_note:
        flash("Note not found", "error")
        return redirect(url_for("admin.dashboard")), 404

```

**Result**: Lines shift down by 5 (including blank line)

### No Other Changes Required

- Docstring: No changes
- Confirmation check: No changes (shifts to line 199)
- Deletion logic: No changes (shifts to line 203)
- Return statement: No changes (shifts to line 211)

## Implementation Details

### Function Call: `get_note(id=note_id, load_content=False)`

**Purpose**: Check if note exists in database

**Parameters**:
- `id=note_id`: Look up by database ID (primary key)
- `load_content=False`: Metadata only (no file I/O)

**Returns**:
- `Note` object if found
- `None` if not found or soft-deleted

**Performance**: ~0.1ms (single SELECT query)

### Flash Message: `"Note not found"`

**Purpose**: User-facing error message

**Category**: `"error"` (red alert in UI)

**Why this wording**:
- Consistent with edit route (line 151)
- Simple and clear
- Test checks for "not found" substring
- ADR-012 example uses this exact message

### Return Statement: `return redirect(url_for("admin.dashboard")), 404`

**Purpose**: Return HTTP 404 with redirect

**Flask Pattern**: Tuple `(response, status_code)`
- First element: Response object (redirect)
- Second element: HTTP status code (404)

**Result**:
- HTTP 404 status sent to client
- Location header: `/admin/`
- Flash message stored in session
- Client can follow redirect to see error

**Why not just return 404 error page**:
- Better UX (user sees dashboard with error, not blank 404 page)
- Consistent with update route pattern
- Per ADR-012: "404 responses MAY redirect to a safe location"

## Validation Checklist

### Before Implementing

- [ ] Read ADR-012 (HTTP Error Handling Policy)
- [ ] Review similar implementation in `update_note_submit()` (line 148-152)
- [ ] Verify `get_note` is imported (line 15 - already imported ✅)
- [ ] Verify test expectations in `test_delete_nonexistent_note_shows_error`

### After Implementing

- [ ] Code follows exact pattern from update route
- [ ] Existence check happens BEFORE confirmation check
- [ ] Flash message is "Note not found" with category "error"
- [ ] Return statement includes 404 status code
- [ ] No other logic changed
- [ ] Imports unchanged (get_note already imported)

### Testing

- [ ] Run failing test: `uv run pytest tests/test_routes_admin.py::TestAdminDeleteRoutes::test_delete_nonexistent_note_shows_error -v`
- [ ] Verify test now passes
- [ ] Run all delete route tests: `uv run pytest tests/test_routes_admin.py::TestAdminDeleteRoutes -v`
- [ ] Verify all tests still pass (no regressions)
- [ ] Run full admin route tests: `uv run pytest tests/test_routes_admin.py -v`
- [ ] Verify 406/406 tests pass

## Expected Test Results

### Before Fix

```
FAILED tests/test_routes_admin.py::TestAdminDeleteRoutes::test_delete_nonexistent_note_shows_error
AssertionError: assert False
 +  where False = (b'error' in b'...deleted successfully...' or b'not found' in b'...')
```

### After Fix

```
PASSED tests/test_routes_admin.py::TestAdminDeleteRoutes::test_delete_nonexistent_note_shows_error
```

## Edge Cases Handled

### Case 1: Note Exists

**Scenario**: User deletes existing note
**Behavior**:
1. Existence check passes (note found)
2. Confirmation check (if confirmed, proceed)
3. Deletion succeeds
4. Flash: "Note deleted successfully"
5. Return: 302 redirect

**Test Coverage**: `test_delete_redirects_to_dashboard`

### Case 2: Note Doesn't Exist

**Scenario**: User deletes nonexistent note (ID 99999)
**Behavior**:
1. Existence check fails (note not found)
2. Flash: "Note not found"
3. Return: 404 with redirect (no deletion attempted)

**Test Coverage**: `test_delete_nonexistent_note_shows_error` ← This fix

### Case 3: Note Soft-Deleted

**Scenario**: User deletes note that was already soft-deleted
**Behavior**:
1. `get_note()` excludes soft-deleted notes (WHERE deleted_at IS NULL)
2. Existence check fails (note not found from user perspective)
3. Flash: "Note not found"
4. Return: 404 with redirect

**Test Coverage**: Covered by `get_note()` behavior (implicit)

### Case 4: Deletion Not Confirmed

**Scenario**: User submits form without `confirm=yes`
**Behavior**:
1. Existence check passes (note found)
2. Confirmation check fails
3. Flash: "Deletion cancelled"
4. Return: 302 redirect (no deletion, no 404)

**Test Coverage**: Existing tests (no change)

## Performance Impact

### Database Queries

**Before**:
1. DELETE query (inside delete_note)

**After**:
1. SELECT query (get_note - existence check)
2. DELETE query (inside delete_note)

**Overhead**: ~0.1ms per deletion request

### Why This is Acceptable

1. Single-user system (not high traffic)
2. Deletions are rare operations
3. Correctness > performance for edge cases
4. Consistent with update route (already accepts this overhead)
5. `load_content=False` avoids file I/O (only metadata query)

## Consistency with Other Routes

### Edit Routes (Already Implemented)

**GET /admin/edit/<id>** (line 118-122):
```python
note = get_note(id=note_id)

if not note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404
```

**POST /admin/edit/<id>** (line 148-152):
```python
# Check if note exists first
existing_note = get_note(id=note_id, load_content=False)
if not existing_note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404
```

### Delete Route (This Implementation)

**POST /admin/delete/<id>** (new lines 193-197):
```python
# Check if note exists first (per ADR-012)
existing_note = get_note(id=note_id, load_content=False)
if not existing_note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404
```

**Pattern Consistency**: ✅ 100% identical to update route

## ADR-012 Compliance

### Required Elements

| Requirement | Status |
|-------------|--------|
| Return 404 for nonexistent resource | ✅ Yes (`return ..., 404`) |
| Check existence before operation | ✅ Yes (`get_note()` before `delete_note()`) |
| Include user-friendly flash message | ✅ Yes (`flash("Note not found", "error")`) |
| Redirect to safe location | ✅ Yes (`redirect(url_for("admin.dashboard"))`) |

### Implementation Pattern (ADR-012, lines 56-74)

**Spec Pattern**:
```python
@bp.route("/operation/<int:resource_id>", methods=["GET", "POST"])
@require_auth
def operation(resource_id: int):
    # 1. CHECK EXISTENCE FIRST
    resource = get_resource(id=resource_id)
    if not resource:
        flash("Resource not found", "error")
        return redirect(url_for("admin.dashboard")), 404  # ← MUST return 404
    # ...
```

**Our Implementation**: ✅ Follows pattern exactly

## Common Implementation Mistakes to Avoid

### Mistake 1: Check Existence After Confirmation

**Wrong**:
```python
# Check for confirmation
if request.form.get("confirm") != "yes":
    # ...

# Check if note exists  ← TOO LATE
existing_note = get_note(id=note_id, load_content=False)
```

**Why Wrong**: User confirms deletion of nonexistent note, then gets 404

**Correct**: Check existence FIRST (before any user interaction)

### Mistake 2: Forget load_content=False

**Wrong**:
```python
existing_note = get_note(id=note_id)  # Loads file content
```

**Why Wrong**: Unnecessary file I/O (we only need to check existence)

**Correct**: `get_note(id=note_id, load_content=False)` (metadata only)

### Mistake 3: Return 302 Instead of 404

**Wrong**:
```python
if not existing_note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard"))  # ← Missing 404
```

**Why Wrong**: Returns HTTP 302 (redirect), not 404 (not found)

**Correct**: `return redirect(...), 404` (tuple with status code)

### Mistake 4: Wrong Flash Message Category

**Wrong**:
```python
flash("Note not found", "info")  # ← Should be "error"
```

**Why Wrong**: Not an error in UI (blue alert, not red)

**Correct**: `flash("Note not found", "error")` (red error alert)

### Mistake 5: Catching NoteNotFoundError from delete_note()

**Wrong**:
```python
try:
    delete_note(id=note_id, soft=False)
except NoteNotFoundError:  # ← delete_note doesn't raise this
    flash("Note not found", "error")
    return redirect(...), 404
```

**Why Wrong**:
- `delete_note()` is idempotent (doesn't raise on missing note)
- Existence check should happen BEFORE calling delete_note
- Violates separation of concerns (route layer vs data layer)

**Correct**: Explicit existence check before deletion (as specified)

## Final Verification

### Code Review Checklist

- [ ] Existence check is first operation (after docstring)
- [ ] Uses `get_note(id=note_id, load_content=False)` exactly
- [ ] Flash message is `"Note not found"` with category `"error"`
- [ ] Return statement is `return redirect(url_for("admin.dashboard")), 404`
- [ ] No changes to confirmation logic
- [ ] No changes to deletion logic
- [ ] No changes to exception handling
- [ ] No changes to imports
- [ ] Code matches update route pattern exactly

### Test Validation

1. Run specific test: Should PASS
2. Run delete route tests: All should PASS
3. Run admin route tests: All should PASS (406/406)
4. Run full test suite: All should PASS

### Documentation

- [ ] This implementation spec reviewed
- [ ] Root cause analysis document reviewed
- [ ] ADR-012 referenced and understood
- [ ] Changes documented in changelog
- [ ] Version incremented per versioning strategy

## Summary

**Change**: Add 4 lines (existence check + error handling)
**Location**: After line 192, before confirmation check
**Impact**: 1 test changes from FAIL to PASS
**Result**: 406/406 tests passing ✅

This is the minimal, correct implementation that complies with ADR-012 and maintains consistency with existing routes.