Fixes #21357: Add API for registering custom model actions#21560
Fixes #21357: Add API for registering custom model actions#21560
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
b473572 to
975910a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This feels like scope creep to me. Let's tie it off here and revisit with a followup issue if needed. |
jeremystretch
left a comment
There was a problem hiding this comment.
The UI is a little scattered IMO: It's not made clear to the user the differences among actions, custom actions, and additional actions.
Instead of dynamically adding/removing custom actions based on the selected objects, what do you think about starting by showing checkboxes for all available actions (both built-in and custom), and merely enabling/disabling them based on the selected objects? This would simplify the UI considerably, and it looks like you have the bulk of the necessary scripting in place already.
We should also extend the help text of the "additional actions" field to clarify that it is needed only when dealing with plugins which don't yet register their actions.
netbox/core/apps.py
Outdated
| register_models(*self.get_models()) | ||
|
|
||
| # Register custom permission actions | ||
| register_model_actions(DataSource, [ |
There was a problem hiding this comment.
If we determine that it's feasible to declare permissions on each model directly, we can probably move this logic into register_models().
|
Just saw the merge conflicts, fixing those now. |
- Use verbose labels (App | Model) for action group headers - Simplify template layout with h5 headers instead of cards - Consolidate Standard/Custom/Additional Actions into single Actions fieldset
The entire field row is now hidden when no selected object types have registered custom actions, avoiding an empty "Custom actions" label.
- Add plugin development guide for registering custom actions - Update admin permissions docs to mention custom actions UI - Add docstrings to ModelAction and register_model_actions
- Define RESERVED_ACTIONS in users/constants.py for the four built-in permission actions (view, add, change, delete) - Replace hardcoded action lists in ObjectPermissionForm with the constant - Fix duplicate action names in clean() when the same action is registered across multiple models (e.g. render_config for Device and VirtualMachine) - Fix template substring matching bug in objectpermission.html detail view by passing RESERVED_ACTIONS through view context for proper list membership
Convert the ObjectPermission detail view to use the new panel-based layout from #21568. Add ObjectPermissionCustomActionsPanel that cross-references assigned object types with the model_actions registry to display which models each custom action applies to. Also fix dark-mode visibility of disabled action checkboxes in the permission form by overriding Bootstrap's disabled opacity.
30bfed1 to
2db5976
Compare
|
This is ready for review now, as soon as CI finishes. |
jeremystretch
left a comment
There was a problem hiding this comment.
To clarify my earlier suggestion, let's consolidate these into a single list. Users don't know or care that a particular action is "custom" so there's no need to separate them out.
It's also a little confusing to list duplicate actions, e.g. render_config. There's no significant different between them: So long as the render_config action is present for an ObjectPermission, it doesn't matter which model it came from.
Implement two changes requested in review of #21560: 1. Use Meta.permissions for action declaration - Add Meta.permissions to DataSource, Device, and VirtualMachine - register_models() auto-registers actions from Meta.permissions - Remove explicit register_model_actions() calls from apps.py - Add get_action_model_map() utility to utilities/permissions.py 2. Flatten the ObjectPermission form UI - Show a single deduplicated list of action checkboxes (one per unique action name) instead of grouped-by-model checkboxes - RegisteredActionsWidget uses create_option() to inject model_keys and help_text; JS enables/disables based on selected object types - render_field.html bypasses outer wrapper for registeredactionswidget so widget emits rows with identical DOM structure to CRUD checkboxes - Unchecking a model now also unchecks unsupported action checkboxes Fixes #21357
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- Sort model_keys in data-models attribute for deterministic output - Rename registered_actions field label to 'Registered actions' - Target object_types selected list via data-object-types-selected attribute instead of hardcoded DOM ID - Reduce setTimeout delay to 0ms since moveOption() is synchronous
Merge ObjectPermissionActionsPanel and ObjectPermissionCustomActionsPanel into a single Actions panel that shows CRUD booleans and all registered actions in one table, matching the form's consolidated layout. Also fix data-object-types-selected attribute value (True -> 'true') and update plugin docs to show Meta.permissions as the primary registration approach.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- clean() collects all validation errors before raising instead of stopping at the first - Fix stale admin docs (still referenced "Custom actions" and "grouped by model")
058429e to
4c291f0
Compare
Another round:
|
jeremystretch
left a comment
There was a problem hiding this comment.
While the UI functionality is pretty slick, I'm afraid there's way too much logic baked into the form and widgets to justify it. There's too much going on in the frontend for me to even review in a reasonable amount of time.
In the interest of moving this forward, I'm going to suggest a different approach: Rip out the fancy UI bits and just implement a regular form with a checkbox for each available permission. (Don't worry about enabling/disabling them dynamically.) IMO that's plenty sufficient to meet the needs of FR #21357. We can always revisit the advanced functionality at a later date, after the upcoming v4.6 release.
There was a problem hiding this comment.
Can we rename these migrations to something like foo_x_permission?
netbox/utilities/permissions.py
Outdated
| if not action.name: | ||
| raise ValueError("Action name must not be empty.") | ||
| if action.name in RESERVED_ACTIONS: | ||
| raise ValueError(f"'{action.name}' is a reserved action and cannot be registered.") |
There was a problem hiding this comment.
Can we move these validation checks into ModelAction itself to ensure they're always applied?
class ModelAction:
def __post_init__(self):
if not self.name:
raise ValueError("Action name must not be empty.")
if self.name in RESERVED_ACTIONS:
raise ValueError(f"'{action.name}' is a reserved action and cannot be registered.")
netbox/utilities/permissions.py
Outdated
| raise ValueError("Action name must not be empty.") | ||
| if action.name in RESERVED_ACTIONS: | ||
| raise ValueError(f"'{action.name}' is a reserved action and cannot be registered.") | ||
| if action not in registry['model_actions'][label]: |
There was a problem hiding this comment.
We can skip this check if we change registry['model_actions'] to defaultdict(set).
netbox/dcim/models/devices.py
Outdated
| verbose_name = _('device') | ||
| verbose_name_plural = _('devices') | ||
| permissions = [ | ||
| ('render_config', 'Render device configuration'), |
There was a problem hiding this comment.
Suggest dropping the model name from these to avoid confusion in the UI.
| ('render_config', 'Render device configuration'), | |
| ('render_config', 'Render configuration'), |
| verbose_name = _('virtual machine') | ||
| verbose_name_plural = _('virtual machines') | ||
| permissions = [ | ||
| ('render_config', 'Render VM configuration'), |
There was a problem hiding this comment.
| ('render_config', 'Render VM configuration'), | |
| ('render_config', 'Render configuration'), |
|
|
||
| enabled_actions = set(obj.actions) - set(RESERVED_ACTIONS) | ||
|
|
||
| # Collect all registered actions from the full registry, deduplicating by name. |
There was a problem hiding this comment.
This seems like a lot of logic to bake into a UI panel. Suggest moving the logic for resolving registered actions to the ObjectPermission class.
|
|
||
|
|
||
| # | ||
| # ObjectType-specific widgets for ObjectPermissionForm |
There was a problem hiding this comment.
This seems like it's getting out-of-hand. We should avoid creating custom widgets for specific fields. This splits off from the original SplitMultiSelectWidget class, which is now unused.
Co-authored-by: Jeremy Stretch <jstretch@netboxlabs.com>
The registry was changed to defaultdict(set) but the registration code still used list methods. Update .append() to .add() and fix tests to use set-compatible access patterns.
Fixes: #21357
Adds
register_model_actions()API allowing plugins and core to register custom permission actions that appear as checkboxes in ObjectPermissionForm.Changes:
model_actionsregistry storeModelActiondataclass andregister_model_actions()function inutilities.permissionsObjectTypeSplitMultiSelectWidgetwith data attributes for JS targetingRegisteredActionsWidgetfor rendering grouped action checkboxesObjectPermissionFormwith consolidated Actions fieldsetDataSource.sync,Device.render_config,VirtualMachine.render_configModelActionandregister_model_actionsPlugin usage:
Testing instructions: