Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix create and delete organisation #1867

Merged
merged 8 commits into from
Nov 15, 2024
6 changes: 2 additions & 4 deletions src/backend/app/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ignore_conflict used anywhere now?

I forgot why I added it in the first place.
Perhaps it can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is used in the init default HOTOSM organisation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still there in SQL to create an organization.

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)
Expand Down Expand Up @@ -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]]:
Expand Down
14 changes: 6 additions & 8 deletions src/backend/app/organisations/organisation_crud.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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

Expand All @@ -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()
2 changes: 2 additions & 0 deletions src/backend/app/organisations/organisation_deps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was moving this related to the 'my organisations' endpoint?

The logic is a bit duplicated now though, as you could probably just raise a HTTPException without needing the KeyError.

Is it possible to keep the KeyError in the model .one method though, and instead catch the error wherever you needed it? Using try/except blocks actually has very little cost in the latest versions of Python, so it's good to keep the KeyError at the lowest level, then catch exceptions higher up (if no organisation exists with that name/id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I got your point. But the reason why I removed it from the .one function is that if you see on the create function of the organisation , there is a logic to check if the organisation already exists or not to allow the creation of a new organisation where .one function is called, so if there isn't any organisation with the same name, it should have allowed creating new organisation but the .one function raises a KeyError as it won't find any db record which blocks .create function to create a new organisation.

Copy link
Member

@spwoodcock spwoodcock Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense πŸ‘

I'm not on my laptop to really dig into the logic.

But it work using the ignore_conflict param on DbOrganisation.create?

We just need to ideally check if the name already exists on creation, but don't need to get the org with .one. Not sure what the best solution is, so I'll let you decide what you think is best 😁

(just be sure that removing the keyerror doesn't break logic elsewhere)

Copy link
Collaborator Author

@Sujanadh Sujanadh Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to check if that name already exists, so we need to write SQL again to check it in the create function. Or, we might create a new function to check if the organisation exists or not and use it in other endpoints too. We already have the org_exists function in organisation_deps we can replace it with a new class function by defining it in DbOrganisation?

except KeyError as e:
raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail=str(e)) from e

Expand Down
36 changes: 19 additions & 17 deletions src/backend/app/organisations/organisation_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
APIRouter,
Depends,
File,
HTTPException,
Response,
UploadFile,
)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe org_user_dict actually has keys org and user so the org id is nested one level more

org_user_id.get("org").id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah actually, I tested and overdid it while refactoring without noticing. Thanks for pointing out.

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}")
Expand All @@ -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)
Expand Down