Conversation
| referral = Referral(**_prepare_payload(data)) | ||
| db.add(referral) | ||
|
|
||
| try: |
There was a problem hiding this comment.
really nitpick, but maybe another exception handler, so that it doesn't only handle integrity errors, but any error in general
except Exception:
await db.rollback()
raise
something like that just so we roll it back
|
|
||
| try: | ||
| await db.commit() | ||
| except IntegrityError as e: |
There was a problem hiding this comment.
same here, a more broad exception handler
|
|
||
| try: | ||
| await db.commit() | ||
| except IntegrityError as e: |
There was a problem hiding this comment.
same here, a more broad exception handler
| return prepared | ||
|
|
||
|
|
||
| async def _validate_related_records(db: AsyncSession, data: dict[str, Any]) -> None: |
There was a problem hiding this comment.
i like this validation, @kenzysoror out of curiosity could we make this function a little more generic instead of beling specific to referrals so we can do the same validation for other CRUD models asw? like donations, furniture, etc, etc? just a thought
There was a problem hiding this comment.
i like this idea!! it would definitely reduce our code duplication in validating all the models
| return await referrals_service.create_referral(db, payload) | ||
| except ValueError as e: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, |
There was a problem hiding this comment.
it looks like most of the CRUD implementations the team is working on use HTTP_400_BAD_REQUEST over HTTP_422_UNPROCESSABLE_ENTITY, so i think we should keep it consistent for now
|
|
||
| async def _validate_related_records(db: AsyncSession, data: dict[str, Any]) -> None: | ||
| """Ensure related client and agency records exist when provided.""" | ||
| if "client_id" in data: |
There was a problem hiding this comment.
can we maybe skip lookups when the FK is None or an empty string to avoid hitting the database for
"obviously" invalid values?
| """Ensure related client and agency records exist when provided.""" | ||
| if "client_id" in data: | ||
| client_result = await db.execute( | ||
| select(Client).where(Client.id == data["client_id"]) |
There was a problem hiding this comment.
if all we care about here is checking the existence of the client/agency, then we can select(Client.id) and select(Agency.id) instead of the full rows
| db: AsyncSession, referral: Referral, payload: ReferralUpdate | ||
| ) -> Referral: | ||
| """Update an existing referral with validated, normalized data.""" | ||
| data = payload.model_dump(exclude_unset=True) |
There was a problem hiding this comment.
potentially add a check like
if not data:
raise ValueError("No update fields were provided.")
to avoid committing without modifying anything (#17 for reference)
3 participants
Jira ticket link
Jira ticket
Implementation description
Steps to test
agency_idto create a clientagency_idandclient_id)What should reviewers focus on?
Checklist
Format for branch, commit, and PR title: docs/GIT.md.