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

Use a custom validation method to validate the uniqueness of name #1009

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

FunnyHector
Copy link

@FunnyHector FunnyHector commented Oct 28, 2020

Use a custom validation method to validate the uniqueness of name

The intention of this change is to address the the following deprecation seen on Rails 6:

DEPRECATION WARNING: Uniqueness validator will no longer enforce
case sensitive comparison in Rails 6.1. To continue case sensitive
comparison on the :name attribute in ActsAsTaggableOn::Tag model,
pass `case_sensitive: true` option explicitly to the uniqueness
validator.

Since there is the complexity of the config ActsAsTaggableOn.strict_case_match, we can't just explicitly set case_sensitive option as suggested in the deprecation warning.

The tests are quite blargh to deal with since the test database has the collation set to case-insensitive and has an uniqueness index, which is not realistic. To deal with the tests, I used a similar approach as in cb8d6e6. In cases where case sensitivity matters, we alter the collation on name column, run the test, then alter the collation back.

This PR tries to solve the same problem mentioned in #1005, but since the author of #1005 hasn't responded, I'm trying my luck here.

This change has also adopted @graaff's suggestion

@FunnyHector FunnyHector force-pushed the uniquess_validator branch 3 times, most recently from 6935055 to 370a55c Compare October 30, 2020 07:37
@FunnyHector FunnyHector changed the title Explicitly set the case_sensitive option for uniqueness validators Use a custom validation method to validate the uniqueness of name Oct 30, 2020
@FunnyHector
Copy link
Author

Any idea if this would get noticed soon?

1 similar comment
@FunnyHector
Copy link
Author

Any idea if this would get noticed soon?

@seuros
Copy link
Collaborator

seuros commented May 19, 2021

@FunnyHector , can you rebase this PR. It good to merge.

@FunnyHector
Copy link
Author

Hi @seuros , I have rebased this change onto the latest master. There was some conflict with this line in #1013.

Could you please have another look?

Sorry for late response. Somehow I must have missed the ping.

@alto
Copy link

alto commented Jan 26, 2022

@FunnyHector Do you mind merging master back into the PR again?

@seuros seuros self-assigned this Jan 26, 2022
@seuros
Copy link
Collaborator

seuros commented Jan 26, 2022

@alto the only issue left here is the non i18n error message.

@alto
Copy link

alto commented Jan 26, 2022

@seuros Yeah, the one in name_is_unique. I agree. 👍🏻 /cc @FunnyHector

@FunnyHector
Copy link
Author

Hiya, sorry I haven't really worked on any project that used i18n :p, that often slips my mind.

I'll try to make a fix to use i18n, and rebase this work. I'm a little busy this week but hopefully I can get it done by the end of this week. Will ping you when I'm done.

The intention of this change is to address the the following
deprecation seen on Rails 6:

    DEPRECATION WARNING: Uniqueness validator will no longer enforce
    case sensitive comparison in Rails 6.1. To continue case sensitive
    comparison on the :name attribute in ActsAsTaggableOn::Tag model,
    pass `case_sensitive: true` option explicitly to the uniqueness
    validator.

Since there is the complexity of the config
`ActsAsTaggableOn.strict_case_match`, we can't just explicitly set
case_sensitive option as suggested in the deprecation warning.

The tests are quite blargh to deal with since the test database has the
collation set to case-insensitive and has an uniqueness index, which is
not realistic. To deal with the tests, I used a similar approach as in
cb8d6e6. In cases where case
sensitivity matters, we alter the collation on `name` column, run the
test, then alter the collation back.
@FunnyHector
Copy link
Author

@seuros @alto I've rebased this work to resolve the conflicts. I've also added a new commit to use I18n error message. Please have another look. Sorry I haven't found time to look at this until now.

This is the first custom error message so a locale file is added. For
any non-en locale, this error message will raise a translation missing
error. Apps that uses this gem should add their own i18n translations.
@alto
Copy link

alto commented Jun 14, 2022

@seuros Hi, I just updated this pull request with your latest master changes (resolved conflicts). Do you mind considering a merge now? /cc @FunnyHector

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.

4 participants