Skip to content

feat(core): add CellarEntry model + migration (VPA-17) - #862

Open
vpatrin wants to merge 1 commit into
mainfrom
feat/cellar-entry-model
Open

feat(core): add CellarEntry model + migration (VPA-17)#862
vpatrin wants to merge 1 commit into
mainfrom
feat/cellar-entry-model

Conversation

@vpatrin

@vpatrin vpatrin commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Summary

Adds the cellar_entries table — one row per (user_id, sku) with a quantity, not one row per bottle. This is the persistence layer only: no endpoints, no UI. Enabler for the Cellar feature (VPA-18..22).

Related issue(s)

Fixes VPA-17

Changes

  • core/db/models.py — new CellarEntry model: id (PK), user_id (Integer FK → users.id, ondelete="CASCADE"), sku (String FK → products.sku, indexed), quantity (default 1), added_at
  • core/alembic/versions/a321897f6ef7_add_cellar_entries_table.py — migration, chains onto prior head d54ff3a506b5
  • backend/tests/test_cellar_entry_model.py — two constraint tests (unique (user_id, sku), quantity > 0)
  • Docs: session log, ROADMAP, session-log index

user_id is Integer, not the legacy String pattern

Watch.user_id / TastingNote.user_id are channel-prefixed String columns (tg:123456), inherited from Telegram-first auth (ADR 0004, superseded by ADR 0008 OAuth) — VPA-42 tracks migrating those. CellarEntry is a brand-new table with no reason to reproduce that shape and migrate it twice, so it goes straight to Integer, ForeignKey("users.id").

Consequence for VPA-18: the cellar service/repo code cannot be copy-pasted from watches.py — it has to resolve the authenticated user's integer id, not build a channel-prefixed string.

No standalone index on user_id

The (user_id, sku) unique constraint's composite btree already covers user_id-only lookups via leftmost prefix, so a separate index would be redundant. Deliberate divergence from Watch, which carries that redundancy. sku keeps its own index — it's an FK-referencing column, and an unindexed one would seq-scan cellar_entries on a future products hard-delete.

Migration review checklist

  • Constraint names are literal: uq_cellar_entries_user_sku, ck_cellar_entries_quantity
  • ondelete="CASCADE" on the user_id FK only — sku stays a bare FK (products soft-delete via delisted_at, never hard-deleted)
  • Exactly one index: ix_cellar_entries_sku
  • downgrade() is a real drop (index, then table), not a stub

Testing limitation

The tests use a sync SQLite session that creates only CellarEntry.__table__, not the full schema — the full schema fails under SQLite because products.tasting_profile is JSONB. UNIQUE and CHECK are genuinely exercised (both are falsifiable — remove either constraint and the corresponding test fails). FK behavior and ondelete="CASCADE" are not covered by these tests; that lands with VPA-18, once real rows exist to delete a parent through. CI's migrate job (fresh Postgres, alembic upgrade head) proves the DDL itself is valid.

The tests live under backend/tests/ rather than core/core has no pytest dependency, no test directory, and no CI job that would execute tests placed there.

How to test

Fresh-DB apply is already covered by CI's migrate job on this PR. Outstanding manual step (not done yet, owed separately): apply to a copy of prod data.

cd core && DB_HOST=<scratch-host> DB_NAME=<scratch-db> poetry run alembic upgrade head

Confirm exit 0, then inspect the table:

psql -c "\d cellar_entries"

Table tracking bottles a user owns at home — one row per (user, SKU)
with a quantity, not one row per bottle. Enabler for the Cellar
endpoints and UI (VPA-18..22).

user_id is an Integer FK to users.id rather than the channel-prefixed
String used by Watch and TastingNote: a brand-new table has no reason
to reproduce that legacy shape and migrate it twice (VPA-42 tracks
migrating the existing ones). Consequence: cellar service code cannot
be copy-pasted from watches.py — it must resolve the integer user id.

No standalone index on user_id — the (user_id, sku) unique constraint's
composite btree already covers user_id-only lookups via leftmost prefix.

Closes VPA-17
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant