Updated DAO, added migration, model_validator, and pytests (#114)

Closes #113 - Updated FinancialInstitutionDAO.tax_id to be 10 characters - Updated alembic script to change financial_institutions and financial_institutions_history tables tax_id column to be 10 characters - Added model_validator to FinancialInstitutionDTO to check regex on tax_id - Added pytest to check for invalid tax_id - Updated existing tax_id fields in pytests to contain the dash
cfpb · Mar 13, 2024 · 4f2224b · 4f2224b
1 parent 76202f5
commit 4f2224b
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 14 deletions.
diff --git a/db_revisions/versions/0e520c01fb42_update_tax_id_to_10_characters.py b/db_revisions/versions/0e520c01fb42_update_tax_id_to_10_characters.py
@@ -0,0 +1,32 @@
+"""update tax-id to 10 characters
+
+Revision ID: 0e520c01fb42
+Revises: 8106d83ff594
+Create Date: 2024-03-12 11:40:25.790658
+
+"""
+from typing import Sequence, Union
+
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision: str = "0e520c01fb42"
+down_revision: Union[str, None] = "8106d83ff594"
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    with op.batch_alter_table("financial_institutions") as batch_op:
+        batch_op.alter_column("tax_id", existing_type=sa.String(9), type_=sa.String(10))
+    with op.batch_alter_table("financial_institutions_history") as batch_op:
+        batch_op.alter_column("tax_id", existing_type=sa.String(9), type_=sa.String(10))
+
+
+def downgrade() -> None:
+    with op.batch_alter_table("financial_institutions") as batch_op:
+        batch_op.alter_column("tax_id", existing_type=sa.String(10), type_=sa.String(9))
+    with op.batch_alter_table("financial_institutions_history") as batch_op:
+        batch_op.alter_column("tax_id", existing_type=sa.String(10), type_=sa.String(9))
diff --git a/src/entities/models/dao.py b/src/entities/models/dao.py
@@ -46,7 +46,7 @@ class FinancialInstitutionDao(AuditMixin, Base):
     domains: Mapped[List["FinancialInstitutionDomainDao"]] = relationship(
         "FinancialInstitutionDomainDao", back_populates="fi", lazy="selectin"
     )
-    tax_id: Mapped[str] = mapped_column(String(9), unique=True, nullable=True)
+    tax_id: Mapped[str] = mapped_column(String(10), unique=True, nullable=True)
     rssd_id: Mapped[int] = mapped_column(unique=True, nullable=True)
     primary_federal_regulator_id: Mapped[str] = mapped_column(ForeignKey("federal_regulator.id"), nullable=True)
     primary_federal_regulator: Mapped["FederalRegulatorDao"] = relationship(lazy="selectin")

diff --git a/src/entities/models/dto.py b/src/entities/models/dto.py
@@ -1,3 +1,5 @@
+import re
+
 from typing import Generic, List, Set, Sequence
 from pydantic import BaseModel, model_validator
 from typing import TypeVar
@@ -76,6 +78,16 @@ class FinancialInstitutionDto(FinancialInstitutionBase):
     top_holder_rssd_id: int | None = None
     version: int | None = None
 
+    @model_validator(mode="after")
+    def validate_fi(self) -> "FinancialInstitutionDto":
+        if self.tax_id:
+            match = re.match(r"^([0-9]{2}-[0-9]{7})", self.tax_id)
+            if not match:
+                raise ValueError(
+                    f"Invalid tax_id {self.tax_id}. FinancialInstitution tax_id must conform to XX-XXXXXXX pattern."
+                )
+        return self
+
     class Config:
         from_attributes = True
 

diff --git a/tests/api/conftest.py b/tests/api/conftest.py
@@ -64,7 +64,7 @@ def get_institutions_mock(mocker: MockerFixture, authed_user_mock: Mock) -> Mock
             lei="TESTBANK123",
             is_active=True,
             domains=[FinancialInstitutionDomainDao(domain="test.bank", lei="TESTBANK123")],
-            tax_id="123456789",
+            tax_id="12-3456789",
             rssd_id=1234,
             primary_federal_regulator_id="FRI1",
             primary_federal_regulator=FederalRegulatorDao(id="FRI1", name="FRI1"),

diff --git a/tests/api/routers/test_institutions_api.py b/tests/api/routers/test_institutions_api.py
@@ -37,14 +37,46 @@ def test_create_institution_unauthed(self, app_fixture: FastAPI, unauthed_user_m
         res = client.post("/v1/institutions/", json={"name": "testName", "lei": "testLei"})
         assert res.status_code == 403
 
+    def test_invalid_tax_id(self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock):
+        client = TestClient(app_fixture)
+        res = client.post(
+            "/v1/institutions/",
+            json={
+                "name": "testName",
+                "lei": "testLei",
+                "is_active": True,
+                "tax_id": "123456789",
+                "rssd_id": 12344,
+                "primary_federal_regulator_id": "FRI2",
+                "hmda_institution_type_id": "HIT2",
+                "sbl_institution_type_ids": ["SIT2"],
+                "hq_address_street_1": "Test Address Street 1",
+                "hq_address_street_2": "",
+                "hq_address_city": "Test City 1",
+                "hq_address_state_code": "VA",
+                "hq_address_zip": "00000",
+                "parent_lei": "PARENTTESTBANK123",
+                "parent_legal_name": "PARENT TEST BANK 123",
+                "parent_rssd_id": 12345,
+                "top_holder_lei": "TOPHOLDERLEI123",
+                "top_holder_legal_name": "TOP HOLDER LEI 123",
+                "top_holder_rssd_id": 123456,
+            },
+        )
+        assert (
+            res.json()["detail"][0]["msg"]
+            == "Value error, Invalid tax_id 123456789. FinancialInstitution tax_id must conform to XX-XXXXXXX pattern."
+        )
+        assert res.status_code == 422
+
     def test_create_institution_authed(self, mocker: MockerFixture, app_fixture: FastAPI, authed_user_mock: Mock):
         upsert_institution_mock = mocker.patch("entities.repos.institutions_repo.upsert_institution")
         upsert_institution_mock.return_value = FinancialInstitutionDao(
             name="testName",
             lei="testLei",
             is_active=True,
             domains=[FinancialInstitutionDomainDao(domain="test.bank", lei="TESTBANK123")],
-            tax_id="123456789",
+            tax_id="12-3456789",
             rssd_id=1234,
             primary_federal_regulator_id="FRI2",
             primary_federal_regulator=FederalRegulatorDao(id="FRI2", name="FRI2"),
@@ -73,7 +105,7 @@ def test_create_institution_authed(self, mocker: MockerFixture, app_fixture: Fas
                 "name": "testName",
                 "lei": "testLei",
                 "is_active": True,
-                "tax_id": "123456789",
+                "tax_id": "12-3456789",
                 "rssd_id": 12344,
                 "primary_federal_regulator_id": "FRI2",
                 "hmda_institution_type_id": "HIT2",
@@ -181,7 +213,7 @@ def test_create_institution_authed_no_permission(self, app_fixture: FastAPI, aut
                 "name": "testName",
                 "lei": "testLei",
                 "is_active": True,
-                "tax_id": "123456789",
+                "tax_id": "12-3456789",
                 "rssd_id": 12344,
                 "primary_federal_regulator_id": "FIR2",
                 "hmda_institution_type_id": "HIT2",
@@ -214,7 +246,7 @@ def test_get_institution_authed(self, mocker: MockerFixture, app_fixture: FastAP
             lei="TESTBANK123",
             is_active=True,
             domains=[FinancialInstitutionDomainDao(domain="test.bank", lei="TESTBANK123")],
-            tax_id="123456789",
+            tax_id="12-3456789",
             rssd_id=1234,
             primary_federal_regulator_id="FRI1",
             primary_federal_regulator=FederalRegulatorDao(id="FRI1", name="FRI1"),
@@ -311,7 +343,7 @@ def test_get_associated_institutions(
                 lei="TESTBANK123",
                 is_active=True,
                 domains=[FinancialInstitutionDomainDao(domain="test123.bank", lei="TESTBANK123")],
-                tax_id="123456789",
+                tax_id="12-3456789",
                 rssd_id=1234,
                 primary_federal_regulator_id="FRI1",
                 primary_federal_regulator=FederalRegulatorDao(id="FRI1", name="FRI1"),
@@ -336,7 +368,7 @@ def test_get_associated_institutions(
                 lei="TESTBANK234",
                 is_active=True,
                 domains=[FinancialInstitutionDomainDao(domain="test234.bank", lei="TESTBANK234")],
-                tax_id="123456879",
+                tax_id="12-3456879",
                 rssd_id=6879,
                 primary_federal_regulator_id="FRI1",
                 primary_federal_regulator=FederalRegulatorDao(id="FRI1", name="FRI1"),
@@ -437,7 +469,7 @@ def test_get_sbl_types(self, mocker: MockerFixture, app_fixture: FastAPI, authed
             lei="TESTBANK123",
             is_active=True,
             domains=[FinancialInstitutionDomainDao(domain="test.bank", lei="TESTBANK123")],
-            tax_id="123456789",
+            tax_id="12-3456789",
             rssd_id=1234,
             primary_federal_regulator_id="FRI1",
             primary_federal_regulator=FederalRegulatorDao(id="FRI1", name="FRI1"),

diff --git a/tests/entities/repos/test_institutions_repo.py b/tests/entities/repos/test_institutions_repo.py
@@ -55,7 +55,7 @@ async def setup(
                 lei="TESTBANK123",
                 is_active=True,
                 domains=[FinancialInstitutionDomainDao(domain="test.bank.1", lei="TESTBANK123")],
-                tax_id="123456789",
+                tax_id="12-3456789",
                 rssd_id=1234,
                 primary_federal_regulator_id="FRI1",
                 hmda_institution_type_id="HIT1",
@@ -78,7 +78,7 @@ async def setup(
                 lei="TESTBANK456",
                 is_active=True,
                 domains=[FinancialInstitutionDomainDao(domain="test.bank.2", lei="TESTBANK456")],
-                tax_id="987654321",
+                tax_id="98-7654321",
                 rssd_id=4321,
                 primary_federal_regulator_id="FRI2",
                 hmda_institution_type_id="HIT2",
@@ -101,7 +101,7 @@ async def setup(
                 lei="TESTSUBBANK456",
                 is_active=True,
                 domains=[FinancialInstitutionDomainDao(domain="sub.test.bank.2", lei="TESTSUBBANK456")],
-                tax_id="765432198",
+                tax_id="76-5432198",
                 rssd_id=2134,
                 primary_federal_regulator_id="FRI3",
                 hmda_institution_type_id="HIT3",
@@ -206,7 +206,7 @@ async def test_add_institution(self, transaction_session: AsyncSession):
                 name="New Bank 123",
                 lei="NEWBANK123",
                 is_active=True,
-                tax_id="654321987",
+                tax_id="65-4321987",
                 rssd_id=6543,
                 primary_federal_regulator_id="FRI3",
                 hmda_institution_type_id="HIT3",

diff --git a/tests/entities/test_listeners.py b/tests/entities/test_listeners.py
@@ -20,7 +20,7 @@ class TestListeners:
         name="Test Bank 123",
         lei="TESTBANK123",
         is_active=True,
-        tax_id="123456789",
+        tax_id="12-3456789",
         rssd_id=1234,
         primary_federal_regulator_id="FRI1",
         hmda_institution_type_id="HIT1",

diff --git a/tests/migrations/test_migrations.py b/tests/migrations/test_migrations.py
@@ -47,3 +47,14 @@ def test_fi_history_tables_8106d83ff594(alembic_runner: MigrationContext, alembi
     tables = inspector.get_table_names()
     assert "financial_institutions_history" in tables
     assert "fi_to_type_mapping_history" in tables
+
+
+def test_fi_history_tables_0e520c01fb42(alembic_runner: MigrationContext, alembic_engine: Engine):
+    alembic_runner.migrate_up_to("0e520c01fb42")
+    inspector = sqlalchemy.inspect(alembic_engine)
+
+    tax_column = [c for c in inspector.get_columns("financial_institutions") if c["name"] == "tax_id"][0]
+    assert tax_column["type"].length == 10
+
+    tax_column = [c for c in inspector.get_columns("financial_institutions_history") if c["name"] == "tax_id"][0]
+    assert tax_column["type"].length == 10