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

Reduce inconsistency between filters and constraints #12441

Closed
stavr666 opened this issue May 2, 2023 · 2 comments
Closed

Reduce inconsistency between filters and constraints #12441

stavr666 opened this issue May 2, 2023 · 2 comments
Labels
type: feature Introduction of new functionality to the application

Comments

@stavr666
Copy link

stavr666 commented May 2, 2023

NetBox version

v3.5.0

Feature type

Change to existing functionality

Proposed functionality

In 3.5.0 at least 5 places, where we can use some filtering logic:

  • API
  • UI filtering
  • Saved filters
  • Widget filters
  • Permissions constraints in Admin panel

With new Dashboards and upcoming rework for Django Admin UI, I hope, there is some opportunity to fix some pain with filters logic.

There is number of issues:

  1. UI filtering generates links with digital IDs, that require to additionally re-process it for external readability. I can read and adopt any structured slug-names from external sources (Grafana, Git etc.), but it's impossible to change on-the-fly api link, generated as "filter from UI > copy to API" by any other engineer from our team, without logging in to Netbox.

  2. UI filtering still miss bunch of options, that available from API documentation. I aware about some FR from devs (did not found it in 3.6 or 4.0 tracks), that proposes modification toggles, multi-selections etc. Just asking you to make it this way, so representation in Swagger will have some correlations to new UI naming and structure, to reduce confusion.

  3. Added (discovered from API) options, that have have no any representation in filtered results.
    Example ('__n' toggle used):
    image
    image

  4. Saved filters does not allow to use multiple conditions for same field, grouping it in single sentence.
    Example: it is possible to filter platforms by conditions "one of [] OR null", "none of [] AND NOT null", but impossible to create saved filters with same conditions.

  5. Admin UI have totally different syntaxis for filters (constraints) without any obvious reasons.
    Example: 'tenant_id' and 'tenant' naming in API/UI, 'tenant_id' and 'tenant__slug' in constraints.

  6. Admin UI not ignoring non-existent properties for permissions definition. If I make a mistake and try to filter Modules or Interfaces by Tenant, I'll still get results by other fields ("status__n=failed", for example). But If I wish to limit someone's ability of editing objects to only "Tenant X or Site Y", then I must create at least 4 permission roles, with all possible combinations of Tenant+Site exitance, and experimentally find type of object that support this fields.

  7. New Dashboards "Object filters" looks inconsistent with both Constraints and Saved Filters.
    Example 1: Object Counts filters 'tenant_id' and 'tenant' is equal, but it also uses 'tenant__slug'; and it throws error (without description) for any '__n' toggle.
    Example 2: Object List filter works same, as API/UI, without throwing any errors on mistake.

Proposal: more consistency in syntaxis. Use the same approach as when developing UI elements.

Use case

Filters reuse. Any scenario, where you can get filter from one source and consistently use it with similar result in another filtered target.

Database changes

No response

External dependencies

No response

@stavr666 stavr666 added the type: feature Introduction of new functionality to the application label May 2, 2023
@kkthxbye-code
Copy link
Contributor

Admin UI have totally different syntaxis for filters (constraints) without any obvious reasons.
Example: 'tenant_id' and 'tenant' naming in API/UI, 'tenant_id' and 'tenant__slug' in constraints.

I specifically told you right here why permission contraints are different from django-filter filtersets, which you seem to have ignored.

#12325 (comment)

The django ORM can, for good reason, access all fields on a model and traverse all relationships in its filters. There's really no way to make the two identical, and I see little reason to. Permission constraints are a power user feature, and will probably stay that way as it is a very large task to make it UX friendly.

Regarding the entire FR I appreciate that you want improvements, but it's both too wide (with duplicates also) and you do a poor job of documenting exactly what you are suggesting. I just don't see anything here being actionable and it feels better suited for a discussion post.

I'll try to give feedback regardless:

  1. I'm just not sure what you are saying here. Are you suggesting that we dump ID's and use slugs for all filters? That's just not feasible (not everything has slugs and slugs are not idempotent) and I don't really understand your reasoning for it being necessary. Regardless, this would need its own FR.
  2. This is a duplicate of another FR.
  3. Also a duplicate, not sure how this is different than 2.
  4. I don't understand what you are saying here. Saved filters are just URL params encoded as a json array. If you are unable to represent an URL query string with it, that's a bug and needs a seperate issue. We recently fixed this for the ObjectList widget, maybe its the same issue.
  5. I already explained this, but permission constraints are django QuerySet filters, which is explained in the docs: https://docs.netbox.dev/en/stable/administration/permissions/#constraints
  6. Related to 5. It's hard to improve on that, as they are just QuerySet filters, but again it would need seperate FR.
  7. I don't see how they are inconsistent with saved filters as they are exactly the same. A JSON representation of a query string.

I would suggest closing this and splitting it up into specific bug reports and FR's. I personally don't see a lot of stuff here that's likely to change, maybe some of it could be helped by better documentation though.

@kkthxbye-code kkthxbye-code added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels May 3, 2023
@kkthxbye-code
Copy link
Contributor

I'll close this for being too wide and not refined enough. I recommend trying a discussion, I'll gladly help answer any technical question regarding why stuff works like it does. You are more than welcome to create individual bug reports or FR's for any specific suggestion. A good rule of thumb is that the solution to a FR should be able to be captured in a single PR.

@kkthxbye-code kkthxbye-code closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature Introduction of new functionality to the application
Projects
None yet
Development

No branches or pull requests

2 participants