Fixes #38718 - Register feature scope in belongs_to_proxy

[adamruzicka]

Prompted by theforeman/foreman_monitoring#99 (comment)

Requires:

• Make autocomplete honor relation scope wvanbergen/scoped_search#231

TODOs:

• bump dependency on scoped_search

[ofedoren]

Thanks, @adamruzicka, makes sense.

Although, I'd like to see the issue filled with some information on why it's a useful feature or if it's intended to fix some issue.

Ideally, this should have some... example or showcase?..

Checked with scoped_search counterpart, seems to be working as intended.

[adamruzicka]

Well, it is not exactly flashy, but this should allow proxy-feature-based search to offer you only proxies with relevant feature. Trying to search hosts by puppetmaster proxy should only offer you hosts with the puppetmaster feature, rather than offering you all the proxies you have, no matter if they have the right feature or not.

[adamruzicka]

Additionally, the scoped search fix seems to address SAT-30581 as well

[evgeni]

📦 ack, didn't look at the rest

[adamruzicka]

The scope (and the one for nics) was probably never evaluated before. The auditable type for hosts should be Host::Base, but Host::Base is not auditable so I had to hack around a bit. Having the join condition set to limit it to Host::Base should work too, but eh

[ofedoren]

For anyone else: these were broken before changes to the scoped_search. It's just we noticed this due to the changes.

It seems we should've covered these with tests :/ Also, even with your fix (given that it at least works now) the results are might be weird (or that's me missing something). Essentially in some cases the results are the same as there was no scope defined (remove the whole lambda):

I've got a host with ID 1 and an audit record with auditable_id being also 1, but auditable_type being Setting. I'd assume that typing audit_record.search_hosts will return nil, but it finds that host instead and returns it, which was... unexpected. The same goes with search_nics:

Is that expected?

[adamruzicka]

Is that expected?

Not really. Added a fix

Edit: god dammit rails, it takes an argument optionally

ArgumentError: The association scope 'search_nics' is instance dependent (the scope block takes an argument). Eager loading instance dependent scopes is not supported.

[adamruzicka]

Hrm, I've probably run out of tricks to make this somehow work, I'll sleep on it.

In case I come up short, would it be possible to accept it's current limitations? I assume the association is there primarily for scoped search to work, with primary audit->resource access point being Audit#auditable.

[ofedoren]

What if we go with a workaround, which seems to make that association work and return the valid results? Something like:

    belongs_to :search_hosts, :class_name => 'Host::Base', :foreign_key => :auditable_id

    def search_hosts
      return nil unless auditable_type&.match?(/^(::)?Host::/)
      super
    end

[adamruzicka]

This was getting out of hand, so I went ahead and forked off the "compatibility with newer scoped search" into #10703 . I'd prefer to first get that thing green and merged and then start leveraging the new features

[ofedoren]

@adamruzicka, this awaits for a rebase :)

[ofedoren]

Thanks, @adamruzicka, the feature seems to be working just fine, so ACK from that perspective.

The bug with Audits I've mentioned earlier is still present though :/ Not sure if it should be handled within this PR (probably not).

Test failures should not be related.

[adamruzicka]

Not sure if it should be handled within this PR (probably not).

Could you please file an issue for that?

[ofedoren]

Could you please file an issue for that?

Done: https://projects.theforeman.org/issues/38798

As for this PR, let's get this in :)