Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More reliable deletion of duplicate users #153

Open
dstrojny opened this issue Jun 4, 2015 · 5 comments
Open

More reliable deletion of duplicate users #153

dstrojny opened this issue Jun 4, 2015 · 5 comments

Comments

@dstrojny
Copy link
Contributor

dstrojny commented Jun 4, 2015

WordPress doesn't enforce unique constraints on the email field in the user table, which allows users with duplicate email addresses to occasionally be created under certain conditions. Memberful itself does enforce unique email addresses. Ideally, we'd wrap the whole WordPress user creation and Memberful mapping process in a transaction, but as WordPress doesn't require InnoDB, we can't rely on it being present.

Right now, we insert the user, then attempt to create a mapping record for them. If we can't create the mapping record because of a uniqueness constraint violation, we assume a race condition, and delete the WordPress user. Unfortunately, this deletion can sometimes fail, resulting in duplicate WordPress users.

We need to find a way to make sure the deletion is more bulletproof, so it never fails. One idea would be to record which members are created by Memberful, and the number of times they've signed in via Memberful. Then, if duplicate users are detected, the user who is not mapped can be deleted IF they 1) were created by Memberful and 2) have not had any successful sign ins. I'm sure there are also other ways this problem could be solved.

@robertuniqid
Copy link
Contributor

How about an enforced way to not allow wordpress duplicate users if memberful is active ?
And a regular daily check, via cron job, that checks if we have user duplicates, if we have user duplicates we'll display an admin warning asking them to press the "delete" button. ( This should be the case if they're added via the DB or from an API, because we won't allow them to create users with the same address in the admin, we will hook in the native user creation )

@dstrojny
Copy link
Contributor Author

dstrojny commented Jun 4, 2015

  • It's important the process is hidden. It shouldn't require manual admin action.
  • It's important it's bulletproof, as deleting users that aren't duplicates would be really bad.

Also, checking with a cron job feels like a cleanup operation vs. a prevention strategy. Is there any way we can get closer to the root of the problem and deal with it before it happens or immediately after?

@robertuniqid
Copy link
Contributor

Okay, ideally, what happens if someone with a user base of 10,000 installs memberful, and it's not an instalation on a fresh system, we can't afford to do a cleanup here, we'll ask them to do a backup before we do the cleanup ( BUT just in case we actually need to do it )

@dstrojny
Copy link
Contributor Author

dstrojny commented Jun 4, 2015

Right, so ideally we solve closer to the problem itself: when we create the new user. Dealing with it after the fact is messier and much more error prone. Can we be more aggressive and leak proof around our checking against our mapping when we create the new user? It seems like we're trying to do that, but sometimes the deletion fails.

@mrhead
Copy link
Contributor

mrhead commented Aug 10, 2017

This issue can happen not only when deletion fails. Actually I doubt that it is caused by failed deletion. We had an issue recently where user creation was successful but a 3rd party plugin caused a server error before we created the mapping so we ended up with incomplete user information. I presume that this happens more often than failed deletion.

However wrapping the user creation in a DB transaction would totally help in both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants