Keep current scope when calling roots #437

p8 · 2020-12-05T10:01:04Z

6c5040c Fixed the Rails 6.1 deprecation warnings for inherited scoping by
making scopes explicit using Model.unscoped, or Model.default_scoped.

However the default_scoped method added to Model.nested_set_scope
broke calling roots on Relatable when the nested set is scoped:

bob = User.create!(name: 'Bob')
bobs_root = bob.categories.create(name: 'Root Category')
alice = User.create!(name: 'Alice')

# Alice has no categories so root should return nil
# Instead it returned bobs_root
assert_equal(nil, alice.categories.root)

Instead of using two separate class methods,
nested_set_scope_without_default_scope and nested_set_scope, that
apply the scope, we move the default_scoped and unscoped calls to
self.class.base_class. This fixes the roots scope and removes the
deprecation warnings.

Also see the Rails discussion:
rails/rails#41066 (comment)

Fixes: #436

parndt · 2021-01-04T02:32:44Z

@p8 thanks for this; I take it that being marked as draft means it's not ready?

p8 · 2021-01-04T07:38:04Z

@parndt Yes, I'm not sure this is the correct implementation. I'll create an issue in Rails to make sure.

clwy-cn · 2021-01-08T02:36:57Z

@parndt Please merge it。

parndt · 2021-01-08T02:40:41Z

@parndt Please merge it。

@clwy-cn no, thanks... I'm waiting for @p8 to confirm, as it's their PR.

p8 · 2021-01-09T17:33:58Z

I've created an issue in Rails: rails/rails#41066

6c5040c Fixed the Rails 6.1 deprecation warnings for inherited scoping by making scopes explicit using `Model.unscoped`, or `Model.default_scoped`. However the `default_scoped` method added to `Model.nested_set_scope` broke calling `roots` on `Relatable` when the nested set is scoped: bob = User.create!(name: 'Bob') bobs_root = bob.categories.create(name: 'Root Category') alice = User.create!(name: 'Alice') # Alice has no categories so root should return nil # Instead it returned bobs_root assert_equal(nil, alice.categories.root) Instead of using two separate classmethods `nested_set_scope_without_default_scope` and `nested_set_scope` that apply the scope, we move the `default_scoped` and `unscoped` calls to `self.class.base_class`. This fixes the `roots` scope and removes the deprecation warnings. Also see the Rails discussion: rails/rails#41066 (comment) Fixes: collectiveidea#436

p8 · 2021-01-10T10:10:39Z

@parndt I've made some changes as suggested by the @kamipo, the author of the deprecation warning:
rails/rails#41066 (comment)

p8 · 2021-01-10T11:23:23Z

lib/awesome_nested_set/move.rb

@@ -96,7 +96,7 @@ def case_condition_for_parent

        def lock_nodes_between!(left_bound, right_bound)
          # select the rows in the model between a and d, and apply a lock
-          instance_base_class.nested_set_scope.
+          instance_base_class.default_scoped.nested_set_scope.


Substituting default_scoped with unscoped makes the specs pass as well.
Looking at the previous implementation it seems unscoped could be used, but the scope was probably inherited, hence the warnings...
6c5040c#diff-b1bc4bf228f372cd9fa501fa4d3ade576476fba30f75fbc2ec6de15fb9004444L99

This is a tricky one. My typical case where things can get tricky is a default scope that filters out records like

default_scope -> { where(deleted: false) }

In this case it might seem safer to use unscoped and make sure you are locking all records

@marcrohloff Good point! I think you are correct. I've created a PR to change this:
#450

Ok that PR still needs some work...

parndt

Thanks for all your effort here @p8. I think it looks good, and we've followed the advice from the Rails team, and all the tests pass.

p8 · 2021-01-11T06:52:31Z

Thanks @parndt !

p8 · 2021-01-15T12:59:31Z

@marcrohloff as the author of the original change, I thought you probably wanted to know about these changes.

clwy-ito · 2021-01-18T12:27:22Z

@parndt Has the problem been solved?
Please push to rubygems.org

parndt · 2021-01-18T21:48:01Z

@clwy-cn to clarify, are you asking about the Rails 4.0 regression mentioned in #443, or are you asking about this work on the master branch being pushed as 4.0.0 ?

clwy-ito · 2021-01-19T02:29:30Z

