Add inventory table models (Item, Batch, Category, Location)

[podmar]

The models are now complete enough to start building CRUD endpoints.

Decisions worth noting

location_id lives on Batch, not Item. The same product can be stored in multiple places at once (e.g. 4 passatas in the cellar, 2 in the pantry). Batch = one group of units sharing the same location and expiry date.

location_id on Batch is NOT NULL. Safe because registration now seeds defaults.

brand and image_url on Item. Left out of the original spec, added now because retrofitting requires a migration.

Timezone-aware datetimes. asyncpg rejects tz-aware datetime objects in TIMESTAMP WITHOUT TIME ZONE columns — all datetime fields use sa_column=sa.Column(sa.DateTime(timezone=True)) explicitly.

No Relationship() declarations. Not needed until queries require eager loading; adding them later is a zero-migration change.

Added .claude/commands/pr-description.md. Project-scoped /pr-description slash command for generating PR descriptions from the branch diff.

What's next

CRUD endpoints for Item and Batch, starting with the barcode lookup + add-to-inventory flow.

[claude]

Review Summary

Great work on the data model refactoring! The architectural decision to move location from Item to Batch is spot-on — it properly models the real-world scenario where the same product can be stored in multiple places. The documentation updates in CLAUDE.md and the spec are clear and helpful.

Critical Issue 🚨

Missing household isolation on Batch model: The Batch table needs a household_id field for proper multi-tenant security. Right now, queries against batches can't easily enforce "only show batches for the current user's household" without joining through Item. See the detailed comment on batch.py for two options (denormalization vs composite FK). I recommend denormalization for simplicity.

Important Validation Gaps

1. No validation on quantity: Batch accepts zero or negative quantities, which will cause bugs
2. No string length limits: Fields like name, brand, notes have no max length validation
3. Barcode uniqueness unclear: Should one barcode = one item per household? Needs a DB constraint if yes

Design Questions

