# Phase 4: Error Handling Fix - Implementation Guide

**Created**: 2025-11-18
**Status**: Ready for Implementation
**Related ADR**: ADR-012 HTTP Error Handling Policy
**Related Review**: `/home/phil/Projects/starpunk/docs/reviews/error-handling-rest-vs-web-patterns.md`
**Test Failure**: `test_update_nonexistent_note_404`

## Problem Summary

The POST route for updating notes (`/admin/edit/<id>`) returns HTTP 302 (redirect) when the note doesn't exist, but the test expects HTTP 404. The GET route for the edit form already returns 404 correctly, so this is an inconsistency in the implementation.

## Solution

Add an existence check at the start of `update_note_submit()` in `/home/phil/Projects/starpunk/starpunk/routes/admin.py`, matching the pattern used in `edit_note_form()`.

## Implementation Steps

### Step 1: Modify `update_note_submit()` Function

**File**: `/home/phil/Projects/starpunk/starpunk/routes/admin.py`
**Lines**: 127-164
**Function**: `update_note_submit(note_id: int)`

**Add the following code after the function definition and decorator, before processing form data:**

```python
@bp.route("/edit/<int:note_id>", methods=["POST"])
@require_auth
def update_note_submit(note_id: int):
    """
    Handle note update submission

    Updates existing note with submitted form data.
    Requires authentication.

    Args:
        note_id: Database ID of note to update

    Form data:
        content: Updated markdown content (required)
        published: Checkbox for published status (optional)

    Returns:
        Redirect to dashboard on success, back to form on error

    Decorator: @require_auth
    """
    # CHECK IF NOTE EXISTS FIRST (ADDED)
    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

    # Rest of the function remains the same
    content = request.form.get("content", "").strip()
    published = "published" in request.form

    if not content:
        flash("Content cannot be empty", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))

    try:
        note = update_note(id=note_id, content=content, published=published)
        flash(f"Note updated: {note.slug}", "success")
        return redirect(url_for("admin.dashboard"))
    except ValueError as e:
        flash(f"Error updating note: {e}", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))
    except Exception as e:
        flash(f"Unexpected error updating note: {e}", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))
```

### Step 2: Verify Fix with Tests

Run the failing test to verify it now passes:

```bash
uv run pytest tests/test_routes_admin.py::TestEditNote::test_update_nonexistent_note_404 -v
```

Expected output:
```
tests/test_routes_admin.py::TestEditNote::test_update_nonexistent_note_404 PASSED
```

### Step 3: Run Full Admin Route Test Suite

Verify no regressions:

```bash
uv run pytest tests/test_routes_admin.py -v
```

All tests should pass.

### Step 4: Verify Existing GET Route Still Works

The GET route should still return 404:

```bash
uv run pytest tests/test_routes_admin.py::TestEditNote::test_edit_nonexistent_note_404 -v
```

Should still pass (no changes to this route).

## Code Changes Summary

### File: `/home/phil/Projects/starpunk/starpunk/routes/admin.py`

**Location**: After line 129 (after function docstring, before form processing)

**Add**:
```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
```

**No other changes needed** - the import for `get_note` already exists (line 15).

## Why This Fix Works

### Pattern Consistency

This matches the existing pattern in `edit_note_form()` (lines 118-122):

```python
note = get_note(id=note_id)

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

### Prevents Exception Handling

Without this check, the code would:
1. Try to call `update_note(id=note_id, ...)`
2. `update_note()` calls `get_note()` internally (line 603)
3. `get_note()` returns `None` for missing notes (line 368)
4. `update_note()` raises `NoteNotFoundError` (line 607)
5. Exception caught by `except Exception` (line 162)
6. Returns redirect with 302 status

With this check, the code:
1. Calls `get_note(id=note_id)` first
2. Returns 404 immediately if not found
3. Never calls `update_note()` for nonexistent notes

### HTTP Semantic Correctness

- **404 Not Found**: The correct HTTP status for "resource does not exist"
- **302 Found (Redirect)**: Used for successful operations that redirect elsewhere
- The test expects 404, which is semantically correct

### User Experience

While returning 404, we still:
1. Flash an error message ("Note not found")
2. Redirect to the dashboard (safe location)
3. User sees the error in context

Flask allows returning both: `return redirect(...), 404`

## Testing Strategy

### Unit Test Coverage

This test should now pass:

```python
def test_update_nonexistent_note_404(self, authenticated_client):
    """Test that updating a nonexistent note returns 404"""
    response = authenticated_client.post(
        "/admin/edit/99999",
        data={"content": "Updated content", "published": "on"},
        follow_redirects=False,
    )
    assert response.status_code == 404  # ✓ Should pass now
```

### Manual Testing (Optional)

1. Start the development server
2. Log in as admin
3. Try to access `/admin/edit/99999` (GET)
   - Should redirect to dashboard with "Note not found" message
   - Network tab shows 404 status
4. Try to POST to `/admin/edit/99999` with form data
   - Should redirect to dashboard with "Note not found" message
   - Network tab shows 404 status

## Additional Considerations

### Performance Impact

**Minimal**: The existence check adds one database query:
- Query: `SELECT * FROM notes WHERE id = ? AND deleted_at IS NULL`
- With `load_content=False`: No file I/O
- SQLite with index: ~0.1ms
- Acceptable for single-user system

### Alternative Approaches Rejected

1. **Catch `NoteNotFoundError` specifically**: Possible, but less explicit than checking first
2. **Let error handler deal with it**: Less flexible for per-route flash messages
3. **Change test to expect 302**: Wrong - test is correct, implementation is buggy

### Future Improvements

Consider adding a similar check to `delete_note_submit()` for consistency:

```python
@bp.route("/delete/<int:note_id>", methods=["POST"])
@require_auth
def delete_note_submit(note_id: int):
    if request.form.get("confirm") != "yes":
        flash("Deletion cancelled", "info")
        return redirect(url_for("admin.dashboard"))

    # ADD EXISTENCE CHECK
    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

    # Rest of delete logic...
```

However, this requires updating the test `test_delete_nonexistent_note_shows_error` to expect 404 instead of 200.

## Expected Outcome

After implementing this fix:

1. ✓ `test_update_nonexistent_note_404` passes
2. ✓ `test_edit_nonexistent_note_404` still passes
3. ✓ All other admin route tests pass
4. ✓ GET and POST routes have consistent behavior
5. ✓ HTTP semantics are correct (404 for missing resources)

## References

- Architectural review: `/home/phil/Projects/starpunk/docs/reviews/error-handling-rest-vs-web-patterns.md`
- ADR: `/home/phil/Projects/starpunk/docs/decisions/ADR-012-http-error-handling-policy.md`
- Current implementation: `/home/phil/Projects/starpunk/starpunk/routes/admin.py`
- Test file: `/home/phil/Projects/starpunk/tests/test_routes_admin.py`