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 into
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 
|
|
Would the user be able to figure out why the warning mark is there and what caused it to appear? |
It doesn't show up in the screenshot but there's a tooltip that reads "Hosts use this Smart Proxy in this taxonomy, but the proxy is not assigned to it." |
|
for testers/reviewers, the only way I could reproduce this was via generating some external capsule, I have created this dummy PR under robottelo SatelliteQE/robottelo#21605 that can create the setup for you. Maybe someone can distill the reproducer into something simpler but I wasn't able to |
If I as a user saw that, then my next question would be "which hosts?". Can the users figure that out somehow or would they have to go for "fix mismatches" and hope for the best? |
You could also create a mismatches report to get some overview first. I could probably come up with a way to show the exact host list somehow in the UI area this PR is touching, but I don't know, that would be quite bit of a rescope or the original issue |
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