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

Fix Rails 5 sash validation fail because of required belong_to #266

Closed
wants to merge 1 commit into from

Conversation

tamycova
Copy link

@tamycova tamycova commented Nov 3, 2016

#265

It fails when I use rake to test, but works on my Rails 5 app. Also the line is over 80 characters but wasn't sure how do you prefer to break the line.

@@ -7,7 +7,7 @@ def has_merit(options = {})
# That's why MeritableModel belongs_to Sash. Can't use
# dependent: destroy as it may raise FK constraint exceptions. See:
# https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1079-belongs_to-dependent-destroy-should-destroy-self-before-assocation
belongs_to :sash, class_name: 'Merit::Sash', inverse_of: nil
belongs_to :sash, class_name: 'Merit::Sash', inverse_of: nil, optional: true

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Line is too long. [82/80]

@tute
Copy link
Member

tute commented Nov 3, 2016

Looking good! This will need a conditional for the Rails major version, though.

Can you please add - RAILS_VERSION=4.2 to the .travis.yml file, to make sure it continues to work on that version? I don't know why 4.2 is not being built in Travis anymore. Then we will see the following error for that version:

activesupport-4.2.6/lib/active_support/core_ext/hash/keys.rb:75:in `block in assert_valid_keys': 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 (ArgumentError)

We should address that, and have it tested.

Thanks!

@tute
Copy link
Member

tute commented Nov 3, 2016

Also, a regression spec for your failing User.create is a good addition for this PR. Thanks so much!

@tamycova
Copy link
Author

tamycova commented Nov 4, 2016

Will do!

@tute
Copy link
Member

tute commented Nov 21, 2016

ping @tamycova. Thanks!

@tute
Copy link
Member

tute commented Nov 21, 2016

If you need help, please share your questions and I'll do my best.

@louisdb03
Copy link

I'm having the same issue here, how do I implement the fix? Or when is the update comming? :)

@tute
Copy link
Member

tute commented Dec 4, 2016

I'm having the same issue here, how do I implement the fix?

This works only in Rails 5. You can freely use this branch in your project and it will work.

Or when is the update comming? :)

When someone invests the time to make merit work with both versions, and tests both versions in Travis. Then I'll release it.

Thanks!

@louisdb03
Copy link

louisdb03 commented Dec 10, 2016 via email

@tute
Copy link
Member

tute commented Dec 10, 2016

You could do gem 'merit', github: "tamycova/merit", branch: 'f657596409d98fc7c8e3274b213b86cc47d87495'

@tute
Copy link
Member

tute commented Mar 31, 2017

I find it surprising, but it looks like CI is passing for Rails 5: https://travis-ci.org/merit-gem/merit/builds/159191419. Closing this issue, but if it continues to be a problem we reopen.
Thanks for sharing your work! 😃 👏

@tute tute closed this Mar 31, 2017
@wa0x6e
Copy link

wa0x6e commented Aug 8, 2017

Hello,

I'm having the same issue, using the fix in this PR did fix the issue, belongs_to do require the optional option.

I am not sure as I didn't read through the whole test code, but the reason the CI is passing on Rails 5 without that patch is because that precise use case (creating a basic user with has_merit) is not tested.

@tute
Copy link
Member

tute commented Aug 11, 2017

I am not sure as I didn't read through the whole test code, but the reason the CI is passing on Rails 5 without that patch is because that precise use case (creating a basic user with has_merit) is not tested.

On Rails 5, doing User.create! in master's test suite works well. If you have a regression test, please copy it here, or submit it in the form of a PR.

Thanks!

@mrsweaters
Copy link

@tute I'm getting the Sash must exist error on User.create! in Rails 5.1.4 as well.

@mrsweaters
Copy link

I just tested against @tamycova's update and it fixes the issue.

@Velora
Copy link

Velora commented Jan 19, 2018

I have the same issue with Rails 5 and belongs_to :sash, class_name: 'Merit::Sash', inverse_of: nil, optional: true would also fix it for me.

This might not be the preferred solution, but for some it might also be possible that they set the following config to false to have the same behavior that was default in Rails 4: Rails.application.config.active_record.belongs_to_required_by_default = false

@ronyfhebrian
Copy link

I have tried all your ways but I still have error, what should I do?

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

Successfully merging this pull request may close these issues.

None yet

8 participants