thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
499 stars 30 forks source link

Enforce unique emails on User #226

Closed pbrisbin closed 9 years ago

pbrisbin commented 9 years ago

Caveats:

Rationale:

Adding the DB constraint fails almost all of our tests. Our user factory actually does create users with unique emails, but authenticating with the dummy plugin always attempts to update it to the same address. Modifying that behavior will be both messy and unrelated to this validation, so I want to do it separately.

I thought it made sense to do this first, since it should prevent creating users with non-unique emails in production while I'm working on adding the more robust, DB-level constraint.

Once the constraint is in and the test behavior corrected, I'll change the update path to also handle uniqueness violations gracefully.

pbrisbin commented 9 years ago

Adjusting the tests to allow for unique emails was easier than I thought. #226 should be merged first, then I'll rebase this (and add code for the update path).

jferris commented 9 years ago

226 should be merged first, then I'll rebase this (and add code for the update path).

This is #226?

pbrisbin commented 9 years ago

Sorry, #227, where we're having the other discussion.

pbrisbin commented 9 years ago

Just realized: once the merge-on-existing-email logic is in place, this path would never be hit. Closing in favor of that, which I'll open soon.