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

Q search box in Secrets fails #4572

Closed
itdependsnetworks opened this issue Sep 30, 2023 · 6 comments · Fixed by #4578
Closed

Q search box in Secrets fails #4572

itdependsnetworks opened this issue Sep 30, 2023 · 6 comments · Fixed by #4578
Assignees
Labels
emergent Unplanned work that is brought into a sprint after it's started. type: bug Something isn't working as expected

Comments

@itdependsnetworks
Copy link
Contributor

Environment

  • Nautobot version (Docker tag too if applicable): 2.0
  • Python version: 3.11
  • Database platform, version: psql
  • Middleware(s):

Steps to Reproduce

  1. Go to https://next.demo.nautobot.com/extras/secrets/
  2. Type in the search box for the model (not the global one) anything
  3. Press enter

Expected Behavior

A filtered list view

Observed Behavior

JS crashes with:

image
@itdependsnetworks itdependsnetworks added type: bug Something isn't working as expected triage This issue is new and has not been reviewed. labels Sep 30, 2023
@itdependsnetworks
Copy link
Contributor Author

Whomever at core looks into this, please put your analysis here, even if not coding a fix. I have a similar issue in GC, and have to presume either this will be fixed in core (in which case I will just wait) or potentially need to implement same fix in GC, which I can potentially do right away even if this issue in core is not fixed.

@glennmatthews
Copy link
Contributor

Looks related to #3574's changes.

@itdependsnetworks
Copy link
Contributor Author

I put in a JS breakpoint, the one that worked said name before the length and the other did not, but I couldn't see what was populating that.

@bryanculver bryanculver added emergent Unplanned work that is brought into a sprint after it's started. and removed triage This issue is new and has not been reviewed. labels Oct 2, 2023
@jathanism jathanism self-assigned this Oct 2, 2023
@jathanism
Copy link
Contributor

jathanism commented Oct 2, 2023

I put in a JS breakpoint, the one that worked said name before the length and the other did not, but I couldn't see what was populating that.

Fortunately in this case the traceback tells us the exact line where the error is occurring (Line 615 in forms.js).

        let default_filter_form_query = $("#default-filter form").serialize().split("&").filter(params => params.split("=")[1].length)

At the time of initial rendering this value is undefined (the name field is not part of the "Default") filters for this object, only provider and tags are), so we need to add a little defensive programming on the .filter() argument:

        let default_filter_form_query = $("#default-filter form").serialize().split("&").filter(params => params.split("=")[1]?.length || 0 )

@itdependsnetworks FYI

You can introspect this in the console by calling $("#default-filter form").serialize(). It will return an empty string (''). If you populate a filter value in the "Default" dynamic form, such as Provider: Constant Value, you'll get this:

> $("#default-filter form").serialize()
<- 'provider=constant-value'

Lastly, you might be wondering "but I entered this in the Search box, not the dynamic filter form!" They're all processed as a single form input underneath the hood using a formset (a form of forms). So even the search form is included in the dynamic filter form processing.

@itdependsnetworks
Copy link
Contributor Author

That was helpful, thanks!!! I now know what the question mark operator is in JS and I figured it just required some defensive programming (I was looking for a .get() 😆 ).

So I also get that the name is not defined on the filterform defaults (which aligns with my original issue), I still don't understand why that matters? Put another way, adding a name = forms.DynamicModelChoiceField() should fix my issue, but what is special about name in this context?

@jathanism
Copy link
Contributor

jathanism commented Oct 2, 2023

That was helpful, thanks!!! I now know what the question mark operator is in JS and I figured it just required some defensive programming (I was looking for a .get() 😆 ).

So I also get that the name is not defined on the filterform defaults (which aligns with my original issue), I still don't understand why that matters? Put another way, adding a name = forms.DynamicModelChoiceField() should fix my issue, but what is special about name in this context?

I think it only matters for the two lines that look identical in the JS but one is "dynamic" and one is "default". It's really just this:

class SecretFilterForm(NautobotFilterForm):
    model = Secret
    q = forms.CharField(required=False, label="Search")
    provider = forms.MultipleChoiceField(
        choices=provider_choices_with_blank, widget=StaticSelect2Multiple(), required=False
    )
    tags = TagFilterField(model)

Since only provider and tags are defined there, they are the "default" filter fields. All others (such as name) get deferred to "dynamic" filter fields. So the only thing that is special about name in this context is that it's non-default for this particular object. The basis of the logic for that being (I believe) is that it is redundant to include a default field that is already covered by the search (q) filter.

For reference, here is the q filter for SecretFilterSet:

    q = SearchFilter(
        filter_predicates={
            "name": "icontains",
        },
    )

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
emergent Unplanned work that is brought into a sprint after it's started. type: bug Something isn't working as expected
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants