[fix] CI and a bug in admin detail page

- Restored the stderr capture in the two command task tests which still
  used redirect_stderr after its import was removed, fixing the failing
  test suite
- Built the filters of the detail page from the organization of the batch
  instead of its command rows, so that they are rendered right after the
  execution instead of appearing only once the worker created the commands
- Hid the location and the device group filter when the batch was targeted
  on one of them
- Formatted the initial and the live timestamps through one locale aware
  path, so that a row does not change format after it is updated

Author: dee077

docs/user/websocket-api.rst

@@ -220,7 +220,10 @@ When the mass command itself changes, for example when it moves from
 
 The status and the timestamps are sent as they are stored, without
 translation or formatting, so that each client can render them with its
-own language and time zone.
+own language and time zone. Command rows carry ``modified_display`` as
+well, which is the same timestamp already formatted with the locale and
+the time zone of the server: the admin uses it so that a row updated over
+the websocket reads exactly like the rows rendered with the page.
 
 When the command of one device changes:
 
@@ -237,6 +240,8 @@ When the command of one device changes:
         "output": "<string>",            // Output collected so far
         "created": "<string>",           // ISO 8601 timestamp
         "modified": "<string>",          // ISO 8601 timestamp
+        "modified_display": "<string>",  // Modified, formatted by the server with its
+                                         // own locale and time zone
         "index": <integer>,              // Position of the row, sent only for new commands
         "affected_devices": <integer>,   // Commands created so far, sent with "index"
         "total_rows": <integer>          // Affected plus skipped devices, sent with "index"

openwisp_controller/connection/admin.py

@@ -10,7 +10,7 @@
 from django.contrib import admin, messages
 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError
 from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
-from django.db.models import Count, Q
+from django.db.models import Count
 from django.http import HttpResponseForbidden, HttpResponseNotAllowed, JsonResponse
 from django.shortcuts import redirect
 from django.template.response import TemplateResponse
@@ -28,6 +28,7 @@
 from .commands import ORGANIZATION_COMMAND_SCHEMA
 from .filters import GroupFilter, LocationFilter, TypeFilter
 from .schema import schema