1. Nullable location_id on Batch: What does a batch with no location mean? Consider making this required.
2. location_id on Item: The spec says location lives on Batch, but Item still has this field. Is it needed? If it's for "last used default," consider moving that to UI state instead of the DB.
3. Unique constraints: Should Category/Location names be unique per household? Probably yes for categories (since they're seeded).

What's Done Well ✅

• Proper household_id + index on Category, Location, and Item models
• Correct timezone-aware datetime implementation
• Good use of SQLAlchemy onupdate for updated_at
• Clear inline comments explaining business logic
• Comprehensive spec updates

The bones are really solid! The critical issue is the household isolation on Batch — definitely fix that before building the API endpoints. The validation and constraint questions are important for production readiness but not blockers for continued development.

Let me know if you'd like to discuss any of these points!

[claude]

Question about location_id on Item:

The spec says location is tracked at the Batch level (which makes sense!), but this field is still here on Item with the comment "defaults to last used location."

A couple of concerns:

1. Data consistency risk: If location_id exists on both Item and Batch, which one is the source of truth? This could lead to confusion or bugs.
2. Unclear purpose: If it's meant to be a "default for next batch," that's UI/app state, not really part of the Item's core data model.

Suggestion: Consider removing location_id from Item entirely, and track "last used location" either:

• In the frontend (localStorage/session state), or
• Via a separate UserPreferences table if you need it server-side

This would make the model cleaner and eliminate the risk of location data getting out of sync. What do you think?

[claude]

Best practice note on onupdate:

The onupdate parameter in SQLAlchemy fires on every ORM update, which is great! However, there's a subtle gotcha: it only fires when you use the ORM (like session.add(item) then session.commit()). If you ever use raw SQL updates or bulk operations, this won't trigger.

For learning purposes: in production apps, some teams prefer database-level triggers or DEFAULT CURRENT_TIMESTAMP ON UPDATE (MySQL) to guarantee the timestamp updates regardless of how the row is modified. But for this project, the ORM approach is perfect — just something to be aware of if you ever work on larger systems.

Your implementation here is textbook correct! 👍

[claude]

Critical: Missing household_id isolation!

This is the most important security issue in the PR. Batch needs to enforce household isolation, but it only has item_id and location_id as foreign keys.

The problem:

• A malicious user could craft a request with a valid item_id from another household and create/modify batches for items they don't own.
• Even if your API endpoints check permissions, having the FK structure correct is defense-in-depth.

Two options:

Option A: Denormalize (recommended for this project):
Add household_id directly to Batch:

household_id: int = Field(foreign_key="households.id", index=True)

Then every query can filter WHERE household_id = current_user.household_id. Yes, it's redundant with Item.household_id, but it makes queries simpler and safer.

Option B: Use composite foreign keys:
Define a FK constraint like (item_id, household_id) → (items.id, items.household_id). This is more normalized but adds complexity.

For a learning/portfolio project, Option A is cleaner and easier to explain in an interview. The denormalization trade-off (a bit of redundancy for query simplicity and security) is a real-world pattern used by companies like Stripe and GitHub.

Same issue exists for Category and Location models below.

[podmar]

Applied to batch, category and location are already correct.

[claude]

Edge case: Nullable location_id

The comment says "defaults to last used location, which may not exist yet," but this creates an awkward state: a Batch with no location.

Questions to consider:

1. What does it mean for a batch to have location_id = NULL? Is it "in limbo" waiting for the user to assign it?
2. How will the inventory view handle this? Will these batches show up under "Unknown location"?

Suggestion: If location is core to the data model (which it seems to be per the spec), consider:

• Making location_id required (NOT NULL)
• Ensuring the API always assigns a location when creating a batch (using the last-used default or prompting the user)

This would eliminate an entire class of edge cases. If you need flexibility during item creation, handle the default location logic in the API layer before saving to DB.

What's your thinking here?

[podmar]

registration now seeds defaults for category and location.

[claude]

Input validation missing:

What happens if someone tries to create a batch with:

• quantity = 0 (or negative)?
• expiry_date in the past?

Right now, the DB would accept these values, which could lead to weird UI states or bugs in the "expiring soon" logic.

Suggestion: Add validation either via:

1. Pydantic validators in the model (cleaner, happens before DB):

from pydantic import field_validator

@field_validator('quantity')
def validate_quantity(cls, v):
    if v < 1:
        raise ValueError('quantity must be at least 1')
    return v

2. DB constraints via SQLAlchemy:

quantity: int = Field(sa_column=sa.Column(sa.Integer, sa.CheckConstraint('quantity > 0')))

For a FastAPI app, option 1 (Pydantic validators) is more idiomatic and gives better error messages to the frontend. The DB constraint is backup defense-in-depth.

Negative/zero quantities are a real bug risk — I've seen production apps where this caused inventory to go haywire!

[podmar]

fix on quantity added, the remaining points to be considered with endpoints, not a consideration as db constraints

[claude]

Question: Should barcode be unique per household?

Right now barcode has index=True but no uniqueness constraint. This means:

• Multiple items in the same household could have the same barcode
• Different households can definitely have the same barcode (which is correct!)

Real-world scenario:
User scans a barcode, app checks Open Food Facts, creates Item A. Later they scan the same barcode again — should it:

1. Find and reuse Item A (adding a new Batch), or
2. Allow creating Item B with the same barcode?

Suggestion: If the answer is (1), add a composite unique constraint:

from sqlalchemy import UniqueConstraint

class Item(SQLModel, table=True):
    # ... fields ...
    
    __table_args__ = (
        UniqueConstraint('household_id', 'barcode', name='uq_household_barcode'),
    )

This enforces "one item per barcode per household" at the DB level, which matches the expected behavior in Flow 1 step 5 of the spec ("If existing product: show current batches...").

If you don't add this constraint, you'll need to handle duplicate barcodes in the API layer, which is trickier.

[claude]

String field length limits:

Fields like name, brand, image_url, and notes are defined as str with no max length. In PostgreSQL, this creates a TEXT column (unlimited), which is fine for storage, but...

Potential issues:

1. No validation: A malicious or buggy client could send a 10MB string for name, which would be accepted.
2. UI concerns: If name is 1000 characters, how does it display on mobile?

Suggestion: Add reasonable max lengths via Pydantic:

from sqlmodel import Field

name: str = Field(max_length=200)
brand: str | None = Field(default=None, max_length=100)
image_url: str | None = Field(default=None, max_length=500)
notes: str | None = Field(default=None, max_length=1000)

These get enforced by Pydantic validation and create VARCHAR(N) columns in Postgres (though TEXT is also fine for Postgres — it's more about the validation).

For image_url specifically, 500 chars should be plenty. For notes, 1000 is reasonable for a mobile form. Adjust to taste!

Not a critical bug, but good defensive programming.

[podmar]

validation to be implemented in the request schemas, db schemas and table model is not the correct place

[claude]

Good pattern here! ✅

Same as Category — household_id with index is exactly right. Locations are properly isolated per household.

One thing to consider: Should location names be unique within a household? E.g., prevent two "Fridge" locations?

If yes, add:

from sqlalchemy import UniqueConstraint

__table_args__ = (
    UniqueConstraint('household_id', 'name', name='uq_household_location'),
)

This is a UX question: do you want to prevent duplicates, or allow users to have "Fridge 1" and "Fridge 2" if they want? Either is valid — just something to decide explicitly.

[podmar]

good catch

[claude]

Same question as Location: Should category names be unique per household?

Given that categories are seeded on household creation (per CLAUDE.md), you probably don't want duplicates. Users shouldn't be able to manually create a second "Food" category.

Consider adding the same UniqueConstraint('household_id', 'name') as suggested for Location.

Alternatively, if categories are meant to be managed only by seeding (not user-editable), you could enforce that at the API level by not exposing create/update endpoints. But the DB constraint is safer defense-in-depth.

[claude]

Excellent additions! 🎉

These new sections (datetime columns, schema changes, fastapi-users imports) are super helpful for future work. The datetime guidance especially — the asyncpg timezone issue is a common gotcha.

One small suggestion: the "No Alembic yet" section might want to note that this is intentional for now and will be added before production. Just to make it clear this is a conscious decision, not an oversight. E.g.:

### Schema changes

No Alembic yet — schema changes require dropping tables in Neon console and restarting the server. **This is intentional for rapid prototyping.** Add Alembic before any production data exists.

Makes it clear to a reviewer (or future you) that this is a deliberate trade-off for v1.

[claude]

💰 Review cost: $0.18