AccountDeletion raises exception on performing

[denschub]

Currently, AccountDeletions seem to be broken. I have several unperformed deletions, and when triggering them manually, they fail with:

NoMethodError: undefined method `disconnect' for nil:NilClass
	from app/models/aspect_membership.rb:16:in `block in <class:AspectMembership>'
	from lib/account_deleter.rb:100:in `delete_contacts_of_me'
	from lib/account_deleter.rb:32:in `block in perform!'
	from lib/account_deleter.rb:28:in `perform!'
	from app/models/account_deletion.rb:23:in `perform!'
	from (irb):6

Can't figure out when this regressed, but we should probably have a look at this...

[cmrd-senya]

Aren't there contacts in the DB with user_id nil?

[SuperTux88]

The error message looked familiar and I thought it is maybe something similar to what I fixed with #7538 for the user (it was only a cached contact membership but the aspect_membership was already removed). But this time it's for the person, not the user. It's also weird that it even happens when you run it twice (so it isn't a cache problem, because the second time the object creating the cache problem should be already deleted from the first run). so it's maybe a user that's deleted for some reason (user_id is NOT NULL, so it can't be nil, but maybe the user doesn't exist anymore)?

I currently don't have much time to analyze this, but I wanted to give hints that I already saw that error once with another topic related to account deletion.

[cmrd-senya]

Can't that be that we added user_id is NOT NULL validation to the DB schema only after these contact entries actually were created on the pod where this happens? This way it is possible that there are some contacts left with user_id=NULL.

[SuperTux88]

I think databases fail when you add a constraint that is invalid.

[cmrd-senya]

You're probably right.

I looked at the sequence of SQL queries produced by the account deletion and it looks like it can happen when the user entry is missing from the database (the local user which had in contacts the person who are trying to delete themselves).

Since we don't have dependent: :destroy on has_many :contacts relation for user such contacts can stay on the pod if somebody deleted the user entry manually.

Since we don't normally delete entries from users I don't think it makes much sense to add dependent: :destroy (somebody still can run deletion using an sql query). We could even add some validation against deleting user entries.

In order to solve the issue I think it's okay just to ignore the user.disconnect call when user is nil. The respective contact and membership will be deleted anyway.

I can PR this.

[denschub]

I checked, and all the invalid contact relations are made pre-2012, so likely that this is just a prehistorc relict of some sort. Preventing this case from failing is probably good enough, so ++ to a PR to skip disconnecting if there is no user.

[SuperTux88]

I removed the regression label, because this was a problem that was always there, but just with old accounts.

[SuperTux88]

Fixed by #7632