Fixes #69: Add append-only notes on equipment records

[jantman]

Summary

Adds free-form, append-only notes on equipment records (closes #69). Each note is attributed to an author with a UTC timestamp and shown newest-first in a new "Notes" section at the bottom of the equipment detail page.

Implemented from the tech-spec at _bmad-output/implementation-artifacts/tech-spec-equipment-notes.md.

Changes

• Model — EquipmentNote (equipment_notes table): FK to equipment, nullable author FK + cached author_name, content, created_at; deterministic created_at DESC, id DESC ordering and an equipment_id index. Registered in models/__init__.py.
• Service — add_equipment_note() / get_equipment_notes() with log_mutation auditing and a shared NOTE_MAX_LENGTH (5000) limit enforced authoritatively on stripped content.
• Form — EquipmentNoteForm validates stripped length, so it never rejects a note the service would store.
• Route + view — permission-gated (_require_doc_edit), archived-blocked, 404/validation handling. On validation failure add_note re-renders the detail page (new _render_equipment_detail helper) so the typed note is preserved rather than discarded.
• Template — inline always-visible Notes card; content rendered with white-space: pre-wrap under Jinja autoescape (XSS-safe, no nl2br/|safe).
• Migration — 77b248bd052d_add_equipment_notes_table.py. Downgrade drops the table directly to avoid MariaDB error 1553 on the FK-backing index.

Testing

• make lint clean; full suite: 1840 passed.
• New model, service, and view tests mirroring the external-links suite (permissions, 403/404, archived block, over-length, XSS-escaping, mutation log, append-only, empty state, content-preservation).
• Alembic migration verified upgrade↔downgrade against the MariaDB container.

Reviewer notes

• An independent adversarial review returned 6 Low findings (no Critical/High); 2 were fixed (stripped-length form validation; preserve typed note on failure), 4 acknowledged as intentional/out-of-scope. See the tech-spec's Review Notes.
• Out of scope (per issue): editing/deleting notes, pagination, notifications/Slack, and notes on public/kiosk/QR pages.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QzHqw96wXCi9i6aK349yZa

[jantman]

@claude please review this change

[claude]

Claude finished @jantman's task in 3m 40s —— View job

---

Reviewing PR #74

• Gather context (PR description, tech-spec, diff)
• Review model, migration, service, form, view, template
• Review tests
• Run make lint and the affected test suites locally
• Post final review

Summary

This is a clean, well-scoped implementation that closely follows the tech-spec at _bmad-output/implementation-artifacts/tech-spec-equipment-notes.md. I read every changed file and ran the suite locally — no correctness issues found.

Verification performed:

• make lint → clean (no ruff findings).
• pytest tests/test_models/test_equipment_note.py tests/test_services/test_equipment_service.py tests/test_views/test_equipment_views.py → 309 passed, 1 failed. The one failure (TestEquipmentQRTemplate::test_post_download_returns_png_with_preset_dimensions) is a pre-existing, unrelated environment issue — my sandbox is missing the native libzbar shared library used by pyzbar for QR decoding, not something introduced by this PR.

Things I specifically checked and found solid:

• XSS safety — equipment/detail.html:371 renders {{ note.content }} under Jinja autoescape with white-space: pre-wrap for newlines; no nl2br/|safe/Markup path exists. Confirmed via test_content_is_html_escaped.
• Length enforcement — service (equipment_service.py:497-502) is authoritative on stripped length; the form's validate_content (equipment_forms.py:137-148) mirrors that exactly (rather than WTForms' raw-length Length), so the form can never reject something the service would accept. Good catch on the earlier "not identical" version noted in the tech spec.
• Permission/archived precedence — add_note (equipment.py:660-693) calls _require_doc_edit() before the is_archived check, consistent with add_link/upload_document/etc., so an ungated technician correctly gets 403 rather than the archived flash. Verified by test_archived_block_for_permitted_user and test_ungated_technician_sees_no_add_form.
• Append-only guarantee — no edit/delete route exists; test_no_edit_or_delete_route_for_notes asserts this against app.url_map.
• Ordering determinism — created_at.desc(), id.desc() tiebreaker in both the service query and the model's notes backref order_by, tested with two notes sharing an explicit created_at.
• Content preservation on validation failure — _render_equipment_detail(eq, note_form=form) (equipment.py:693) is a nice touch: it re-renders the detail page with the typed note bound to the form instead of redirecting and discarding it. This is a sensible, well-tested refactor of detail()/add_note() that share logic without behavior drift (confirmed via diff — can_edit_docs calculation is unchanged).
• Migration — downgrade() drops the table directly rather than dropping the index first, correctly avoiding MariaDB error 1553 on the FK-backing index.
• Model registration — EquipmentNote is correctly alphabetized in esb/models/__init__.py between equipment and equipment_reservation_settings.

Minor, non-blocking observations (not requesting changes):

• esb/models/equipment_note.py:22 and the migration both declare an explicit index=True/ix_equipment_notes_equipment_id on equipment_id. Some MariaDB/InnoDB versions already auto-create an index to back a FK constraint, which could mean a redundant index — but this exactly mirrors the existing ExternalLink pattern in the codebase, so it's consistent rather than a new problem.
• In add_note, if equipment_service.add_equipment_note() raised ValidationError after form validation passed (essentially unreachable today, since the form already re-checks equipment existence indirectly via the earlier get_equipment(id) call and stripped-length), the redirect path would discard the typed note rather than preserving it like the validation-failure branch does. This is an extremely narrow edge case with no realistic trigger given current validation, so not worth extra complexity.

No blocking issues. The PR does what it says: append-only, permission-gated, XSS-safe equipment notes, with thorough tests mirroring the existing ExternalLink patterns and a solid migration.

[Copilot]

Pull request overview

Adds an append-only “Notes” capability to equipment records, including persistence, service-layer APIs, permission-gated creation, and UI rendering on the equipment detail page (newest-first), with comprehensive tests.

Changes:

• Introduces EquipmentNote model + Alembic migration for equipment_notes (FK to equipment, optional author FK + cached author name, content, created_at, deterministic ordering).
• Adds add_equipment_note() / get_equipment_notes() service functions with length enforcement and log_mutation auditing; wires a new POST route and detail-page rendering helper.
• Adds an always-visible Notes section + add-note form to the equipment detail template, plus new model/service/view tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_views/test_equipment_views.py	Adds end-to-end view tests for note creation, permissions, archived blocking, escaping, and append-only behavior.
tests/test_services/test_equipment_service.py	Adds unit tests for note service validation, ordering, and mutation logging.
tests/test_models/test_equipment_note.py	Adds model tests for persistence, defaults, and relationships/backrefs.
migrations/versions/77b248bd052d_add_equipment_notes_table.py	Adds equipment_notes table with FKs + equipment_id index; downgrade drops table directly.
esb/views/equipment.py	Adds _render_equipment_detail helper and /equipment/<id>/notes POST handler; includes notes in detail context.
esb/templates/equipment/detail.html	Adds Notes card with add form (permission-gated) and newest-first list rendering with pre-wrap and autoescape.
esb/services/equipment_service.py	Adds NOTE_MAX_LENGTH constant plus note create/list service functions with auditing.
esb/models/equipment_note.py	Adds EquipmentNote model, relationships, and ordering/backref behavior.
esb/models/init.py	Registers EquipmentNote for import/discovery.
esb/forms/equipment_forms.py	Adds EquipmentNoteForm with stripped-length validation aligned to the service constant.
_bmad-output/implementation-artifacts/tech-spec-equipment-notes.md	Adds the feature tech spec and acceptance criteria context.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.