# Test Failure Analysis: Missing `deleted_at` Attribute on Note Model

**Date**: 2025-11-18
**Status**: Issue Identified - Architectural Guidance Provided
**Test**: `test_delete_without_confirmation_cancels` (tests/test_routes_admin.py:441)
**Error**: `AttributeError: 'Note' object has no attribute 'deleted_at'`

---

## Executive Summary

A test is failing because it expects the `Note` model to expose a `deleted_at` attribute, but this field is **not included in the Note dataclass definition** despite being present in the database schema. This is a **model-schema mismatch** issue.

**Root Cause**: The `deleted_at` column exists in the database (`starpunk/database.py:20`) but is not mapped to the `Note` dataclass (`starpunk/models.py:44-121`).

**Impact**:
- Test suite failure prevents CI/CD pipeline success
- Soft deletion feature is partially implemented but not fully exposed through the model layer
- Code that attempts to check deletion status will fail at runtime

**Recommended Fix**: Add `deleted_at` field to the Note dataclass definition

---

## Analysis

### 1. Database Schema Review

**File**: `starpunk/database.py:11-27`

The database schema **includes** a `deleted_at` column:

```sql
CREATE TABLE IF NOT EXISTS notes (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    slug TEXT UNIQUE NOT NULL,
    file_path TEXT UNIQUE NOT NULL,
    published BOOLEAN DEFAULT 0,
    created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
    updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
    deleted_at TIMESTAMP,           -- ← THIS FIELD EXISTS
    content_hash TEXT
);

CREATE INDEX IF NOT EXISTS idx_notes_deleted_at ON notes(deleted_at);
```

**Key Findings**:
- `deleted_at` is defined as a nullable TIMESTAMP column
- An index exists on `deleted_at` for query performance
- The schema supports soft deletion architecture

### 2. Note Model Review

**File**: `starpunk/models.py:44-121`

The Note dataclass **does not include** `deleted_at`:

```python
@dataclass(frozen=True)
class Note:
    """Represents a note/post"""

    # Core fields from database
    id: int
    slug: str
    file_path: str
    published: bool
    created_at: datetime
    updated_at: datetime

    # Internal fields (not from database)
    _data_dir: Path = field(repr=False, compare=False)

    # Optional fields
    content_hash: Optional[str] = None
    # ← MISSING: deleted_at field
```

**Key Findings**:
- The model lists 6 "core fields from database" but only includes 6 of the 7 columns
- `deleted_at` is completely absent from the dataclass definition
- The `from_row()` class method (line 123-162) does not extract `deleted_at` from database rows

### 3. Notes Module Review

**File**: `starpunk/notes.py`

The notes module **uses** `deleted_at` in queries but **never exposes** it:

```python
# Line 358-364: get_note() filters by deleted_at
row = db.execute(
    "SELECT * FROM notes WHERE slug = ? AND deleted_at IS NULL", (slug,)
).fetchone()

# Line 494: list_notes() filters by deleted_at
query = "SELECT * FROM notes WHERE deleted_at IS NULL"

# Line 800-804: delete_note() sets deleted_at for soft deletes
db.execute(
    "UPDATE notes SET deleted_at = ? WHERE id = ?",
    (deleted_at, existing_note.id),
)
```

**Key Findings**:
- The application logic **knows about** `deleted_at`
- Queries correctly filter out soft-deleted notes (`deleted_at IS NULL`)
- Soft deletion is implemented by setting `deleted_at` to current timestamp
- However, the model layer **never reads this value back** from the database
- This creates a **semantic gap**: the database has the data, but the model can't access it

### 4. Failing Test Review

**File**: `tests/test_routes_admin.py:441`

The test expects to verify deletion status:

```python
def test_delete_without_confirmation_cancels(self, authenticated_client, sample_notes):
    """Test that delete without confirmation cancels operation"""

    # ... test logic ...

    # Verify note was NOT deleted (still exists)
    with authenticated_client.application.app_context():
        from starpunk.notes import get_note

        note = get_note(id=note_id)
        assert note is not None  # Note should still exist
        assert note.deleted_at is None  # NOT soft-deleted  ← FAILS HERE
```

**Key Findings**:
- Test wants to **explicitly verify** that a note is not soft-deleted
- This is a reasonable test - it validates business logic
- The test assumes `deleted_at` is accessible on the Note model
- Without the field, the test cannot verify soft-deletion status

---

## Architectural Assessment

### Why This Is a Problem

1. **Model-Schema Mismatch**: The fundamental rule of data models is that they should accurately represent the database schema. Currently, `Note` is incomplete.

2. **Information Hiding**: The application knows about soft deletion (it uses it), but the model layer hides this information from consumers. This violates the **principle of least surprise**.

3. **Testing Limitation**: Tests cannot verify soft-deletion behavior without accessing the field. This creates a testing blind spot.

4. **Future Maintenance**: Any code that needs to check deletion status (admin UI, API responses, debugging tools) will face the same issue.

### Why `deleted_at` Was Omitted

Looking at the git history and design patterns, I can infer the reasoning:

