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

Order of multiple conditions qualifier in have_many - v5.3.0 #1561

Open
thedean7 opened this issue Jun 1, 2023 · 4 comments
Open

Order of multiple conditions qualifier in have_many - v5.3.0 #1561

thedean7 opened this issue Jun 1, 2023 · 4 comments

Comments

@thedean7
Copy link

thedean7 commented Jun 1, 2023

Not sure if I'm missing something...

Scope in model:

has_many :active_website_resources, -> {
  joins(:resource)
  .where(is_active: true)
  .where(resource: { status: :active }) },
  class_name: 'WebsiteResource'

This test passes:

it { should have_many(:active_website_resources)
  .conditions(resource: { status: :active }) #1 <==
  .conditions(is_active: true) #2 <==
  .class_name('WebsiteResource') }

But when conditions are reversed then it fails:

it { should have_many(:active_website_resources)
  .conditions(is_active: true) #2 <==
  .conditions(resource: { status: :active }) #1 <==
  .class_name('WebsiteResource') }

with message:

Expected Website to have a has_many association called active_website_resources (active_website_resources should have the following conditions: {:resource=>{:status=>:active}})
@matsales28
Copy link
Member

Hello! I hope you're doing well. Thank you for raising this issue. After quickly reviewing the source code, I believe you can achieve the desired outcome by combining both where clauses within the conditions method, as shown in the example below.

it { should have_many(:active_website_resources)
  .conditions(resource: { status: :active }, is_active: true)
  .class_name('WebsiteResource') }

Please try using this code and see if it works as expected. Let me know if you need any further assistance!

@thedean7
Copy link
Author

thedean7 commented Jun 2, 2023

Hi!

I tried many combinations, inline and chained but the result is the same. The culprit is nested condition. Order of conditions matters but attribute names and values of nested condition do not.

Only is_active is tested, nested attribute is ignored. Notice attribute changes and order:

# passes
it { should have_many(:active_website_resources)
  .conditions(whatever: { whatever: :whatever }) # <==
  .conditions(is_active: true)
  .class_name('WebsiteResource') }

# fails
it { should have_many(:active_website_resources)
  .conditions(is_active: true)
  .conditions(whatever: { whatever: :whatever })
  .class_name('WebsiteResource') }

Inline conditions:

# passes
it { should have_many(:active_website_resources)
  .conditions(is_active: true, whatever: { whatever: :whatever })
  .class_name('WebsiteResource') }

# fails
it { should have_many(:active_website_resources)
  .conditions(is_active: false, whatever: { whatever: :whatever })
  .class_name('WebsiteResource') }

# passes
it { should have_many(:active_website_resources)
  .conditions(whatever: { whatever: :whatever }, is_active: true)
  .class_name('WebsiteResource') }

# fails
it { should have_many(:active_website_resources)
  .conditions(whatever: { whatever: :whatever }, is_active: false)
  .class_name('WebsiteResource') }

@matsales28
Copy link
Member

matsales28 commented Jun 2, 2023

Yes, I dived into the code now, and we are using the where_values_hash to build a hash of conditions and compare it with the given conditions for the matcher modifier.

def extract_relation_clause_from(relation, name)
case name
when :conditions
relation.where_values_hash

Unfortunately, this method only returns conditions for the queried primary table, in this case, "website_resources", so any nested condition will be discarded. So, making this spec using shoulda is impossible for now 😭 .
We could implement a new solution to build the hash using some of the methods Rails uses on the where_values_hash, but I'm not sure if that's a nice approach because they're internal and not even documented methods.

https://github.com/rails/rails/blob/254f1d8ded07dfd73462b40d6aaa8c19032259f6/activerecord/lib/active_record/relation.rb#L772-L774

@mcmire I'd love to hear your thoughts on this.

@thedean7
Copy link
Author

thedean7 commented Jun 2, 2023

Thank you, @matsales28. I appreciate the detailed explanation.

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

2 participants