Fix create and delete organisation #1867
Conversation
…factory to fetch db results
github-actions
bot
added
bug
|
@spwoodcock , here I haven't updated the status of NO_CONTENT, which would be the best to use? Use it as it is or update it to return a success message? |
| @@ -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.") | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
|
|
||
| if not deleted_org_id: | ||
| return Response( | ||
| org_deleted = await DbOrganisation.delete(db, org_user_dict.id) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
oh yeah actually, I tested and overdid it while refactoring without noticing. Thanks for pointing out.
Good question as it's a bit counter intuitive, but NO_CONTENT is a pretty common response code for delete endpoints (often recommended). So the code looks good as it is 👍 |
…and delete org managers on deletion of org
for more information, see https://pre-commit.ci
|
I have made some changes keeping |
| @@ -397,11 +397,6 @@ 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): | |||
There was a problem hiding this comment.
Is ignore_conflict used anywhere now?
I forgot why I added it in the first place.
Perhaps it can be removed.
There was a problem hiding this comment.
Yes, it is used in the init default HOTOSM organisation.
There was a problem hiding this comment.
It is still there in SQL to create an organization.
What type of PR is this? (check all applicable)
Related Issue
Describe this PR
This PR fixes the issue related to creating and deleting organizations awaiting the async functions that were missing. It also fixes the
get_my_organisationsusing proper parameter injection androw_factoryto define the db cursor to use Pydantic modelOrganisationOutfor the response.frontend
There is a payload mismatch with the body and query in frontend (yet to be fixed), @NSUWAL123
Screenshots
N/A
Alternative Approaches Considered
Did you attempt any other approaches that are not documented in code?
Review Guide
Notes for the reviewer. How to test this change?
Checklist before requesting a review
[optional] What gif best describes this PR or how it makes you feel?