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

Change association matchers to raise a error when the model is missing some declarations #723

Closed

Conversation

maurogeorge
Copy link
Contributor

Closes #646

@@ -18,6 +18,7 @@
require "shoulda/matchers/active_record/define_enum_for_matcher"
require "shoulda/matchers/active_record/uniqueness"
require "shoulda/matchers/active_record/validate_uniqueness_of_matcher"
require "shoulda/matchers/active_record/errors"

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

When using the `have_many` for example the user maybe have some
misdeclarations on your model, like missed a has many through that dont
declare the has many/belongs to in the through model.
In this case ActiveRecord raise specific excepetions like[1].

With this changes we capture the ActiveRecord exception and re-raise as
a Shoulda::Matchers exception.

[1]:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations.rb#L19
Since we raise a exception when we have a invalid
model definition some specs are no more needed.

Change the model definition to define the right
association to fix a broken spec, that expect
a right association.
On current release of Rails 4.1 the correct exception was not raised
when you define a relationship with a wrong associations.

This was fixed on this PR[1].

We need to keep this until the release of Rails 4.1.11.

[1]: rails/rails#20324
class Error < StandardError
def initialize(original_exception)
message = <<-EOT.strip_heredoc
Check your models definitions, the following error was raised by ActiveRecord:

Choose a reason for hiding this comment

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

Line is too long. [90/80]

@maurogeorge
Copy link
Contributor Author

Not sure why this is broken on travis, works on my machine™ 😄

I think this is related to cache: bundler config not sure.

If you guys have a idea let me know.

@guialbuk
Copy link
Collaborator

Hi @maurogeorge 👋
Can you please merge/rebase master into your branch, as the CI settings are outdated?
Ans there are some merge conflicts as well.
I'd love to merge this ticket into master to fix that issue.

@guialbuk guialbuk modified the milestones: v4.2.0, v4.next Feb 18, 2020
@mcmire
Copy link
Collaborator

mcmire commented Jul 11, 2020

Sorry for the long wait on this. I took these changes and modified them a bit to fail the matcher instead of raising an error. I also made it so that check_validity! is only called for through associations so that we don't break backward compatibility with non-through associations: c0a1578.

@mcmire mcmire closed this Jul 11, 2020
@mcmire mcmire mentioned this pull request Jul 25, 2020
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.

should have_many().through gives undefined method class_name' for nil:NilClass`
4 participants