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

Make belongs_to relation optional again due to Rails 5 changes #355

Merged
merged 3 commits into from Apr 6, 2017

Conversation

Projects
None yet
3 participants
@Marahin
Contributor

Marahin commented Sep 14, 2016

Rails 5 changes brought a new default setting for relations that breaks this Gem.

Following the example shown in README / Wiki's cheatsheet, there is a ActiveRecord::RecordInvalid: Validation failed: Parent must exist validation error thrown.

Logically, assuming we are creating a rootnode there cannot be a Parent, as the record we are creating is going to be a parent itself.

Changes introduced make the relation optional again as it was before Rails 5 introduction.

@Marahin

This comment has been minimized.

Show comment
Hide comment
@Marahin

Marahin Sep 14, 2016

Contributor

I just also noticed @ travis-ci test results that Rails4 test fail, because these do not include :optional scope that was provided in Rails 5 in order to make the relation optional.
At rails5 environments the tests pass.

Contributor

Marahin commented Sep 14, 2016

I just also noticed @ travis-ci test results that Rails4 test fail, because these do not include :optional scope that was provided in Rails 5 in order to make the relation optional.
At rails5 environments the tests pass.

@Obversity

This comment has been minimized.

Show comment
Hide comment
@Obversity

Obversity Apr 5, 2017

Yo @Marahin, would be great to get a fix for this merged and released!

Would something like this work?

def acts_as_nested_set_relate_parent!
  options = {
    :class_name    => self.base_class.to_s,
    :foreign_key   => parent_column_name,
    :primary_key   => primary_column_name,
    :counter_cache => acts_as_nested_set_options[:counter_cache],
    :inverse_of    => (:children unless acts_as_nested_set_options[:polymorphic]),
    :polymorphic   => acts_as_nested_set_options[:polymorphic],
    :touch         => acts_as_nested_set_options[:touch]
  }
  options[:optional] = true if Rails::VERSION::MAJOR >= 5
  
  belongs_to :parent, options
end

(I haven't tested the above code, so don't quote me on it.)

Obversity commented Apr 5, 2017

Yo @Marahin, would be great to get a fix for this merged and released!

Would something like this work?

def acts_as_nested_set_relate_parent!
  options = {
    :class_name    => self.base_class.to_s,
    :foreign_key   => parent_column_name,
    :primary_key   => primary_column_name,
    :counter_cache => acts_as_nested_set_options[:counter_cache],
    :inverse_of    => (:children unless acts_as_nested_set_options[:polymorphic]),
    :polymorphic   => acts_as_nested_set_options[:polymorphic],
    :touch         => acts_as_nested_set_options[:touch]
  }
  options[:optional] = true if Rails::VERSION::MAJOR >= 5
  
  belongs_to :parent, options
end

(I haven't tested the above code, so don't quote me on it.)

@Marahin

This comment has been minimized.

Show comment
Hide comment
@Marahin

Marahin Apr 5, 2017

Contributor

@Obversity I haven't seen this issue for a long time. Thank you for taking a peek into it :)
I believe it would work, yes. Would setting it like that:

def acts_as_nested_set_relate_parent!
  options = {
    :class_name    => self.base_class.to_s,
    :foreign_key   => parent_column_name,
    :primary_key   => primary_column_name,
    :counter_cache => acts_as_nested_set_options[:counter_cache],
    :inverse_of    => (:children unless acts_as_nested_set_options[:polymorphic]),
    :polymorphic   => acts_as_nested_set_options[:polymorphic],
    :touch         => acts_as_nested_set_options[:touch],
    :optional => true
  }  
  belongs_to :parent, options
end

be an issue though? Before Rails 5 it was optional by default. I think it's prettier than setting it outside of the options hash, conditionally, but I am not sure if it works on <Rails5.

Contributor

Marahin commented Apr 5, 2017

@Obversity I haven't seen this issue for a long time. Thank you for taking a peek into it :)
I believe it would work, yes. Would setting it like that:

def acts_as_nested_set_relate_parent!
  options = {
    :class_name    => self.base_class.to_s,
    :foreign_key   => parent_column_name,
    :primary_key   => primary_column_name,
    :counter_cache => acts_as_nested_set_options[:counter_cache],
    :inverse_of    => (:children unless acts_as_nested_set_options[:polymorphic]),
    :polymorphic   => acts_as_nested_set_options[:polymorphic],
    :touch         => acts_as_nested_set_options[:touch],
    :optional => true
  }  
  belongs_to :parent, options
end

be an issue though? Before Rails 5 it was optional by default. I think it's prettier than setting it outside of the options hash, conditionally, but I am not sure if it works on <Rails5.

@Obversity

This comment has been minimized.

Show comment
Hide comment
@Obversity

Obversity Apr 5, 2017

@Marahin I'm pretty sure in Rails 4 it was required: true to add the validation. In Rails 5, they switched it to optional: true to turn off the validation. I found this article helpful.

