-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: allow user deletion without deleting task references #1848
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,13 @@ | |
|
||
from typing import Annotated, List | ||
|
||
from fastapi import APIRouter, Depends | ||
from fastapi import APIRouter, Depends, Response | ||
from loguru import logger as log | ||
from psycopg import Connection | ||
|
||
from app.auth.roles import mapper, super_admin | ||
from app.db.database import db_conn | ||
from app.db.enums import HTTPStatus | ||
from app.db.enums import UserRole as UserRoleEnum | ||
from app.db.models import DbUser | ||
from app.users import user_schemas | ||
|
@@ -66,3 +68,18 @@ async def get_user_roles(current_user: Annotated[DbUser, Depends(mapper)]): | |
for role in UserRoleEnum: | ||
user_roles[role.name] = role.value | ||
return user_roles | ||
|
||
|
||
@router.delete("/{id}") | ||
async def delete_user_by_identifier( | ||
user: Annotated[DbUser, Depends(get_user)], | ||
current_user: Annotated[DbUser, Depends(super_admin)], | ||
db: Annotated[Connection, Depends(db_conn)], | ||
): | ||
"""Delete a single user.""" | ||
log.info( | ||
f"User {current_user.username} attempting deletion of user {user.username}" | ||
) | ||
await DbUser.delete(db, user.id) | ||
log.info(f"User {user.id} deleted successfully.") | ||
return Response(status_code=HTTPStatus.NO_CONTENT) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's best to return a response with a similar message as in the log instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good status code to use, but you are right we could include a message too π I think this was replicated from elsewhere where I did similar, so we could also update those deletion APIs to return a message too! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
-- ## Migration to: | ||
-- * Enable user data deletion | ||
|
||
-- Start a transaction | ||
BEGIN; | ||
|
||
-- Add column 'username' to 'task_events' table | ||
DO $$ | ||
BEGIN | ||
IF NOT EXISTS ( | ||
SELECT 1 | ||
FROM information_schema.columns | ||
WHERE table_name='task_events' AND column_name='username' | ||
) THEN | ||
ALTER TABLE public.task_events | ||
ADD COLUMN username VARCHAR; | ||
END IF; | ||
END $$; | ||
|
||
-- Make 'user_id' nullable in 'task_events' table | ||
DO $$ | ||
BEGIN | ||
IF EXISTS ( | ||
SELECT 1 | ||
FROM information_schema.columns | ||
WHERE table_name='task_events' AND column_name='user_id' AND is_nullable = 'NO' | ||
) THEN | ||
ALTER TABLE public.task_events | ||
ALTER COLUMN user_id DROP NOT NULL; | ||
END IF; | ||
END $$; | ||
|
||
-- Make 'author_id' nullable in 'projects' table | ||
DO $$ | ||
BEGIN | ||
IF EXISTS ( | ||
SELECT 1 | ||
FROM information_schema.columns | ||
WHERE table_name='projects' AND column_name='author_id' AND is_nullable = 'NO' | ||
) THEN | ||
ALTER TABLE public.projects | ||
ALTER COLUMN author_id DROP NOT NULL; | ||
END IF; | ||
END $$; | ||
|
||
-- Commit the transaction | ||
COMMIT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to a single SQL and await the execution at once? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that's not possible with psycopg! It's only one statement per cursor π
(SQLAlchemy hides this fact!).
It looks inefficient, but it still executes them all in the same session, so it's not bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have done a similar delete api for the organisation with a single sql.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Does it actually work? I would think it might error, but could be wrong.
I think the project deletion has it separated for this reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ignore me, I see what you mean now!
The org deletion is a single (chained) prepared statement, so that works fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it works, I haven't tested it yet. π€
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whereas the project deletion is multiple statements I think!
The approach of a single statement is actually quite nice - we can go with that if possible π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does!