Feature/app 101 support creating multiple geographies in the backend (#…

…280)

* feat: update serializer with geographies property

* refactor: use the geographies property that we are now getting from the payload when creating geographies

* refactor: bulk add all the geo families

db.addall is more efficient and idiomatic

* WIP: test add integration test for multi geos

* test: update integration test for multi geos on create

* fix: make geography get ids from values more dynamic

returns the list of elements depending on whats passed to the mocked function

* tests: update unit tests for the create endpoint

the geo get id from value has been replaced with the gets ids, so updating tests accordingly

* test: update expected errors

* fix: accidentally duplicated the assertions in test

* test: fixes to integration tests on create endpoint

* chore: update pyproject

* refactor: add geographies to bulk import model

* chore: update package version

* test: add a bit more clarity to unit test for invalid geo id

---------

Co-authored-by: Osneil Drakes <osneildrakes@Osneils-MacBook-Pro.local>
Co-authored-by: Osneil Drakes <osneildrakes@MacBookPro.communityfibre.co.uk>

app/model/bulk_import.py

@@ -54,6 +54,7 @@ def to_family_create_dto(self, corpus_import_id: str) -> FamilyCreateDTO:
             title=self.title,
             summary=self.summary,
             geography=self.geographies,
+            geographies=self.geographies,
             category=self.category,
             metadata=self.metadata.model_dump(),
             collections=self.collections,

app/model/family.py

@@ -63,6 +63,7 @@ class FamilyCreateDTO(BaseModel):
     title: str
     summary: str
     geography: Union[str, list[str]]
+    geographies: list[str]
     category: str
     metadata: dict[str, list[str]]
     collections: list[str]

app/repository/family.py

@@ -468,9 +468,11 @@ def create(
         )
         db.add(new_family)
 