+from .utils import format_modified
 from .widgets import (
     BatchCommandSchemaWidget,
     CommandSchemaWidget,
@@ -895,52 +896,52 @@ def _make_choice(current_value, display, param_name, value):
 
         filter_specs.append(SimpleNamespace(title=_("status"), choices=status_choices))
 
-        batch_devices = Device.objects.filter(
-            Q(command__batch_command=obj) | Q(pk__in=obj.skipped_device_ids)
-        )
+        locations = Location.objects.all()
+        groups = DeviceGroup.objects.all()
+        if obj.organization_id:
+            locations = locations.filter(organization_id=obj.organization_id)
+            groups = groups.filter(organization_id=obj.organization_id)
         if not request.user.is_superuser:
-            batch_devices = batch_devices.filter(
+            locations = locations.filter(
+                organization_id__in=request.user.organizations_managed
+            )
+            groups = groups.filter(
                 organization_id__in=request.user.organizations_managed
             )
 
         # Location filter
-        location_spec = self._build_related_filter(
-            _("location"),
-            "location_id",
-            current_location or "",
-            batch_devices.exclude(devicelocation__location__isnull=True)
-            .values_list(
-                "devicelocation__location__id",
-                "devicelocation__location__name",
+        location_spec = None
+        if not obj.location_id:
+            location_spec = self._build_related_filter(
+                _("location"),
+                "location_id",
+                current_location or "",
+                locations.values_list("id", "name"),
+                _make_choice,
             )
-            .distinct(),
-            _make_choice,
-        )
         if location_spec:
             filter_specs.append(location_spec)
 
         # Group filter
-        group_spec = self._build_related_filter(
-            _("device group"),
-            "group_id",
-            current_group or "",
-            batch_devices.filter(group__isnull=False)
-            .values_list("group__id", "group__name")
-            .distinct(),
-            _make_choice,
-        )
+        group_spec = None
+        if not obj.group_id:
+            group_spec = self._build_related_filter(
+                _("device group"),
+                "group_id",
+                current_group or "",
+                groups.values_list("id", "name"),
+                _make_choice,
+            )
         if group_spec:
             filter_specs.append(group_spec)
 
-        # Organization filter (superusers only)
-        if request.user.is_superuser:
+        # Organization filter (system wide batches only, superusers only)
+        if request.user.is_superuser and not obj.organization_id:
             org_spec = self._build_related_filter(
                 _("organization"),
                 "organization_id",
                 current_org or "",
-                batch_devices.values_list(
-                    "organization__id", "organization__name"
-                ).distinct(),
+                Organization.objects.values_list("id", "name"),
                 _make_choice,
             )
             if org_spec:
@@ -967,7 +968,7 @@ def _command_row(command):
             "status": command.status,
             "status_display": command.get_status_display(),
             "output": command.output_preview,
-            "modified": command.modified,
+            "modified_display": format_modified(command.modified),
             "is_skipped": False,
         }
 

openwisp_controller/connection/base/models.py

@@ -855,6 +855,7 @@ def build_skipped_row(device_pk, skipped):
             "status_display": gettext("skipped"),
             "output": skipped["error"],
             "modified": None,
+            "modified_display": "",
             "is_skipped": True,
         }
 

openwisp_controller/connection/channels/consumers.py

@@ -7,6 +7,7 @@
 
 from ...config.base.channels_consumer import BaseDeviceConsumer
 from ..api.serializers import BatchCommandSerializer, CommandSerializer
+from ..utils import format_modified
 
 logger = logging.getLogger(__name__)
 
@@ -126,6 +127,7 @@ def _handle_current_state_request(self, page=None, filters=None):
             row.pop("input", None)
             row["device_name"] = command.device.name
             row["output"] = command.output_preview
+            row["modified_display"] = format_modified(command.modified)
             commands.append(row)
         commands += batch.get_skipped_rows(
             max(0, start - commands_count),

openwisp_controller/connection/handlers.py

@@ -7,6 +7,8 @@
 from django.dispatch import receiver
 from swapper import load_model
 
+from .utils import format_modified
+
 logger = logging.getLogger(__name__)
 
 Command = load_model("connection", "Command")
@@ -44,6 +46,7 @@ def command_save_handler(sender, created, instance, **kwargs):
         batch_data.pop("input", None)
         batch_data["device_name"] = instance.device.name
         batch_data["output"] = instance.output_preview
+        batch_data["modified_display"] = format_modified(instance.modified)
         batch_data["type"] = "command_update"
         if created:
             batch = instance.batch_command

openwisp_controller/connection/static/connection/js/batch-command.js

@@ -94,17 +94,6 @@ function getStatusLabel(status) {
   return labels[status] || status;
 }
 
-function getFormattedDateTimeString(dateTimeString) {
-  if (!dateTimeString) {
-    return "-";
-  }
-  const formattedString = new Date(dateTimeString).strftime("%B %d, %Y %I:%M %p"),
-    stringArray = formattedString.split(" ");
-  stringArray[0] = stringArray[0].substring(0, 4) + ".";
-  stringArray[4] = stringArray[4] == "AM" ? "a.m." : "p.m.";
-  return stringArray.join(" ");
-}
-
 function handleBatchStatusMessage($, data, websocket) {
   const $status = $(".field-colored_status .readonly .command-status");
   if ($status.length && data.status) {
@@ -258,7 +247,7 @@ function updateRow($, $row, data) {
     .addClass("command-status " + data.status)
     .text(getStatusLabel(data.status));
   $row.find(".command-output pre").text(data.output || "-");
-  $row.find("td:last-child").text(getFormattedDateTimeString(data.modified));
+  $row.find("td:last-child").text(data.modified_display || "-");
 }
 
 function insertRow($, data) {
@@ -300,7 +289,7 @@ function insertRow($, data) {
       .addClass("command-output")
       .append($("<pre>").text(data.output || "-")),
   );
-  $row.append($("<td>").text(getFormattedDateTimeString(data.modified)));
+  $row.append($("<td>").text(data.modified_display || "-"));
   $tableBody.append($row);
 }
 

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

@@ -137,7 +137,7 @@ <h3 id="changelist-filter-clear" class="filter-clear-heading">
         <td class="command-output">
           <pre>{{ command.output|default:"-" }}</pre>
         </td>
-        <td>{{ command.modified|date:"DATETIME_FORMAT"|default:"-" }}</td>
+        <td>{{ command.modified_display|default:"-" }}</td>
       </tr>
       {% empty %}
       <tr>

openwisp_controller/connection/tests/pytest.py

@@ -15,6 +15,7 @@
 
 from .. import handlers
 from ..channels.consumers import BatchCommandConsumer
+from ..utils import format_modified
 from .test_models import BaseTestModels
 
 User = get_user_model()
@@ -264,6 +265,7 @@ async def test_batch_command_consumer_current_state(
                 command_row["modified"]
                 == timezone.localtime(command.modified).isoformat()
             )
+            assert command_row["modified_display"] == format_modified(command.modified)
             assert "input" not in command_row
             await communicator.send_json_to(
                 {"type": "request_current_state", "page": 2}
@@ -309,7 +311,14 @@ async def test_batch_command_consumer_current_state(
                     {"type": "request_current_state", "page": page}
                 )
                 response = await communicator.receive_json_from()
+                assert response["page"] == 1
                 assert response["commands"] == page1["commands"]
+            await communicator.send_json_to(
+                {"type": "request_current_state", "page": 99}
+            )
+            clamped = await communicator.receive_json_from()
+            assert clamped["page"] == 2
+            assert clamped["commands"] == page2["commands"]
         await communicator.disconnect()
         communicator, connected = await self._connect(batch.pk, admin_user)
         assert connected is True

openwisp_controller/connection/tests/test_admin.py

@@ -24,6 +24,7 @@
 from ..admin import BatchCommandAdmin, BatchCommandExecutionForm
 from ..connectors.ssh import Ssh
 from ..filters import GroupFilter, LocationFilter, TypeFilter
+from ..utils import format_modified
 from ..widgets import CredentialsSchemaWidget
 from .utils import BatchCommandMixin, CreateConnectionsMixin
 
@@ -969,6 +970,28 @@ def test_change_view_command_rows(self):
                 self.assertEqual(rows[0]["output"], "… last")
                 self.assertEqual(rows[0]["status_display"], "in progress")
                 self.assertFalse(rows[0]["is_skipped"])
+                self.assertEqual(
+                    rows[0]["modified_display"],
+                    format_modified(commands[0].modified),
+                )
+
+            with self.subTest("the rows follow the locale and the time zone"):
+                default = self.client.get(url).context["commands"][0]
+                with override_settings(
+                    LANGUAGE_CODE="it", TIME_ZONE="Pacific/Auckland"
+                ):
+                    localized = self.client.get(url).context["commands"][0]
+                    self.assertEqual(
+                        localized["modified_display"],
+                        format_modified(commands[0].modified),
+                    )
+                self.assertNotEqual(
+                    localized["modified_display"], default["modified_display"]
+                )
+
+            with self.subTest("skipped devices have no timestamp"):
+                rows = self.client.get(url, {"page": 3}).context["commands"]
+                self.assertEqual([row["modified_display"] for row in rows], [""] * 3)
 
             with self.subTest("the page spanning commands and skipped devices"):
                 rows = self.client.get(url, {"page": 2}).context["commands"]
@@ -1025,7 +1048,7 @@ def test_change_view_filters(self):
         )
         device = self._create_device(organization=org, group=group)
         DeviceLocation.objects.create(content_object=device, location=location)
-        batch = self._create_batch_command(organization=org, group=group)
+        batch = self._create_batch_command(organization=org)
         other_group = DeviceGroup.objects.create(name="skipped-group", organization=org)
         skipped_device = self._create_device(
             name="skipped-device",
@@ -1133,20 +1156,58 @@ def test_change_view_filters(self):
             titles = [str(spec.title) for spec in response.context["filter_specs"]]
             self.assertNotIn("organization", titles)
 
-        with self.subTest("the filters do not offer other organizations"):
-            specs = {
+        def _filter_specs(target):
+            return {
                 str(spec.title): [str(choice["display"]) for choice in spec.choices]
-                for spec in self.client.get(url).context["filter_specs"]
+                for spec in self.client.get(target).context["filter_specs"]
             }
-            self.assertNotIn(transferred_group.name, specs["device group"])
-            self.assertIn(other_group.name, specs["device group"])
-            self.assertNotIn(transferred_location.name, specs["location"])
+
+        with self.subTest("the filters do not offer other organizations"):
+            for login in (lambda: self.client.force_login(operator), self._login):
+                login()
+                specs = _filter_specs(url)
+                self.assertNotIn(transferred_group.name, specs["device group"])
+                self.assertIn(other_group.name, specs["device group"])
+                self.assertNotIn(transferred_location.name, specs["location"])
+                self.assertIn(location.name, specs["location"])
+
+        with self.subTest("the filters do not wait for the commands to be created"):
+            fresh = self._create_batch_command(organization=org)
+            specs = _filter_specs(
+                reverse(f"admin:{self.app_label}_batchcommand_change", args=[fresh.pk])
+            )
+            self.assertIn(group.name, specs["device group"])
             self.assertIn(location.name, specs["location"])
-            self._login()
-            specs = {
-                str(spec.title): [str(choice["display"]) for choice in spec.choices]
-                for spec in self.client.get(url).context["filter_specs"]
-            }
+            self.assertNotIn("organization", specs)
+
+        with self.subTest("the target of the batch is not offered as a filter"):
+            targeted = self._create_batch_command(organization=org, group=group)
+            specs = _filter_specs(
+                reverse(
+                    f"admin:{self.app_label}_batchcommand_change", args=[targeted.pk]
+                )
+            )
+            self.assertNotIn("device group", specs)
+            self.assertIn("location", specs)
+            targeted = self._create_batch_command(organization=org, location=location)
+            specs = _filter_specs(
+                reverse(
+                    f"admin:{self.app_label}_batchcommand_change", args=[targeted.pk]
+                )
+            )
+            self.assertNotIn("location", specs)
+            self.assertIn("device group", specs)
+
+        with self.subTest("a system wide batch offers every organization"):
+            system_wide = self._create_batch_command(organization=None)
+            specs = _filter_specs(
+                reverse(
+                    f"admin:{self.app_label}_batchcommand_change",
+                    args=[system_wide.pk],
+                )
+            )
+            self.assertIn(org.name, specs["organization"])
+            self.assertIn(org2.name, specs["organization"])
             self.assertIn(transferred_group.name, specs["device group"])
             self.assertIn(transferred_location.name, specs["location"])
 

openwisp_controller/connection/tests/test_models.py

@@ -1078,6 +1078,7 @@ def test_batch_command_skipped_devices(self):
                     "status_display": "skipped",
                     "output": "error 0",
                     "modified": None,
+                    "modified_display": "",
                     "is_skipped": True,
                 },
             )