If I try to add optional: true to a belongs_to relationship in a Rails 4 project (4.2.3 in this case) then I get this error:

ArgumentError: Unknown key: :optional. Valid keys are: :class_name, :anonymous_class, :foreign_key, :validate, :autosave, :dependent, :primary_key, :inverse_of, :required, :foreign_type, :polymorphic, :touch, :counter_cache

Which is why the tests are failing on Travis CI for rails 4 at the moment.

Obversity commented Apr 5, 2017

@Marahin I'm pretty sure in Rails 4 it was required: true to add the validation. In Rails 5, they switched it to optional: true to turn off the validation. I found this article helpful.

If I try to add optional: true to a belongs_to relationship in a Rails 4 project (4.2.3 in this case) then I get this error:

ArgumentError: Unknown key: :optional. Valid keys are: :class_name, :anonymous_class, :foreign_key, :validate, :autosave, :dependent, :primary_key, :inverse_of, :required, :foreign_type, :polymorphic, :touch, :counter_cache

Which is why the tests are failing on Travis CI for rails 4 at the moment.

@Marahin

This comment has been minimized.

Show comment
Hide comment
@Marahin

Marahin Apr 5, 2017

Contributor

@Obversity I see. Well then I think your original snippet will work best, as it's conditional. And it's not that ugly either ;-)

Contributor

Marahin commented Apr 5, 2017

@Obversity I see. Well then I think your original snippet will work best, as it's conditional. And it's not that ugly either ;-)

@Marahin

This comment has been minimized.

Show comment
Hide comment
@Marahin

Marahin Apr 5, 2017

Contributor

@Obversity there should be no conflicts now.

Contributor

Marahin commented Apr 5, 2017

@Obversity there should be no conflicts now.

@Obversity

This comment has been minimized.

Show comment
Hide comment
@Obversity

Obversity Apr 6, 2017

@Marahin Ugh, I should have tested my snippet before posting it.

Replace

options[:optional] = true if Rails::VERSION::MAJOR >= 5

With

options[:optional] = true if ActiveRecord::VERSION::MAJOR >= 5

Sorry!

Obversity commented Apr 6, 2017

@Marahin Ugh, I should have tested my snippet before posting it.

Replace

options[:optional] = true if Rails::VERSION::MAJOR >= 5

With

options[:optional] = true if ActiveRecord::VERSION::MAJOR >= 5

Sorry!

@Marahin

This comment has been minimized.

Show comment
Hide comment
@Marahin

Marahin Apr 6, 2017

Contributor

@Obversity done.

Contributor

Marahin commented Apr 6, 2017

@Obversity done.

@Obversity

This comment has been minimized.

Show comment
Hide comment
@Obversity

Obversity Apr 6, 2017

Hrm, it looks like most of the tests are passing now, other than the jruby and rbx ones.

The plain ruby tests that are failing, are doing so because they're failing to bundle—for some reason, the version of nokogiri it's trying to install doesn't support the version of ruby the config specifies. This seems unrelated to the this pull request.

I don't know nearly enough about travis CI, jruby or rubinus to pitch in as to why those are failing, but it doesn't look obviously related to this pull request. Is it possible to re-run the specs on travis CI for master, to see if they still work?

@danielmorrison, any thoughts? This change is going to be necessary for anyone wanting to use this gem in Rails 5.

Obversity commented Apr 6, 2017

Hrm, it looks like most of the tests are passing now, other than the jruby and rbx ones.

The plain ruby tests that are failing, are doing so because they're failing to bundle—for some reason, the version of nokogiri it's trying to install doesn't support the version of ruby the config specifies. This seems unrelated to the this pull request.

I don't know nearly enough about travis CI, jruby or rubinus to pitch in as to why those are failing, but it doesn't look obviously related to this pull request. Is it possible to re-run the specs on travis CI for master, to see if they still work?

@danielmorrison, any thoughts? This change is going to be necessary for anyone wanting to use this gem in Rails 5.

@danielmorrison

This comment has been minimized.

Show comment
Hide comment
@danielmorrison

danielmorrison Apr 6, 2017

Member

I'm taking a look at getting those tests passing.

Member

danielmorrison commented Apr 6, 2017

I'm taking a look at getting those tests passing.

@danielmorrison

This comment has been minimized.

Show comment
Hide comment
@danielmorrison

danielmorrison Apr 6, 2017

Member

I'm removing Rubinius and jRuby testing. I don't use either of them, and don't personally care enough to spend the time on making those tests pass.

I'm very open to others figuring it out. PRs welcome.

Member

danielmorrison commented Apr 6, 2017

I'm removing Rubinius and jRuby testing. I don't use either of them, and don't personally care enough to spend the time on making those tests pass.

I'm very open to others figuring it out. PRs welcome.

@danielmorrison danielmorrison merged commit e55b999 into collectiveidea:master Apr 6, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment