feat: allow user deletion without deleting task references #1848
Conversation
github-actions
bot
added
backend
|
So sorry @Anuj-Gupta4 , but this PR is based on staging, which is using SQLAlchemy 😅 We have since done a huge refactor on development to remove this and use psycopg, so this PR might need to be started from scratch with development as the base 😥 |
|
There is some really nice code here 😄 Could you possibly rebase with development (might be a lot of hassle considering the amount of change), or stash your changes somewhere, then reset the branch and reapply? Also feel free to make a new PR if it's easier, then I'll take a look! |
|
@spwoodcock I have reset my local branch and am working over development code. I will be overwriting the git history once its completed. |
Anuj-Gupta4
force-pushed
the
user-deletion
branch
from
a87d027
to
6b82eb3
Compare
Anuj-Gupta4
force-pushed
the
user-deletion
branch
from
6b82eb3
to
cefc600
Compare
Anuj-Gupta4
temporarily deployed
to
1848/merge
GitHub Actions
Inactive
Anuj-Gupta4
temporarily deployed
to
1848/merge
GitHub Actions
Inactive
Anuj-Gupta4
temporarily deployed
to
1848/merge
GitHub Actions
Inactive
Anuj-Gupta4
force-pushed
the
user-deletion
branch
from
cefc600
to
30978f0
Compare
Anuj-Gupta4
temporarily deployed
to
1848/merge
GitHub Actions
Inactive
Anuj-Gupta4
temporarily deployed
to
1848/merge
GitHub Actions
Inactive
| @classmethod | ||
| async def delete(cls, db: Connection, user_id: int) -> bool: | ||
| """Delete a user and their related data.""" | ||
| async with db.cursor() as cur: |
There was a problem hiding this comment.
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.
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.
I see you have done a similar delete api for the organisation with a single sql.
There was a problem hiding this comment.
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.
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.
I am not sure if it works, I haven't tested it yet. 🤔
There was a problem hiding this comment.
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.
I am not sure if it works, I haven't tested it yet. 🤔
I think it does!
|
@spwoodcock The tests are passing locally. I am not sure why it is failing here. |
| ) | ||
| 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.
I think it's best to return a response with a similar message as in the log instead of NO_CONTENT.
There was a problem hiding this comment.
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!
|
Oops forgot to wait for the other PR comments to be addressed before merge. It would be nice to possibly address them in another PR if time @Anuj-Gupta4 🙏 |
What type of PR is this? (check all applicable)
Related Issue
Fixes #1661
Describe this PR
On user deletion, task history will retain username from user
and rest of the user data will be deleted.
Checklist before requesting a review