Fixes #39220 - Exclude host taxonomies that don't match the capsule t…#10952
Fixes #39220 - Exclude host taxonomies that don't match the capsule t…#10952pondrejk wants to merge 2 commits intotheforeman:developfrom
Conversation
| @@ -78,7 +78,14 @@ def used_taxonomy_ids(type) | |||
| return [] if new_record? || !respond_to?(:hosts) | |||
|
|
|||
| conditions = "#{id} IN (#{Host::Managed.proxy_column_list})" | |||
| ::Host::Managed.with_smart_proxies.where(conditions).distinct.pluck(type).compact | |||
| host_ids = ::Host::Managed.with_smart_proxies.where(conditions).distinct.pluck(type).compact | |||
There was a problem hiding this comment.
A bit misleading name, consider host_taxonomy_ids?
| when :location_id then location_ids | ||
| when :organization_id then organization_ids | ||
| else | ||
| [] |
There was a problem hiding this comment.
Even though we're dealing with a bug already, and the current implementation takes whatever is provided as type (even though we only expect :<location|organization>_id), maybe it's worth to raise an error since it's something not expected?
| @@ -38,6 +38,41 @@ class SmartProxyTest < ActiveSupport::TestCase | |||
| assert_equal [taxonomies(:location1).id], smart_proxies(:puppetmaster).used_or_selected_location_ids | |||
| end | |||
|
|
|||
| describe '#used_taxonomy_ids' do | |||
There was a problem hiding this comment.
Maybe it's also worth to add a test which doesn't test only for emptiness? Something like proxy has orgs [A, B], host references orgs [B, C] => should return only [B].
There was a problem hiding this comment.
Makes sense, I added test on line 62
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @pondrejk, code-wise LGTM.
Although, I'm not sure about the actual idea of the fix. The issue seems more like a data mismatch caused by something, since it seems to be fixed by explicit Fix mismatches button. I'm afraid the underlying issue is more complex, this fix would just mask it (although correctly). E.g. the link between a host in orgC and a proxy that is in orgA + orgB will still be there, we would just not show it.
Also, I've tried to reproduce it naively: assign orgA, orgB to the proxyA, create a host in orgC using proxyA and it just didn't allow me. What are your reproduction steps that show current state and the fixed state?
That is true, from what I can tell the situation is a result of data mismatch. What about I'd add some explicit warning somewhere to the proxy details UI, that would show up only when this occurs?
I could reproduce trough downstream automation, though I think you could create the situation it by creating host-capsule relationsips with matching orgs, and then dropping the orgs from the capsule via cli (which is allowed -- maybe another item to improve) |
Hmm, might be a good idea. Although, I'd like to get rid of the possibility of mismatching, stating on the proxy details page that it's being used by something from a different org/loc with a potential fix could help users a bit. |
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
Looking at some other methods in taxonomix (like used_or_selected_location_ids) it seems that the sets of directly assigned taxonomies and used (aka indirectly assigned) taxonomies are expected to possibly be disjoint.
If I'm reading it correctly then in the edit form the used taxonomies should be disabled (you can't toggle them). What if we went the other direction and instead of showing directly assigned taxonomies everywhere, we displayed the used ones so that instead of masking the issue we force the user to reconcile it?
…axonomies from used_taxonomy_ids
github-actions
Bot
added
the
Legacy JS
|
@ofedoren @adamruzicka to sum up findings since the last time:
My proposal is to not include the taxonomy if its not assigned, but mark it as used, so users find out about it and can select what to do about it, the current form 
once you include the offending org and submit, on the next visit it shows up as included and associated with a host 
|
3 participants
…axonomies from used_taxonomy_ids
SAT-35949 - the the taxonomies listed in the smart proxy edit dialog could exceed the actual taxonomies assigned to proxy