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

Allow admins to request text input confirmation when deleting certain objects #13954

Closed rmanyari closed 10 months ago

rmanyari commented 1 year ago

NetBox version

v3.5.9

Feature type

New functionality

Proposed functionality

As Netbox administrators we'd love to have additional an additional safety check to prevent accidental deletes. The proposed change is the following:

This is very similar to how other systems prevent accidental deletes. For example, Github requires this type of user input when archiving a project.

I implemented a quick proof of concept in my fork to show the intended workflow. This doesn't have anything to make the confirmation required on some object types only, but it's enough to get the idea across. Here's a recording of the workflow:

https://github.com/netbox-community/netbox/assets/1471029/d57af4db-b145-43f0-ada0-9ba8484dd054

It goes without saying that I'm more than glad to contribute this feature if approved. I believe this would require some frontend changes, and a bit of fiddling around with the configuration to introduce a new field.

Use case

We use Netbox across our company to keep an inventory of assets, drive automation, and configuration management. If someone accidentally deletes an object it can have side effects that are difficult to track, and undo. To prevent this, we'd like to be able to protect a little bit better some object types. We already prevent deletion of some object types using permissions and conditions, but that isn't always enough as some resources still need to be modified by many people, and the more people we have making changes, the greater the chances of blunders. This new feature would make people even more aware of the change they are making.

Database changes

None

External dependencies

None

PieterL75 commented 1 year ago

For me, that delete dialog should be much more detailed.. A list of all objects that will be deleted with that action, would be very handy. I once openend https://github.com/netbox-community/netbox/issues/8551 for that, but it was not (yet) approved.

I stil find it troublesome that the deletion of a device, also deletes IP addresses, ...

rmanyari commented 1 year ago

While it would be nice, I think that's a much more complex change. I image we'd need to traverse all the foreign key relationships from the object being deleted (recursively) and I'm not positive the amount of complexity of that change is justified here. I'd be happy to implement this as suggested, and if we think we need additional visibility in the future we can reconsider displaying the cascading effects. (Unless others completely disagree ofc)

jsenecal commented 1 year ago

For me, that delete dialog should be much more detailed.. A list of all objects that will be deleted with that action, would be very handy. I once openend #8551 for that, but it was not (yet) approved.

I stil find it troublesome that the deletion of a device, also deletes IP addresses, ...

There used to be a "native" django way to do this:

from django.db.utils import DEFAULT_DB_ALIAS
from django.contrib.admin.utils import NestedObjects

collector = NestedObjects(using=DEFAULT_DB_ALIAS)
collector.collect(queryset)
related_objects = collector.data

Perhaps both your (@PieterL75) and OP's FR could be combined

jeremystretch commented 1 year ago

Let's keep these as separate FRs, as they each propose related but discrete functionality, and can easily be implemented individually.

rmanyari commented 1 year ago

Fair enough, thoughts on moving forward with a more complete implementation for this? I got some free cycles, and we'd love to have this feature upstream. FWIW I'd also be happy to work on the other request once we wrap this up, I do think there's value in having it and the snippet that @jsenecal shared seems to make it pretty straightforward to implement.

PieterL75 commented 1 year ago

What if... When one object is deleted, no extra confirmation is required If the delete action also deletes other objects, then a confirmation is needed.

also, When doing bulk deletes, it's not very handy to always have to copy/paste a message.

rmanyari commented 1 year ago

What if... When one object is deleted, no extra confirmation is required If the delete action also deletes other objects, then a confirmation is needed.

That doesn't seem consistent. Unless you're deeply familiar with the Netbox object model you wouldn't really know why you're getting asked for additional confirmation on some cases vs. others.

also, When doing bulk deletes, it's not very handy to always have to copy/paste a message.

There's already a page dedicated to bulk deletes, and we haven't run into accidental bulk deletes, which to me means that it's sufficient.

--

Overall I'd say I like better your initial suggestion (show the cascading effects) but as @jeremystretch mentioned, it's probably best tackled in a separate FR.

We have a real need for the improvement I'm proposing in this FR, and would love to keep it small and scoped.

rmanyari commented 12 months ago

To add a bit more context on the following:

Be able to configure which object types require additional confirmation before deletion

What I'm thinking is adding a new optional setting, say REQUIRE_CONFIRMATION_ON_DELETE that can take a list of strings in the form of <app>.<model> or a special keyword __all__.

