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

Should belong_to with 'optional' not working #1153

Closed
jasonperrone opened this issue Oct 31, 2018 · 10 comments
Closed

Should belong_to with 'optional' not working #1153

jasonperrone opened this issue Oct 31, 2018 · 10 comments
Milestone

Comments

@jasonperrone
Copy link

4.0.0.rc1

Model Event has this:
belongs_to :event_category, optional: true

Test has this:
should belong_to(:event_category).optional

Get this:
Expected Event to have a belongs_to association called event_category (the association should have been defined with optional: true, but was not)

@mcmire mcmire added this to the v4.0 milestone Oct 31, 2018
@derekcannon
Copy link

For reference, looks like this is the PR that added that: #1058

@mcmire
Copy link
Collaborator

mcmire commented Nov 15, 2018

@jasonperrone @derekcannon Do you have anything else in your Event model, such as a presence validation, that could be overriding the behavior of belongs_to in this case?

@derekcannon
Copy link

@mcmire I was just referencing the PR. I am not currently on 4.0.0 RC-1, and I haven't personally experienced this problem.

@pareeohnos
Copy link

pareeohnos commented Dec 4, 2018

I recently hit this exact same issue and have done some digging - we had a number of models that had optional associations and the tests were passing so the feature does appear to work just fine but it was just on a single model that it was failing.

I isolated the issue down to some additional validation rules which appear to be conflicting with the require checking so at a guess it is just checking for the presence of the association in the validation errors.

The scenario where we were hitting the error was on a User model. The user belongs_to a company which would usually be required, however if the user is a system administrator then the company is optional. Because of this the association has to be defined as belongs_to :company, optional: true but to ensure it is present in every other scenario, we've got something like this:

class User < ApplicationRecord
  ...
  belongs_to :company, optional: true

  validates_presence_of :company, unless: :is_superadmin

  validate :absence_of_company, if: :is_superadmin
  ...

  def absence_of_company
    errors.add(:company_id, 'must be blank for superadmin users') if company_id.present?
  end
end

I would assume then that when the test runs, because the company_id is blank and the is_superadmin is false, the :company has a validation error which shoulda-matchers is expecting to be blank and rather than checking for the correct validation message it is just checking for a validation error? This is just a guess though as I don't know the code.

@mcmire
Copy link
Collaborator

mcmire commented Dec 4, 2018

Hey @pareeohnos, thanks for the example. That makes perfect sense. I need to think about this further, but just as a quick sketch, I wonder if, for these kinds of scenarios, it would be better to have an additional qualifier to belongs_to which is something like:

it { should belong_to(:company).without_validating_presence }

This way, the matcher wouldn't even bother checking any kind of presence validation, and this would allow you to test presence or absence manually without the matcher interfering.

@pareeohnos
Copy link

Ah that would work - I assume that the optional qualifier is just backwards compatible with older version of Rails so that it's just checking that the validation error doesn't exist? If so then yeah I think an additional qualifier would be needed to get around that

@mcmire
Copy link
Collaborator

mcmire commented Feb 15, 2019

I just merged in a commit that adds a without_validating_presence qualifier to belongs_to and this will be going out in 4.0.

@mcmire mcmire closed this as completed Feb 15, 2019
@salmagomaa
Copy link

without_validating_presence

@mcmire
Copy link
Collaborator

mcmire commented Apr 3, 2020

Sorry, I've updated the comment now :)

@kofiasare
Copy link

Many thanks @pareeohnos and @mcmire. You helped me.

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

No branches or pull requests

6 participants