fix: set non-required fields as nullable #63
Conversation
lchen-2101
requested review from
hkeeler,
guffee23,
jcadam14 and
nargis-sultani
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)src/entities/models/dao.py
src/entities/models/dto.py
src/entities/repos/institutions_repo.py
|
| @@ -74,6 +74,22 @@ async def upsert_institution(session: AsyncSession, fi: FinancialInstitutionDto) | |||
| session.add(db_fi) | |||
| else: | |||
| db_fi.name = fi.name | |||
| db_fi.tax_id = fi.tax_id | |||
There was a problem hiding this comment.
Don't want to hold up this PR over this, but might be worth checking into SQL Alchemy's built-in upsert functionality.
It'd at least remove the duplicate property setting bits...I think. Of course, there's probably a performance question to consider for this endpoint since it's probably going to be hit a lot at least during initial institution data load. Then again, we'll probably want to support a bulk upload in the long run, so many it's not an issue.
There was a problem hiding this comment.
Yeap, this endpoint was originally thought of as a "convenience" endpoint; when we load actual data, it should be outside of using the API; I'll create a ticket on the upsert part, had a glance at the beginning and they were more direct table insertion based rather than the ORM way of doing things; doesn't hurt to see of the ORM upsert is there, and easily implementable, would certainly get rid of a lot of boilerplate.
closes #62