@parndt thanks for reply, I am asking about this work.
This is because the version 3.31 , scoped is not working properly，I am using this gem by

gem 'awesome_nested_set', git: 'https://github.com/collectiveidea/awesome_nested_set'

right now.

parndt · 2021-01-19T02:39:38Z

@clwy-cn what is your Rails version?

clwy-cn · 2021-01-26T17:05:18Z

@parndt

rails 6.1.1
ruby 3.0.0

parndt · 2021-01-26T21:39:32Z

ruby 3.0.0

can you find out for me whether it works on Ruby 2.7.2?

p8 marked this pull request as draft December 5, 2020 10:24

p8 force-pushed the fix-root-scope branch 2 times, most recently from f735acd to 2389a9e Compare January 9, 2021 16:01

p8 changed the title ~~Use current_scope if available on Model.nested_set_scope~~ Use scoping to reuse scopes in Model.nested_set_scope Jan 9, 2021

p8 force-pushed the fix-root-scope branch from 2389a9e to 138cdaf Compare January 9, 2021 17:48

p8 changed the title ~~Use scoping to reuse scopes in Model.nested_set_scope~~ Use current_scope if available on Model.nested_set_scope Jan 9, 2021

p8 force-pushed the fix-root-scope branch 3 times, most recently from 1bd0137 to ecce33c Compare January 10, 2021 09:55

p8 changed the title ~~Use current_scope if available on Model.nested_set_scope~~ Keep current scope when calling roots Jan 10, 2021

p8 force-pushed the fix-root-scope branch from ecce33c to 39c7d92 Compare January 10, 2021 10:00

p8 marked this pull request as ready for review January 10, 2021 10:08

p8 commented Jan 10, 2021

View reviewed changes

parndt approved these changes Jan 10, 2021

View reviewed changes

parndt merged commit 247716b into collectiveidea:master Jan 10, 2021

parndt added a commit that referenced this pull request Jan 10, 2021

Document #437

f1b2bc1

p8 deleted the fix-root-scope branch January 11, 2021 06:52

parndt mentioned this pull request Jan 14, 2021

default_scoped doesn't work in rails 4.0 #443

Open

kvognar mentioned this pull request Mar 31, 2021

Fixes Rails 6.1 deprecations #431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep current scope when calling roots #437

Keep current scope when calling roots #437

p8 commented Dec 5, 2020 •

edited by parndt

Loading

parndt commented Jan 4, 2021

p8 commented Jan 4, 2021

clwy-cn commented Jan 8, 2021

parndt commented Jan 8, 2021

p8 commented Jan 9, 2021

p8 commented Jan 10, 2021

p8 Jan 10, 2021

marcrohloff Jan 15, 2021

p8 May 19, 2021

p8 May 19, 2021

parndt left a comment

p8 commented Jan 11, 2021

p8 commented Jan 15, 2021

clwy-ito commented Jan 18, 2021

parndt commented Jan 18, 2021

clwy-ito commented Jan 19, 2021 •

edited

Loading

parndt commented Jan 19, 2021

clwy-cn commented Jan 26, 2021 •

edited

Loading

parndt commented Jan 26, 2021

Keep current scope when calling roots #437

Keep current scope when calling roots #437

Conversation

p8 commented Dec 5, 2020 • edited by parndt Loading

parndt commented Jan 4, 2021

p8 commented Jan 4, 2021

clwy-cn commented Jan 8, 2021

parndt commented Jan 8, 2021

p8 commented Jan 9, 2021

p8 commented Jan 10, 2021

p8 Jan 10, 2021

Choose a reason for hiding this comment

marcrohloff Jan 15, 2021

Choose a reason for hiding this comment

p8 May 19, 2021

Choose a reason for hiding this comment

p8 May 19, 2021

Choose a reason for hiding this comment

parndt left a comment

Choose a reason for hiding this comment

p8 commented Jan 11, 2021

p8 commented Jan 15, 2021

clwy-ito commented Jan 18, 2021

parndt commented Jan 18, 2021

clwy-ito commented Jan 19, 2021 • edited Loading

parndt commented Jan 19, 2021

clwy-cn commented Jan 26, 2021 • edited Loading

parndt commented Jan 26, 2021

p8 commented Dec 5, 2020 •

edited by parndt

Loading

clwy-ito commented Jan 19, 2021 •

edited

Loading

clwy-cn commented Jan 26, 2021 •

edited

Loading