Rails 5.2 support, drop Rails 4.2 support. #887

Fodoj · 2018-02-24T15:56:52Z

Based on #872 with extra changes on top

I've also dropped Rails 4.2 support, as it requires lots of hacking around in the code - dirty implementation changed a lot since then.

This PR rolls back cc7a2c0 as it breaks all PostgreSQL builds (including master branch).

Upd 26.05.18: ready to merge!

thromera

Thanks for taking care of it :).

thromera · 2018-02-25T21:32:32Z

spec/acts_as_taggable_on/dirty_spec.rb

+    context 'when language_list changed by association' do
+      let(:tag) { ActsAsTaggableOn::Tag.new(name: 'one') }
+
+      before(:each) do


I would refactor this to

it 'flags language_list as changed' do expect(@taggable.changes).to be_empty @taggable.languages << tag expect(@taggable.language_list_changed?).to be_truthy end

Unless there is a specific guideline around it, I usually tend to not over-use the before-each, harder to read. Since here you have only one test in your context, I would drop it.

thromera · 2018-02-25T21:33:08Z

spec/acts_as_taggable_on/dirty_spec.rb

+        expect(@taggable.language_list_changed?).to be_truthy
+      end
+    end
+


Un-needed new line

thromera · 2018-02-25T21:34:26Z

lib/acts_as_taggable_on/taggable/core.rb

@@ -28,12 +28,16 @@ def initialize_acts_as_taggable_on_core
            has_many context_taggings, -> { includes(:tag).order(taggings_order).where(context: tags_type) },
                     as: :taggable,
                     class_name: 'ActsAsTaggableOn::Tagging',
-                     dependent: :destroy
+                     dependent: :destroy,
+                     after_add: :dirty_tag_list,


Shall we name the callback dirtify_tag_list instead? Same for the after_remove callback.

Fodoj · 2018-03-03T09:54:37Z

@Erowlin I've incorporated your valuable feedback :) I've also updated PR description, dropped Rails 4.2 support and fixed tests. They still somehow failed in same combinations, I will try re-triggering to see what happens.

criticerz · 2018-03-05T18:31:50Z

@Fodoj Thanks! Really saved our butts with this PR.

lunaru · 2018-03-09T00:18:11Z

I noticed that there's also another PR for a similar fix: #862

Any relationship between these two branches?

Anyway, I'd love to see this get merged!

Fodoj · 2018-03-09T05:17:49Z

@lunaru yes, I used code from 862 in this PR, forgot to mention it

PhilCoggins · 2018-03-13T19:42:47Z

@Fodoj Shouldn't the old version also be tracked?

Example:

i.label_list
=> ["foo", "bar", "low priority"]
i.label_list = "foo"
=> "foo"
i.changes
=> {"label_list"=>[nil, ["foo"]]}

I would expect the old array to display as the first value in i.changes array.

travisofthenorth · 2018-04-24T13:04:19Z

🎉 very happy to see this close to the finish line

cantino · 2018-04-27T22:39:47Z

Is this fully blocked on Rspec 3.8?

spaghetticode · 2018-05-15T13:39:21Z

👍 I'm having issues with rails 5.2 and the current gem release so I hope this gets merged soon

Fodoj · 2018-05-16T05:18:02Z

@cantino yep, this is the last dep, once there is a new rspec release, I will update it here and PR is good to merge

PhilCoggins · 2018-05-16T13:13:20Z

@Fodoj Any thoughts on my comment above? I think this is at least worth addressing before merging. The changeset always being nil -> [foo, bar] isn't correct.

The reason this isn't working is because the model's attribute is not being initialized with the current list.

cireficc · 2018-05-17T17:51:59Z

You, my man, are a fricking life saver! I'm very excited to see this merged!

Fodoj · 2018-05-18T15:26:59Z

@PhilCoggins which Rails version do you use? There are tests for this behavior and from what I can see tracking works fine, for example:

taggable = TaggableModel.create(tag_list: 'awesome, epic')
#=> #<TaggableModel:0x000056219f1b3200 id: 2, name: nil, type: nil>
taggable.tag_list = 'foo'
#=> "foo"
taggable changes
#=> {"tag_list"=>[["awesome", "epic"], ["foo"]]}

See also spec/acts_as_taggable_on/taggable/dirty_spec.rb

Basically, the gem already had pretty good test coverage around dirty changes, so I relied on them to tell me if behaviour is the same.

PhilCoggins · 2018-05-18T16:15:09Z

@Fodoj Yes, if the model is initialized with the label list, then it works properly. But what about when I instantiate an already existing record?

i = Issue.create(label_list: "foo, bar")
i.label_list = "baz"
i.changes
=> {"label_list"=>[["foo", "bar"], ["baz"]]} # Good
i = Issue.find(i.id)
i.label_list = "baz"
i.changes
=> {"label_list"=>[nil, ["baz"]]} # Bad

Using Rails 5.1.4, FWIW.

Fodoj · 2018-05-18T16:27:53Z

Got it now, let me fix it :)

Fodoj · 2018-05-18T20:19:28Z

@PhilCoggins should be fixed now, though let's wait for Travis to confirm. I've added a spec as a proof that it's fixed: 7b4911a.

PhilCoggins · 2018-05-18T20:53:35Z

@Fodoj I just tested on my end, works great! Thank you for addressing this 💯 .

brchristian · 2018-05-26T15:39:47Z

Hi @Fodoj, it looks to me like all of the Rspec commits since 3.7 are just edits to the README. Are you sure we can’t just use Rspec 3.7 instead of the master branch?

Then we wouldn’t have to wait to merge. :)

Fodoj · 2018-06-04T15:11:09Z

@seuros shot myself in a leg, but now it's fine, single beautiful commit with all the changes. Also added changes you've requested.

seuros · 2018-06-04T20:39:16Z

@Fodoj Thank you. Do you know why Postgresql build started to fail after the rebase ?

…n warning

…sses mbleigh#869" This reverts commit ac585c8.

…e; addresses mbleigh#869"" This reverts commit bc7ec0d.

Fodoj · 2018-06-05T07:07:35Z

@seuros it started to fail because master branch is failing https://github.com/mbleigh/acts-as-taggable-on/commits/master

This reverts commit cc7a2c0.

Fodoj · 2018-06-05T07:25:17Z

@seuros I rolled back this commit, which started breaking builds 372741a.

Perhaps @rsl will have time to re-do it so that CI doesn't fail. I do not have time to dig deep into this escaping fix and really I think fixing is out of scope of this PR.

thromera

Hey, thanks again for your work!

All minor comments, except one on the way we format the dirty tags, I think it's a breaking change and we should bump the gem to a major version just for that specific change.

I would like someone very familiar with the code base to review this as well.

Happy to discuss it if you need.

Thanks for your work!

thromera · 2018-06-05T07:28:08Z

Appraisals

@@ -1,11 +1,11 @@
+appraise 'activerecord-5.2' do
+  gem 'activerecord', '5.2.0'


Why not using a version like ~> 5.2.0?

Also, why specifically 5.2 and not 5.0? In the Gemspec you specified 5.0, I suggest we stick with that version.

I don't get that. This is PR to add support for 5.2, why would I specify 5.0 in 5.2 appraisal?

Well, we're dropping support for 4.2, it does not mean that we drop support for 5.0 I guess, correct?

Correct, that's why Appraisal file has definitions for 5.0, 5.1 and now for 5.2

gem 'activerecord', '~> 5.2.0' will install the latest minor version of the 5.2 serie

thromera · 2018-06-05T07:29:33Z

gemfiles/activerecord_5.2.gemfile

+
+source "https://rubygems.org"
+
+gem "activerecord", "5.2.0"


Same comment, why not sticking with 5.0?

thromera · 2018-06-05T07:30:09Z

lib/acts_as_taggable_on/taggable/core.rb

          end

          taggable_mixin.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+


No need for this new line!

thromera · 2018-06-05T07:31:25Z

lib/acts_as_taggable_on/taggable/core.rb

              set_tag_list_on('#{tags_type}', new_tags)
            end

            def all_#{tags_type}_list
              all_tags_list_on('#{tags_type}')
            end
+
+            private
+            def dirtify_tag_list tagging


You should add parenthesis for the parameters, it seems to be what's done in this project.

def dirtify_tag_list(tagging) ... end

thromera · 2018-06-05T07:31:57Z

lib/acts_as_taggable_on/taggable/core.rb

@@ -315,3 +316,4 @@ def find_or_create_tags_from_list_with_context(tag_list, _context)
    end
  end
 end
+


This one was fine actually, It's good to have a blank line at the end of files :). Sorry, I should have been more specific than a 👍
https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-file

thromera · 2018-06-05T07:33:00Z

lib/acts_as_taggable_on/taggable/tagged_with_query/query_base.rb

@@ -48,7 +48,7 @@ def tags_match_type

    def escaped_tag(tag)
      tag = tag.downcase unless ActsAsTaggableOn.strict_case_match
-      ActsAsTaggableOn::Utils.escape_like(tag)
+      tag.gsub(/[!%_]/) { |x| '!' + x }


This one should be crazily spec'ed :).

It's just a rollback of cc7a2c0, nothing I wrote for this PR. That commit breaks builds in master branch.

we can use ActsAsTaggableOn::Utils.escape_like(tag)

Not to repeat myself, but this change is unrelated to the scope of this PR, as it barely rollbacks commit that broke the build here and in master branch

thromera · 2018-06-05T07:35:27Z

spec/acts_as_taggable_on/dirty_spec.rb

@@ -14,19 +14,27 @@
      end

      it 'should show changes of dirty object' do
-        expect(@taggable.changes).to eq({'tag_list' => ['awesome, epic', ['one']]})
+        expect(@taggable.changes).to eq({'tag_list' => [['awesome', 'epic'], ['one']]})


⚠️

This is a breaking change.

Either rollback your change or we have to communicate crazy about that. Our own internal code depends on the previous way it was formatted, and I bet a bunch of projects have the same.

@Erowlin this PR is tagged for release 6.0.0, which is supposed to have breaking changes, no?

Then I guess we're fine!

I think this is a major breaking change that we should avoid.
People expect to have tags in the root of the array not inside another array.
Can we try to avoid this ?

I can not promise I will have any time to invest into it in next days/weeks. Even though new behaviour seems much more logical to me, because previously it was a change from string to array, and now it's change from array to array ( no akward change of data type).

Fodoj · 2018-06-05T07:43:09Z

I pushed more fixes. I actually assumed that after 3.5 months someone familiar with the code base already reviewed it, but seems like it's not the case?..

thromera · 2018-06-05T07:45:20Z

I am not very familiar with the acts-as-taggable codebase unfortunately.

Fodoj · 2018-06-05T07:46:19Z

I assume as @seuros has permissions to merge to master, he is familiar. Let's wait for his final go and merge then.

Added new line back also.

ebihara99999

Thanks for the PR. I met with the error and it's very helpful. I'd appreciate it if you could answer.

ebihara99999 · 2018-06-05T09:46:56Z

lib/acts_as_taggable_on/taggable/core.rb


            has_many context_tags, -> { order(taggings_order) },
                     class_name: 'ActsAsTaggableOn::Tag',
                     through: context_taggings,
                     source: :tag
+
+            attribute "#{tags_type.singularize}_list".to_sym, ActiveModel::Type::Value.new


This line makes taggable_obj.attribute_names changed. Specs failed in the following case, using json.extract! @item, *@item.attribute_names as response.

I saw you comment this PR will be released as the version 6.0.0 and allow breaking changes, but the impact of this change, if you don't expect, is very big.

What do you think about this case?

Not sure I understand the issue. Could you provide a failing spec?

Though I think if it's 6.0.0 release, then breaking changes are totally ok, just repo owner has to update readme before reasling this new version.

Failing is request spec with the rspec-json_matcher gem. I copy and modify some from the real codes and output of running rspec:

# request spec it { is_expected.to be_json_as [product: expected_product_response] }

