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

Support qualifier(optional, required) of 'belongs_to' #956

Closed
wants to merge 6 commits into from

Conversation

riseshia
Copy link

related to #870, #861

This is to support qualifiers(optional, required) of belongs_to, Please review and let me know if it seems good. Then I will write documents for this :)

btw, there is one problem, test suites require protected_attributes gem so that we could not run test with rails 5 environment. ;(
Hence, I could grant required works well(with rails 4.2) with test, but not optional with rails 5.x.
Because of this, I had to confirm whether these work well or not in my personal rails 5 project.
anyway, we need to support test with rails 5 environment.

Thanks for reading this. 👍

I don't take care implements, it only care correct option passed
or not, and its value is boolean or not.
Validating this qualifier is supported or not doesn't seem its role.
In that case ActiveRecord will notice that loudly.
if option_verifier.correct_for_boolean?(:optional, options[:optional])
true
else
@missing = "#{name} should have optional set to #{options[:optional]}"

Choose a reason for hiding this comment

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

Line is too long. [84/80]

@@ -975,6 +986,8 @@ def matches?(subject)
primary_key_exists? &&
class_name_correct? &&
join_table_correct? &&
required_correct? && # TODO: Removed when Rails 4.2 support finished.

Choose a reason for hiding this comment

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

Line is too long. [81/80]

@mcmire
Copy link
Collaborator

mcmire commented Aug 19, 2016

Cool, thanks for this. This looks good, although of course you're right, we will need to make sure that this works with Rails 5.

I'm going to start working on adding Rails 5 support soon, and I'll have to come back to this when that's done. But for now I've tagged this issue appropriately to remind myself.

@riseshia
Copy link
Author

Ok, then I will update documents soon :)

# TODO: Removed when Rails 4.2 support finished.
def required_correct?
if options.key?(:required)
if option_verifier.correct_for_boolean?(:required, options[:required])

Choose a reason for hiding this comment

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

Line is too long. [82/80]

@JoeWoodward
Copy link

What's the status on this? Anything I might be able to help with?

@mcmire
Copy link
Collaborator

mcmire commented Nov 21, 2016

@JoeWoodward This PR is dependent on adding support for Rails 5 (#960). So it can't be finished and merged until that is complete.

I really haven't had time to work on this gem, by the way, so if you're looking to help out, that's the number item to finish right now.

@edwardmp
Copy link
Contributor

edwardmp commented Dec 9, 2016

@mcmire I'll try to look into this. By the way, I use this gem with Rails 5 and besides this haven't experienced any issues.

By the way, would it make sense to support that belongs_to :assocation_name, [required: true] can also be tested by just putting should validate_presence_of(:assocation_name) explicitly in a test?

@mcmire
Copy link
Collaborator

mcmire commented Dec 9, 2016

@edwardmp Yes, that would be the manual way to test it. So the belongs_to matcher somehow needs to automatically run that matcher as well. Perhaps this is the point at which composable matchers would be useful. If both belongs_to and validate_presence_of were composable, then the test could effectively say:

should belong_to(:association_name).and.validate_presence_of(:association_name)

alyssais added a commit to alyssais/shoulda-matchers that referenced this pull request Apr 23, 2017
riseshia/optional

Support qualifier(optional, required) of 'belongs_to'
@mcmire mcmire added this to the v4.0 milestone Sep 19, 2017
@mcmire
Copy link
Collaborator

mcmire commented Oct 3, 2017

@riseshia I realize it's been a long time, but thank you for this PR! I've taken your changes and put them into a new PR with additional tweaks: #1058. Instead of merely checking that the association has a required or optional option, we actually verify the given association is or is not allowed to be nil (a la the presence matcher). I've given you credit in the commit message and of course the NEWS file will link back to this PR.

@riseshia riseshia deleted the optional branch October 18, 2017 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants