allow user deletion from admin #1262
Conversation
|
I'm thinking this will fail to delete any activated users, since it delegates to https://github.com/openstax/accounts/blob/main/app/routines/destroy_user.rb You also need to think what you want to do with all the records that have foreign keys to user, like external_id. It may be easier to just erase the user name, password and contact infos instead (and username but that's old)? |
|
I think that's what GitHub does anyway. The reason why you become "ghost" when you are deleted here. |
|
Actually they say they delete your record but associate some of your foreign keys with "ghost". |
|
Are you sure this is delegated to that routine? I'm not seeing the connection, but maybe it's some rails magic I don't understand. It looks like it's only using that routine on merge_unclaimed_user and transfer_authentications. Emptying out the identifying info seems reasonable to me, if you think that is a better way. It's what CS is doing now, so building an automated way to do that is what I'm trying to accomplish. |
|
FWIW, it did work locally on an activated student and instructor account, but I guess I should make sure it deleted all those foreign keys in the DB. |
|
The delegation is supposed to be handled by lev: https://github.com/lml/lev/blob/573a1ce6a54a313c27522245a5f965cda40e598b/README.md?plain=1#L285 |
|
If actually deleting the record: You have to go through all the
Without |
app/models/application_user.rb
Outdated
| @@ -1,7 +1,7 @@ | |||
| class ApplicationUser < ApplicationRecord | |||
| belongs_to :application, class_name: 'Doorkeeper::Application', | |||
| inverse_of: :application_users | |||
| belongs_to :user, inverse_of: :application_users | |||
There was a problem hiding this comment.
This should probably not be optional, since the DB still requires it.
| class Authentication < ApplicationRecord | ||
| belongs_to :user, inverse_of: :authentications | ||
| belongs_to :user, inverse_of: :authentications, optional: true |
There was a problem hiding this comment.
Oddly enough, optional is correct here because the column is nullable in the DB.
app/models/authentication.rb
Outdated
| @@ -20,6 +22,8 @@ def display_name | |||
| protected | |||
|
|
|||
| def check_not_last | |||
| return if user.is_deleted? | |||
There was a problem hiding this comment.
Given the above, you may want user.nil? || user.is_deleted? or something similar.
| user.save! | ||
|
|
||
| user.external_ids.destroy_all | ||
| user.external_uuids.destroy_all |
There was a problem hiding this comment.
I didn't even realize we had both. Maybe external_uuids can be deleted. I think they just stored the UUID that Tutor used? Eh for the purposes of this PR don't worry about it.
There was a problem hiding this comment.
I'm sure we could have an entire PR dedicated to remove tutor-specific features 😂
| @@ -0,0 +1,5 @@ | |||
| class AddDeleteFlagToUser < ActiveRecord::Migration[5.2] | |||
| def change | |||
| add_column :users, :is_deleted, :boolean | |||
There was a problem hiding this comment.
I would make this null: false, default: false just to prevent having 3 possible values. Adding a column with null: false is fine as long as default is also set.
There was a problem hiding this comment.
Actually ignore what I said because adding a column with a default rewrites the entire table and we have a ton of users.
To incorporate a better customer support process and have mechanisms to comply with privacy requirements.
This adds a button to the edit user page, which allows a user account to be deleted.