-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fixes 8553: Fix contacts and ASNs missing in the search dropdown and … #8573
Conversation
netbox/netbox/views/__init__.py
Outdated
'table': table, | ||
'url': f"{reverse(url)}?q={form.cleaned_data.get('q')}" | ||
}) | ||
for _, value in SEARCH_TYPE_HIERARCHY.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to avoid looping with for
to find a key. We should be looking up a key directly in a dict.
Rather than breaking SEARCH_TYPES
into a separate dict per app, I would keep it as-is (actually, I'd change it to a regular dict), and create a separate tuple of nested two-tuples to be used for the search type choices (as with any ChoiceField). This way we can use SEARCH_TYPES
to directly reference the bits we need to perform a search, and use the nested choices just to render the form fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the for loop is not a very nice solution.
Regarding having the hierarchy separate:
I thought the fundamental problem of this bug is that 2 partially redundant data structures have to be maintained by hand.
Separating the hierarchy from the dict containing the search types still results in 2 different places holding the information. Or did I misunderstand you?
I could keep the SEARCH_TYPE_HIERARCHY
as is, but add code to build a flattened version for building the view. This should solve the loop problem while not having to maintain partially redundant data structures by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prepared it as described above, the SEARCH_TYPE_HIERARCHY
still exists, and then OBJECT_CHOICES
and SEARCH_TYPES
are generated from that at program start.
The hierarchy is split into per-app dicts to keep in readable. That many layers of brackets and indentation would get to messy. Also the OrderedDicts are required because we might want to preserve the order in OBJECT_CHOICES
The result is that I don't even have to touch the code in the view and therefore not introduce a for-loop there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremystretch This PR is waiting for a response from you.
14c36da
to
928152f
Compare
…rom global search selector
928152f
to
538984c
Compare
Location.objects.all(), | ||
Device, | ||
|
||
CIRCUIT_TYPES = OrderedDict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there's no need to use OrderedDict
in this case on Python 3.7+ since dictionary ordering is now guaranteed out of the box. It's fine as-is though, I'll clean it up along with some other stuff in a project-wide sweep at some point.
Thanks for this! |
…contacts not showing the number of assigned objects
Fixes: #8553
The contact and ASN object were not shown in the global object search dropdown.
Also the contant queryset used in the global search was not annotated with the
assigned_count