Skip to content

Furniture CRUD (DEV-45)#15

Open
petersenmatthew wants to merge 12 commits intomainfrom
feature/DEV-45/furniture-crud
Open

Furniture CRUD (DEV-45)#15
petersenmatthew wants to merge 12 commits intomainfrom
feature/DEV-45/furniture-crud

Conversation

@petersenmatthew
Copy link

@petersenmatthew petersenmatthew commented Feb 27, 2026

Jira ticket link

Jira ticket

Implementation description

  • Made furniture CRUD
  • Created services folder, with init file

Steps to test

  1. Manually insert a test donor (CRUD to-be-implemented)
docker exec -it hafb_db psql -U hafb_user -d hafb
INSERT INTO donors (
  id,
  name
)
VALUES (
  'test-donor-1',
  'Test Donor'
);
  1. Test all API routes with sample furniture data
  • Set donor_id="test-donor-1"
  • Remove client_id="string"
  • Remove route_id="string"
  • Remove "Z" at the end of date_donated (timezone misalignment to-be-fixed)
  • Remove "Z" at the end of date_received (timezone misalignment to-be-fixed)

What should reviewers focus on?

  • Services logic

Checklist

Format for branch, commit, and PR title: docs/GIT.md.

  • My branch name includes the Jira ticket key
  • My PR name is descriptive and in imperative tense
  • My PR name includes the Jira ticket key
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • My commit messages include the Jira ticket key
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

Copy link
Contributor

@brian-fu brian-fu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. compress routes layer by moving db logic to service layer ex:

  2. dont use dictionary for creation use the pydantic model itself; heres what gemini said:

Loss of Introspection: When you pass a dict, your IDE (VS Code/PyCharm) has no idea what keys are inside it. You lose autocomplete and linting.

Double Work: The API route already validated the data into a FurnitureCreate Pydantic model. By calling .model_dump(), you are throwing away that structured object and turning it back into a "bag of strings."

Runtime Errors: If you change a field name in your Pydantic schema but forget to update the service logic, the dict will still pass through, but the database operation will crash at runtime.

this uses a custom set and a dictionary comprehension to filter out "unwritable" columns:
filtered = {k: v for k, v in data.items() if k in _WRITABLE_COLUMNS}

Why this is a failure:

Redundancy: Pydantic is literally built to do this. Your FurnitureCreate schema should already only contain writable fields.

Maintenance Nightmare: Every time you add a column to your database, you have to ensure it’s in the model, the schema, AND that your _WRITABLE_COLUMNS logic isn't accidentally blocking it or allowing something it shouldn't (like id).

Logic Leak: The service layer shouldn't be "guessing" what is writable by inspecting Furniture.table.c.keys(). The Schema defines what the user is allowed to send.

The Fix: Trust your Pydantic Schema.

fixed code should look sumn like this:

async def update_furniture(
    furniture_id: str, 
    payload: FurnitureUpdate, # Use the Pydantic model
    db: AsyncSession
) -> Furniture | None:
    furniture = await get_furniture(furniture_id, db)
    if not furniture:
        return None

    # model_dump(exclude_unset=True) ensures we ONLY update 
    # the fields the user actually provided in the request.
    update_data = payload.model_dump(exclude_unset=True)
    
    # Update the DB object
    for key, value in update_data.items():
        setattr(furniture, key, value)

    await db.commit()
    await db.refresh(furniture)
    return furniture

Copy link
Member

@kenzysoror kenzysoror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work matthew!!🥳

@kenzysoror kenzysoror force-pushed the feature/DEV-45/furniture-crud branch from 31fdffa to 2feafaf Compare March 5, 2026 08:50
@kenzysoror kenzysoror mentioned this pull request Mar 5, 2026
7 tasks
@kenzysoror
Copy link
Member

#17 also involves a schema update. if that gets merged in before this PR:

  1. delete backend/migrations/versions/20260305_rename_furniture_dispatch_id_to_route_id.py
  2. run
docker exec -it hafb_backend bash
alembic revision --autogenerate -m "rename_furniture_dispatch_id_to_route_id"
alembic upgrade head

@kenzysoror kenzysoror force-pushed the feature/DEV-45/furniture-crud branch from df33a73 to 1a4c186 Compare March 9, 2026 03:01
Copy link
Member

@kenzysoror kenzysoror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 i've regenerated the migration, so there shouldn't be any merge issues there!

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.

3 participants