If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add ability for admins to delete users

VERIFIED FIXED in 4.0.2

Status

addons.mozilla.org Graveyard
Administration
P1
normal
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: morgamic, Assigned: wenzel)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Need to add an admin ability to delete users from the admin interface.

Updated

9 years ago
Blocks: 432614

Updated

9 years ago
Target Milestone: --- → 4.0.2
(Reporter)

Updated

9 years ago
Duplicate of this bug: 432614
(Assignee)

Updated

9 years ago
Blocks: 454953

Updated

9 years ago
Priority: -- → P1
(Reporter)

Updated

9 years ago
Assignee: nobody → fwenzel
(Assignee)

Comment 2

9 years ago
Mike: are you sure, bug 432614 is supposed to be a dupe of this? One is asking for a user-facing feature to delete their own account, while this one is for an admin tool.
(Assignee)

Comment 3

9 years ago
When a user is deleted, what happens to their reviews? Are they deleted as well? What about their add-ons? Any opinions on that?

Comment 4

9 years ago
Here are three options to comment 3:
Option A: We delete the review.
Option B: We create a "(User Deleted)" or "Anonymous" that will own these deleted account reviews.
Option C: We annotate reviews with "{Nickname} (Account Deleted)" or similar

One of the main complains of why users want their accounts deleted is that their names are showing up on Google results. So, Option C although enticing might not appease those users. Option A removes potentially valuable data from AMO which I'm not keen on. Option B seems to balance the two extremes but I'm open to other options or ideas that folks have.

Comment 5

9 years ago
Fred, I just realized, I have a bunch of comments in bug 432614 comment 5 that are relevant to this bug. Hmmm, maybe that should have been dup'ed.

Comment 6

9 years ago
Ugh.. in my last comment, that should say "shouldn't have been dup'ed"
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> Ugh.. in my last comment, that should say "shouldn't have been dup'ed"

That is correct, I'll un-dupe it and make clear what each of the bugs is about.
Summary: Add ability to delete users → Add ability for admins to delete users
(Assignee)

Updated

9 years ago
No longer blocks: 432614
Depends on: 432614

Updated

9 years ago
Blocks: 449002

Comment 8

9 years ago
(In reply to comment #3)
> When a user is deleted, what happens to their reviews? Are they deleted as
> well? What about their add-ons? Any opinions on that?

With regards to the user facing feature (bug 432614) I think it makes the most sense to anonymize reviews/discussions/etc, however from an admin point of view I think both options should be implemented. An admin should have the option to either perform the equivalent of a user account deletion OR delete everything associated with a user account completely. (i.e. delete a spam account; see bug 449002)
(Assignee)

Updated

9 years ago
No longer blocks: 454953
(Assignee)

Comment 9

9 years ago
Created attachment 342532 [details] [diff] [review]
Admin-facing user deletion

Wil, will you be so nice again to review?

This admin-facing feature allows two "modes": Either associating the reviews with the anonymous user 0 before deletion, or deleting both the account and its associations.
Attachment #342532 - Flags: review?(clouserw)
(Assignee)

Comment 10

9 years ago
Created attachment 342534 [details]
SQL to add anonymous user 0

Here's the SQL that's needed.

Comment 11

9 years ago
(In reply to comment #9)
> This admin-facing feature allows two "modes": Either associating the reviews
> with the anonymous user 0 before deletion, or deleting both the account and its
> associations.

Why the user 0 method instead of the same method used in bug 432614? What's the benefit to having 2 different anonymizing systems?
(Assignee)

Comment 12

9 years ago
The benefit is that we need a method to delete spamming users.

Comment 13

9 years ago
Yes, that's what the full delete is for. (you wouldn't want to re-map spam to be anonymous) I'm talking about the first mode where reviews are anonymized. In the version that was eventually used in bug 432614 the accounts are wiped and with this method the reviews are re-mapped to user 0. The spam risk of the user 0 method isn't here, but I was thinking you'd just re-use the same functions that users use for deletion.
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> The spam risk of the user
> 0 method isn't here, but I was thinking you'd just re-use the same functions
> that users use for deletion.

Now that you say that, I am starting to wonder why I did that... *uhm* ... I will change that tomorrow.
(Assignee)

Updated

9 years ago
Attachment #342532 - Attachment is obsolete: true
Attachment #342532 - Flags: review?(clouserw)

Comment 15

9 years ago
Comment on attachment 342534 [details]
SQL to add anonymous user 0

Also, this attachment is just the patch again and not the SQL.  ;)
Attachment #342534 - Attachment is obsolete: true
(Assignee)

Comment 16

9 years ago
(In reply to comment #15)
> (From update of attachment 342534 [details])
> Also, this attachment is just the patch again and not the SQL.  ;)

Wow, I was being spacey tonight. *Shakes head*... time for a good night's sleep!
(Assignee)

Comment 17

9 years ago
Created attachment 342589 [details] [diff] [review]
Two features: anonymization, deletion

This patch should be better: The "anonymize" feature uses the same code as the user-facing piece. The deletion actually removes the DB row.

Thanks, Dave, for catching my unfortunate duplication of code earlier [here's a cookie for you: http://is.gd/3PW1 :)] This implementation should make much more sense than the other.
Attachment #342589 - Flags: review?(clouserw)
Is there some SQL that needs to be run here?  I'm getting "SQL Error: 1062: Duplicate entry '' for key 2" when anonymizing a user

Comment 19

9 years ago
(In reply to comment #18)
> Is there some SQL that needs to be run here?  I'm getting "SQL Error: 1062:
> Duplicate entry '' for key 2" when anonymizing a user

attachment 341269 [details] from bug 432614, where the function was added
(Assignee)

Comment 20

9 years ago
(In reply to comment #19)
> (In reply to comment #18)
> > Is there some SQL that needs to be run here?  I'm getting "SQL Error: 1062:
> > Duplicate entry '' for key 2" when anonymizing a user
> 
> attachment 341269 [details] from bug 432614, where the function was added

Correct! (And the fact that this was pushed live without that SQL makes it not work in prod at the moment, which makes me a sad panda...)
Comment on attachment 342589 [details] [diff] [review]
Two features: anonymization, deletion

- If a user has add-ons and someone tries to delete the account it just says there was an error but doesn't say what it is.

- Having to type your admin password to delete an account seems overkill.

Just my observations and opinions; patch works as advertised. r+
Attachment #342589 - Flags: review?(clouserw) → review+
(Assignee)

Comment 22

9 years ago
All right, gonna take away the admin pw question. As for existing add-ons, if that's not obvious to the admins, should I add a check?
I think a check would be good if you've got time
(Assignee)

Comment 24

9 years ago
Checked into r19021. I now warn when a user is an add-on author. I also removed the admin PW check, as the feature is an admin-level feature but not exceptionally dangerous.

Once this lands, we can fix bug 449002.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: push-needed
Resolution: --- → FIXED
Verified FIXED;

[1] Couldn't delete users with add-ons
[2] Successfully deleted users without add-ons
[3] Made an account anonymous, and it showed that user's account as "Deleted User."
Status: RESOLVED → VERIFIED
(Reporter)

Updated

9 years ago
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.