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

feat: allow user deletion without deleting task references #1848

Merged
merged 1 commit into from
Nov 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions src/backend/app/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,41 @@ async def all(
)
return await cur.fetchall()

@classmethod
async def delete(cls, db: Connection, user_id: int) -> bool:
"""Delete a user and their related data."""
async with db.cursor() as cur:
Copy link
Contributor

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?

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member

@spwoodcock spwoodcock Nov 8, 2024

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

Copy link
Contributor

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. πŸ€”

Copy link
Member

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 πŸ‘

Copy link
Member

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. πŸ€”

I think it does!

await cur.execute(
"""
UPDATE task_events SET user_id = NULL WHERE user_id = %(user_id)s;
""",
{"user_id": user_id},
)
await cur.execute(
"""
UPDATE projects SET author_id = NULL WHERE author_id = %(user_id)s;
""",
{"user_id": user_id},
)
await cur.execute(
"""
DELETE FROM organisation_managers WHERE user_id = %(user_id)s;
""",
{"user_id": user_id},
)
await cur.execute(
"""
DELETE FROM user_roles WHERE user_id = %(user_id)s;
""",
{"user_id": user_id},
)
await cur.execute(
"""
DELETE FROM users WHERE id = %(user_id)s;
""",
{"user_id": user_id},
)

Anuj-Gupta4 marked this conversation as resolved.
Show resolved Hide resolved
@classmethod
async def create(
cls,
Expand Down Expand Up @@ -605,11 +640,11 @@ class DbTaskEvent(BaseModel):

project_id: Annotated[Optional[int], Field(gt=0)] = None
user_id: Annotated[Optional[int], Field(gt=0)] = None
username: Optional[str] = None
comment: Optional[str] = None
created_at: Optional[AwareDatetime] = None

# Computed
username: Optional[str] = None
profile_img: Optional[str] = None
# Computed via database trigger
state: Optional[MappingState] = None
Expand Down Expand Up @@ -662,13 +697,12 @@ async def all(

sql = f"""
SELECT
*,
u.username,
the.*,
u.profile_img
FROM
public.task_events
JOIN
users u ON u.id = task_events.user_id
public.task_events the
LEFT JOIN
users u ON u.id = the.user_id
WHERE {filters_joined}
ORDER BY created_at DESC;
"""
Expand Down Expand Up @@ -696,18 +730,19 @@ async def create(
INSERT INTO public.task_events (
event_id,
project_id,
username,
{columns}
)
VALUES (
gen_random_uuid(),
(SELECT project_id FROM tasks WHERE id = %(task_id)s),
(SELECT username FROM users WHERE id = %(user_id)s),
{value_placeholders}
)
RETURNING *
)
SELECT
inserted.*,
u.username,
u.profile_img
FROM inserted
JOIN users u ON u.id = inserted.user_id;
Expand Down
19 changes: 18 additions & 1 deletion src/backend/app/users/user_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 NO_CONTENT.

Copy link
Member

Choose a reason for hiding this comment

The 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!

47 changes: 47 additions & 0 deletions src/backend/migrations/010-delete-user.sql
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;
Loading