Repository: netbox-community/netbox
Pull request: Closes #13690: List all objects to be deleted
<!--
    Thank you for your interest in contributing to NetBox! Please note that
    our contribution policy requires that a feature request or bug report be
    approved and assigned prior to opening a pull request. This helps avoid
    waste time and effort on a proposed change that we might not be able to
    accept.

    IF YOUR PULL REQUEST DOES NOT REFERENCE AN ISSUE WHICH HAS BEEN ASSIGNED
    TO YOU, IT WILL BE CLOSED AUTOMATICALLY.

    Please specify your assigned issue number on the line below.
-->
### Fixes: #13690

List all objects to be deleted when deleting an item.

Currently, the list includes the original item being deleted. we can change that pretty easily but I would like feedback first as to whether the list should be exhaustive or just the additional items (see #13842 as to why including the original item in the list might make sense)

<img width="1318" alt="Screenshot 2023-10-20 at 07 52 22" src="https://github.com/netbox-community/netbox/assets/1613241/10e311c4-09ef-4cba-8082-fc9069d0cdbc">
<img width="1531" alt="Screenshot 2023-10-20 at 07 53 26" src="https://github.com/netbox-community/netbox/assets/1613241/a9f89239-876f-4ab5-bd63-aa4f3c984561">



Problem statement:
when deleteing an object in webui. list the related objects that would also be deleted
### 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

Hints from issue discussion:
related slack convo: https://netdev-community.slack.com/archives/C01P0FRSXRV/p1693924009439029
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)
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.
> 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.
> 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?
If this was to be accepted I have put together a quick example of how this could be achieved [here](https://github.com/netbox-community/netbox/compare/develop...ITJamie:netbox:show-what-extra-objects-would-be-deleted)

<img width="1171" alt="Screenshot 2023-09-08 at 00 19 37" src="https://github.com/netbox-community/netbox/assets/1613241/2a025712-1daf-4d44-893d-ecdb78f02217">
<img width="1168" alt="Screenshot 2023-09-08 at 00 22 11" src="https://github.com/netbox-community/netbox/assets/1613241/9ef90915-a824-4d1d-a769-b3bfe1ded84f">

obviously it could use some work styling wise, maybe some links to the objects..., but for a POC it seems to be working 


> If this was to be accepted I have put together a quick example of how this could be achieved [here](https://github.com/netbox-community/netbox/compare/develop...ITJamie:netbox:show-what-extra-objects-would-be-deleted)
> <img alt="Screenshot 2023-09-08 at 00 19 37" width="1171" src="https://user-images.githubusercontent.com/1613241/266462663-2a025712-1daf-4d44-893d-ecdb78f02217.png"> <img alt="Screenshot 2023-09-08 at 00 22 11" width="1168" src="https://user-images.githubusercontent.com/1613241/266462776-9ef90915-a824-4d1d-a769-b3bfe1ded84f.png">
> 
> 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]
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.
> > If this was to be accepted I have put together a quick example of how this could be achieved [here](https://github.com/netbox-community/netbox/compare/develop...ITJamie:netbox:show-what-extra-objects-would-be-deleted)
> 
> > <img alt="Screenshot 2023-09-08 at 00 19 37" width="1171" src="https://user-images.githubusercontent.com/1613241/266462663-2a025712-1daf-4d44-893d-ecdb78f02217.png"> <img alt="Screenshot 2023-09-08 at 00 22 11" width="1168" src="https://user-images.githubusercontent.com/1613241/266462776-9ef90915-a824-4d1d-a769-b3bfe1ded84f.png">
> 
> > 
> 
> > 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. 
> 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.

Commit message: cleanup

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

- adjust object names and text output

Patch to review:
diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py
index 99d8ff5401c..de6b5cb9a6d 100644
--- a/netbox/netbox/views/generic/object_views.py
+++ b/netbox/netbox/views/generic/object_views.py
@@ -2,8 +2,9 @@
 from copy import deepcopy
 
 from django.contrib import messages
-from django.db import transaction
+from django.db import transaction, router
 from django.db.models import ProtectedError
+from django.db.models.deletion import Collector
 from django.shortcuts import redirect, render
 from django.urls import reverse
 from django.utils.html import escape
@@ -334,6 +335,18 @@ def get(self, request, *args, **kwargs):
         obj = self.get_object(**kwargs)
         form = ConfirmationForm(initial=request.GET)
 
+        using = router.db_for_write(obj._meta.model)
+        collector = Collector(using=using)
+        collector.collect([obj])
+        deletion_objects = []
+        if collector.instances_with_model():
+            for model, instance in collector.instances_with_model():
+                # we could ignore the instance == obj so that the list doesnt contain itself...
+                deletion_objects.append({
+                    "modelname":f"{model.__name__}", 
+                    "object": instance,
+                })
+
         # If this is an HTMX request, return only the rendered deletion form as modal content
         if is_htmx(request):
             viewname = get_viewname(self.queryset.model, action='delete')
@@ -343,6 +356,7 @@ def get(self, request, *args, **kwargs):
                 'object_type': self.queryset.model._meta.verbose_name,
                 'form': form,
                 'form_url': form_url,
+                'deletion_objects': deletion_objects,
                 **self.get_extra_context(request, obj),
             })
 
