From 9a9e44f9a1bfa5dc1b0312b963ec9497b1196b9c Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Mon, 11 Nov 2024 14:33:27 +0545 Subject: [PATCH 1/7] fix(backend): await create and delete organisation with proper response format --- src/backend/app/db/models.py | 6 ++-- .../app/organisations/organisation_deps.py | 2 ++ .../app/organisations/organisation_routes.py | 36 ++++++++++--------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/backend/app/db/models.py b/src/backend/app/db/models.py index df7b50e881..a2292335d8 100644 --- a/src/backend/app/db/models.py +++ b/src/backend/app/db/models.py @@ -352,9 +352,6 @@ async def one(cls, db: Connection, org_identifier: int | str) -> Self: ) db_org = await cur.fetchone() - if db_org is None: - raise KeyError(f"Organisation ({org_identifier}) not found.") - return db_org @classmethod @@ -397,7 +394,7 @@ async def create( # Set requesting user to the org owner org_in.created_by = user_id - if not ignore_conflict and cls.one(db, org_in.name): + if not ignore_conflict and await cls.one(db, org_in.name): msg = f"Organisation named ({org_in.name}) already exists!" log.warning(f"User ({user_id}) failed organisation creation: {msg}") raise HTTPException(status_code=HTTPStatus.CONFLICT, detail=msg) @@ -536,6 +533,7 @@ async def delete(cls, db: Connection, org_id: int) -> bool: settings.S3_BUCKET_NAME, f"/{org_id}/", ) + return True @classmethod async def unapproved(cls, db: Connection) -> Optional[list[Self]]: diff --git a/src/backend/app/organisations/organisation_deps.py b/src/backend/app/organisations/organisation_deps.py index cf3b81351e..a5b0e218be 100644 --- a/src/backend/app/organisations/organisation_deps.py +++ b/src/backend/app/organisations/organisation_deps.py @@ -58,6 +58,8 @@ async def get_organisation( pass db_org = await DbOrganisation.one(db, id) + if db_org is None: + raise KeyError(f"Organisation ({id}) not found.") except KeyError as e: raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail=str(e)) from e diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index a35e211c04..eba6d7edb4 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -23,6 +23,7 @@ APIRouter, Depends, File, + HTTPException, Response, UploadFile, ) @@ -99,7 +100,7 @@ async def create_organisation( Either a logo can be uploaded, or a link to the logo provided in the Organisation JSON ('logo': 'https://your.link.to.logo.png'). """ - return DbOrganisation.create(db, org_in, current_user.id, logo) + return await DbOrganisation.create(db, org_in, current_user.id, logo) @router.patch("/{org_id}/", response_model=OrganisationOut) @@ -120,19 +121,14 @@ async def delete_org( org_user_dict: Annotated[AuthUser, Depends(org_admin)], ): """Delete an organisation.""" - org = org_user_dict.get("org") - deleted_org_id = await DbOrganisation.delete(db, org.id) - - if not deleted_org_id: - return Response( + org_deleted = await DbOrganisation.delete(db, org_user_dict.id) + if not org_deleted: + log.error(f"Failed deleting org ({org_user_dict.name}).") + raise HTTPException( status_code=HTTPStatus.UNPROCESSABLE_ENTITY, - details=f"Failed deleting org {(org.name)}.", + detail=f"Failed deleting org ({org_user_dict.name}).", ) - - return Response( - status_code=HTTPStatus.NO_CONTENT, - details=f"Deleted org {(org.deleted_org_id)}.", - ) + return Response(status_code=HTTPStatus.NO_CONTENT) @router.delete("/unapproved/{org_id}") @@ -148,11 +144,17 @@ async def delete_unapproved_org( will also check if the organisation is approved and error if it's not. This is an ADMIN-only endpoint for deleting unapproved orgs. """ - await DbOrganisation.delete(db, org_id) - return Response( - status_code=HTTPStatus.NO_CONTENT, - detail=f"Deleted org ({org_id}).", - ) + org_deleted = await DbOrganisation.delete(db, org_id) + + if not org_deleted: + log.error(f"Failed deleting org ({org_id}).") + raise HTTPException( + status_code=HTTPStatus.UNPROCESSABLE_ENTITY, + detail=f"Failed deleting org ({org_id}).", + ) + + log.info(f"Successfully deleted org ({org_id}).") + return Response(status_code=HTTPStatus.NO_CONTENT) @router.post("/approve/", response_model=OrganisationOut) From da3f02ff8f202d5c8e0438179274e957bd319294 Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Mon, 11 Nov 2024 15:06:45 +0545 Subject: [PATCH 2/7] fix(backend)/my_organisations: use parameteruser_id properly and row_factory to fetch db results --- src/backend/app/organisations/organisation_crud.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/backend/app/organisations/organisation_crud.py b/src/backend/app/organisations/organisation_crud.py index 672238ec51..c94adf2c4f 100644 --- a/src/backend/app/organisations/organisation_crud.py +++ b/src/backend/app/organisations/organisation_crud.py @@ -19,12 +19,13 @@ from fastapi import UploadFile from psycopg import Connection +from psycopg.rows import class_row from app.auth.auth_schemas import AuthUser from app.config import settings from app.db.enums import MappingLevel, UserRole from app.db.models import DbOrganisation, DbOrganisationManagers, DbUser -from app.organisations.organisation_schemas import OrganisationIn +from app.organisations.organisation_schemas import OrganisationIn, OrganisationOut from app.users.user_schemas import UserIn @@ -97,14 +98,12 @@ async def get_my_organisations( Returns: list[dict]: A list of organisation objects to be serialised. """ - user_id = current_user.id - sql = """ SELECT DISTINCT org.* FROM organisations org JOIN organisation_managers managers ON managers.organisation_id = org.id - WHERE managers.user_id = :user_id + WHERE managers.user_id = %(user_id)s UNION @@ -114,7 +113,6 @@ async def get_my_organisations( ON project.organisation_id = org.id WHERE project.author_id = %(user_id)s; """ - async with db.cursor() as cur: - await cur.execute(sql, {"user_id": user_id}) - orgs = cur.fetchall() - return orgs + async with db.cursor(row_factory=class_row(OrganisationOut)) as cur: + await cur.execute(sql, {"user_id": current_user.id}) + return await cur.fetchall() From 9e5baa6333e57a0c30a58add889994db318c890f Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Mon, 11 Nov 2024 23:23:40 +0545 Subject: [PATCH 3/7] fix(backend): get org_id from db_org (org) from dependency org_admin and delete org managers on deletion of org --- src/backend/app/db/models.py | 6 +++++- src/backend/app/organisations/organisation_routes.py | 7 ++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/backend/app/db/models.py b/src/backend/app/db/models.py index a2292335d8..bd62346174 100644 --- a/src/backend/app/db/models.py +++ b/src/backend/app/db/models.py @@ -516,7 +516,11 @@ async def delete(cls, db: Connection, org_id: int) -> bool: DELETE FROM projects WHERE organisation_id = %(org_id)s RETURNING organisation_id - ), deleted_org AS ( + ), deleted_org_managers AS ( + DELETE FROM organisation_managers + WHERE organisation_id = %(org_id)s + RETURNING organisation_id + ),deleted_org AS ( DELETE FROM organisations WHERE id = %(org_id)s RETURNING id diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index eba6d7edb4..62c49eb4fe 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -121,12 +121,13 @@ async def delete_org( org_user_dict: Annotated[AuthUser, Depends(org_admin)], ): """Delete an organisation.""" - org_deleted = await DbOrganisation.delete(db, org_user_dict.id) + org = org_user_dict.get("org") + org_deleted = await DbOrganisation.delete(db, org.id) if not org_deleted: - log.error(f"Failed deleting org ({org_user_dict.name}).") + log.error(f"Failed deleting org ({org.name}).") raise HTTPException( status_code=HTTPStatus.UNPROCESSABLE_ENTITY, - detail=f"Failed deleting org ({org_user_dict.name}).", + detail=f"Failed deleting org ({org.name}).", ) return Response(status_code=HTTPStatus.NO_CONTENT) From 58765669193c696ec680c40ad4c2fae2b35d8887 Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Thu, 14 Nov 2024 12:25:10 +0545 Subject: [PATCH 4/7] refactor: create organisation with proper log and schema to generate form fields --- src/backend/app/db/models.py | 47 +++++++++++-------- .../app/organisations/organisation_deps.py | 2 - .../app/organisations/organisation_routes.py | 3 +- .../app/organisations/organisation_schemas.py | 40 ++++++++++++++-- 4 files changed, 66 insertions(+), 26 deletions(-) diff --git a/src/backend/app/db/models.py b/src/backend/app/db/models.py index bd62346174..907c8bff93 100644 --- a/src/backend/app/db/models.py +++ b/src/backend/app/db/models.py @@ -33,6 +33,7 @@ from loguru import logger as log from psycopg import Connection from psycopg.rows import class_row +from psycopg.errors import UniqueViolation from pydantic import AwareDatetime, BaseModel, Field, ValidationInfo from pydantic.functional_validators import field_validator @@ -352,6 +353,8 @@ async def one(cls, db: Connection, org_identifier: int | str) -> Self: ) db_org = await cur.fetchone() + if db_org is None: + raise KeyError(f"Organisation ({id}) not found.") return db_org @classmethod @@ -394,11 +397,6 @@ async def create( # Set requesting user to the org owner org_in.created_by = user_id - if not ignore_conflict and await cls.one(db, org_in.name): - msg = f"Organisation named ({org_in.name}) already exists!" - log.warning(f"User ({user_id}) failed organisation creation: {msg}") - raise HTTPException(status_code=HTTPStatus.CONFLICT, detail=msg) - model_dump = dump_and_check_model(org_in) columns = ", ".join(model_dump.keys()) value_placeholders = ", ".join(f"%({key})s" for key in model_dump.keys()) @@ -411,25 +409,34 @@ async def create( {'ON CONFLICT ("name") DO NOTHING' if ignore_conflict else ''} RETURNING *; """ + + try: + async with db.cursor(row_factory=class_row(cls)) as cur: + await cur.execute(sql, model_dump) + new_org = await cur.fetchone() + + if new_org and new_org.logo is None and new_logo: + from app.organisations.organisation_schemas import OrganisationUpdate + + logo_url = await cls.upload_logo(new_org.id, new_logo) + await cls.update(db, new_org.id, OrganisationUpdate(logo=logo_url)) + + return new_org + except UniqueViolation as e: + # Return conflict error when organisation name already exists + log.error(f"Organisation named ({org_in.name}) already exists!") + raise HTTPException( + status_code=HTTPStatus.CONFLICT, + detail=f"Organisation named ({org_in.name}) already exists!" + ) from e - async with db.cursor(row_factory=class_row(cls)) as cur: - await cur.execute(sql, model_dump) - new_org = await cur.fetchone() - - if new_org is None and not ignore_conflict: + except Exception as e: + # Log errors only for actual failures (e.g., DB errors) msg = f"Unknown SQL error for data: {model_dump}" - log.error(f"User ({user_id}) failed organisation creation: {msg}") + log.error(f"User ({user_id}) failed organisation creation: {e}") raise HTTPException( status_code=HTTPStatus.INTERNAL_SERVER_ERROR, detail=msg - ) - - if new_org and new_org.logo is None and new_logo: - from app.organisations.organisation_schemas import OrganisationUpdate - - logo_url = await cls.upload_logo(new_org.id, new_logo) - await cls.update(db, new_org.id, OrganisationUpdate(logo=logo_url)) - - return new_org + ) from e @classmethod async def upload_logo( diff --git a/src/backend/app/organisations/organisation_deps.py b/src/backend/app/organisations/organisation_deps.py index a5b0e218be..cf3b81351e 100644 --- a/src/backend/app/organisations/organisation_deps.py +++ b/src/backend/app/organisations/organisation_deps.py @@ -58,8 +58,6 @@ async def get_organisation( pass db_org = await DbOrganisation.one(db, id) - if db_org is None: - raise KeyError(f"Organisation ({id}) not found.") except KeyError as e: raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail=str(e)) from e diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index 62c49eb4fe..f7b8560d5a 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -42,6 +42,7 @@ OrganisationIn, OrganisationOut, OrganisationUpdate, + parse_organisation_input, ) router = APIRouter( @@ -92,7 +93,7 @@ async def create_organisation( db: Annotated[Connection, Depends(db_conn)], current_user: Annotated[AuthUser, Depends(login_required)], # Depends required below to allow logo upload - org_in: OrganisationIn = Depends(), + org_in: OrganisationIn = Depends(parse_organisation_input), logo: Optional[UploadFile] = File(None), ) -> OrganisationOut: """Create an organisation with the given details. diff --git a/src/backend/app/organisations/organisation_schemas.py b/src/backend/app/organisations/organisation_schemas.py index 1c32302e81..0f9d2f980e 100644 --- a/src/backend/app/organisations/organisation_schemas.py +++ b/src/backend/app/organisations/organisation_schemas.py @@ -19,14 +19,14 @@ from typing import Annotated, Optional, Self +from fastapi import Form from pydantic import BaseModel, Field from pydantic.functional_validators import model_validator -from app.central.central_schemas import ODKCentralIn -from app.db.enums import OrganisationType +from app.central.central_schemas import ODKCentralIn +from app.db.enums import OrganisationType, CommunityType from app.db.models import DbOrganisation, slugify - class OrganisationInBase(ODKCentralIn, DbOrganisation): """Base model for project insert / update (validators). @@ -51,6 +51,40 @@ class OrganisationIn(OrganisationInBase): # Name is mandatory name: str +def parse_organisation_input( + name: str = Form(...), + slug: Optional[str] = Form(None), + created_by: Optional[int] = Form(None), + community_type: CommunityType = Form(None), + type: OrganisationType = Form(None, alias="type"), + odk_central_url: Optional[str] = Form(None), + odk_central_user: Optional[str] = Form(None), + odk_central_password: Optional[str] = Form(None) +) -> OrganisationIn: + """ + Parse organisation input data from a FastAPI Form. + + The organisation fields are passed as keyword arguments. The + ODKCentralIn model is used to parse the ODK credential fields, and + the OrganisationIn model is used to parse the organisation fields. + + The parsed data is returned as an OrganisationIn instance, with the + ODKCentralIn fields merged in. + """ + odk_central_data = ODKCentralIn( + odk_central_url=odk_central_url, + odk_central_user=odk_central_user, + odk_central_password=odk_central_password, + ) + org_data = OrganisationIn( + name=name, + slug=slug, + created_by=created_by, + community_type=community_type, + type=type, + **odk_central_data.dict(exclude_unset=True) + ) + return org_data class OrganisationUpdate(OrganisationInBase): """Edit an org from user input.""" From 75e3e2588ed30cd0206e9544a4992d980234e68a Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Thu, 14 Nov 2024 12:26:03 +0545 Subject: [PATCH 5/7] fix: update organisation using same parser function --- .../app/organisations/organisation_routes.py | 9 ++++-- .../app/organisations/organisation_schemas.py | 32 ++++++++++--------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index f7b8560d5a..748cdbd5dc 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -101,6 +101,11 @@ async def create_organisation( Either a logo can be uploaded, or a link to the logo provided in the Organisation JSON ('logo': 'https://your.link.to.logo.png'). """ + if org_in.name is None: + raise HTTPException( + status_code=HTTPStatus.BAD_REQUEST, + detail="The `name` is required to create an organisation.", + ) return await DbOrganisation.create(db, org_in, current_user.id, logo) @@ -108,12 +113,12 @@ async def create_organisation( async def update_organisation( db: Annotated[Connection, Depends(db_conn)], org_user_dict: Annotated[AuthUser, Depends(org_admin)], - new_values: OrganisationUpdate = Depends(), + new_values: OrganisationUpdate = Depends(parse_organisation_input), logo: UploadFile = File(None), ): """Partial update for an existing organisation.""" org_id = org_user_dict.get("org").id - return DbOrganisation.update(db, org_id, new_values, logo) + return await DbOrganisation.update(db, org_id, new_values, logo) @router.delete("/{org_id}") diff --git a/src/backend/app/organisations/organisation_schemas.py b/src/backend/app/organisations/organisation_schemas.py index 0f9d2f980e..cc8f86581d 100644 --- a/src/backend/app/organisations/organisation_schemas.py +++ b/src/backend/app/organisations/organisation_schemas.py @@ -23,10 +23,11 @@ from pydantic import BaseModel, Field from pydantic.functional_validators import model_validator -from app.central.central_schemas import ODKCentralIn -from app.db.enums import OrganisationType, CommunityType +from app.central.central_schemas import ODKCentralIn +from app.db.enums import CommunityType, OrganisationType from app.db.models import DbOrganisation, slugify + class OrganisationInBase(ODKCentralIn, DbOrganisation): """Base model for project insert / update (validators). @@ -51,18 +52,25 @@ class OrganisationIn(OrganisationInBase): # Name is mandatory name: str + +class OrganisationUpdate(OrganisationInBase): + """Edit an org from user input.""" + + # Allow the name field to be omitted / not updated + name: Optional[str] = None + + def parse_organisation_input( - name: str = Form(...), + name: Optional[str] = Form(None), slug: Optional[str] = Form(None), created_by: Optional[int] = Form(None), community_type: CommunityType = Form(None), type: OrganisationType = Form(None, alias="type"), odk_central_url: Optional[str] = Form(None), odk_central_user: Optional[str] = Form(None), - odk_central_password: Optional[str] = Form(None) -) -> OrganisationIn: - """ - Parse organisation input data from a FastAPI Form. + odk_central_password: Optional[str] = Form(None), +) -> OrganisationUpdate: + """Parse organisation input data from a FastAPI Form. The organisation fields are passed as keyword arguments. The ODKCentralIn model is used to parse the ODK credential fields, and @@ -76,22 +84,16 @@ def parse_organisation_input( odk_central_user=odk_central_user, odk_central_password=odk_central_password, ) - org_data = OrganisationIn( + org_data = OrganisationUpdate( name=name, slug=slug, created_by=created_by, community_type=community_type, type=type, - **odk_central_data.dict(exclude_unset=True) + **odk_central_data.dict(exclude_unset=True), ) return org_data -class OrganisationUpdate(OrganisationInBase): - """Edit an org from user input.""" - - # Allow the name field to be omitted / not updated - name: Optional[str] = None - class OrganisationOut(BaseModel): """Organisation to display to user.""" From 94c5e02a9c23f65e992260b64a8ed8760b732a73 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 14 Nov 2024 06:41:55 +0000 Subject: [PATCH 6/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/backend/app/db/models.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backend/app/db/models.py b/src/backend/app/db/models.py index 907c8bff93..f28bc7cf27 100644 --- a/src/backend/app/db/models.py +++ b/src/backend/app/db/models.py @@ -32,8 +32,8 @@ from fastapi import HTTPException, UploadFile from loguru import logger as log from psycopg import Connection -from psycopg.rows import class_row from psycopg.errors import UniqueViolation +from psycopg.rows import class_row from pydantic import AwareDatetime, BaseModel, Field, ValidationInfo from pydantic.functional_validators import field_validator @@ -409,12 +409,12 @@ async def create( {'ON CONFLICT ("name") DO NOTHING' if ignore_conflict else ''} RETURNING *; """ - + try: async with db.cursor(row_factory=class_row(cls)) as cur: await cur.execute(sql, model_dump) new_org = await cur.fetchone() - + if new_org and new_org.logo is None and new_logo: from app.organisations.organisation_schemas import OrganisationUpdate @@ -427,7 +427,7 @@ async def create( log.error(f"Organisation named ({org_in.name}) already exists!") raise HTTPException( status_code=HTTPStatus.CONFLICT, - detail=f"Organisation named ({org_in.name}) already exists!" + detail=f"Organisation named ({org_in.name}) already exists!", ) from e except Exception as e: From 4fdc84ecf9ba124e3db604fb490d5153e21b7a32 Mon Sep 17 00:00:00 2001 From: Sujan Adhikari Date: Fri, 15 Nov 2024 10:33:13 +0545 Subject: [PATCH 7/7] fix: replace id by org_identifier param --- src/backend/app/db/models.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/backend/app/db/models.py b/src/backend/app/db/models.py index 907c8bff93..fdabbabea9 100644 --- a/src/backend/app/db/models.py +++ b/src/backend/app/db/models.py @@ -32,8 +32,8 @@ from fastapi import HTTPException, UploadFile from loguru import logger as log from psycopg import Connection -from psycopg.rows import class_row from psycopg.errors import UniqueViolation +from psycopg.rows import class_row from pydantic import AwareDatetime, BaseModel, Field, ValidationInfo from pydantic.functional_validators import field_validator @@ -354,7 +354,7 @@ async def one(cls, db: Connection, org_identifier: int | str) -> Self: db_org = await cur.fetchone() if db_org is None: - raise KeyError(f"Organisation ({id}) not found.") + raise KeyError(f"Organisation ({org_identifier}) not found.") return db_org @classmethod @@ -409,12 +409,12 @@ async def create( {'ON CONFLICT ("name") DO NOTHING' if ignore_conflict else ''} RETURNING *; """ - + try: async with db.cursor(row_factory=class_row(cls)) as cur: await cur.execute(sql, model_dump) new_org = await cur.fetchone() - + if new_org and new_org.logo is None and new_logo: from app.organisations.organisation_schemas import OrganisationUpdate @@ -427,7 +427,7 @@ async def create( log.error(f"Organisation named ({org_in.name}) already exists!") raise HTTPException( status_code=HTTPStatus.CONFLICT, - detail=f"Organisation named ({org_in.name}) already exists!" + detail=f"Organisation named ({org_in.name}) already exists!", ) from e except Exception as e: