search

mbleigh / acts-as-taggable-on

A tagging plugin for Rails applications that allows for custom tagging along dynamic contexts.
http://mbleigh.lighthouseapp.com/projects/10116-acts-as-taggable-on
MIT License
4.96k stars 1.2k forks source link

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

Open FunnyHector opened 3 years ago

FunnyHector commented 3 years ago

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 cb8d6e65cbb65a02a05f3deb28c12d5439d745af. 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 commented 3 years ago

Any idea if this would get noticed soon?

FunnyHector commented 3 years ago

Any idea if this would get noticed soon?

seuros commented 3 years ago

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

FunnyHector commented 3 years ago

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 commented 2 years ago

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

seuros commented 2 years ago

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

alto commented 2 years ago

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

FunnyHector commented 2 years ago

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.

FunnyHector commented 2 years ago

@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.

alto commented 2 years ago

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