Closed Bug 617963 Opened 14 years ago Closed 13 years ago

Users can no longer be deleted

Categories

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
5.12.6

People

(Reporter: jorgev, Assigned: davedash)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

This has been happening to me a lot recently, at least for a few weeks. Whenever I try to delete a user from the admin page, I get an error saying "Error deleting user". This is a big problem, specially when trying to delete spammer accounts.

Steps to reproduce:
1) Go to a user admin page, like https://addons.mozilla.org/en-US/firefox/admin/users/5562067
2) Select Delete user and remove all their reviews/ratings (must not be an add-on author)
3) Click the Delete User Account Now button.
mysql> delete from users where id=5562067;
ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`addons_remora/log_activity_user`, CONSTRAINT `user_id_refs_id_e987c199` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`))

That's the table for zamboni's UserLog.  We should move this page over to z so all the relationships are cleaned up.  Or set the right CASCADE in the db.

cc'ing wenzel since he's the cascade master.
Assignee: nobody → dd
You probably either want to do:

FOREIGN KEY `user_id` REFERENCES `users` (`id`) ON DELETE CASCADE

which will delete all related log activity when the user is deleted.

or you can do:

... ON DELETE SET NULL

which will set the foreign key field NULL when the corresponding user is removed. That'll keep the logs around, just not associate them with a user.
http://github.com/jbalogh/zamboni/commit/852b666


This should get applied to allizom, if you can verify that it works for you Jorge that'd be awesome.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][jorgev!]
I want the delete to cascade.  The users that get deleted are spam, and our app doesn't like null values for user_id.

Thanks!
Here's users who are in the table on preview: http://paste.pocoo.org/show/302785/
Actually wait til Krupa gives you a few ids to delete.
I created two new users:

5547700- User who has reported a review
5547701- User who has written a review and has created a collection

Another existing user you can delete is 5540769 who has submitted a search-tool.

Let me know whether this will suffice.
In that list 5547701 is in the log_activity_user table.  So try that one Jorge
Still seeing the error for user 5547701.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Wil,

This is in a migration - but it doesn't look like it was applied on allizom - can you look into it?
(In reply to comment #10)
> Wil,
> 
> This is in a migration - but it doesn't look like it was applied on allizom -
> can you look into it?

https://addons.allizom.org/media/updater.output.txt
(In reply to comment #11)
> (In reply to comment #10)
> > Wil,
> > 
> > This is in a migration - but it doesn't look like it was applied on allizom -
> > can you look into it?
> 
> https://addons.allizom.org/media/updater.output.txt

Yeah, looks like it's hating on the FKs.  Do you want me to run something on PAMO?
http://github.com/jbalogh/zamboni/commit/c55b277

Sorry about that, I made sure the db changes applied.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Still seeing the error at https://addons.allizom.org/en-US/firefox/admin/users/5547701
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Is there a way to see these errors from remora?
mysql> delete from users where id=5547701;ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`addons_reskin/collections`, CONSTRAINT `collections_ibfk_7` FOREIGN KEY (`author_id`) REFERENCES `users` (`id`))
http://github.com/jbalogh/zamboni/commit/21776d9
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Okay this is a clusterfuck... why are we getting these issues now, everything says it's got FK relations... how did this ever work?  I'm getting errors with reviews now.
We need this fixed before 5.12.6 -- there are too many spammers for us to manually delete all of their reviews and lock them out of their accounts.
Target Milestone: 5.12.6 → 5.12.5
mysql> SELECT TABLE_NAME FROM KEY_COLUMN_USAGE WHERE REFERENCED_TABLE_NAME='users'  
    -> ;
+--------------------------+
| TABLE_NAME               |
+--------------------------+
| addons_collections       | 
| addons_users             | 
| api_auth_tokens          | 
| approvals                | 
| collection_subscriptions | 
| collections              | 
| collections_users        | 
| collections_votes        | 
| editor_subscriptions     | 
| file_uploads             | 
| groups_users             | 
| howto_votes              | 
| hubrsskeys               | 
| log_activity             | 
| log_activity_user        | 
| reviewratings            | 
| reviews                  | 
| reviews_moderation_flags | 
| users_versioncomments    | 
| versioncomments          | 

20 tables have this... RESTRICT relationship, I'll move that to cascade for everything but addons_users since we special case addon authors.
No reason why QA can't verify this I guess,

5547700 is a safe user to delete - I think it's one of krupa's and it's in the log
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][jorgev!]
What commit did this and can we cherry pick it to next?
a7d0261 better migrations
7dc808a better migrations
80b5f16 better migrations
0ed37e6 better migrations
a74948c better migrations
faa6cea log_activity_user deletions should cascade from log_activity.
13acc9d log_activity_addon deletions should cascade from log_activity.
717156b Addons_collections needs cascades from collections.
4db7cfd uncomment out sql drop
4825e1e SQL cascading everywhere!
4fe2baa ActivityLog deletions should cascade from user.
71b958e Review deletions should cascade from user.
21776d9 bug 617963, Better cascading of collections table.
c55b277 Removing drop FKs from migration
c6c9d4b INT(11) not INTEGER
852b666 bug 617963, Better cascading of log_activity_user table.

After talking to Jeff, most of the issues were probably caused by log_activity since he says remora will delete the reviews by hand, so we could filter these down to:

migrations/103-cascade.sql
migrations/108-log-cascade.sql
migrations/111-log_activity-cascade.sql
migrations/112-log_activity-cascade.sql
migrations/90-logaddons.sql
migrations/95-index-activity.sql

I would just run these manually though rather than cherry picking them.
verified by deleting user- https://addons.allizom.org/en-US/firefox/user/5547706/
Status: RESOLVED → VERIFIED
I still can't delete this user: https://addons.mozilla.org/en-US/firefox/user/5518152/

When can this be fixed and pushed live?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Krupa says 5.12.5 didn't go out on Thursday like the AMO schedule says, and the next push is scheduled for January 14. This can't wait for then.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Looks like this should be reopened, then?
Yes.  5.12.5 didn't go out, but the parts of it that were supposed to fix this did.  -> reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 5.12.5 → 5.12.6
User deletion works fine in allizom. Fligtar confirmed this yesterday.
Attached file fix (obsolete) —
The problem:

mysql> delete from users where id=5583455;
ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`remora/reviews`, CONSTRAINT `reviews_ibfk_2` FOREIGN KEY (`user_id`) REFERENCES `users` (`id`))

I'm attaching what fixed it for me.  r? jbalogh because I think this was something you changed - something about the way django cascaded things?  It's been a while.

Edit: Had to steal some stuff from other migrations (in the attachment).  I grabbed what dd told me to in next's 98th migration, but it doesn't look like #98 ever ran (AMO is on 97), but it also doesn't include the reviews table so it wouldn't have fixed it anyway.  I actually don't see the reviews table in any of the migrations.

So, why didn't we just run all of these?
Attachment #499483 - Flags: review?(jbalogh)
Attachment #499483 - Flags: review?(dd)
Attached file fix
sigh, the real fix
Attachment #499483 - Attachment is obsolete: true
Attachment #499484 - Flags: review?(jbalogh)
Attachment #499484 - Flags: review?(dd)
Attachment #499483 - Flags: review?(jbalogh)
Attachment #499483 - Flags: review?(dd)
Comment on attachment 499484 [details]
fix

That looks fine to me.  Isn't your "Brand new code" from migrations/107?
Attachment #499484 - Flags: review?(jbalogh) → review+
Hmm, it appears to be, but that wasn't in comment 27 or migration 98.  /me shakes fist at this bug
This takes anywhere from 10-100 seconds to run for each change.  In my brief testing adding/dropping FKs doesn't read lock a table, but people wouldn't be able to write at all.  I suspect IT will want to do this in real downtime, which means next Tuesday.
Is there a bug filed to do this tomorrow night?
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
I can verify that it works.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: