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

Rails 6 fixes #951

Conversation

damianlegawiec
Copy link
Contributor

No description provided.

@damianlegawiec damianlegawiec marked this pull request as ready for review May 2, 2019 14:06
@igorkasyanchuk
Copy link

with Rails 6 it doesn't work, see travis.ci logs.

@damianlegawiec
Copy link
Contributor Author

@igorkasyanchuk fixed!

@igorkasyanchuk
Copy link

great!

@dankmitchell
Copy link

Nice work!

@seuros
Copy link
Collaborator

seuros commented Jun 19, 2019

We need to poing out in the changelog before pushing a new version out.
I see that tag_list changed it format in rails 6

@behindthebid
Copy link

+1

@gkubaryk
Copy link

can we also update the validator in lib/acts_as_taggable_on/tag.rb as such:

-    validates_uniqueness_of :name, if: :validates_name_uniqueness?
+    validates_uniqueness_of :name, if: :validates_name_uniqueness?, case_sensitive: true

This will address the deprecation warning:
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.

@seuros
Copy link
Collaborator

seuros commented Jun 28, 2019

@gkubaryk i don't think tags should be case_sensitive.

@seuros seuros added the feature label Jun 28, 2019
@gkubaryk
Copy link

@seuros i have no strong opinion either way; the important thing (to me) is to set it to something so that the behavior is defined moving forward. the deprecation warning implied to me that it's silently been acting case sensitive and no longer will in the future. if you have a different reading of it or if we should take the opportunity to explicitly set it false, no argument here. :)

@taf2
Copy link

taf2 commented Jun 28, 2019

Pretty sure the default is case sensitive false

@seuros
Copy link
Collaborator

seuros commented Jun 28, 2019

@taf2 , When you use case_sensitive : false, the column is wrapped with lower() function in both side to remove sensitivity.
You dont need it if your field is already case insensitive eq citext.

spec/acts_as_taggable_on/dirty_spec.rb Outdated Show resolved Hide resolved
if Rails.version < "6.0.0.beta1"
set_attribute_was('#{tag_type}_list', #{tag_type}_list)
else
duplicated_mutations_from_database.change_to_attribute('#{tag_type}_list')
Copy link

Choose a reason for hiding this comment

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

Are you sure this is even working? I get the same result without it.

Copy link

@tbuehl tbuehl left a comment

Choose a reason for hiding this comment

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

The Rails version check needs to be fixed to work with the official Rails 6 release.

I made a PR for this: spark-solutions#2

lib/acts_as_taggable_on/taggable/core.rb Outdated Show resolved Hide resolved
@tbuehl
Copy link

tbuehl commented Aug 19, 2019

For anyone on Rails 6 and as long as this PR isn't updated, here is a working solution:

https://github.com/tbuehl/acts-as-taggable-on/tree/fix/rails-6-and-failing-specs

damianlegawiec and others added 3 commits August 19, 2019 16:30
use Gem::Version object to compare rails version instead of string
use Gem::Version object instead of string in specs
@@ -11,5 +11,5 @@ appraise 'activerecord-5.0' do
end

appraise 'activerecord-6.0' do
gem 'activerecord', "~> 6.0.0.beta1"
gem 'activerecord', "~> 6.0.0.rc1"

Choose a reason for hiding this comment

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

@damianlegawiec - thanks for the MR contribution!

Should this be bumped to 6.0.0 instead of 6.0.0.rc1? https://rubygems.org/gems/activerecord/versions/6.0.0

@@ -2,16 +2,18 @@

source "https://rubygems.org"

gem "activerecord", "~> 6.0.0.beta1"
gem "activerecord", "~> 6.0.0.rc1"

Choose a reason for hiding this comment

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

@damianlegawiec - dupe comment...

Should this be bumped to 6.0.0 instead of 6.0.0.rc1? https://rubygems.org/gems/activerecord/versions/6.0.0

@@ -18,6 +18,7 @@
end

it 'should show changes of freshly initialized dirty object' do
pending if Gem.loaded_specs["rails"].version >= Gem::Version.new("6.0.0.beta1")

Choose a reason for hiding this comment

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

@damianlegawiec - one last one

Should this be bumped to 6.0.0 instead of 6.0.0.beta1?

damianlegawiec pushed a commit to spark-solutions/spree that referenced this pull request Aug 27, 2019
Since acts-as-taggable-on still doesn't support Rails 6.0 we will drop
dependency on it and we'll release an extension later providing those
featues.

mbleigh/acts-as-taggable-on#951
@seuros seuros self-assigned this Aug 28, 2019
@edd1312
Copy link

edd1312 commented Oct 4, 2019

I changed the core.rb file to:

if self.class.preserve_tag_order? || parsed_new_list.sort != #{tag_type}_list.sort
                # set_attribute_was('#{tag_type}_list', #{tag_type}_list)
                attribute_will_change! "#{tag_type}_list"
                save
                write_attribute("#{tag_type}_list", parsed_new_list)
              end

and it works !!!

@seuros seuros closed this in 950c010 Oct 29, 2019
@wpliao1989
Copy link

can we also update the validator in lib/acts_as_taggable_on/tag.rb as such:

-    validates_uniqueness_of :name, if: :validates_name_uniqueness?
+    validates_uniqueness_of :name, if: :validates_name_uniqueness?, case_sensitive: true

This will address the deprecation warning:
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.

I'm wondering if there's an official fix for this issue? This is generating a lot of deprecation warnings.

rolandoalvarado pushed a commit to rolandoalvarado/acts-as-taggable-on that referenced this pull request Mar 5, 2020
@curtp
Copy link

curtp commented May 4, 2020

We're using gem version 6.5.0 and we're seeing a lot of the deprecation warnings about the uniqueness validator.

@fnguyen-tc
Copy link

fnguyen-tc commented Jul 27, 2020

Please change to validates_uniqueness_of :name, if: :validates_name_uniqueness?, case_sensitive: true

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

Successfully merging this pull request may close these issues.