-
Notifications
You must be signed in to change notification settings - Fork 0
Add inventory table models (Item, Batch, Category, Location) #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
126e698
3156fc9
23106a6
91a22f6
fbebf2e
e6209ae
ac65cce
f813541
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| Write a PR description for the current branch and save it to `docs/pr-description.md`. | ||
|
|
||
| Use `git log main..HEAD` and `git diff main..HEAD` to understand what changed. | ||
|
|
||
| Rules: | ||
| - Do not list files changed — that's visible in the diff | ||
| - Focus on WHY, not WHAT | ||
| - Explain decisions that aren't obvious from the code | ||
| - Note anything a reviewer should pay attention to | ||
| - Keep it concise — one bullet per decision, one sharp reason per bullet | ||
| - End with a "What's next" line if there's a clear next step | ||
| - Use plain GitHub-flavoured markdown, no emoji | ||
| - Tone: professional, humble, clear | ||
|
|
||
| Sharpness rules: | ||
| - Don't restate the title in the opening summary — if the sentence adds nothing, cut it | ||
| - "What is in diff, is in diff" — never describe which files changed or say "this is reflected in X, Y, Z" | ||
| - No anecdote or backstory ("this bit us", "we discovered") — state the technical constraint directly | ||
| - Each bullet: decision + the one sharp reason it was made, nothing else | ||
| - Don't over-explain consequences that follow obviously from the constraint | ||
| - Cut filler: "zero complexity", "available to anyone", "particularly useful for" | ||
|
|
||
| Structure: | ||
| ## <short title> | ||
| <1 sentence — only if it adds something the title doesn't> | ||
| ### Decisions worth noting | ||
| ### What's next |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,4 +229,5 @@ frontend/.vite/ | |
| Thumbs.db | ||
|
|
||
| # Project specific | ||
| docs/context.md | ||
| docs/context.md | ||
| docs/pr-description.md | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| from app.models.batch import Batch | ||
| from app.models.category import Category | ||
| from app.models.household import Household | ||
| from app.models.item import Item | ||
| from app.models.location import Location | ||
| from app.models.user import User | ||
|
|
||
| __all__ = ["Household", "User"] | ||
| __all__ = ["Batch", "Category", "Household", "Item", "Location", "User"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| from datetime import UTC, date, datetime, timedelta | ||
|
|
||
| import sqlalchemy as sa | ||
| from sqlmodel import Field, SQLModel | ||
|
|
||
|
|
||
| class Batch(SQLModel, table=True): | ||
| __tablename__ = "batches" # type: ignore[assignment] | ||
|
|
||
| id: int | None = Field(default=None, primary_key=True) | ||
| # Denormalized from Item for query-level household isolation — every batch query can filter | ||
| # directly on household_id without joining through items. | ||
| household_id: int = Field(foreign_key="households.id", index=True) | ||
| item_id: int = Field(foreign_key="items.id", index=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Missing household_id isolation! This is the most important security issue in the PR. The problem:
Two options: Option A: Denormalize (recommended for this project): household_id: int = Field(foreign_key="households.id", index=True)Then every query can filter Option B: Use composite foreign keys: 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.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied to batch, category and location are already correct. |
||
| quantity: int = Field(sa_column=sa.Column(sa.Integer, sa.CheckConstraint("quantity > 0"), nullable=False)) | ||
| # Defaults to approximately +12 months from today. | ||
| # Month + year precision only — the day component is not shown in the UI. | ||
| expiry_date: date = Field( | ||
| default_factory=lambda: date.today() + timedelta(days=365) | ||
| ) | ||
| location_id: int = Field(foreign_key="locations.id") | ||
| created_at: datetime = Field( | ||
| default_factory=lambda: datetime.now(UTC), | ||
| sa_column=sa.Column(sa.DateTime(timezone=True), nullable=False), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import sqlalchemy as sa | ||
| from sqlmodel import Field, SQLModel | ||
|
|
||
|
|
||
| class Category(SQLModel, table=True): | ||
| __tablename__ = "categories" # type: ignore[assignment] | ||
| __table_args__ = (sa.UniqueConstraint("household_id", "name", name="uq_category_household_name"),) | ||
|
|
||
| id: int | None = Field(default=None, primary_key=True) | ||
| household_id: int = Field(foreign_key="households.id", index=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good: But missing index: You have Already done: Actually, looking at this again, Nice work! This is the correct pattern. Now just need to apply the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
| name: str | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| from datetime import UTC, datetime | ||
|
|
||
| import sqlalchemy as sa | ||
| from sqlmodel import Field, SQLModel | ||
|
|
||
|
|
||
| class Item(SQLModel, table=True): | ||
| __tablename__ = "items" # type: ignore[assignment] | ||
| # NULL barcodes are exempt — PostgreSQL does not consider NULLs equal in unique constraints, | ||
| # so multiple items without a barcode in the same household are allowed. | ||
| __table_args__ = (sa.UniqueConstraint("household_id", "barcode", name="uq_item_household_barcode"),) | ||
|
|
||
| id: int | None = Field(default=None, primary_key=True) | ||
| household_id: int = Field(foreign_key="households.id", index=True) | ||
| name: str | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String field length limits: Fields like Potential issues:
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 For Not a critical bug, but good defensive programming.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validation to be implemented in the request schemas, db schemas and table model is not the correct place |
||
| barcode: str | None = Field(default=None, index=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Should barcode be unique per household? Right now
Real-world scenario:
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. |
||
| brand: str | None = Field(default=None) | ||
| image_url: str | None = Field(default=None) | ||
| # Nullable: category may be unknown if barcode lookup returns no result. | ||
| category_id: int | None = Field(default=None, foreign_key="categories.id") | ||
| unit: str | ||
| notes: str | None = None | ||
| created_at: datetime = Field( | ||
| default_factory=lambda: datetime.now(UTC), | ||
| sa_column=sa.Column(sa.DateTime(timezone=True), nullable=False), | ||
| ) | ||
| # onupdate fires automatically on every ORM-level UPDATE to this row. | ||
| updated_at: datetime = Field( | ||
| default_factory=lambda: datetime.now(UTC), | ||
| sa_column=sa.Column( | ||
| sa.DateTime(timezone=True), | ||
| nullable=False, | ||
| onupdate=lambda: datetime.now(UTC), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Best practice note on The For learning purposes: in production apps, some teams prefer database-level triggers or Your implementation here is textbook correct! 👍 |
||
| ), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import sqlalchemy as sa | ||
| from sqlmodel import Field, SQLModel | ||
|
|
||
|
|
||
| class Location(SQLModel, table=True): | ||
| __tablename__ = "locations" # type: ignore[assignment] | ||
| __table_args__ = (sa.UniqueConstraint("household_id", "name", name="uq_location_household_name"),) | ||
|
|
||
| id: int | None = Field(default=None, primary_key=True) | ||
| household_id: int = Field(foreign_key="households.id", index=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good pattern here! ✅ Same as Category — 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.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch |
||
| name: str | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.:
Makes it clear to a reviewer (or future you) that this is a deliberate trade-off for v1.