# output expected to match: { :product => { :id => Integer < Numeric, :user_id => Integer < Numeric, :product_id => Integer < Numeric, :created_at => String < Object, :updated_at => String < Object, } } actual: { :product => { "id" => 2, "user_id" => 11, "product_id" => 216, "tag_list" => [] # This is the difference "created_at" => "2018-06-05T17:36:33+09:00", "updated_at" => "2018-06-05T17:36:33+09:00" } } reason: product "tag_list"

The case mentioned above is with the rspec-json_matcher gem, but if not using the rspec-json_matcher gem, the test suites testing api response would all fail because of the change.

In my opinion this change is harmful #1064

seuros · 2018-06-05T11:20:15Z

Appraisals

@@ -1,11 +1,11 @@
+appraise 'activerecord-5.2' do
+  gem 'activerecord', '5.2.0'


gem 'activerecord', '~> 5.2.0' will install the latest minor version of the 5.2 serie

seuros · 2018-06-05T11:21:05Z

acts-as-taggable-on.gemspec

@@ -22,7 +22,7 @@ Gem::Specification.new do |gem|
    gem.post_install_message = File.read('UPGRADING.md')
  end

-  gem.add_runtime_dependency 'activerecord', ['>= 4.2.8']
+  gem.add_runtime_dependency 'activerecord', ['>= 5.0']


gem.add_runtime_dependency 'activerecord', ['~> 5.0.0'] To avoid install in 6.*

seuros · 2018-06-05T11:21:48Z

lib/acts_as_taggable_on/taggable/collection.rb

@@ -174,7 +174,7 @@ module CalculationMethods
      # See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L38
      def count(column_name = :all, options = {})


i think the whole method can be removed now.

I tried - some of tests fail, especially sqlite ones. Let's not risk it this time :)

Onikoroshi · 2018-06-12T22:54:43Z

Any idea when this will be available as a version? I'm still only able to get version 5.0.0 which doesn't seem to include this fix ...

rsl · 2018-06-15T22:02:01Z

@Fodoj i'm trying to work on "fixing" the PR that supposedly breaks the build. looking at the Travis report: https://travis-ci.org/mbleigh/acts-as-taggable-on/jobs/339031800#L657 it seems to be a gem/config issue that i cannot replicate nor see how my changes could possibly have affected.

Specified 'postgresql' for database adapter, but the gem is not loaded. Add gem 'pg' to your Gemfile (and ensure its version is at the minimum required by ActiveRecord).

i cannot get travis to retry my fork because the PR was closed.

Fodoj mentioned this pull request Feb 24, 2018

rails 5.2 compability #872

Closed

Fodoj changed the title ~~Rails 5.2 compatibility~~ WIP: Rails 5.2 compatibility Feb 24, 2018

Fodoj changed the title ~~WIP: Rails 5.2 compatibility~~ Rails 5.2 compatibility Feb 24, 2018

Fodoj mentioned this pull request Feb 24, 2018

[Rails 5.1] Fix dirty.rb to use the new API. *_changed?, *_was... #836

Closed

thromera reviewed Feb 25, 2018

View reviewed changes

Fodoj force-pushed the rails-5.2 branch 2 times, most recently from 37d5f77 to 2bcf065 Compare March 3, 2018 09:02

Fodoj changed the title ~~Rails 5.2 compatibility~~ Rails 5.2 support, drop Rails 4.2 support. Mar 3, 2018

seuros added this to the 6.0.0 milestone Apr 20, 2018

aalbagarcia mentioned this pull request Apr 20, 2018

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "max(taggings.created_at)" #892

Closed

jrdnbwmn mentioned this pull request Apr 25, 2018

Set up categories jrdnbwmn/paperclip#3

Closed

9 tasks

aeschylus mentioned this pull request Apr 26, 2018

Update RIIIF to 2.0 projectblacklight/spotlight#1986

Merged

Fodoj force-pushed the rails-5.2 branch from aac3250 to eee01af Compare May 18, 2018 20:47

Fodoj force-pushed the rails-5.2 branch from 12b0a15 to 8b6be6d Compare June 4, 2018 15:02

Rails 5.2 support; Drop Rails 4.2 support

17eeacd