The idea here is that we wouldn't enforce a new behavior, and instead allow admins to select the models for which they want this additional protection. If the setting is not defined, we'd also not be requiring confirmation.

We can iterate a bit on the design of this (e.g. what if I want to enforce this on all models expect X, Y and Z? Do we even want to build for that use case?), but that's the intent.

ITJamie commented 10 months ago

FYI https://github.com/netbox-community/netbox/pull/14089#issuecomment-1788095122 solved the issue of showing what related objects will be deleted when you delete an object. it's in the feature branch so it will be released with netbox 3.7.

I would still like to see this fr happen. one example where I've seen user confusion is on the interface list on a device, a user selected a few interfaces to delete then pressed the delete button at the top of the page (device delete button) instead of the "delete selection" button, its an easy mistake to make as object child-list views have multiple delete buttons on show.

rmanyari commented 10 months ago

This is great, thanks a ton for the update @ITJamie - I'd still love to contribute this FR. If you're part of the team that prioritizes updates, lmk if there's anything I can do to ship this. We've ran into exactly the same problem you're describing 🙃

topranks commented 10 months ago

We would love to see this implement (and/or the other proposal in #8551)

one example where I've seen user confusion is on the interface list on a device, a user selected a few interfaces to delete then pressed the delete button at the top of the page (device delete button) instead of the "delete selection" button, its an easy mistake to make as object child-list views have multiple delete buttons on show.

We've hit this numerous times and it is a real pain. The presence of two delete buttons which will do different things is confusing alright. The proposed changes would go a long way to preventing people making this mistake.

jeremystretch commented 10 months ago

This honestly strikes me as a terribly abrasive disruption to the object deletion workflow. Where I've encountered similar controls in the wild, I've never considered them useful. Even for such risky operations such as deleting a GitHub repository, I've never seen a point in forcing the user to validate his or her intention in such an obtuse manner. At best, it merely trains the user to blindly copy and paste text into a form field. I can't imagine how annoying it would be to repeat this every time one goes to delete an object in NetBox.

Further, NetBox v3.6 will introduce an improved deletion confirmation dialog (see #13690), listing all dependent objects to be deleted (e.g. interfaces being removed in response to deleting a device). IMO this is a far more valuable improvement and further invalidates the already questionable justification for this change.

The presence of two delete buttons which will do different things is confusing alright.

@topranks it sounds like your issue is with the page layout then, not with the deletion dialog. I invite you to open your own FR proposing an improved layout, however bear in mind that we're planning a UI refresh for v4.0 anyway (see #12128).

topranks commented 10 months ago

Thanks Jeremy.

Yeah I could see it being an annoyance if displayed for every object deletion. It's probably only realistic if there was a way to control it so it would only appear for certain types of objects, based on user preference.

The improved "Confirm Deletion" (#13690) box looks great! It definitely addresses the problem we have so I think that solution is fine.

@topranks it sounds like your issue is with the page layout then, not with the deletion dialog. I invite you to open your own FR proposing an improved layout, however bear in mind that we're planning a UI refresh for v4.0 anyway (see https://github.com/netbox-community/netbox/issues/12128).

Ok yes. I will have a think about it, one option might be to move the "Clone, Edit, Delete" buttons under the (for example on a device) 'Device' tab, so they didn't appear if one moved to the 'Interfaces' or other tabs. Thanks for the feedback.

rmanyari commented 10 months ago

I'm sure you've read this part of the FR, but just as a reminder, the very first bullet point is:

Be able to configure which object types require additional confirmation before deletion

I do agree that requiring confirmation for every object deletion can be disruptive, which is why I think this should be configurable. Deleting certain objects by accident in our environment can cause issues (domino effect) which is why we'd want to prompt our users for confirmation. Note that it's not the case for most object types.

The improved confirmation dialog that will get released in 3.6 is great and I think it will help prevent differenty type of blunders like the ones mentioned by @topranks

jeremystretch commented 10 months ago

It's probably only realistic if there was a way to control it so it would only appear for certain types of objects, based on user preference.

Users would simply turn it off once it (quickly) becomes annoying, defeating its purpose.

For the reasons I cite above, I remain unconvinced that this feature is a worthwhile addition to NetBox, so I'm going to pass on it. Of course if it's something you really need, you always have the freedom to fork the project and make whatever changes you deem necessary to the application.

rmanyari commented 10 months ago

All good, thanks for the feedback and time spent evaluating this!