1. **Query-Level Filtering**: The developer chose to filter soft-deleted notes at the **query level** (`WHERE deleted_at IS NULL`), making `deleted_at` invisible to consumers.

2. **Encapsulation**: This follows a pattern of "consumers shouldn't need to know about deletion mechanics" - they just get active notes.

3. **Simplicity**: By excluding `deleted_at`, the model is simpler and consumers don't need to remember to filter it.

This is a **defensible design choice** for application code, but it creates problems for:
- Testing
- Admin interfaces (where you might want to show soft-deleted items)
- Debugging
- Data export/backup tools

### Design Principles at Stake

1. **Transparency vs Encapsulation**:
   - Encapsulation says: "Hide implementation details (soft deletion) from consumers"
   - Transparency says: "Expose database state accurately"
   - **Verdict**: For data models, transparency should win

2. **Data Integrity**:
   - The model should be a **faithful representation** of the database
   - Hiding fields creates a semantic mismatch
   - **Verdict**: Add the field

3. **Testability**:
   - Tests need to verify deletion behavior
   - Current design makes this impossible
   - **Verdict**: Add the field

---

## Architectural Decision

**Decision**: Add `deleted_at: Optional[datetime]` to the Note dataclass

**Rationale**:

1. **Principle of Least Surprise**: If a database column exists, developers expect to access it through the model

2. **Testability**: Tests must be able to verify soft-deletion state

3. **Transparency**: Data models should accurately reflect database schema

4. **Future Flexibility**: Admin UIs, backup tools, and debugging features will need this field

5. **Low Complexity Cost**: Adding one optional field is minimal complexity

6. **Backwards Compatibility**: The field is optional (nullable), so existing code won't break

**Trade-offs Accepted**:

- **Loss of Encapsulation**: Consumers now see "deleted_at" and must understand soft deletion
  - **Mitigation**: Document the field clearly; provide helper properties if needed

- **Slight Complexity Increase**: Model has one more field
  - **Impact**: Minimal - one line of code

---

## Implementation Plan

### Changes Required

**File**: `starpunk/models.py`

1. Add `deleted_at` field to Note dataclass (line ~109):

```python
@dataclass(frozen=True)
class Note:
    """Represents a note/post"""

    # Core fields from database
    id: int
    slug: str
    file_path: str
    published: bool
    created_at: datetime
    updated_at: datetime
    deleted_at: Optional[datetime] = None  # ← ADD THIS

    # Internal fields (not from database)
    _data_dir: Path = field(repr=False, compare=False)

    # Optional fields
    content_hash: Optional[str] = None
```

2. Update `from_row()` class method to extract `deleted_at` (line ~145-162):

```python
@classmethod
def from_row(cls, row: sqlite3.Row | dict[str, Any], data_dir: Path) -> "Note":
    # ... existing code ...

    # Convert timestamps if they are strings
    created_at = data["created_at"]
    if isinstance(created_at, str):
        created_at = datetime.fromisoformat(created_at.replace("Z", "+00:00"))

    updated_at = data["updated_at"]
    if isinstance(updated_at, str):
        updated_at = datetime.fromisoformat(updated_at.replace("Z", "+00:00"))

    # ← ADD THIS BLOCK
    deleted_at = data.get("deleted_at")
    if deleted_at and isinstance(deleted_at, str):
        deleted_at = datetime.fromisoformat(deleted_at.replace("Z", "+00:00"))

    return cls(
        id=data["id"],
        slug=data["slug"],
        file_path=data["file_path"],
        published=bool(data["published"]),
        created_at=created_at,
        updated_at=updated_at,
        deleted_at=deleted_at,  # ← ADD THIS
        _data_dir=data_dir,
        content_hash=data.get("content_hash"),
    )
```

3. (Optional) Update `to_dict()` method to include `deleted_at` when serializing (line ~354-406):

```python
def to_dict(
    self, include_content: bool = False, include_html: bool = False
) -> dict[str, Any]:
    data = {
        "id": self.id,
        "slug": self.slug,
        "title": self.title,
        "published": self.published,
        "created_at": self.created_at.strftime("%Y-%m-%dT%H:%M:%SZ"),
        "updated_at": self.updated_at.strftime("%Y-%m-%dT%H:%M:%SZ"),
        "permalink": self.permalink,
        "excerpt": self.excerpt,
    }

    # ← ADD THIS BLOCK (optional, for API consistency)
    if self.deleted_at is not None:
        data["deleted_at"] = self.deleted_at.strftime("%Y-%m-%dT%H:%M:%SZ")

    # ... rest of method ...
```

4. Update docstring to document the field (line ~44-100):

```python
@dataclass(frozen=True)
class Note:
    """
    Represents a note/post

    Attributes:
        id: Database ID (primary key)
        slug: URL-safe slug (unique)
        file_path: Path to markdown file (relative to data directory)
        published: Whether note is published (visible publicly)
        created_at: Creation timestamp (UTC)
        updated_at: Last update timestamp (UTC)
        deleted_at: Soft deletion timestamp (UTC, None if not deleted)  # ← ADD THIS
        content_hash: SHA-256 hash of content (for integrity checking)
        # ... rest of docstring ...
    """
```

