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

Alters email column to citext type #51

Merged

Conversation

rmarronnier
Copy link
Contributor

@rmarronnier rmarronnier commented Dec 2, 2020

Fixes #9

Inspired by : http://shuber.io/case-insensitive-unique-constraints-in-postgres/

Do not merge this as it is :

  • I believe there must be a more elegant solution (Create a CIString type in Avram to automatically create a citext column type and use it in Authentic ?)
  • There is no spec written to test this
  • There is no error handling

Edit : https://www.postgresql.org/docs/current/citext.html

@jwoertink
Copy link
Member

We now have support for citext luckyframework/avram#608 This migration, however, is actually for the specs for Authentic. Generated apps would use this migration. So we'd need to update both spots as well as I'd like to see a spec on this shard to just ensure that having a citext email will actually work with different case emails.

Thanks for the suggestion on this!

@rmarronnier
Copy link
Contributor Author

Thanks for the migration pointer ! I'll take care of this in the next days.

Right off the bat, the two specs to be added I can think of are :

  • Ensure that a user can't sign up with John@example.com if an account with john@example.com already exists
  • Ensure that a user can login with John@example.com to the john@example.com account

@jwoertink
Copy link
Member

Yup! Those two specs would be great! Thanks

@rmarronnier
Copy link
Contributor Author

While trying to write the specs, I realized :

  • The existing Authentic specs don't rely on the User db table. The migration and the db are totally unused until now.
  • A FakeAuthenticatable object is used in the specs, mostly for password and session utilities testing.
  • The SignUp and SignIn actions and operations code are not in this repository (but in lucky-cli)
    We have to create a UserFactory to really test how the db handle the citext column. It's the easy part (already done but not pushed).

We have nevertheless some decisions to make :

  • Should we test the SignUp and SignIn operations ? I couldn't find on the lucky website the relevant documentation to test only operations in specs. Do you have an existing example ?
  • Should we test the SignUp and SignIn actions ? If so, only the API actions ? For the web actions, should we use lucky flow ?

@rmarronnier
Copy link
Contributor Author

Also, copy-pasting code from the lucky-cli repo to test seems to me it could bite us later if we forget to update both repo. Is there any way to move all Authentic specific code from lucky-cli to the Authentic repo and make lucky-cli rely on Authentic shard for code generation ?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well get this in since it'll now be default. Thanks for doing this!

@jwoertink jwoertink merged commit 110d429 into luckyframework:master Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emails should be case insensitive
2 participants