fix: add postgres triggers to remove deleted users from user_links #12117
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6f09151 to
24e767f
Compare
4113828 to
b7fd329
Compare
|
Why are we removing the ability to undelete a user? |
We never exposed this via the CLI or the API. It only exists in the DB query. However, un-deleting a user is more work than just the query that I removed. It can now collide with existing users, their Essentially undeleting a user can cause issues (if it can succeed at all), so we never supported it. And we should remove the query that will very likely fail in prod. I fear someone would just add a query param, use the existing query, merge it (because we do not test deleted users very much in tests). Then almost immediately it'll cause issues in an actual database. |
|
I know we never supported it, but it feels odd to keep the soft delete and a real delete. Can we change it to completely do one or the other? |
We do not support hard deletes at all on most resources. For auditing purposes, we need to keep these objects around. |
|
@Emyrk I'm confused why we should then on users. It seems like a fork in the road. It seems better to either leave this and not permit undeleting a user (e.g. actually blocking it), or making it work instead of allowing it for a single resource type. |
|
I do not totally follow. The product feature set is unchanged. We previously only supported soft deleting users from the API. I just made the DB query reflect this, so now you cannot un-delete users even from a DB query. So just trying to prevent developer error. The |
24e767f to
b0c3cbb
Compare
Changing emails on github fails if another deleted user exists with the same link.
56b8a0d to
a54b652
Compare
There was a problem hiding this comment.
Even though it is idiomatic with our data structure, I'm a bit nervous about having a term "soft delete" in one resource, even though I know we have it in others.
We've coined the term deleted as a soft-delete already. And since we called it UpdateDeleted I feel like it's a bit more confusing to call this SoftDelete now.
A few questions:
- Can we just fix this in the Go code? Do we need to have the triggers added? It seems harder to test, and more spread complexity.
- Is it alright if we continue to use
UpdateDeletedinstead ofSoftDelete? Does my rationale make sense in this context?
How about I return the name to |
|
That sounds good to me! |
What this does
un-deletea user. We never exposed this outside the db layer, doing this to make that decision something that takes more thought than just changing an api param.