[fix] Address comments

Author: dee077

openwisp_controller/connection/admin.py

@@ -143,6 +143,13 @@ def __init__(
         self.fields["type"].choices = empty_choices + list(allowed_commands.items())
 
     def _scope_devices(self, device_ids):
+        """Keeps only the selected devices that exist and that the user can use.
+
+        Returns the ids which are left. It also notes which organizations
+        those devices belong to, and whether any selected device was dropped
+        because it was deleted or the user does not manage its organization,
+        so that clean() can reject the form in those cases.
+        """
         self._organization_ids = set()
         self._dropped_devices = False
         if not device_ids:
@@ -158,6 +165,14 @@ def _scope_devices(self, device_ids):
         return [str(row[0]) for row in rows]
 
     def _selected_organization_id(self):
+        """Returns the organization of the selected devices, or None.
+
+        When devices are picked from the device list, the organization field
+        is locked and filled in with their organization, so the user can see
+        where the command will run. This works only when all the devices
+        belong to one organization: if they belong to several, None is
+        returned and clean() rejects the form.
+        """
         if len(self._organization_ids) != 1:
             return None
         return str(next(iter(self._organization_ids)))
@@ -610,7 +625,9 @@ def execute_command_view(self, request):
             wizard = request.session.get(self.session_key)
             if request.GET.get("back") and wizard:
                 form = BatchCommandExecutionForm(
-                    initial=self._wizard_initial(wizard), request=request
+                    initial=self._wizard_initial(wizard),
+                    request=request,
+                    device_ids=wizard.get("device_ids"),
                 )
             else:
                 # the wizard is left in the session: another tab may be
@@ -1194,9 +1211,12 @@ def execute_mass_command_admin_action(modeladmin, request, queryset):
         """
         batch_admin = modeladmin.admin_site.get_model_admin(BatchCommand)
         batch_admin._check_add_permission(request)
-        organization_ids = set(queryset.values_list("organization_id", flat=True))
-        if len(organization_ids) > 1:
-            if request.user.is_superuser and queryset.count() == Device.objects.count():
+        organizations = queryset.order_by().values_list("organization_id", flat=True)
+        if organizations.distinct().count() > 1:
+            if (
+                request.user.is_superuser
+                and not Device.objects.exclude(pk__in=queryset.values("pk")).exists()
+            ):
                 return batch_admin._render_execute_page(
                     request,
                     BatchCommandExecutionForm(request=request, system_wide=True),

openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html

@@ -32,7 +32,14 @@
 
 {% block content %}
 <div id="content-main">
-  {# the action renders this page from the device changelist, so the target is explicit #}
+  {% comment %}
+  this page can be opened in two ways: from the menu, or from the "Execute mass
+  command" action of the device list. The action shows this page without a
+  redirect, so the URL in the browser stays on the device list. A form without
+  an "action" attribute posts to the URL in the browser, which would send the
+  data to the device list. Setting "action" makes the form always post to the
+  execute page.
+  {% endcomment %}
   <form method="post" novalidate class="execute-form" action="{% url opts|admin_urlname:'execute' %}">
     {% csrf_token %}
     {{ form.devices }}

openwisp_controller/connection/tests/test_admin.py

@@ -863,6 +863,7 @@ def test_device_action_selection(self):
             name="device-org2", mac_address="00:11:22:33:44:09", organization=org2
         )
         self._login()
+
         with self.subTest("the selection prefills the form"):
             response = self._post_device_action(devices)
             self.assertEqual(response.status_code, 200)
@@ -875,20 +876,23 @@ def test_device_action_selection(self):
             self.assertEqual(form.fields["organization"].initial, str(org.pk))
             for field_name in ("organization", "group", "location"):
                 self.assertTrue(form.fields[field_name].disabled)
+
         with self.subTest("the selection is announced and the wider targets hidden"):
             self.assertContains(
                 response, "The command will run on the 2 devices you selected."
             )
             self.assertContains(response, 'name="devices"')
             self.assertNotContains(response, 'name="group"')
             self.assertNotContains(response, 'name="location"')
+
         with self.subTest("devices of different organizations are refused"):
             response = self._post_device_action([devices[0], device_org2])
             self.assertRedirects(response, self.device_changelist_url)
             self.assertIn(
                 "All devices must belong to the same organization",
                 " ".join(self._messages(response)),
             )
+
         with self.subTest("the selection travels to the review step"):
             response = self._post_execute(devices=self._pk_list(devices))
             self.assertEqual(response.status_code, 302)
@@ -900,23 +904,52 @@ def test_device_action_selection(self):
             self.assertEqual(response.context["device_count"], 2)
             self.assertEqual(response.context["targets_display"], "2 selected devices")
             self.assertEqual(set(response.context["cl"].queryset), set(devices))
+
         with self.subTest("only the selected devices are executed"):
             self._post_confirm(wizard["token"])
             batch = BatchCommand.objects.get()
             self.assertEqual(set(batch.devices.all()), set(devices))
-        with self.subTest("a single device is announced in the singular"):
+
+        with self.subTest("the texts use the singular when one device is selected"):
             response = self._post_device_action(devices[:1])
             self.assertContains(
                 response, "The command will run on the device you selected."
             )
+            self.assertNotContains(response, "devices you selected")
             self._start_wizard(devices=self._pk_list(devices[:1]))
             response = self.client.get(self.confirm_url)
             self.assertEqual(response.context["targets_display"], "1 selected device")
 
+        with self.subTest("the selection is kept when going back to edit"):
+            self._create_device(
+                name="device-not-selected",
+                mac_address="00:11:22:33:44:08",
+                organization=org,
+            )
+            selected = {str(device.pk) for device in devices}
+            self._start_wizard(devices=self._pk_list(devices))
+            self.client.get(self.confirm_url)
+            response = self.client.get(f"{self.execute_url}?back=1")
+            form = response.context["form"]
+            self.assertEqual(set(form.device_ids), selected)
+            for field_name in ("organization", "group", "location"):
+                self.assertTrue(form.fields[field_name].disabled)
+            self.assertContains(
+                response, "The command will run on the 2 devices you selected."
+            )
+            self._start_wizard(
+                devices=form.fields["devices"].initial or "", label="edited-label"
+            )
+            wizard = self.client.session[BatchCommandAdmin.session_key]
+            self.assertEqual(wizard["label"], "edited-label")
+            self.assertEqual(set(wizard["device_ids"]), selected)
+            response = self.client.get(self.confirm_url)
+            self.assertEqual(set(response.context["cl"].queryset), set(devices))
+
     def test_device_action_system_wide(self):
         org = self._get_org()
         org2 = self._create_org(name="org2", slug="org2")
-        devices = [
+        org1_devices = [
             self._create_device(
                 name=f"device{index}",
                 mac_address=f"00:11:22:33:44:0{index}",
@@ -928,10 +961,10 @@ def test_device_action_system_wide(self):
             name="device-org2", mac_address="00:11:22:33:44:09", organization=org2
         )
         self._login()
-        with self.subTest("selecting every device is system wide"):
-            response = self._post_device_action(
-                devices + [device_org2], select_across=True
-            )
+
+        with self.subTest("selecting every device runs the command on all of them"):
+            all_devices = org1_devices + [device_org2]
+            response = self._post_device_action(all_devices, select_across=True)
             self.assertEqual(response.status_code, 200)
             form = response.context["form"]
             self.assertEqual(form.device_ids, [])
@@ -943,6 +976,19 @@ def test_device_action_system_wide(self):
             self.assertNotContains(response, 'name="organization"')
             self.assertNotContains(response, 'name="group"')
             self.assertNotContains(response, 'name="location"')
+            wizard = self._start_wizard()
+            response = self.client.get(self.confirm_url)
+            self.assertEqual(response.context["targets_display"], "All devices")
+            self.assertEqual(set(response.context["cl"].queryset), set(all_devices))
+            response = self._post_confirm(wizard["token"])
+            self.assertIn(
+                "Mass command executed successfully.", self._messages(response)
+            )
+            batch = BatchCommand.objects.get()
+            self.assertIsNone(batch.organization_id)
+            self.assertEqual(set(batch.devices.all()), set(all_devices))
+        BatchCommand.objects.all().delete()
+
         with self.subTest("the excluded devices are left out of the batch"):
             wizard = self._start_wizard()
             self.client.get(self.confirm_url)
@@ -955,15 +1001,16 @@ def test_device_action_system_wide(self):
             self.assertIsNone(batch.organization_id)
             self.assertIsNone(batch.group_id)
             self.assertIsNone(batch.location_id)
-            self.assertEqual(set(batch.devices.all()), set(devices))
+            self.assertEqual(set(batch.devices.all()), set(org1_devices))
+
         with self.subTest("a partial multi organization selection is refused"):
             self._create_device(
                 name="excluded-by-the-search",
                 mac_address="00:11:22:33:44:08",
                 organization=org2,
             )
             response = self._post_device_action(
-                devices + [device_org2],
+                org1_devices + [device_org2],
                 select_across=True,
                 query={"q": "device"},
             )
@@ -983,6 +1030,7 @@ def test_device_action_permissions_and_scope(self):
         device_admin = admin.site.get_model_admin(Device)
         request = RequestFactory().get(self.device_changelist_url)
         action_name = "execute_mass_command_admin_action"
+
         with self.subTest("the device change permission is required"):
             viewer = self._create_operator(
                 organizations=[org], username="viewer", email="viewer@test.com"
@@ -993,10 +1041,12 @@ def test_device_action_permissions_and_scope(self):
             )
             request.user = viewer
             self.assertNotIn(action_name, device_admin.get_actions(request))
+
         with self.subTest("the operator group can use the action"):
             operator = self._create_operator(organizations=[org])
             request.user = operator
             self.assertIn(action_name, device_admin.get_actions(request))
+
         with self.subTest("the batch command add permission is enforced"):
             viewer.user_permissions.set(
                 Permission.objects.filter(
@@ -1005,6 +1055,7 @@ def test_device_action_permissions_and_scope(self):
             )
             self.client.force_login(viewer)
             self.assertEqual(self._post_device_action([device]).status_code, 403)
+
         with self.subTest("devices of unmanaged organizations are dropped"):
             self.client.force_login(operator)
             response = self._post_execute(devices=self._pk_list([device, device2]))
@@ -1014,10 +1065,12 @@ def test_device_action_permissions_and_scope(self):
                 "Some of the selected devices are no longer available.",
                 form.errors["__all__"],
             )
+
         with self.subTest("devices which disappeared are dropped"):
             self._login()
             response = self._post_execute(devices=f"{device.pk},{uuid4()}")
             self.assertEqual(response.context["form"].device_ids, [str(device.pk)])
+
         with self.subTest("mixed organizations are refused by the form"):
             response = self._post_execute(devices=self._pk_list([device, device2]))
             form = response.context["form"]
@@ -1026,6 +1079,7 @@ def test_device_action_permissions_and_scope(self):
                 "All devices must belong to the same organization",
                 " ".join(form.errors["__all__"]),
             )
+
         with self.subTest("selecting every device is not system wide for operators"):
             multi_operator = self._create_operator(
                 organizations=[org, org2], username="multi", email="multi@test.com"
@@ -1037,6 +1091,7 @@ def test_device_action_permissions_and_scope(self):
                 "All devices must belong to the same organization",
                 " ".join(self._messages(response)),
             )
+
         with self.subTest("an operator executes the devices it manages"):
             self.client.force_login(operator)
             wizard = self._start_wizard(devices=self._pk_list([device]))