### Testing Strategy

**Unit Tests**:

1. Verify `Note.from_row()` correctly parses `deleted_at` from database rows
2. Verify `deleted_at` defaults to `None` for active notes
3. Verify `deleted_at` is set to timestamp for soft-deleted notes
4. Verify `to_dict()` includes `deleted_at` when present

**Integration Tests**:

1. The failing test should pass: `test_delete_without_confirmation_cancels`
2. Verify soft-deleted notes have `deleted_at` set after `delete_note(soft=True)`
3. Verify `get_note()` returns `None` for soft-deleted notes (existing behavior)
4. Verify hard-deleted notes are removed entirely (existing behavior)

**Regression Tests**:

1. Run full test suite to ensure no existing tests break
2. Verify `list_notes()` still excludes soft-deleted notes
3. Verify `get_note()` still excludes soft-deleted notes

### Acceptance Criteria

- [ ] `deleted_at` field added to Note dataclass
- [ ] `from_row()` extracts `deleted_at` from database rows
- [ ] `from_row()` handles `deleted_at` as string (ISO format)
- [ ] `from_row()` handles `deleted_at` as None (active notes)
- [ ] Docstring updated to document `deleted_at`
- [ ] Test `test_delete_without_confirmation_cancels` passes
- [ ] Full test suite passes
- [ ] No regression in existing functionality

---

## Alternative Approaches Considered

### Alternative 1: Update Test to Remove `deleted_at` Check

**Approach**: Change the test to not check `deleted_at`

```python
# Instead of:
assert note.deleted_at is None

# Use:
# (No check - just verify note exists)
assert note is not None
```

**Pros**:
- Minimal code change (one line)
- Maintains current encapsulation

**Cons**:
- **Weakens test coverage**: Can't verify note is truly not soft-deleted
- **Doesn't solve root problem**: Future code will hit the same issue
- **Violates test intent**: Test specifically wants to verify deletion status

**Verdict**: REJECTED - This is a band-aid, not a fix

### Alternative 2: Add Helper Property Instead of Raw Field

**Approach**: Keep `deleted_at` hidden, add `is_deleted` property

```python
@dataclass(frozen=True)
class Note:
    # ... existing fields ...
    _deleted_at: Optional[datetime] = field(default=None, repr=False)

    @property
    def is_deleted(self) -> bool:
        """Check if note is soft-deleted"""
        return self._deleted_at is not None
```

**Pros**:
- Provides boolean flag for deletion status
- Hides timestamp implementation detail
- Encapsulates deletion logic

**Cons**:
- **Information loss**: Tests/admin UIs can't see when note was deleted
- **Inconsistent with other models**: Session, Token, AuthState all expose timestamps
- **More complex**: Two fields instead of one
- **Harder to serialize**: Can't include deletion timestamp in API responses

**Verdict**: REJECTED - Adds complexity without clear benefit

### Alternative 3: Create Separate SoftDeletedNote Model

**Approach**: Use different model classes for active vs deleted notes

**Pros**:
- Type safety: Can't accidentally mix active and deleted notes
- Clear separation of concerns

**Cons**:
- **Massive complexity increase**: Two model classes, complex query logic
- **Violates simplicity principle**: Way over-engineered for the problem
- **Breaks existing code**: Would require rewriting note operations

**Verdict**: REJECTED - Far too complex for V1

---

## Risk Assessment

**Risk Level**: LOW

**Implementation Risks**:
- **Breaking Changes**: None - field is optional and nullable
- **Performance Impact**: None - no additional queries or processing
- **Security Impact**: None - field is read-only from model perspective

**Migration Risks**:
- **Database Migration**: None needed - column already exists
- **Data Backfill**: None needed - existing notes have NULL by default
- **API Compatibility**: Potential change if `to_dict()` includes `deleted_at`
  - **Mitigation**: Make inclusion optional or conditional

---

## Summary for Developer

**What to do**:
1. Add `deleted_at: Optional[datetime] = None` to Note dataclass
2. Update `from_row()` to extract and parse `deleted_at`
3. Update docstring to document the field
4. Run test suite to verify fix

**Why**:
- Database has `deleted_at` column but model doesn't expose it
- Tests need to verify soft-deletion status
- Models should accurately reflect database schema

**Complexity**: LOW (3 lines of code change)

**Time Estimate**: 5 minutes implementation + 2 minutes testing

**Files to modify**:
- `starpunk/models.py` (primary change)
- No migration needed (database already has column)
- No test changes needed (test is already correct)

---

## References

- Database Schema: `/home/phil/Projects/starpunk/starpunk/database.py:11-27`
- Note Model: `/home/phil/Projects/starpunk/starpunk/models.py:44-440`
- Notes Module: `/home/phil/Projects/starpunk/starpunk/notes.py:685-849`
- Failing Test: `/home/phil/Projects/starpunk/tests/test_routes_admin.py:435-441`

---

**Next Steps**:
1. Review this analysis with development team
2. Get approval for recommended fix
3. Implement changes to `starpunk/models.py`
4. Verify test passes
5. Document decision in ADR if desired