netbox-community / netbox

The premier source of truth powering network automation. Open source under Apache 2. Try NetBox Cloud free: https://netboxlabs.com/free-netbox-cloud/
http://netboxlabs.com/oss/netbox/
Apache License 2.0
15.93k stars 2.56k forks source link

Change permission selection to checkboxes and make the space bigger #14297

Open gdprdatasubect opened 10 months ago

gdprdatasubect commented 10 months ago

NetBox version

v3.6.5

Feature type

Change to existing functionality

Proposed functionality

Currently the selection for permissions looks like this: grafik

This does not seem like a good design choice (A lot of options crammed in a small field, ctrl required to add things to selection) and could IMHO better be handled with individual checkboxes and not using a selection-field in the first place, e.g.:

grafik

etc.

Use case

Improve admin-user-experience, not having to press control to add things to the selection, improving accessibility due to being able to use tab to switch to next field

Database changes

No response

External dependencies

No response

JonasEinfach commented 9 months ago

I think it would be very useful if you implement this feature. I struggled the last 15 minutes to select 10 object types. You have to press the Strg key and then scroll down ... then you select the next one and all previous selected object types are deselected.

DanSheps commented 9 months ago

Sorry, this is non-viable straight out of the gate. We have over 100 models within NetBox that can have permissions assigned to them. We cannot generate 100+ check boxes to allow for this.

Additionally, this also doesn't take into account permissions added by plugins.

jsenecal commented 9 months ago

I think it would be very useful if you implement this feature. I struggled the last 15 minutes to select 10 object types. You have to press the Strg key and then scroll down ... then you select the next one and all previous selected object types are deselected.

Maybe the help text could be more explicit and the widget made bigger, but the current way really is the only logical way.

JonasEinfach commented 9 months ago

Ok, yeah you're right 100+ check boxes aren't good to handle. An what about the same method like the filter methods:

image

You can search for the object and can select it and then you see the selected object in the top line. And with the x you can remove the object from the selection. The same ui style is implemented by the user group assignment and the permissions.

image

So we also have the benefit that the same ui styles used 🤔

jsenecal commented 9 months ago

You can search for the object and can select it and then you see the selected object in the top line. And with the x you can remove the object from the selection. The same ui style is implemented by the user group assignment and the permissions.

What do you do when you get 90+ permissions selected ? Wouldnt that break UX ?

abhi1693 commented 9 months ago

How about something similar to this?

image

DanSheps commented 9 months ago

I think #13435 brings up using a dynamic selector (not clear about it from the description IMO though).

gdprdatasubect commented 9 months ago

Ok, yeah you're right 100+ check boxes aren't good to handle

Well, to me the current workflow seems worse to handle than checkboxes, so that's why i suggested them.

Personally I think the whole permission workflow is counterintuitive and the ideal solution would be a Permission Constraint text field per available option, since filtering could be more fine grained and would involve creating less groups (or maybe i'm just using it wrongly)

DanSheps commented 9 months ago

Well, to me the current workflow seems worse to handle than checkboxes, so that's why i suggested them.

Try checking 100 checkboxes because you need to give every permission except for a few (compared to Ctrl-A then Ctrl-click on ones you don't need)

Personally I think the whole permission workflow is counterintuitive and the ideal solution would be a Permission Constraint text field per available option, since filtering could be more fine grained and would involve creating less groups (or maybe i'm just using it wrongly)

There is a permission constraint field, but not everyone can be bothered to learn JSON or they have simpler needs.

Additionally, not every model has every field so you typically have to create multiple permissions for permission constraints anyways.

jeffgdotorg commented 9 months ago

Checkboxes feel untenable, but we could change this control to use a dynamic multi-select widget. That would make it work like the Content Types control from the existing Add Custom Link flow, giving type-to-filter and removing the need to hold down Ctrl / Strg.

image
jeffgdotorg commented 9 months ago

Folding #13435 into this issue. The two are similar enough that we think a common approach (dynamic multi-select, see above) will do the trick for both.

DanSheps commented 9 months ago

Folding #13435 into this issue. The two are similar enough that we think a common approach (dynamic multi-select, see above) will do the trick for both.

It might be a good idea to maybe make it a DynamicModelMultipleChoiceField and see if we can get a selector widget working with it (also make the selector widget work with multi-selection by allowing Ctrl+click to select multiple items).

There would be a little work involved if we wanted to go that route. We would likely need to make ContentType a proxy model of it's own.

DanSheps commented 9 months ago

Did we ever decide on which field we should use for this? I am kind of coming around to @abhi1693's idea as any other could get rather messy with the amount of objects we have.

github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

JonasEinfach commented 4 months ago

Did we ever decide on which field we should use for this? I am kind of coming around to @abhi1693's idea as any other could get rather messy with the amount of objects we have.

How can we procceed with this topic? From my site the idea from @abhi1693 ist the best? The other fields can/will break the UI if you select 100+ permissions. Or maybe some other ideas from other software?

jeremystretch commented 4 months ago

@JonasEinfach this issue needs a volunteer to implement it. Would you like to own it?

JonasEinfach commented 4 months ago

@jeremystretch Im not familar with developing in netbox, but I can try :)

At first look, it looks like that's only a change in the forms of the permission object.

JonasEinfach commented 4 months ago

Today I checked the forms.py for the ObjectPermission.

So that's the actual python code:

    object_types = ContentTypeMultipleChoiceField(
        label=_('Object types'),
        queryset=ObjectType.objects.all(),
        limit_choices_to=OBJECTPERMISSION_OBJECT_TYPES,
        widget=forms.SelectMultiple(attrs={'size': 6})
    )

If Im right you limit the choice of form rendering with the constants OBJECTPERMISSION_OBJECT_TYPES?

In the form we use the ContentTypeMultipleChoiceField. I searched in the netbox github for all forms which use this form as well. For example I found that the CustomFieldForm also use this form. https://github.com/netbox-community/netbox/blob/103c08c2d2bc3e32d8274b8d8ec8dd2380857388/netbox/extras/forms/model_forms.py#L42

The funny thing - in the custom field form we have definitely a better UI form to select / search / remove / see the content types.

image

But the ObjectPermission Form have the "not well to handle" Form :D

image

The widget attribute forms.SelectMultiple(attrs={'size': 6}) overwrite the default form ... I commented the widget attribute and we have the better multiple select form.

image

The form can also handle >80 selected objects. Tbh who use permissions with more than 80 content type objects? It's definitley smarter to use multiple permissions and assign them to a group ...

image

From my site the UI experience is definitely better without widget attribute forms.SelectMultiple(attrs={'size': 6})

What do you think? @jeremystretch @DanSheps