CRUD operations for Donors (DEV-39)

[nuthanan06]

Jira ticket link

Jira ticket

Implementation description

• Added CRUD Operations for Donor Schema via API Endpoint and Service File

Steps to test

1. Run docker images, Swagger UI, and play around with the endpoints.

• get /donors --> gets all donors
• post /donors --> creates a new donor
• get /donors/:id --> get a particular donor
• put /donors/:id --> updates a particular donor
• delete /donors/:id --> delets a particular donor

What should reviewers focus on?

• testing the API

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

[petersenmatthew]

run black backend/app/services/donors.py to apply the lint suggestions

[Copilot]

Pull request overview

Implements full Donors CRUD functionality in the FastAPI backend, replacing the prior placeholder endpoints and introducing a small service layer to hold donor business logic.

Changes:

• Added donor CRUD service functions (list/create/get/update/delete) using AsyncSession.
• Implemented full REST CRUD endpoints for /api/donors wired to the new service layer.
• Introduced a services package initializer exporting donor_service.

Reviewed changes

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

File	Description
backend/app/services/donors.py	Adds async CRUD service functions for the Donor model (DB read/write + rollback on failure).
backend/app/services/init.py	Exposes donor_service from the new services package.
backend/app/api/donors.py	Replaces placeholder donors endpoints with full CRUD routes calling into donor_service.

Comments suppressed due to low confidence (3)

backend/app/api/donors.py:70

• This except HTTPException: raise / except Exception pattern is repeated and largely redundant (re-raising an HTTPException has no effect). To keep endpoints consistent and reduce duplication, prefer letting exceptions bubble to a global handler (or catching specific DB exceptions) rather than wrapping each route.

    except HTTPException:
        raise
    except Exception as e:
        if get_settings().DEBUG:
            raise HTTPException(
                status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
                detail=str(e),
            ) from e
        raise

backend/app/services/donors.py:28

• Catching Exception broadly in the service makes it easy to accidentally treat non-DB failures (e.g., programmer errors) as transactional errors. Consider catching SQLAlchemy-specific exceptions (e.g., SQLAlchemyError) for the rollback path, and let unexpected exceptions propagate without being wrapped here.

    try:
        db_donor = Donor(**donor.model_dump())
        db.add(db_donor)
        await db.commit()
        await db.refresh(db_donor)
        return db_donor
    except Exception:
        await db.rollback()
        raise

backend/app/services/donors.py:70

• Same broad except Exception rollback pattern as create/update: consider narrowing this to SQLAlchemyError (or similar) so only DB/transaction errors trigger rollback handling, while unexpected exceptions can surface normally during development.

    try:
        await db.delete(donor)
        await db.commit()
        return True
    except Exception:
        await db.rollback()
        raise

---

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

[petersenmatthew]

• Delete: real functional bug.
• Duplicate email: correctness/error-handling bug

1. Delete breaks for donors linked to furniture

   • In backend/app/services/donors.py the delete path does a direct
     db.delete(donor) with no handling for related furniture rows.
   • In backend/app/models/base.py furniture.donor_id is a non-null foreign key
     to donors.id.
   • So deleting a donor who has donated furniture can raise a foreign key error and
     return a 500 instead of a controlled 4xx

2. Duplicate email surfaces as a server error

   • In backend/app/services/donors.py create/update just commit and re-raise
     generic exceptions.
   • The donor email uniqueness constraint still exists in migrations at backend/ migrations/versions/20260215_hafb_tables.py.
   • That means duplicate email writes are a normal client conflict, but the API
     currently lets them become unhandled 500s instead of a controlled client error.

[kenzysoror]

great work, nuth!!💪

[petersenmatthew]

Deleting a donor can fail here if that donor is still referenced by furniture.donor_id. In that case this will raise a foreign key error and the endpoint returns a 500 instead of a controlled client response.

Should either block deletion with a 4xx (for example 409) when linked furniture exists, or explicitly handle those dependent records.

[nuthanan06]

This is a good point, we might want to think about whether furniture should still exist if the donor is deleted. Is it safe to assume that if a donor is deleted, we should also delete all furniture associated with that donor?

To me, it doesn’t make much sense to throw an error just because a furniture item references a donor that’s being deleted. A bulk delete might make more sense here, otherwise someone would have to delete all the furniture before deleting the donor?

@kenzysoror thoughts?

[kenzysoror]

mmm really good catch! i spoke with the PMs and it seems we actually want to allow furniture to exist without a donor (e.g., if HAFB adds existing inventory into the platform when they first adopt it).

it turns out that we don't currently foresee any use cases where HAFB would need to delete a donor from the system. so for now, and given the above, we'd want deleting a donor to simply nullify the donor field within the associated furniture and keep the furniture intact.

updating the furniture schema to reflect this can be another PR, and i believe this delete logic should work as-is once that's done.

does this make sense @nuthanan06 @petersenmatthew ?

[kenzysoror]

👌