From e70a063398fc96ed83f054350a054b97948e92b3 Mon Sep 17 00:00:00 2001 From: James Gorrie Date: Tue, 3 Dec 2024 12:57:00 +0000 Subject: [PATCH] fix geography filter on family search (#257) * fix: fix geogrpahy filter on family search * fix: update new version number * fix: move test_search_geographies to table based test * fix: update project toml * fix: fix URL on test_search_geographies test --- app/repository/family.py | 26 +++++------- pyproject.toml | 2 +- tests/integration_tests/family/test_search.py | 21 ++++++++++ tests/integration_tests/family/test_update.py | 42 ++++++++++++------- tests/integration_tests/setup_db.py | 6 +-- 5 files changed, 62 insertions(+), 35 deletions(-) diff --git a/app/repository/family.py b/app/repository/family.py index ffe42476..cb36c279 100644 --- a/app/repository/family.py +++ b/app/repository/family.py @@ -22,7 +22,7 @@ from db_client.models.organisation.users import Organisation from sqlalchemy import Column, and_ from sqlalchemy import delete as db_delete -from sqlalchemy import desc, func, or_ +from sqlalchemy import desc, or_ from sqlalchemy import update as db_update from sqlalchemy.exc import NoResultFound, OperationalError from sqlalchemy.orm import Query, Session @@ -34,30 +34,24 @@ _LOGGER = logging.getLogger(__name__) -FamilyGeoMetaOrg = Tuple[Family, str, FamilyMetadata, Corpus, Organisation] +FamilyGeoMetaOrg = Tuple[Family, Geography, FamilyMetadata, Corpus, Organisation] def _get_query(db: Session) -> Query: # NOTE: SqlAlchemy will make a complete hash of query generation # if columns are used in the query() call. Therefore, entire # objects are returned. - geo_subquery = ( - db.query( - func.min(Geography.value).label("value"), - FamilyGeography.family_import_id, - ) - .join(FamilyGeography, FamilyGeography.geography_id == Geography.id) - .filter(FamilyGeography.family_import_id == Family.import_id) - .group_by(Geography.value, FamilyGeography.family_import_id) - ).subquery("geo_subquery") - return ( - db.query(Family, geo_subquery.c.value, FamilyMetadata, Corpus, Organisation) # type: ignore + db.query(Family, Geography, FamilyMetadata, Corpus, Organisation) + .join(FamilyGeography, FamilyGeography.family_import_id == Family.import_id) + .join( + Geography, + Geography.id == FamilyGeography.geography_id, + ) .join(FamilyMetadata, FamilyMetadata.family_import_id == Family.import_id) .join(FamilyCorpus, FamilyCorpus.family_import_id == Family.import_id) .join(Corpus, Corpus.import_id == FamilyCorpus.corpus_import_id) .join(Organisation, Corpus.organisation_id == Organisation.id) - .filter(geo_subquery.c.family_import_id == Family.import_id) # type: ignore ) @@ -65,14 +59,14 @@ def _family_to_dto( db: Session, fam_geo_meta_corp_org: FamilyGeoMetaOrg ) -> FamilyReadDTO: fam, geo_value, meta, corpus, org = fam_geo_meta_corp_org - metadata = cast(dict, meta.value) org = cast(str, org.name) + return FamilyReadDTO( import_id=str(fam.import_id), title=str(fam.title), summary=str(fam.description), - geography=geo_value, + geography=str(geo_value.value), category=str(fam.family_category), status=str(fam.family_status), metadata=metadata, diff --git a/pyproject.toml b/pyproject.toml index 0c652ab7..46d2372e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "admin_backend" -version = "2.17.17" +version = "2.17.18" description = "" authors = ["CPR-dev-team "] packages = [{ include = "app" }, { include = "tests" }] diff --git a/tests/integration_tests/family/test_search.py b/tests/integration_tests/family/test_search.py index c3e7d361..0eff7d47 100644 --- a/tests/integration_tests/family/test_search.py +++ b/tests/integration_tests/family/test_search.py @@ -7,6 +7,27 @@ from tests.integration_tests.setup_db import setup_db +def test_search_geographies( + client: TestClient, data_db: Session, superuser_header_token +): + setup_db(data_db) + + tests_cases = [ + ("afghanistan", 2), + ("zimbabwe", 1), + ] + + for country, expected_count in tests_cases: + response = client.get( + f"/api/v1/families/?geography={country}", + headers=superuser_header_token, + ) + assert response.status_code == status.HTTP_200_OK + data = response.json() + assert isinstance(data, list) + assert len(data) == expected_count + + def test_search_family_super( client: TestClient, data_db: Session, superuser_header_token ): diff --git a/tests/integration_tests/family/test_update.py b/tests/integration_tests/family/test_update.py index 958873e2..495a0889 100644 --- a/tests/integration_tests/family/test_update.py +++ b/tests/integration_tests/family/test_update.py @@ -232,7 +232,7 @@ def test_update_family_collections_to_one_that_does_not_exist( assert db_family.description == "" expected_geo = ( - data_db.query(Geography).filter(Geography.display_value == "Other").one() + data_db.query(Geography).filter(Geography.display_value == "Afghanistan").one() ) assert expected_geo.id in [g.id for g in db_family.geographies] assert db_family.family_category == "UNFCCC" @@ -279,7 +279,11 @@ def test_update_fails_family_when_user_org_different_to_family_org( assert db_family.description == "apple" assert db_family.family_category == "UNFCCC" - geo_id = data_db.query(Geography.id).filter(Geography.value == "Other").scalar() + geo_id = ( + data_db.query(Geography.id) + # TODO: PDCT-1406: Properly implement multi-geography support + .filter(Geography.value == db_family.geographies[0].value).scalar() + ) assert geo_id in [g.id for g in db_family.geographies] @@ -355,7 +359,11 @@ def test_update_family_when_collection_org_different_to_family_org( assert db_family.description == "" assert db_family.family_category == "UNFCCC" - geo_id = data_db.query(Geography.id).filter(Geography.value == "Other").scalar() + geo_id = ( + data_db.query(Geography.id) + # TODO: PDCT-1406: Properly implement multi-geography support + .filter(Geography.value == db_family.geographies[0].value).scalar() + ) assert geo_id in [g.id for g in db_family.geographies] @@ -375,28 +383,32 @@ def test_update_family_idempotent_when_ok( client: TestClient, data_db: Session, user_header_token ): setup_db(data_db) - family = EXPECTED_FAMILIES[1] + expected_family = EXPECTED_FAMILIES[1] response = client.put( - f"/api/v1/families/{family['import_id']}", - json=family, + f"/api/v1/families/{expected_family['import_id']}", + json=expected_family, headers=user_header_token, ) assert response.status_code == status.HTTP_200_OK data = response.json() - assert data["title"] == EXPECTED_FAMILIES[1]["title"] - assert data["summary"] == EXPECTED_FAMILIES[1]["summary"] - assert data["geography"] == EXPECTED_FAMILIES[1]["geography"] - assert data["category"] == EXPECTED_FAMILIES[1]["category"] + assert data["title"] == expected_family["title"] + assert data["summary"] == expected_family["summary"] + assert data["geography"] == expected_family["geography"] + assert data["category"] == expected_family["category"] db_family: Family = ( data_db.query(Family) - .filter(Family.import_id == EXPECTED_FAMILIES[1]["import_id"]) + .filter(Family.import_id == expected_family["import_id"]) .one() ) - assert db_family.title == EXPECTED_FAMILIES[1]["title"] - assert db_family.description == EXPECTED_FAMILIES[1]["summary"] - assert db_family.family_category == EXPECTED_FAMILIES[1]["category"] + assert db_family.title == expected_family["title"] + assert db_family.description == expected_family["summary"] + assert db_family.family_category == expected_family["category"] - geo_id = data_db.query(Geography.id).filter(Geography.value == "Other").scalar() + geo_id = ( + data_db.query(Geography.id) + .filter(Geography.value == expected_family["geography"]) + .scalar() + ) assert geo_id in [g.id for g in db_family.geographies] diff --git a/tests/integration_tests/setup_db.py b/tests/integration_tests/setup_db.py index ad112ee2..3c730507 100644 --- a/tests/integration_tests/setup_db.py +++ b/tests/integration_tests/setup_db.py @@ -31,7 +31,7 @@ "import_id": "A.0.0.1", "title": "apple", "summary": "", - "geography": "Other", + "geography": "AFG", "category": "UNFCCC", "status": "Created", "metadata": { @@ -57,7 +57,7 @@ "import_id": "A.0.0.2", "title": "apple orange banana", "summary": "apple", - "geography": "Other", + "geography": "ZWE", "category": "UNFCCC", "status": "Created", "metadata": { @@ -83,7 +83,7 @@ "import_id": "A.0.0.3", "title": "title", "summary": "orange peas", - "geography": "Other", + "geography": "AFG", "category": "UNFCCC", "status": "Created", "metadata": {"author": "CPR", "author_type": "Party"},