[fix] Allowed deleting device with "deactivating" config status #949

docs/user/device-config-status.rst

@@ -36,3 +36,9 @@ scheduled to be removed from the device.
 The device has been deactivated. The configuration applied through
 OpenWISP has been removed, and any other operation to manage the device
 will be prevented or rejected.
+
+.. note::
+
+    If a device becomes unreachable (e.g., lost, stolen, or decommissioned)
+    before it can be properly deactivated, you can remove it from OpenWISP
+    by deleting the device from the system.

openwisp_controller/config/admin.py

@@ -1,11 +1,13 @@
 import json
 import logging
+from collections.abc import Iterable
 
 import reversion
 from django import forms
 from django.conf import settings
 from django.contrib import admin, messages
 from django.contrib.admin import helpers
+from django.contrib.admin.actions import delete_selected
 from django.contrib.admin.models import ADDITION, LogEntry
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import (
@@ -566,8 +568,6 @@ def has_delete_permission(self, request, obj=None):
         perm = super().has_delete_permission(request)
         if not obj:
             return perm
-        if obj._has_config():
-            perm = perm and obj.config.is_deactivated()
         return perm and obj.is_deactivated()
 
     def save_form(self, request, form, change):
@@ -765,22 +765,42 @@ def deactivate_device(self, request, queryset):
     def activate_device(self, request, queryset):
         self._change_device_status(request, queryset, 'activate')
 
-    def get_deleted_objects(self, objs, request, *args, **kwargs):
-        # Ensure that all selected devices can be deleted, i.e.
-        # the device should be flagged as deactivated and if it has
-        # a config object, it's status should be "deactivated".
-        active_devices = []
-        for obj in objs:
-            if not self.has_delete_permission(request, obj):
-                active_devices.append(obj)
-        if active_devices:
-            return (
-                active_devices,
-                {self.model._meta.verbose_name_plural: len(active_devices)},
-                ['active_devices'],
-                [],
+    @admin.action(description=delete_selected.short_description, permissions=['delete'])
+    def delete_selected(self, request, queryset):
+        response = delete_selected(self, request, queryset)
+        if not response:
+            return response
+        if 'active_devices' in response.context_data.get('perms_lacking', {}):
+            active_devices = []
+            for device in queryset.iterator():
+                if not device.is_deactivated() or (
+                    device._has_config() and not device.config.is_deactivated()
+                ):
+                    active_devices.append(self._get_device_path(device))
+            response.context_data.update(
+                {
+                    'active_devices': active_devices,
+                    'perms_lacking': set(),
+                    'title': _('Are you sure?'),
+                }
             )
-        return super().get_deleted_objects(objs, request, *args, **kwargs)
+        return response
+
+    def get_deleted_objects(self, objs, request, *args, **kwargs):
+        to_delete, model_count, perms_needed, protected = super().get_deleted_objects(
+            objs, request, *args, **kwargs
+        )
+        if (
+            isinstance(perms_needed, Iterable)
+            and len(perms_needed) == 1
+            and list(perms_needed)[0] == self.model._meta.verbose_name
+            and objs.filter(_is_deactivated=False).exists()
+        ):
+            if request.POST.get("post"):
+                perms_needed = set()
+            else:
+                perms_needed = {'active_devices'}
+        return to_delete, model_count, perms_needed, protected
 
     def get_fields(self, request, obj=None):
         """
@@ -900,6 +920,17 @@ def recover_view(self, request, version_id, extra_context=None):
         request._recover_view = True
         return super().recover_view(request, version_id, extra_context)
 
+    def delete_view(self, request, object_id, extra_context=None):
+        extra_context = extra_context or {}
+        obj = self.get_object(request, object_id)
+        if obj and obj._has_config() and not obj.config.is_deactivated():
+            extra_context['deactivating_warning'] = True
+        return super().delete_view(request, object_id, extra_context)
+
+    def delete_model(self, request, obj):
+        force_delete = request.POST.get('force_delete') == 'true'
+        obj.delete(check_deactivated=not force_delete)
+
     def get_inlines(self, request, obj):
         inlines = super().get_inlines(request, obj)
         # this only makes sense in existing devices

openwisp_controller/config/api/views.py

@@ -107,6 +107,10 @@ class DeviceDetailView(ProtectedAPIMixin, RetrieveUpdateDestroyAPIView):
     queryset = Device.objects.select_related('config', 'group', 'organization')
     permission_classes = ProtectedAPIMixin.permission_classes + (DevicePermission,)
 
+    def perform_destroy(self, instance):
+        force_deletion = self.request.query_params.get('force', None) == 'true'
+        instance.delete(check_deactivated=(not force_deletion))
+
 
 class DeviceActivateView(ProtectedAPIMixin, GenericAPIView):
     serializer_class = serializers.Serializer

openwisp_controller/config/static/config/css/device-delete-confirmation.css

@@ -0,0 +1,8 @@
+#deactivating-warning .warning p {
+  margin-top: 0px;
+}
+#main ul.messagelist li.warning ul li {
+  display: list-item;
+  padding: 0px;
+  background: inherit;
+}

openwisp_controller/config/static/config/js/device-delete-confirmation.js

@@ -0,0 +1,12 @@
+"use strict";
+
+(function ($) {
+  $(document).ready(function () {
+    $("#warning-ack").click(function (event) {
+      event.preventDefault();
+      $("#deactivating-warning").slideUp("fast");
+      $("#delete-confirm-container").slideDown("fast");
+      $('input[name="force_delete"]').val("true");
+    });
+  });
+})(django.jQuery);

openwisp_controller/config/templates/admin/config/device/delete_confirmation.html

@@ -0,0 +1,62 @@
+{% extends "admin/delete_confirmation.html" %}
+{% load i18n static %}
+
+{% block extrastyle %}
+{{ block.super }}
+<link rel="stylesheet" type="text/css" href="{% static 'config/css/device-delete-confirmation.css' %}" />
+{% endblock extrastyle %}
+
+{% block content %}
+{% if perms_lacking %}
+    <p>{% blocktranslate with escaped_object=object %}Deleting the {{ object_name }} '{{ escaped_object }}' would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:{% endblocktranslate %}</p>
+    <ul>
+    {% for obj in perms_lacking %}
+        <li>{{ obj }}</li>
+    {% endfor %}
+    </ul>
+{% elif protected %}
+    <p>{% blocktranslate with escaped_object=object %}Deleting the {{ object_name }} '{{ escaped_object }}' would require deleting the following protected related objects:{% endblocktranslate %}</p>
+    <ul>
+    {% for obj in protected %}
+        <li>{{ obj }}</li>
+    {% endfor %}
+    </ul>
+{% else %}
+    {% if deactivating_warning %}
+    <div id="deactivating-warning">
+        <ul class="messagelist">
+            <li class="warning">
+                <p>{% translate 'The device is still in the deactivating state, meaning its configuration is still present on the device. If you wish to remove the configuration from the device, please wait until the config status changes to "deactivated". Proceeding will delete the device from OpenWISP without ensuring its configuration has been removed.' %}</p>
+                <form>
+                    <input type="submit" class="button danger-btn" id="warning-ack"
+                        value="{% translate 'I understand the risks, delete the device' %}">
+                    <a class="button cancel-link">{% translate 'No, take me back' %}</a>
+                </form>
+            </li>
+        </ul>
+    </div>
+    {% endif %}
+    <div id="delete-confirm-container" {% if deactivating_warning %}style="display:none;"{% endif %}>
+        <p>{% blocktranslate with escaped_object=object %}Are you sure you want to delete the {{ object_name }}
+            "{{ escaped_object }}"? All of the following related items will be deleted:{% endblocktranslate %}</p>
+        {% include "admin/includes/object_delete_summary.html" %}
+        <h2>{% translate "Objects" %}</h2>
+        <ul id="deleted-objects">{{ deleted_objects|unordered_list }}</ul>
+        <form method="post">{% csrf_token %}
+            <div>
+                <input type="hidden" name="post" value="yes">
+                {% if is_popup %}<input type="hidden" name="{{ is_popup_var }}" value="1">{% endif %}
+                {% if to_field %}<input type="hidden" name="{{ to_field_var }}" value="{{ to_field }}">{% endif %}
+                <input type="submit" value="{% translate 'Yes, I’m sure' %}">
+                {% if deactivating_warning %}<input type="hidden" name="force_delete" value="false">{% endif %}
+                <a href="#" class="button cancel-link">{% translate "No, take me back" %}</a>
+            </div>
+        </form>
+    </div>
+{% endif %}
+{% endblock %}
+
+{% block footer %}
+{{ block.super }}
+<script type="text/javascript" src="{% static 'config/js/device-delete-confirmation.js' %}"></script>
+{% endblock %}

openwisp_controller/config/templates/admin/config/device/delete_selected_confirmation.html

@@ -1,36 +1,76 @@
 {% extends "admin/delete_selected_confirmation.html" %}
 {% load i18n l10n admin_urls static %}
 
+{% block extrastyle %}
+{{ block.super }}
+<link rel="stylesheet" type="text/css" href="{% static 'config/css/device-delete-confirmation.css' %}" />
+{% endblock extrastyle %}
+
 {% block content %}
 {% if perms_lacking %}
-    {% if perms_lacking|first == 'active_devices' %}
-        <p>{% blocktranslate %}You have selected the following active device{{ model_count | pluralize  }} to delete:{% endblocktranslate %}</p>
-        <ul>{{ deletable_objects|first|unordered_list }}</ul>
-        <p>{% blocktrans %}It is required to flag the device as deactivated before deleting the device. If the device has configuration, then wait till the configuration status changes to "deactivated" before deleting the device.{% endblocktrans %}</p>
-    {% else %}
-        <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:{% endblocktranslate %}</p>
-        <ul>{{ perms_lacking|unordered_list }}</ul>
-    {% endif %}
+    <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:{% endblocktranslate %}</p>
+    <ul>{{ perms_lacking|unordered_list }}</ul>
 {% elif protected %}
     <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would require deleting the following protected related objects:{% endblocktranslate %}</p>
     <ul>{{ protected|unordered_list }}</ul>
 {% else %}
-    <p>{% blocktranslate %}Are you sure you want to delete the selected {{ objects_name }}? All of the following objects and their related items will be deleted:{% endblocktranslate %}</p>
-    {% include "admin/includes/object_delete_summary.html" %}
-    <h2>{% translate "Objects" %}</h2>
-    {% for deletable_object in deletable_objects %}
-        <ul>{{ deletable_object|unordered_list }}</ul>
-    {% endfor %}
-    <form method="post">{% csrf_token %}
-    <div>
-    {% for obj in queryset %}
-    <input type="hidden" name="{{ action_checkbox_name }}" value="{{ obj.pk|unlocalize }}">
-    {% endfor %}
-    <input type="hidden" name="action" value="delete_selected">
-    <input type="hidden" name="post" value="yes">
-    <input type="submit" value="{% translate 'Yes, I’m sure' %}">
-    <a href="#" class="button cancel-link">{% translate "No, take me back" %}</a>
+    {% if active_devices %}
+    <div id="deactivating-warning">
+        <ul class="messagelist">
+            <li class="warning">
+                <p>
+                    {% blocktranslate count counter=active_devices|length %}
+                    The following device you selected for deletion is not deactivated
+                    (either it is active or its configuration status is still "deactivating"):
+                    {% plural %}
+                    The following devices you selected for deletion are not deactivated
+                    (either they are active or their configuration status is still "deactivating"):
+                    {% endblocktranslate %}
+                </p>
+                <ul>{{ active_devices|unordered_list }}</ul>
+                <p>
+                    {% blocktranslate count counter=active_devices|length %}
+                    If you wish to remove the configuration from the device, please wait until its
+                    configuration status changes to "deactivated". Proceeding will delete the device
+                    from OpenWISP without ensuring its configuration has been removed.
+                    {% plural %}
+                    If you wish to remove the configurations from the devices, please wait until their
+                    configuration status change to "deactivated." Proceeding will delete the devices
+                    from OpenWISP without ensuring their configurations have been removed.
+                    {% endblocktranslate %}
+                </p>
+                <form>
+                    <input type="submit" class="button danger-btn" id="warning-ack"
+                        value="{% trans "I understand the risks, delete the device" %}">
+                    <a class="button cancel-link">No, take me back</a>
+                </form>
+            </li>
+        </ul>
+    </div>
+    {% endif %}
+    <div id="delete-confirm-container" {% if active_devices %}style="display:none;"{% endif %}>
+        <p>{% blocktranslate %}Are you sure you want to delete the selected {{ objects_name }}? All of the following objects and their related items will be deleted:{% endblocktranslate %}</p>
+        {% include "admin/includes/object_delete_summary.html" %}
+        <h2>{% translate "Objects" %}</h2>
+        {% for deletable_object in deletable_objects %}
+            <ul>{{ deletable_object|unordered_list }}</ul>
+        {% endfor %}
+        <form method="post">{% csrf_token %}
+        <div>
+        {% for obj in queryset %}
+        <input type="hidden" name="{{ action_checkbox_name }}" value="{{ obj.pk|unlocalize }}">
+        {% endfor %}
+        <input type="hidden" name="action" value="delete_selected">
+        <input type="hidden" name="post" value="yes">
+        <input type="submit" value="{% translate 'Yes, I’m sure' %}">
+        <a href="#" class="button cancel-link">{% translate "No, take me back" %}</a>
+        </div>
+        </form>
     </div>
-    </form>
 {% endif %}
 {% endblock %}
+
+{% block footer %}
+{{ block.super }}
+<script type="text/javascript" src="{% static 'config/js/device-delete-confirmation.js' %}"></script>
+{% endblock %}

openwisp_controller/config/tests/test_admin.py

@@ -2120,9 +2120,8 @@ def test_device_with_config_change_deactivate_deactivate(self):
         )
         # Save buttons are absent on deactivated device
         self.assertNotContains(response, self._save_btn_html)
-        # Delete button is not present if config status is deactivating
         self.assertEqual(device.config.status, 'deactivating')
-        self.assertNotContains(response, delete_btn_html)
+        self.assertContains(response, delete_btn_html)
         self.assertNotContains(response, self._deactivate_btn_html)
         self.assertContains(response, self._activate_btn_html)
         # Verify adding a new DeviceLocation and DeviceConnection is not allowed

openwisp_controller/config/tests/test_api.py

@@ -539,6 +539,22 @@ def test_device_delete_api(self):
             self.assertEqual(response.status_code, 204)
             self.assertEqual(Device.objects.count(), 0)
 
+    def test_deactivating_device_force_deletion(self):
+        self._create_template(required=True)
+        device = self._create_device()
+        config = self._create_config(device=device)
+        device.deactivate()
+        path = reverse('config_api:device_detail', args=[device.pk])
+
+        with self.subTest(
+            'Test force deleting device with config in deactivating state'
+        ):
+            self.assertEqual(device.is_deactivated(), True)
+            self.assertEqual(config.is_deactivating(), True)
+            response = self.client.delete(f'{path}?force=true')
+            self.assertEqual(response.status_code, 204)
+            self.assertEqual(Device.objects.count(), 0)
+
     def test_template_create_no_org_api(self):
         self.assertEqual(Template.objects.count(), 0)
         path = reverse('config_api:template_list')