Fodoj force-pushed the rails-5.2 branch from 6b433cc to 17eeacd Compare June 4, 2018 15:10

Kirill Shirinkin added 3 commits June 5, 2018 08:38

Wrap SQL with Arel.sl in any tags for safety and to remove deprecatio…

04e6758

…n warning

Revert "matches_attribute was not being used in tag_match_type; addre…

bc7ec0d

…sses mbleigh#869" This reverts commit ac585c8.

Revert "Revert "matches_attribute was not being used in tag_match_typ…

e766f9f

…e; addresses mbleigh#869"" This reverts commit bc7ec0d.

Kirill Shirinkin added 2 commits June 5, 2018 09:08

Remove checks for AR5, as AR4 is not supported anymore

edc6180

Revert "improve postgres escape sequence"

04c6658

This reverts commit cc7a2c0.

Fodoj force-pushed the rails-5.2 branch from 372741a to 04c6658 Compare June 5, 2018 07:24

thromera suggested changes Jun 5, 2018

View reviewed changes

Lint fixes as in review

6747109

Fodoj force-pushed the rails-5.2 branch from c80423d to 6747109 Compare June 5, 2018 07:45

Remove 4.2 gemfile

33cc9bd

ebihara99999 reviewed Jun 5, 2018

View reviewed changes

seuros reviewed Jun 5, 2018

View reviewed changes

Fix deps specification as in review

ef3797a

seuros merged commit a1d0d30 into mbleigh:master Jun 6, 2018

wayne5540 mentioned this pull request Dec 3, 2018

use ActiveRecord::Migration version 5.0 or 5.1 instead of 4.2 #930

Closed

This was referenced Mar 12, 2021

Bump acts-as-taggable-on from 4.0.0 to 7.0.0 openfoodfoundation/openfoodnetwork#7084

Merged

Bump acts-as-taggable-on from 4.0.0 to 7.0.0 filipefurtad0/openfoodnetwork#29

Closed

tatethurston mentioned this pull request Dec 10, 2023

Adding tags using #tag_ids= breaks when migrating from v5 to v6 #1120

Closed

		@@ -1,11 +1,11 @@
		appraise 'activerecord-5.2' do
		gem 'activerecord', '5.2.0'

		end

		taggable_mixin.class_eval <<-RUBY, __FILE__, __LINE__ + 1

		@@ -174,7 +174,7 @@ module CalculationMethods
		# See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L38
		def count(column_name = :all, options = {})

Rails 5.2 support, drop Rails 4.2 support. #887

Rails 5.2 support, drop Rails 4.2 support. #887

Conversation

Fodoj commented Feb 24, 2018 • edited Loading

thromera left a comment

Choose a reason for hiding this comment

thromera Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fodoj commented Mar 3, 2018

criticerz commented Mar 5, 2018

lunaru commented Mar 9, 2018

Fodoj commented Mar 9, 2018

PhilCoggins commented Mar 13, 2018

travisofthenorth commented Apr 24, 2018

cantino commented Apr 27, 2018

spaghetticode commented May 15, 2018

Fodoj commented May 16, 2018

PhilCoggins commented May 16, 2018

cireficc commented May 17, 2018

Fodoj commented May 18, 2018 • edited Loading

PhilCoggins commented May 18, 2018

Fodoj commented May 18, 2018

Fodoj commented May 18, 2018

PhilCoggins commented May 18, 2018

brchristian commented May 26, 2018

Fodoj commented Jun 4, 2018

seuros commented Jun 4, 2018

Fodoj commented Jun 5, 2018

Fodoj commented Jun 5, 2018

thromera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fodoj commented Jun 5, 2018

thromera commented Jun 5, 2018

Fodoj commented Jun 5, 2018

ebihara99999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebihara99999 Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seuros Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Onikoroshi commented Jun 12, 2018

rsl commented Jun 15, 2018

Fodoj commented Feb 24, 2018 •

edited

Loading

thromera Feb 25, 2018 •

edited

Loading

Fodoj commented May 18, 2018 •

edited

Loading

ebihara99999 Jun 5, 2018 •

edited

Loading

seuros Jun 5, 2018 •

edited

Loading