-        # TODO: PDCT-1406: Properly implement multi-geography support
-        for geo_id in geo_ids:
-            db.add(FamilyGeography(family_import_id=import_id, geography_id=geo_id))
+        family_geographies = [
+            FamilyGeography(family_import_id=import_id, geography_id=geo_id)
+            for geo_id in geo_ids
+        ]
+        db.add_all(family_geographies)
 
         # Add corpus - family link.
         db.add(

app/service/family.py

@@ -194,12 +194,7 @@ def create(
         db = db_session.get_db()
 
     # Validate geographies
-    geo_ids = []
-    if isinstance(family.geography, str):
-        geo_ids.append(geography.get_id(db, family.geography))
-    elif isinstance(family.geography, list):
-        for geo_id in family.geography:
-            geo_ids.append(geography.get_id(db, geo_id))
+    geo_ids = geography.get_ids(db, family.geographies)
 
     # Validate category
     category.validate(family.category)

tests/helpers/family.py

@@ -58,6 +58,7 @@ def create_family_create_dto(
     title: str = "title",
     summary: str = "summary",
     geography: str = "CHN",
+    geographies: list[str] = ["CHN"],
     category: str = FamilyCategory.LEGISLATIVE.value,
     metadata: Optional[dict] = None,
     collections: Optional[list[str]] = None,
@@ -78,6 +79,7 @@ def create_family_create_dto(
         title=title,
         summary=summary,
         geography=geography,
+        geographies=geographies,
         category=category,
         metadata=metadata,
         collections=collections,

tests/integration_tests/family/test_create.py

@@ -145,13 +145,16 @@ def test_create_family_when_invalid_geo(
         title="Title",
         summary="test test test",
     )
-    new_family.geography = "UK"
+    new_family.geographies = ["UK"]
     response = client.post(
         "/api/v1/families", json=new_family.model_dump(), headers=user_header_token
     )
     assert response.status_code == status.HTTP_400_BAD_REQUEST
     data = response.json()
-    assert data["detail"] == "The geography value UK is invalid!"
+    assert (
+        data["detail"]
+        == "One or more of the following geography values are invalid: UK"
+    )
 
 
 def test_create_family_when_invalid_category(
@@ -285,3 +288,35 @@ def test_create_family_when_org_mismatch(
         data["detail"]
         == "User 'cclw@cpr.org' is not authorised to perform operation on 'UNFCCC.corpus.i00000001.n0000'"
     )
+
+
+def test_create_endpoint_creates_family_with_multiple_geographies(
+    client: TestClient, data_db: Session, user_header_token
+):
+    setup_db(data_db)
+    new_family = create_family_create_dto(
+        title="Test Title",
+        summary="test test test",
+        geography="ALB",
+        geographies=["ALB", "BRB", "BHS"],
+    )
+    response = client.post(
+        "/api/v1/families", json=new_family.model_dump(), headers=user_header_token
+    )
+    assert response.status_code == status.HTTP_201_CREATED
+    expected_import_id = "CCLW.family.i00000002.n0000"
+    assert response.json() == expected_import_id
+    actual_family = (
+        data_db.query(Family).filter(Family.import_id == expected_import_id).one()
+    )
+
+    assert actual_family.title == "Test Title"
+
+    actual_geos = (
+        data_db.query(Geography)
+        .join(FamilyGeography, FamilyGeography.geography_id == Geography.id)
+        .filter(FamilyGeography.family_import_id == expected_import_id)
+        .all()
+    )
+    assert len(actual_geos) == 3
+    assert [geo.value for geo in actual_geos] == ["ALB", "BHS", "BRB"]

tests/integration_tests/family/test_update.py

@@ -496,15 +496,18 @@ def test_update_family__invalid_geo(
         title="Updated Title",
         summary="just a test",
     )
-    new_family.geography = "UK"
+    new_family.geographies = ["UK"]
     response = client.put(
         "/api/v1/families/A.0.0.3",
         json=new_family.model_dump(),
         headers=user_header_token,
     )
     assert response.status_code == status.HTTP_400_BAD_REQUEST
     data = response.json()
-    assert data["detail"] == "The geography value UK is invalid!"
+    assert (
+        data["detail"]
+        == "One or more of the following geography values are invalid: UK"
+    )
 
 
 def test_update_family_metadata_if_changed(

tests/mocks/repos/geography_repo.py

@@ -22,10 +22,10 @@ def mock_get_id_from_value(_, __) -> Optional[int]:
             return 1
         return None
 
-    def mock_get_ids_from_values(_, __) -> list[int]:
+    def mock_get_ids_from_values(_, values) -> list[int]:
         maybe_throw()
         if not geography_repo.error:
-            return [1, 2]
+            return list(range(1, len(values) + 1))
         return []
 
     geography_repo.error = False

tests/unit_tests/service/family/test_create_family_service.py

@@ -26,7 +26,7 @@ def test_create(
     family = family_service.create(new_family, admin_user_context)
     assert family is not None
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 1
@@ -54,7 +54,7 @@ def test_create_repo_fails(
     expected_msg = "bad family repo"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 1
@@ -81,7 +81,7 @@ def test_create_raises_when_category_invalid(
     expected_msg = "Invalid is not a valid FamilyCategory"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 0
     assert corpus_repo_mock.get_corpus_org_id.call_count == 0
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 0
@@ -107,7 +107,7 @@ def test_create_raises_when_missing_metadata(
     expected_msg = "Metadata validation failed: Missing metadata keys: {'size'}"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 1
@@ -136,7 +136,7 @@ def test_create_raises_when_missing_taxonomy(
     expected_msg = "No taxonomy found for corpus"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 1
@@ -166,7 +166,7 @@ def test_create_raises_when_collection_org_different_to_usr_org(
 
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 0
@@ -193,7 +193,7 @@ def test_create_raises_when_corpus_missing(
     expected_msg = "Corpus 'CCLW.corpus.i00000001.n0000' not found"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 0
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 0
@@ -221,7 +221,7 @@ def test_create_when_no_org_associated_with_entity(
     expected_msg = "No organisation associated with corpus CCLW.corpus.i00000001.n0000"
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 0
@@ -248,7 +248,7 @@ def test_create_raises_when_corpus_org_different_to_usr_org(
 
     assert e.value.message == expected_msg
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 0
@@ -272,10 +272,31 @@ def test_create_success_when_corpus_org_different_to_usr_org_super(
     family = family_service.create(new_family, super_user_context)
     assert family is not None
 
-    assert geography_repo_mock.get_id_from_value.call_count == 1
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
     assert corpus_repo_mock.verify_corpus_exists.call_count == 1
     assert corpus_repo_mock.get_corpus_org_id.call_count == 1
     assert db_client_metadata_mock.get_taxonomy_from_corpus.call_count == 1
     assert db_client_metadata_mock.get_entity_specific_taxonomy.call_count == 0
     assert collection_repo_mock.get_org_from_collection_id.call_count == 2
     assert family_repo_mock.create.call_count == 1
+
+
+def test_create_endpoint_raises_validation_error_when_validating_invalid_geographies(
+    family_repo_mock,
+    geography_repo_mock,
+    admin_user_context,
+):
+    new_family = create_family_create_dto(
+        geographies=["123"],
+        collections=["x.y.z.1", "x.y.z.2"],
+        metadata={"size": ["100"], "color": ["blue"]},
+    )
+    geography_repo_mock.error = True
+
+    with pytest.raises(ValidationError) as e:
+        family_service.create(new_family, admin_user_context)
+
+    expected_msg = "One or more of the following geography values are invalid: 123"
+    assert e.value.message == expected_msg
+    assert geography_repo_mock.get_ids_from_values.call_count == 1
+    assert family_repo_mock.create.call_count == 0