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

@PieterL75 commented on GitHub (Oct 3, 2023):

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, ...

@PieterL75 commented on GitHub (Oct 3, 2023): 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, ...

adam commented

@rmanyari commented on GitHub (Oct 3, 2023):

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)

@rmanyari commented on GitHub (Oct 3, 2023): 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)

adam commented

@jsenecal commented on GitHub (Oct 3, 2023):

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

@jsenecal commented on GitHub (Oct 3, 2023): > 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: ```python 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

adam commented

@jeremystretch commented on GitHub (Oct 3, 2023):

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

@jeremystretch commented on GitHub (Oct 3, 2023): Let's keep these as separate FRs, as they each propose related but discrete functionality, and can easily be implemented individually.

adam commented

@rmanyari commented on GitHub (Oct 3, 2023):

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.

@rmanyari commented on GitHub (Oct 3, 2023): 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.

adam commented

@PieterL75 commented on GitHub (Oct 4, 2023):

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.

@PieterL75 commented on GitHub (Oct 4, 2023): 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.

adam commented

@rmanyari commented on GitHub (Oct 4, 2023):

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 on GitHub (Oct 4, 2023): > 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.

adam commented

@rmanyari commented on GitHub (Oct 10, 2023):

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.

@rmanyari commented on GitHub (Oct 10, 2023): 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](https://demo.netbox.dev/static/docs/configuration/optional-settings/), 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.

adam commented

@ITJamie commented on GitHub (Nov 12, 2023):

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.

@ITJamie commented on GitHub (Nov 12, 2023): 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.

adam commented

@rmanyari commented on GitHub (Nov 13, 2023):

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 🙃

@rmanyari commented on GitHub (Nov 13, 2023): 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 🙃

adam commented

@topranks commented on GitHub (Nov 29, 2023):

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.

@topranks commented on GitHub (Nov 29, 2023): 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.

adam commented

@jeremystretch commented on GitHub (Nov 29, 2023):

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).

@jeremystretch commented on GitHub (Nov 29, 2023): 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).

adam commented

@topranks commented on GitHub (Nov 30, 2023):

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.

@topranks commented on GitHub (Nov 30, 2023): 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.

adam commented

@rmanyari commented on GitHub (Nov 30, 2023):

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

@rmanyari commented on GitHub (Nov 30, 2023): 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

adam commented

@jeremystretch commented on GitHub (Nov 30, 2023):

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.

@jeremystretch commented on GitHub (Nov 30, 2023): > 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.

adam commented