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

when deleteing an object in webui. list the related objects that would also be deleted #13690

Closed
ITJamie opened this issue Sep 5, 2023 · 14 comments · Fixed by #14089
Closed
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@ITJamie
Copy link
Contributor

ITJamie commented Sep 5, 2023

NetBox version

v3.6.0

Feature type

Change to existing functionality

Proposed functionality

when choosing delete on an object (eg device for this example). the popup that "asks are you sure you want to delete device xxx" should be extended to show the list of related objects that will also be deleted (interfaces, cables, etc)

Use case

some users are not aware that deleting an object will remove some other related objects.
showing the user what additional objects will deleted would be a fantastic safety feature.

Database changes

n/a

External dependencies

n/a

@ITJamie ITJamie added the type: feature Introduction of new functionality to the application label Sep 5, 2023
@ITJamie
Copy link
Contributor Author

ITJamie commented Sep 5, 2023

@ITJamie
Copy link
Contributor Author

ITJamie commented Sep 5, 2023

another option to consider - the delete modal popup could require typing the “name” of the object to enable the delete button on the modal. (I’m personally not a fan of that method, but it is a fairly standard way of helping to stop erroneous deletions in webapps)

@sleepinggenius2
Copy link
Contributor

It looks like something similar was proposed in #8551, but never got an owner. I see this as also related to #6015, which is still a problem in our organization, as the current behavior is unexpected for our engineers.

@pv2b
Copy link
Contributor

pv2b commented Sep 7, 2023

another option to consider - the delete modal popup could require typing the “name” of the object to enable the delete button on the modal. (I’m personally not a fan of that method, but it is a fairly standard way of helping to stop erroneous deletions in webapps)

I don't like it because it doesn't address the core problem, which is making sure a user understands that deleting a device will also potentially delete all related interfaces and cables.

However, one thing that occurrs to me, instead of implementing this in core NetBox, could a similar effect be acheived with a custom validator? The custom validator could for example reject any deletions of any devices that have cables connected to them, requiring the user to disconnect all cables in NetBox before deleting the object. This is similar to how some network vendors set up their equipment, making it impossible to delete configuration object that have other dependent objects.

@pv2b
Copy link
Contributor

pv2b commented Sep 7, 2023

instead of implementing this in core NetBox, could a similar effect be acheived with a custom validator?

I just checked and no, unfortunately the custom validator only triggers when editing an object, it doesn't trigger on object deletion.

Maybe an alternative way to implement this could be to extend the concept of custom validators to also call a method on them prior to deletion?

@ITJamie
Copy link
Contributor Author

ITJamie commented Sep 7, 2023

If this was to be accepted I have put together a quick example of how this could be achieved here

Screenshot 2023-09-08 at 00 19 37 Screenshot 2023-09-08 at 00 22 11

obviously it could use some work styling wise, maybe some links to the objects..., but for a POC it seems to be working

@pv2b
Copy link
Contributor

pv2b commented Sep 8, 2023

If this was to be accepted I have put together a quick example of how this could be achieved here
Screenshot 2023-09-08 at 00 19 37 Screenshot 2023-09-08 at 00 22 11

obviously it could use some work styling wise, maybe some links to the objects..., but for a POC it seems to be working

I think we'd need to talk a little about what's a useful scope. In your example you're listing all related objects. The problem with that is that it's not really a surprise to anyone that if they delete a switch, they'll also be deleting its interfaces, front ports, as well records of related IP addresses etc. With the UI implemented the way you're showing, you'll end up with a long list of related objects for every single device deletion. This will likely cause Alert Fatigue, and you'll end up with users ignoring those warning lists because they're normal and expected.

I think a much more useful scope is limiting yourself to the case where a device is cabled to many other devices or circuits. Something like:

Confirm deletion

Are you sure you want to delete device Super-Important-Distribution-Switch-42? This will remove 27 cables.

[ ] Yes, please delete 27 cables along with the device.

[Cancel] [Submit]

@pv2b
Copy link
Contributor

pv2b commented Sep 8, 2023

I would also like that after sleeping on it, I think personally I prefer an approach where the users have to manually disconnect all cables from a device before it can be deleted, because that automatically makes their deletion intentional. However, I really doubt that's a UX change that NetBox would want to make for everyone. That said I think there's room for a pre-delete hook in the custom validators feature, but that's a seperate FR. That I'm actually going to ahead and write up as a seperate feature to this.

@ITJamie
Copy link
Contributor Author

ITJamie commented Sep 8, 2023

If this was to be accepted I have put together a quick example of how this could be achieved here

Screenshot 2023-09-08 at 00 19 37 Screenshot 2023-09-08 at 00 22 11

obviously it could use some work styling wise, maybe some links to the objects..., but for a POC it seems to be working

I think we'd need to talk a little about what's a useful scope. In your example you're listing all related objects. The problem with that is that it's not really a surprise to anyone that if they delete a switch, they'll also be deleting its interfaces, front ports, as well records of related IP addresses etc. With the UI implemented the way you're showing, you'll end up with a long list of related objects for every single device deletion. This will likely cause Alert Fatigue, and you'll end up with users ignoring those warning lists because they're normal and expected.

I think a much more useful scope is limiting yourself to the case where a device is cabled to many other devices or circuits. Something like:

Confirm deletion

Are you sure you want to delete device Super-Important-Distribution-Switch-42? This will remove 27 cables.

[ ] Yes, please delete 27 cables along with the device.

[Cancel] [Submit]

My view is that this is generic across all models and plugins. Validators would need to be written per model and new additions could be missed.

Showing everything thats going to be deleted is also better for new users to understand the relationships that exist.

@pv2b
Copy link
Contributor

pv2b commented Sep 8, 2023

My view is that this is generic across all models and plugins. Validators would need to be written per model and new additions could be missed.

This is true, but having some more thought put into validation as opposed to warning about everything isn't a bad thing, if your goal is to get your users to actually care about the warnings.

Showing everything thats going to be deleted is also better for new users to understand the relationships that exist.

That is a good point!

I would also like to clarify that I still like the feature being proposed here, it provides useful feedback and information for the user, but I don't think that it'll be effective at one of the stated goals, which is providing safety against accidental deletions, and nothing's stopping both these features from being implemented. (Except possibly limited dev time.) They complement each other.

@ITJamie
Copy link
Contributor Author

ITJamie commented Sep 8, 2023

I agree that we shouldn't allow devices to be deleted with cables connected. So im 100% for that change! Doing this also made me notice that we delete primary and oob ips when deleting a debice which is a bit odd...

I still hope we add this visual to the delete dialog though.

@pv2b
Copy link
Contributor

pv2b commented Sep 8, 2023

I agree that we shouldn't allow devices to be deleted with cables connected. So im 100% for that change! Doing this also made me notice that we delete primary and oob ips which is a bit odd...

I still hope we add this visual to the delete dialog though.

Getting a little off topic in this particular thread here, but my other FR isn't proposing that exactly, it's proposing a mechanism to allow an administrator to extend NetBox to prevent object deletion under specific criteria. It could be used, for example, to enforce that all cables must be deleted before object deletion, but it won't do that unless the user customizes NetBox to do so.

If there were a hypothetical FR to change NetBox to require deleting cables before deleting an object, I'd support it as the right choice, but I can see that being controversial.

@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label Oct 19, 2023
@jeremystretch jeremystretch added this to the v3.7 milestone Oct 19, 2023
@jeremystretch
Copy link
Member

Tagging this for v3.7 as it looks doable. @ITJamie would you like to own this one?

@ITJamie
Copy link
Contributor Author

ITJamie commented Oct 19, 2023

Im happy to take it on

jeremystretch added a commit that referenced this issue Nov 1, 2023
* show objects that would be deleted by cascade

* some items were not showing (eg ips on devices)

* dont include the item being deleted in the list of related items

* Revert "dont include the item being deleted in the list of related items"

This reverts commit 298a786.

* cleanup

- migrate code to use collector directly instead of the NestedObjects wrapper from admin.utils

- adjust object names and text output

* requested adjustments

* remove comma from end of list

* linting

* refactor, add accordion

* migrate to defaultdict, use title for capitalisation of accordian titles

* Misc cleanup

---------

Co-authored-by: Jeremy Stretch <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants