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

Add Rails 6 Support #124

Merged
merged 30 commits into from Aug 24, 2019

Conversation

@danielricecodes
Copy link
Contributor

commented Aug 13, 2019

This pull requests offers Support for Rails 6.0, which was released on August 15th, 2019.

danielricecodes and others added 19 commits Aug 13, 2019
Create explicit V6 class
Create a V6 class, in case there are differences that need to be coded later.
Read Association scope directly from ActiveRecord
the association_scope method was made private in Rails public API.
@RomainAlexandre

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Hi @danielricecodes !
I loved the work you started. @jbryant92 and I are bumping our app to rails 6 and will need this gem to be on rails 6.

We have spent some time investigating the last failing specs on your branch. @jbryant92 was able to fix test/test_associations.rb:235:in test_mass_assignment_of_paranoid_column_enabled'`

We think we understand the reason why the last two are failing. The reason is, named scopes are no longer leaking to class level in rails 6 (See associated Rails PR: rails/rails#32380)

In our case, this line

association.klass.with_deleted.scoping { #{target}_without_unscoped(*args) }

is the culprit.(https://github.com/ActsAsParanoid/acts_as_paranoid/blob/master/lib/acts_as_paranoid/associations.rb#L34)

#{target}_without_unscoped(*args) belongs_to does not get the scope with_deleted when being called.

Few ideas that don't seem quite correct.
The closest we got was this:

association.klass.with_deleted { association.klass.unscoped { #{target}_without_unscoped(*args) } }

Since it is possible to use unscoped with a block, we can remove the default scope coming with act_as_paranoid. But it also remove any other default scope define by a user of this gem where we would only like to call with_deleted on the belongs_to.

We also tried to prepend with_deleted to the scope when creating the belongs_to relationship.
But concatenating two scopes is a bit trickier than plan.

I hope this can help. If you have any ideas or comments, let us know !
We will continue investigating it !

@jbryant92

This comment has been minimized.

Copy link

commented Aug 19, 2019

Here are the changes I made to get it going with Rails 6.0.0, also the small fix for the spec: ldstudios#1

Merge pull request #1 from jbryant92/rails-6-support
Let the rails-6-support branch run with 6.0.0
@danielricecodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@jbryant92 @RomainAlexandre thank you so much for helping with the build!

end

module ClassMethods
def validates_uniqueness_of_without_deleted(*attr_names)
validates_with UniquenessWithoutDeletedValidator[ActiveRecord::VERSION::MAJOR], _merge_attributes(attr_names)
if ActiveRecord::VERSION::MAJOR == '6'

This comment has been minimized.

Copy link
@danielricecodes

danielricecodes Aug 19, 2019

Author Contributor

Looks like unique validations tests should fail here since the code for AR v6 is missing.

This comment has been minimized.

Copy link
@jbryant92

jbryant92 Aug 19, 2019

Looks like ActiveRecord::VERSION::MAJOR is an Int so the condition is returning false and then running the old code and passing the tests 🎉

What was the reason this condition was added?

This comment has been minimized.

Copy link
@danielricecodes

danielricecodes Aug 19, 2019

Author Contributor

You're right. This can be reverted. I had it in here before when I was trying to figure out why I kept getting "6" is not a valid validator, must be "V5" or "V4". I understand the code a lot better now than I did when I made that commit :)

@danielricecodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Almost there but not quite:

# Running:
......F.S....F..............................................................
Finished in 3.188223s, 23.8377 runs/s, 115.7385 assertions/s.
  1) Failure:
AssociationsTest#test_belongs_to_with_deleted [/home/travis/build/ActsAsParanoid/acts_as_paranoid/test/test_associations.rb:56]:
--- expected
+++ actual
@@ -1 +1 @@
-#<ParanoidTime id: 1, name: "paranoid", deleted_at: "2019-08-19 18:18:48", paranoid_belongs_dependant_id: nil, not_paranoid_id: nil, created_at: "2019-08-19 18:18:48", updated_at: "2019-08-19 18:18:48">
+nil
  2) Failure:
AssociationsTest#test_belongs_to_polymorphic_with_deleted [/home/travis/build/ActsAsParanoid/acts_as_paranoid/test/test_associations.rb:69]:
--- expected
+++ actual
@@ -1 +1 @@
-#<ParanoidTime id: 1, name: "paranoid", deleted_at: "2019-08-19 18:18:49", paranoid_belongs_dependant_id: nil, not_paranoid_id: nil, created_at: "2019-08-19 18:18:49", updated_at: "2019-08-19 18:18:49">
+nil
76 runs, 369 assertions, 2 failures, 0 errors, 1 skips
@RomainAlexandre

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Morning @danielricecodes
So here is the solution that I believe seem right: ldstudios#2

Let me know what you think !

Merge pull request #2 from RomainAlexandre/rails-6-support
Using unscoped when retrieving the belongs_to association with deleted
@RomainAlexandre

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Crushing it @danielricecodes !

🙌
I think it's ready for review from the maintainers of acts_as_paranoid !

@danielricecodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@RomainAlexandre @jbryant92 thanks for your help. With a passing build I'd say its safe to merge!

@RomainAlexandre

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Do you know who we should ping to get this PR reviewed and merged ?
Maybe @mvz, I noticed he has been the last one to merge the most recent PRs in the repo.

@mvz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Yes! I'll take a look soon.

@mvz
Copy link
Contributor

left a comment

Thanks for this, @danielricecodes and everyone who helped. I have just a few nits and something I need to check.

.travis.yml Outdated
- bundler
- directories:
- vendor/bundle
language: ruby

This comment has been minimized.

Copy link
@mvz

mvz Aug 21, 2019

Contributor

This key is duplicated.

.gitignore Outdated
@@ -4,3 +4,5 @@ pkg
Gemfile.lock
gemfiles/*.lock
.idea/

\.ruby-version

This comment has been minimized.

Copy link
@mvz

mvz Aug 21, 2019

Contributor

I think the \ can go?

@@ -38,8 +37,11 @@ def validate_each(record, attribute, value)
protected

def build_relation(klass, attribute, value)
return super(klass, klass.arel_table, attribute, value) if ActiveRecord::VERSION::MINOR == 0
super
if ActiveRecord::VERSION::MINOR == 0 && ActiveRecord::VERSION::MAJOR == 5

This comment has been minimized.

Copy link
@mvz

mvz Aug 21, 2019

Contributor

This looks a bit odd because you would think this class would only be used with Rails 5 (except it isn't because it's subclassed to V6). I'm not sure how to resolve this without duplicating all of V5 (except build_relation) to V6, so 🤷‍♂.

This comment has been minimized.

Copy link
@danielricecodes

danielricecodes Aug 21, 2019

Author Contributor

Agreed. I wasn't sure what the best way to code this would be since most options seemed to involve just copying and pasting, so I opted for the fewest lines of code: simply making V6 an empty subclass of V5.

@@ -31,7 +31,7 @@ def #{target}_with_unscoped(*args)
association = association(:#{target})
return nil if association.options[:polymorphic] && association.klass.nil?
return #{target}_without_unscoped(*args) unless association.klass.paranoid?
association.klass.with_deleted.scoping { #{target}_without_unscoped(*args) }
association.klass.with_deleted.scoping { association.klass.unscoped { #{target}_without_unscoped(*args) } }

This comment has been minimized.

Copy link
@mvz

mvz Aug 21, 2019

Contributor

I'm going to check if this doesn't have any unwanted side effects.

This comment has been minimized.

Copy link
@RomainAlexandre

RomainAlexandre Aug 21, 2019

Contributor

I tried to explain here why I made that change, if it helps ldstudios#2

This comment has been minimized.

Copy link
@mvz

mvz Aug 24, 2019

Contributor

Ok, I created some explicit tests for this and it looks like everything is fine 👍. I'll merge this in and then create another PR to add those tests.

@@ -228,7 +228,7 @@ def test_double_belongs_to_with_deleted
end

def test_mass_assignment_of_paranoid_column_enabled
if ActiveRecord::VERSION::MAJOR > 4 && ActiveRecord::VERSION::MINOR > 1
if Gem.loaded_specs['activerecord'].version >= Gem::Version.new('5.2.0')

This comment has been minimized.

Copy link
@danielricecodes

danielricecodes Aug 21, 2019

Author Contributor

@mvz - there are several places in the code that check the current ActiveRecord version. Since Rails 6 is out now, it might be worthwhile to release a new major version of this gem where old versions of Rails/Ruby are no longer supported. A new gem version that supports only Rails 5.2+ w/ Ruby 2.5+ only would be nice clean slate.

This comment has been minimized.

Copy link
@mvz

mvz Aug 21, 2019

Contributor

Yes, that's definitely a good idea!

This comment has been minimized.

Copy link
@danielricecodes

danielricecodes Aug 22, 2019

Author Contributor

I don't mind working on that after we get this PR merged.

@mvz mvz merged commit 9e240b9 into ActsAsParanoid:master Aug 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danielricecodes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Thanks @mvz I’ll work on a Rails 5.2+ only gem and offer a PR for that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.