68 update institutions table and daos institution type fields #75
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ndpoint Adds endpoints for getting address-states and regulators Adds institution repo get_ functions for the above Let commented out code in for specific Dtos in case the general approach isn't desired
Changed the FI DTO to have sbl_institution_type_ids (instead of id) and use a list Changed the FI DAO to have sbl_institution_type_ids and sbl_institution_types, both of which are now lists Changed the FI DAO sbl_institution_type_ids to be an AssociationProxy (view) based on the sbl_institution_types Created an fi_to_type_mapping table that holds the relationship between FI sbl_institution_types and the slb_institution_type table entries Updated the institution_repo fi upsert function to look for incoming DTO sbl_institution_type_ids, and retrieve the corresponding SBLInstitutionTypeDao entries. They are then associated with the fi_data before being inserted/updated (which populates the new mapping table) Updated the pytests to work with this new concept of lists, especially the test_add_institution repo test. Will write a new story for the sbl-project repo once these chanegs are approved and merged in to update the mock_data json
jcadam14
requested review from
hkeeler,
guffee23,
lchen-2101 and
nargis-sultani
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
…le to drop the sbl_institution_type_id column, and to remove the FK and index. On the downgrade, these are added back in.
…ution-type-fields
lchen-2101
reviewed
Jan 5, 2024
| # Populate with model objects from SBLInstitutionTypeDao and clear out | ||
| # the id field since it's just a view | ||
| if "sbl_institution_type_ids" in fi_data: | ||
| sbl_type_ids = tuple(fi_data["sbl_institution_type_ids"]) |
There was a problem hiding this comment.
why does this need to be converted to tuple? the ...in_(...) method just needs an interable, right? so list would be fine?
lchen-2101
reviewed
Jan 9, 2024
src/entities/models/dto.py
Outdated
There was a problem hiding this comment.
one part that's missing is the updation of the FinancialInstitutionWithRelationsDto; that should include sbl_institution_types: List[InstitutionTypeDto] = []
lchen-2101
reviewed
Jan 9, 2024
| @@ -74,6 +74,17 @@ async def upsert_institution(session: AsyncSession, fi: FinancialInstitutionDto) | |||
| db_fi = res.scalar_one_or_none() | |||
| fi_data = fi.__dict__.copy() | |||
| fi_data.pop("_sa_instance_state", None) | |||
|
|
|||
There was a problem hiding this comment.
With the update to the upsert using merge, this is causing the merge conflicts.
I have mine like:
async with session.begin():
fi_data = fi.__dict__.copy()
fi_data.pop("_sa_instance_state", None)
sbl_types = []
if len(fi.sbl_institution_type_ids):
sbl_type_stmt = select(SBLInstitutionTypeDao).filter(
SBLInstitutionTypeDao.id.in_(fi.sbl_institution_type_ids)
)
sbl_types = (await session.scalars(sbl_type_stmt)).all()
del fi_data["sbl_institution_type_ids"]
new_fi = FinancialInstitutionDao(**fi_data)
new_fi.sbl_institution_types.extend(sbl_types)
db_fi = await session.merge(new_fi)
await session.flush([db_fi])
await session.refresh(db_fi)
return db_fiFixed FinancialInstitutionWithRelationsDto to container a list of sbl_institution_types Updated institution_repo to correct merge issues with main
…ution-type-fields
lchen-2101
reviewed
Jan 9, 2024
| SBLInstitutionTypeDao.id.in_(fi_data["sbl_institution_type_ids"]) | ||
| ) | ||
| sbl_types = await session.scalars(sbl_type_stmt) | ||
| fi_data["sbl_institution_types"] = sbl_types.all() |
There was a problem hiding this comment.
One thing to keep in mind with not initializing this field out of the if check is any previous relationships would remain the same if the field if not specified. Since this function will only be used by internal processes, I'm ok with this behavior, but it is worth noting this is different from the rest of the fields.
I think we're good to merge now once the commented out code are cleaned up.
There was a problem hiding this comment.
tested wrong, not an issue; we're good to go after clean up.
…fields' of https://github.com/cfpb/regtech-user-fi-management into 68-update-institutions-table-and-daos-institution-type-fields
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #68
Changed the FI DTO to have sbl_institution_type_ids (instead of id) and use a list
Changed the FI DAO to have sbl_institution_type_ids and sbl_institution_types, both of which are now lists
Changed the FI DAO sbl_institution_type_ids to be an AssociationProxy (view) based on the sbl_institution_types
Created an fi_to_type_mapping table that holds the relationship between FI sbl_institution_types and the slb_institution_type table entries
Updated the institution_repo fi upsert function to look for incoming DTO sbl_institution_type_ids, and retrieve the corresponding
SBLInstitutionTypeDao entries. They are then associated with the fi_data before being inserted/updated (which populates the new mapping table)
Updated the pytests to work with this new concept of lists, especially the test_add_institution repo test.
Will write a new story for the sbl-project repo once these chanegs are approved and merged in to update the mock_data json