-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: development
Are you sure you want to change the base?
Conversation
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. |
a87d027
to
6b82eb3
Compare
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.
Very nice work! 🙌
@@ -95,7 +95,7 @@ def dump_and_check_model(db_model: BaseModel): | |||
class DbUserRole(BaseModel): | |||
"""Table user_roles.""" | |||
|
|||
user_id: int | |||
user_id: Optional[int] = None |
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.
Is there a use case for a UserRole entry with no user_id set?
As we won't know the user for which the role relates to
""", | ||
{"user_id": user_id}, | ||
) | ||
|
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.
Perfect 👍
@@ -520,7 +555,7 @@ class DbOrganisationManagers(BaseModel): | |||
"""Table organisation_managers.""" | |||
|
|||
organisation_id: int | |||
user_id: int | |||
user_id: Optional[int] = None |
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.
Same for this - I don't know when user_id would be empty for a record
WHERE table_name='task_events' AND column_name='username' | ||
) THEN | ||
ALTER TABLE public.task_events | ||
ADD COLUMN username VARCHAR(255); |
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.
No need to specify max 255 chars - it has no efficiency gain in Postgres. Just VARCHAR is good 👍
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