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

Passing :scope option to uniqueness validator of :email #4793

Closed
wants to merge 3 commits into from

Conversation

one-more-alex
Copy link

Uniqueness validator of :email must have :scope option setting to solve many difficulties like for:

Indexes for tokens should have WHERE ...token IS NOT NULL construction.
Else we are admitted to create only one user.
Unfortunatelly, in worse cases people just remove these indexes...
Initially faced with it on MS SQL server (not checked other adapters
        though).
Inspired by rails-sqlserver/activerecord-sqlserver-adapter#153

Probably need to add test case for generated migrations with tokens
add_index :<%= table_name %>, :reset_password_token, unique: true
# add_index :<%= table_name %>, :confirmation_token, unique: true
# add_index :<%= table_name %>, :unlock_token, unique: true
add_index :<%= table_name %>, :reset_password_token, unique: true, where: '([reset_password_token] IS NOT NULL)'
Copy link
Member

@tegon tegon Apr 4, 2018

Choose a reason for hiding this comment

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

Looks like these changes are from #4794, right?
I think we can remove them.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

@tegon
Copy link
Member

tegon commented Apr 4, 2018

@one-more-alex Can you add some tests for this feature?

@one-more-alex
Copy link
Author

I can add some tests, but I can't do t at near time. Maybe at weekend...
What do you think about some wider solution like following?

validates_uniqueness_of :email, {allow_blank: true, if: :will_save_change_to_email?}.merge(other_options_scope_case_etc)

To be able add arbitrary options for validator.

@tegon
Copy link
Member

tegon commented Apr 4, 2018

No problem, you can do when you're able to.

I think your suggestion is a good idea, but since we already have the email_regexp setting, I think it's ok to continue with this approach for now.
Maybe we can introduce the hash of options in a next major release.

@tegon
Copy link
Member

tegon commented Dec 27, 2018

@one-more-alex Just to be sure, are you still going to work on this or can I assign it to someone else?

@one-more-alex
Copy link
Author

@tegon Sorry for delay. Sure. I do not mind to assign to someone else.
I made an attempt to add tests, but not get deep into tests structure yet.
Also I not understood if it is possible to edit this PR commits somehow.

@tegon
Copy link
Member

tegon commented Jan 7, 2019

@one-more-alex No problem, I just wanted to know if you still wanted to work on this.

I made an attempt to add tests, but not get deep into tests structure yet.

If you need help, let me know. You can add a WIP commit here with your progress so far so I can help, or send your questions.

I not understood if it is possible to edit this PR commits somehow

You mean how to remove the commit from #4794?

@tegon
Copy link
Member

tegon commented Jan 28, 2019

I'm closing this pull request because it had no recent activity.
If you find that this can still be relevant, feel free to open a new pull request with a similar implementation.

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants