-
-
Notifications
You must be signed in to change notification settings - Fork 909
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
Deprecate order
and conditions
qualifiers on association matchers
#981
Comments
This is a good idea, but it's much lower priority than the current issues. I'm going to close this in the meantime. |
rioug
added a commit
to rioug/openfoodnetwork
that referenced
this issue
May 5, 2023
It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: thoughtbot/shoulda-matchers#981
rioug
added a commit
to rioug/openfoodnetwork
that referenced
this issue
May 5, 2023
It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: thoughtbot/shoulda-matchers#981
rioug
added a commit
to rioug/openfoodnetwork
that referenced
this issue
May 5, 2023
It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: thoughtbot/shoulda-matchers#981
dacook
pushed a commit
to rioug/openfoodnetwork
that referenced
this issue
May 10, 2023
It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: thoughtbot/shoulda-matchers#981
mkllnk
pushed a commit
to rioug/openfoodnetwork
that referenced
this issue
May 15, 2023
It turns out the "tax_rate" association isn't used and wasn't working. Same for the "voucher" one, which I added to be consistent with existing code. Both of these weren't caught by the specs because you can't test associations with a custome relation with 'shouda-matchers' see: thoughtbot/shoulda-matchers#981
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Historically, the association matchers have had
order
andconditions
qualifiers for testing associations with custom relations. These qualifiers stem from old-style ActiveRecord associations where you could specifyorder
orconditions
options to fine-tune the results you'd get back when querying for the association. These have long been replaced with an argument that takes an ActiveRecord::Relation (wrapped in a lambda). However, the matchers in shoulda-matchers have not been updated to match.That means that currently, if you have an association with a custom relation, there's no way to test that in shoulda-matchers. People have raised GitHub issues about this over time -- see #549, #774, #810, and #952.
Personally, I think we should deprecate and eventually remove
order
andconditions
. I feel like as soon as you add a custom relation to your association, you are adding custom logic to your code, and as such this logic -- the query that's generated -- needs to be tested manually, not by using a matcher.So I think we could lay this issue to rest by deprecating these qualifiers and adding a section to each of the association matchers about testing associations with conditions.
The text was updated successfully, but these errors were encountered: