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)

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

@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:

    # 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/ (line 118-122):

note = get_note(id=note_id)

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

POST /admin/edit/ (line 148-152):

# 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/ (new lines 193-197):

# 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:

@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:

# 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:

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:

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:

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:

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.