@@ -350,6 +364,7 @@ def get(self, request, *args, **kwargs):
             'object': obj,
             'form': form,
             'return_url': self.get_return_url(request, obj),
+            'deletion_objects': deletion_objects,
             **self.get_extra_context(request, obj),
         })
 
diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html
index d0fba9d369e..f108f41eafc 100644
--- a/netbox/templates/htmx/delete_form.html
+++ b/netbox/templates/htmx/delete_form.html
@@ -12,6 +12,14 @@ <h5 class="modal-title">{% trans "Confirm Deletion" %}</h5>
         Are you sure you want to <strong class="text-danger">delete</strong> {{ object_type }} <strong>{{ object }}</strong>?
       {% endblocktrans %}
     </p>
+    {% if deletion_objects %}
+    <p>
+      This will cause deltion of the following items: <br>
+      {% for deletion_object in deletion_objects %} 
+        - {{ deletion_object.modelname }} - {{ deletion_object.object | linkify }} <br>
+      {% endfor %}
+    </p>
+    {% endif %}
     {% render_form form %}
   </div>
   <div class="modal-footer">


Code review comments:
diff_hunk: @@ -334,6 +335,18 @@ def get(self, request, *args, **kwargs):
         obj = self.get_object(**kwargs)
         form = ConfirmationForm(initial=request.GET)
 
+        using = router.db_for_write(obj._meta.model)
+        collector = Collector(using=using)
+        collector.collect([obj])
+        deletion_objects = []
+        if collector.instances_with_model():
+            for model, instance in collector.instances_with_model():
+                # we could ignore the instance == obj so that the list doesnt contain itself...
+                deletion_objects.append({
+                    "modelname":f"{model.__name__}", 
path: netbox/netbox/views/generic/object_views.py
text: @user1:
What's the need of the f-string here? Isn't the name already a string?

@author:
cleaned up. was a leftover from my first pass

diff_hunk: @@ -334,6 +335,18 @@ def get(self, request, *args, **kwargs):
         obj = self.get_object(**kwargs)
         form = ConfirmationForm(initial=request.GET)
 
+        using = router.db_for_write(obj._meta.model)
+        collector = Collector(using=using)
+        collector.collect([obj])
+        deletion_objects = []
+        if collector.instances_with_model():
path: netbox/netbox/views/generic/object_views.py
text: @user1:
Is this if statement necessary? The for loop should be sufficient because if it returns an empty tuple no iteration will happen 

@author:
removed the unneeded if statement

diff_hunk: @@ -334,6 +335,18 @@ def get(self, request, *args, **kwargs):
         obj = self.get_object(**kwargs)
         form = ConfirmationForm(initial=request.GET)
 
+        using = router.db_for_write(obj._meta.model)
+        collector = Collector(using=using)
+        collector.collect([obj])
+        deletion_objects = []
+        if collector.instances_with_model():
+            for model, instance in collector.instances_with_model():
+                # we could ignore the instance == obj so that the list doesnt contain itself...
+                deletion_objects.append({
path: netbox/netbox/views/generic/object_views.py
text: @author:
this append could be gated to not include the original item
```
        if collector.instances_with_model():
            for model, instance in collector.instances_with_model():
                if not instance == obj:
                    deletion_objects.append({
                        "modelname":f"{model.__name__}", 
                        "object": instance,
                    })
```

diff_hunk: @@ -334,6 +335,18 @@ def get(self, request, *args, **kwargs):
         obj = self.get_object(**kwargs)
         form = ConfirmationForm(initial=request.GET)
 
+        using = router.db_for_write(obj._meta.model)
+        collector = Collector(using=using)
+        collector.collect([obj])
+        deletion_objects = []
path: netbox/netbox/views/generic/object_views.py
text: @user1:
I think it'd be better suited to be called related_objects

diff_hunk: @@ -12,6 +12,14 @@ <h5 class="modal-title">{% trans "Confirm Deletion" %}</h5>
         Are you sure you want to <strong class="text-danger">delete</strong> {{ object_type }} <strong>{{ object }}</strong>?
       {% endblocktrans %}
     </p>
+    {% if deletion_objects %}
+    <p>
+      This will cause deltion of the following items: <br>
path: netbox/templates/htmx/delete_form.html
text: @user1:
Minor typo in deletion keyword