From 9c2a817cdd466b8368030dfb7999775858729620 Mon Sep 17 00:00:00 2001 From: dee077 Date: Wed, 1 Jul 2026 03:47:25 +0530 Subject: [PATCH 01/13] [feature] Add Django admin workflow for mass command execution and real-time monitoring #1345 - Custom admin change form with filtered/paginated commands table - Merged skipped device rows into main commands table - Colored status using CSS variables - Real-time polling for in-progress batches - Custom CSS and JS for batch command admin Fixes #1345 --- openwisp_controller/connection/admin.py | 210 +++++++++++++++++- openwisp_controller/connection/base/models.py | 2 +- .../static/connection/css/batch-command.css | 144 ++++++++++++ .../static/connection/js/batch-command.js | 43 ++++ .../batch_command_change_form.html | 174 +++++++++++++++ 5 files changed, 570 insertions(+), 3 deletions(-) create mode 100644 openwisp_controller/connection/static/connection/css/batch-command.css create mode 100644 openwisp_controller/connection/static/connection/js/batch-command.js create mode 100644 openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index f15c98d58..60d853b7c 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,17 +1,20 @@ +import json from datetime import timedelta import reversion import swapper from django import forms from django.contrib import admin +from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.http import HttpResponseForbidden, JsonResponse from django.urls import path, resolve -from django.utils.html import format_html +from django.utils.html import format_html, format_html_join +from django.utils.safestring import mark_safe from django.utils.timezone import localtime from django.utils.translation import gettext_lazy as _ from openwisp_users.multitenancy import MultitenantOrgFilter -from openwisp_utils.admin import TimeReadonlyAdminMixin +from openwisp_utils.admin import ReadOnlyAdmin, TimeReadonlyAdminMixin from ..admin import MultitenantAdminMixin from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin @@ -21,6 +24,7 @@ Credentials = swapper.load_model("connection", "Credentials") DeviceConnection = swapper.load_model("connection", "DeviceConnection") Command = swapper.load_model("connection", "Command") +BatchCommand = swapper.load_model("connection", "BatchCommand") class CredentialsForm(forms.ModelForm): @@ -215,3 +219,205 @@ def schema_view(self, request): CommandInline, ] DeviceAdmin.add_reversion_following(follow=["deviceconnection_set"]) + + +class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): + ordering = ("-created",) + list_display = [ + "id", + "organization_display", + "status", + "type", + "created", + "total_devices", + ] + list_filter = [MultitenantOrgFilter, "status", "type"] + list_select_related = ("organization",) + search_fields = ["id"] + change_form_template = ( + "admin/connection/batch_command/batch_command_change_form.html" + ) + device_commands_per_page = 20 + exclude = ("devices",) + fields = [ + "organization_display", + "total_devices", + "colored_status", + "type", + "formatted_input", + "group", + "location", + "display_skipped_devices", + "created", + "modified", + ] + + class Media: + css = { + "all": [ + "admin/css/changelists.css", + "admin/css/ow-filters.css", + "connection/css/batch-command.css", + ] + } + js = [ + "admin/js/ow-filter.js", + "connection/js/batch-command.js", + ] + + def get_readonly_fields(self, request, obj=None): + return self.fields or [] + + def organization_display(self, obj): + if obj.organization: + return obj.organization.name + return _("All") + + organization_display.short_description = _("organization") + organization_display.admin_order_field = "organization" + + def colored_status(self, obj): + css_class = f"command-status {obj.status}" + return format_html( + '{1}', + css_class, + obj.get_status_display(), + ) + + colored_status.short_description = _("status") + + def formatted_input(self, obj): + if not obj.input: + return "-" + return obj.input.get("command", obj.input) + + formatted_input.short_description = _("input") + + def display_skipped_devices(self, obj): + if not obj.skipped_devices: + return "-" + Device = swapper.load_model("config", "Device") + count = len(obj.skipped_devices) + lines = [str(count)] + for pk_str, errors in obj.skipped_devices.items(): + device = Device.objects.filter(pk=pk_str).first() + name = device.name if device else _("Deleted ({})").format(pk_str) + lines.append(format_html("{}: {}", name, ", ".join(errors))) + return format_html( + '
{}
', + format_html_join(mark_safe("
"), "{}", ((line,) for line in lines)), + ) + + display_skipped_devices.short_description = _("Skipped devices") + + def _build_filter_specs(self, request, current_status): + filter_specs = [] + params = request.GET.copy() + + def _make_choice(current_value, display, param_name, value): + q = params.copy() + q.pop(param_name, None) + if value: + q[param_name] = value + qs = q.urlencode() + query_string = f"?{qs}" if qs else "" + return { + "display": display, + "selected": current_value == value, + "query_string": query_string, + } + + status_choices = [] + for status_value, display_name in ( + (("", _("All")),) + Command.STATUS_CHOICES + (("skipped", _("Skipped")),) + ): + status_choices.append( + _make_choice(current_status, display_name, "status", status_value) + ) + + class StatusFilter: + title = _("status") + choices = status_choices + + filter_specs.append(StatusFilter()) + return filter_specs + + def _paginate_commands(self, items, page_param, per_page=None): + per_page = per_page or self.device_commands_per_page + paginator = Paginator(list(items), per_page) + page_number = page_param or 1 + try: + page_obj = paginator.page(page_number) + except (PageNotAnInteger, EmptyPage): + page_obj = paginator.page(1) + return page_obj, paginator, page_obj.object_list + + def change_view(self, request, object_id, form_url="", extra_context=None): + extra_context = extra_context or {} + obj = self.get_object(request, object_id) + if obj: + Device = swapper.load_model("config", "Device") + commands_qs = Command.objects.filter(batch_command=obj).select_related( + "device" + ) + search_query = request.GET.get("q", "") + if search_query: + commands_qs = commands_qs.filter(device__name__icontains=search_query) + current_status = request.GET.get("status", "") + if current_status and current_status != "skipped": + commands_qs = commands_qs.filter(status=current_status) + rows = [] + for cmd in commands_qs: + rows.append( + { + "device_name": cmd.device.name, + "device_pk": cmd.device.pk, + "status": cmd.status, + "status_display": cmd.get_status_display(), + "output": (cmd.output or "").lstrip(), + "created": cmd.created, + "is_skipped": False, + } + ) + if obj.skipped_devices and current_status in ("", "skipped"): + for pk_str, errors in obj.skipped_devices.items(): + device = Device.objects.filter(pk=pk_str).first() + name = device.name if device else _("Deleted ({})").format(pk_str) + if search_query and search_query.lower() not in name.lower(): + continue + rows.append( + { + "device_name": name, + "device_pk": pk_str, + "status": "skipped", + "status_display": _("Skipped"), + "output": ", ".join(errors), + "created": None, + "is_skipped": True, + } + ) + + def _sort_key(row): + priority = {"success": 0, "failed": 1, "skipped": 2} + return (priority.get(row["status"], 99), row["device_name"].lower()) + + rows.sort(key=_sort_key) + filter_specs = self._build_filter_specs(request, current_status) + page_obj, paginator, commands = self._paginate_commands( + rows, request.GET.get("page", 1) + ) + extra_context.update( + { + "commands": commands, + "page_obj": page_obj, + "paginator": paginator, + "filter_specs": filter_specs, + "has_active_filters": any( + request.GET.get(param) for param in ["status"] + ), + } + ) + return super().change_view(request, object_id, extra_context=extra_context) + + +admin.site.register(BatchCommand, BatchCommandAdmin) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 75290a2d7..edc972df2 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -809,7 +809,7 @@ def __str__(self): @cached_property def total_devices(self): - return self.batch_commands.count() + return self.batch_commands.count() + len(self.skipped_devices or {}) @property def successful(self): diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css new file mode 100644 index 000000000..cff9510f1 --- /dev/null +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -0,0 +1,144 @@ +#batchcommand_form .submit-row { + display: none; +} + +.commands-title { + font-size: 22px; + font-weight: 300; + margin: 0; + padding: 0; +} + +.search-section { + padding: 20px; +} + +.search-form { + display: flex; + align-items: center; +} + +#main #content .search-icon { + width: 20px; + height: 20px; + margin-right: 15px; + font-size: 16px; + margin-top: -3px; +} + +#main #content .search-input { + padding: 10px 15px; +} + +#main #content .search-button { + padding: 10px 20px; + margin-left: 15px; +} + +.filter-clear-link:hover { + color: var(--ow-color-fg-darker); +} + +.results-table { + width: 100%; + border-collapse: collapse; +} + +#main #content .device-link { + color: var(--ow-color-primary); + font-weight: bold; +} + +.device-name-disabled { + color: var(--body-quiet-color); + font-style: italic; +} + +.empty-results { + padding: 40px; + text-align: center; + color: var(--body-quiet-color); + font-style: italic; +} + +.pagination { + padding: 15px 20px; + text-align: right; + border-top: 2px solid var(--hairline-color); + background: var(--darkened-bg); +} + +.pagination a { + color: var(--body-fg); + text-decoration: none; + margin: 0 5px; +} + +.pagination .current-page { + margin: 0 10px; + color: var(--body-quiet-color); +} + +.paginator { + color: var(--body-quiet-color); + padding: 10px 20px; + border-bottom: 1px solid var(--hairline-color); + margin: 0; +} + +.command-status { + font-weight: bold; +} + +.command-status.success { + color: var(--ow-color-success); +} + +.command-status.failed { + color: var(--error-fg); +} + +.command-status.in-progress { + color: var(--body-quiet-color); +} + +.command-status.skipped { + color: var(--body-quiet-color); + opacity: 0.7; +} + +.command-output { + padding: 0; +} + +.command-output pre { + white-space: pre-wrap; + word-wrap: break-word; + margin: 0; + padding: 0; + font: inherit; + color: inherit; + background: transparent; +} + +.skipped-devices-list { + line-height: 1.7; +} + +.field-display_skipped_devices .readonly.readonly { + padding: 0; +} + +/* Adjustments for list filters */ +#main #content .left-arrow { + left: -1.125rem; +} +#main #content .right-arrow { + right: -1.125rem; +} +#main #content #ow-changelist-filter { + padding: 1.25rem 0rem; +} +#main #content .filters-top { + margin-bottom: 0.5rem; +} diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js new file mode 100644 index 000000000..b066770aa --- /dev/null +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -0,0 +1,43 @@ +(function () { + "use strict"; + + var pollInterval = 3000; + var pollTimer = null; + + function getBatchStatus() { + var statusEl = document.querySelector(".field-status .readonly"); + if (!statusEl) return null; + var text = statusEl.textContent.trim().toLowerCase(); + if (text.indexOf("in progress") !== -1) return "in-progress"; + if (text.indexOf("success") !== -1) return "success"; + if (text.indexOf("failed") !== -1) return "failed"; + if (text.indexOf("idle") !== -1) return "idle"; + return null; + } + + function shouldPoll() { + var status = getBatchStatus(); + return status === "in-progress" || status === "idle"; + } + + function reloadPage() { + window.location.reload(); + } + + function startPolling() { + stopPolling(); + if (!shouldPoll()) return; + pollTimer = setInterval(reloadPage, pollInterval); + } + + function stopPolling() { + if (pollTimer) { + clearInterval(pollTimer); + pollTimer = null; + } + } + + document.addEventListener("DOMContentLoaded", function () { + startPolling(); + }); +})(); diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html new file mode 100644 index 000000000..81d0e812d --- /dev/null +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -0,0 +1,174 @@ +{% extends "admin/change_form.html" %} +{% load i18n admin_urls static admin_list ow_tags %} + +{% block extrahead %} +{{ block.super }} + + + +{% endblock %} + +{% block content %} +{{ block.super }} + + +

{% trans "Commands" %}

+ + +{% if filter_specs %} +
+
+ + {% trans 'left' %} + + + {% trans 'right' %} + +
+
+ {% for spec in filter_specs %} +
+ {% for choice in spec.choices %} + {% if choice.selected %} + + {% endif %} + {% endfor %} +
+ +
+
+ {% endfor %} +
+
+
+
+

{% trans 'Filter' %}

+
+ {% if has_active_filters %} +

+ ✖ {% trans "Clear all filters" %} +

+ {% endif %} + {% if filter_specs|length > 4 %} + + {% endif %} +
+
+
+{% endif %} + + +
+
+ + + + + + + {% for param, value in request.GET.items %} + {% if param != 'q' and param != 'page' %} + + {% endif %} + {% endfor %} +
+
+ + +
+ + + + + + + + + + + {% for command in commands %} + + + + + + + {% empty %} + + + + {% endfor %} + +
{% trans "Device" %}{% trans "Status" %}{% trans "Output" %}{% trans "Timestamp" %}
+ {% if command.is_skipped %} + {{ command.device_name }} + {% else %} + + {{ command.device_name }} + + {% endif %} + + {{ command.status_display }} + +
{{ command.output|default:"-" }}
+
{{ command.created|date|default:"-" }}
{% trans "No commands found." %}
+ + + {% if paginator %} +

+ {% blocktrans count counter=paginator.count %} + {{ counter }} command + {% plural %}{{ counter }} commands + {% endblocktrans %} +

+ {% endif %} + + + {% if page_obj.has_other_pages %} + + {% endif %} +
+{% endblock %} + +{% block footer %} +{{ block.super }} + + +{% endblock %} From f5077a03db73f1fff8a8de2ef75038f5ca52b369 Mon Sep 17 00:00:00 2001 From: dee077 Date: Thu, 2 Jul 2026 23:49:48 +0530 Subject: [PATCH 02/13] [feature] Add affected_devices, colored changelist status, and label admin link - Add cached_property on AbstractBatchCommand (excludes skipped) - Use in changelist list_display for consistent status colors - Replace ID with label as the clickable link in admin changelist - Add CSS to command-inline.css for consistency - Add label, notes to change form fields; reorder columns (created last, affected_devices before created) --- openwisp_controller/connection/admin.py | 18 +++++++++++++----- openwisp_controller/connection/base/models.py | 4 ++++ .../static/connection/css/command-inline.css | 4 ++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 60d853b7c..e999dc073 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -224,16 +224,17 @@ def schema_view(self, request): class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): ordering = ("-created",) list_display = [ - "id", + "label", "organization_display", - "status", + "colored_status", "type", + "affected_devices", "created", - "total_devices", ] + list_display_links = ["label"] list_filter = [MultitenantOrgFilter, "status", "type"] list_select_related = ("organization",) - search_fields = ["id"] + search_fields = ["label"] change_form_template = ( "admin/connection/batch_command/batch_command_change_form.html" ) @@ -241,7 +242,9 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): exclude = ("devices",) fields = [ "organization_display", - "total_devices", + "label", + "notes", + "affected_devices", "colored_status", "type", "formatted_input", @@ -293,6 +296,11 @@ def formatted_input(self, obj): formatted_input.short_description = _("input") + def affected_devices(self, obj): + return obj.affected_devices + + affected_devices.short_description = _("affected devices") + def display_skipped_devices(self, obj): if not obj.skipped_devices: return "-" diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index edc972df2..1037f9ece 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -811,6 +811,10 @@ def __str__(self): def total_devices(self): return self.batch_commands.count() + len(self.skipped_devices or {}) + @cached_property + def affected_devices(self): + return self.batch_commands.count() + @property def successful(self): return self.batch_commands.filter(status="success").count() diff --git a/openwisp_controller/connection/static/connection/css/command-inline.css b/openwisp_controller/connection/static/connection/css/command-inline.css index 0da80c341..8deda6c5e 100644 --- a/openwisp_controller/connection/static/connection/css/command-inline.css +++ b/openwisp_controller/connection/static/connection/css/command-inline.css @@ -242,6 +242,10 @@ li.commands:not(.recent) { .command-status.in-progress { color: var(--body-quiet-color); } +.command-status.skipped { + color: var(--body-quiet-color); + opacity: 0.7; +} .command-status { font-weight: bold; } From 45b222473a7cd8e88f066ad528d7551a8cdbb62f Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 7 Jul 2026 02:50:32 +0530 Subject: [PATCH 03/13] [fix] Restructure --- openwisp_controller/connection/admin.py | 68 ++++++++++++++----- openwisp_controller/connection/filters.py | 15 ++++ .../static/connection/css/command-inline.css | 4 -- .../static/connection/js/batch-command.js | 43 ------------ .../batch_command_change_form.html | 1 - 5 files changed, 65 insertions(+), 66 deletions(-) create mode 100644 openwisp_controller/connection/filters.py delete mode 100644 openwisp_controller/connection/static/connection/js/batch-command.js diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index e999dc073..2409c6595 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -18,6 +18,7 @@ from ..admin import MultitenantAdminMixin from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin +from .filters import GroupFilter, LocationFilter from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget @@ -222,7 +223,6 @@ def schema_view(self, request): class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): - ordering = ("-created",) list_display = [ "label", "organization_display", @@ -231,10 +231,23 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): "affected_devices", "created", ] - list_display_links = ["label"] - list_filter = [MultitenantOrgFilter, "status", "type"] + ordering = ("-created",) + list_filter = [ + MultitenantOrgFilter, + "status", + "type", + GroupFilter, + LocationFilter, + ] list_select_related = ("organization",) - search_fields = ["label"] + search_fields = [ + "label", + "notes", + "organization__name", + "devices__name", + "location__name", + "group__name", + ] change_form_template = ( "admin/connection/batch_command/batch_command_change_form.html" ) @@ -244,16 +257,28 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): "organization_display", "label", "notes", - "affected_devices", "colored_status", "type", "formatted_input", + "affected_devices", "group", "location", "display_skipped_devices", "created", "modified", ] + readonly_fields = [ + "organization_display", + "colored_status", + "type", + "formatted_input", + "affected_devices", + "display_skipped_devices", + "group", + "location", + "created", + "modified", + ] class Media: css = { @@ -263,13 +288,18 @@ class Media: "connection/css/batch-command.css", ] } - js = [ - "admin/js/ow-filter.js", - "connection/js/batch-command.js", - ] def get_readonly_fields(self, request, obj=None): - return self.fields or [] + fields = super().get_readonly_fields(request, obj) + return fields + list(self.__class__.readonly_fields) + + def _get_commands(self, request, obj): + qs = Command.objects.filter(batch_command=obj).select_related("device") + if not request.user.is_superuser: + qs = qs.filter( + device__organization_id__in=request.user.organizations_managed + ) + return qs def organization_display(self, obj): if obj.organization: @@ -305,10 +335,12 @@ def display_skipped_devices(self, obj): if not obj.skipped_devices: return "-" Device = swapper.load_model("config", "Device") - count = len(obj.skipped_devices) + pks = list(obj.skipped_devices.keys()) + devices = {str(d.pk): d for d in Device.objects.filter(pk__in=pks)} + count = len(pks) lines = [str(count)] for pk_str, errors in obj.skipped_devices.items(): - device = Device.objects.filter(pk=pk_str).first() + device = devices.get(pk_str) name = device.name if device else _("Deleted ({})").format(pk_str) lines.append(format_html("{}: {}", name, ", ".join(errors))) return format_html( @@ -318,7 +350,7 @@ def display_skipped_devices(self, obj): display_skipped_devices.short_description = _("Skipped devices") - def _build_filter_specs(self, request, current_status): + def _build_filter_specs(self, request, obj, current_status): filter_specs = [] params = request.GET.copy() @@ -365,9 +397,7 @@ def change_view(self, request, object_id, form_url="", extra_context=None): obj = self.get_object(request, object_id) if obj: Device = swapper.load_model("config", "Device") - commands_qs = Command.objects.filter(batch_command=obj).select_related( - "device" - ) + commands_qs = self._get_commands(request, obj) search_query = request.GET.get("q", "") if search_query: commands_qs = commands_qs.filter(device__name__icontains=search_query) @@ -388,8 +418,10 @@ def change_view(self, request, object_id, form_url="", extra_context=None): } ) if obj.skipped_devices and current_status in ("", "skipped"): + pks = list(obj.skipped_devices.keys()) + devices = {str(d.pk): d for d in Device.objects.filter(pk__in=pks)} for pk_str, errors in obj.skipped_devices.items(): - device = Device.objects.filter(pk=pk_str).first() + device = devices.get(pk_str) name = device.name if device else _("Deleted ({})").format(pk_str) if search_query and search_query.lower() not in name.lower(): continue @@ -410,7 +442,7 @@ def _sort_key(row): return (priority.get(row["status"], 99), row["device_name"].lower()) rows.sort(key=_sort_key) - filter_specs = self._build_filter_specs(request, current_status) + filter_specs = self._build_filter_specs(request, obj, current_status) page_obj, paginator, commands = self._paginate_commands( rows, request.GET.get("page", 1) ) diff --git a/openwisp_controller/connection/filters.py b/openwisp_controller/connection/filters.py new file mode 100644 index 000000000..b0623c387 --- /dev/null +++ b/openwisp_controller/connection/filters.py @@ -0,0 +1,15 @@ +from django.utils.translation import gettext_lazy as _ + +from openwisp_users.multitenancy import MultitenantRelatedOrgFilter + + +class GroupFilter(MultitenantRelatedOrgFilter): + field_name = "group" + parameter_name = "group_id" + title = _("group") + + +class LocationFilter(MultitenantRelatedOrgFilter): + field_name = "location" + parameter_name = "location_id" + title = _("location") diff --git a/openwisp_controller/connection/static/connection/css/command-inline.css b/openwisp_controller/connection/static/connection/css/command-inline.css index 8deda6c5e..0da80c341 100644 --- a/openwisp_controller/connection/static/connection/css/command-inline.css +++ b/openwisp_controller/connection/static/connection/css/command-inline.css @@ -242,10 +242,6 @@ li.commands:not(.recent) { .command-status.in-progress { color: var(--body-quiet-color); } -.command-status.skipped { - color: var(--body-quiet-color); - opacity: 0.7; -} .command-status { font-weight: bold; } diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js deleted file mode 100644 index b066770aa..000000000 --- a/openwisp_controller/connection/static/connection/js/batch-command.js +++ /dev/null @@ -1,43 +0,0 @@ -(function () { - "use strict"; - - var pollInterval = 3000; - var pollTimer = null; - - function getBatchStatus() { - var statusEl = document.querySelector(".field-status .readonly"); - if (!statusEl) return null; - var text = statusEl.textContent.trim().toLowerCase(); - if (text.indexOf("in progress") !== -1) return "in-progress"; - if (text.indexOf("success") !== -1) return "success"; - if (text.indexOf("failed") !== -1) return "failed"; - if (text.indexOf("idle") !== -1) return "idle"; - return null; - } - - function shouldPoll() { - var status = getBatchStatus(); - return status === "in-progress" || status === "idle"; - } - - function reloadPage() { - window.location.reload(); - } - - function startPolling() { - stopPolling(); - if (!shouldPoll()) return; - pollTimer = setInterval(reloadPage, pollInterval); - } - - function stopPolling() { - if (pollTimer) { - clearInterval(pollTimer); - pollTimer = null; - } - } - - document.addEventListener("DOMContentLoaded", function () { - startPolling(); - }); -})(); diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html index 81d0e812d..43ae2a83c 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -170,5 +170,4 @@

{% block footer %} {{ block.super }} - {% endblock %} From 95c01074cd292db25eeb676591a49d94dcd29613 Mon Sep 17 00:00:00 2001 From: dee077 Date: Fri, 24 Jul 2026 19:15:08 +0530 Subject: [PATCH 04/13] [fix] Add filters --- openwisp_controller/connection/admin.py | 155 +++++++++++++++++++++- openwisp_controller/connection/filters.py | 21 +++ 2 files changed, 169 insertions(+), 7 deletions(-) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 2409c6595..970eae743 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,4 +1,3 @@ -import json from datetime import timedelta import reversion @@ -18,7 +17,7 @@ from ..admin import MultitenantAdminMixin from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin -from .filters import GroupFilter, LocationFilter +from .filters import GroupFilter, LocationFilter, TypeFilter from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget @@ -235,7 +234,7 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): list_filter = [ MultitenantOrgFilter, "status", - "type", + TypeFilter, GroupFilter, LocationFilter, ] @@ -350,7 +349,15 @@ def display_skipped_devices(self, obj): display_skipped_devices.short_description = _("Skipped devices") - def _build_filter_specs(self, request, obj, current_status): + def _build_filter_specs( + self, + request, + obj, + current_status, + current_location=None, + current_group=None, + current_org=None, + ): filter_specs = [] params = request.GET.copy() @@ -380,6 +387,103 @@ class StatusFilter: choices = status_choices filter_specs.append(StatusFilter()) + + # Location filter + Device = swapper.load_model("config", "Device") + location_qs = ( + Device.objects.filter(command__batch_command=obj) + .exclude(devicelocation__location__isnull=True) + .values_list( + "devicelocation__location__id", + "devicelocation__location__name", + ) + .distinct() + ) + location_choices = [] + location_choices.append( + _make_choice(current_location or "", _("All"), "location_id", "") + ) + for loc_id, loc_name in location_qs: + if loc_id: + location_choices.append( + _make_choice( + current_location or "", + loc_name, + "location_id", + str(loc_id), + ) + ) + + if len(location_choices) > 1: + + class LocationFilterCls: + title = _("location") + choices = location_choices + + filter_specs.append(LocationFilterCls()) + + # Group filter + group_qs = ( + Device.objects.filter( + command__batch_command=obj, + group__isnull=False, + ) + .values_list("group__id", "group__name") + .distinct() + ) + group_choices = [] + group_choices.append( + _make_choice(current_group or "", _("All"), "group_id", "") + ) + for grp_id, grp_name in group_qs: + if grp_id: + group_choices.append( + _make_choice( + current_group or "", + grp_name, + "group_id", + str(grp_id), + ) + ) + + if len(group_choices) > 1: + + class GroupFilterCls: + title = _("device group") + choices = group_choices + + filter_specs.append(GroupFilterCls()) + + # Organization filter (superusers only) + if request.user.is_superuser: + org_qs = ( + Device.objects.filter(command__batch_command=obj) + .values_list("organization__id", "organization__name") + .distinct() + ) + org_choices = [] + org_choices.append( + _make_choice(current_org or "", _("All"), "organization_id", "") + ) + for org_id, org_name in org_qs: + if org_id: + org_choices.append( + _make_choice( + current_org or "", + org_name, + "organization_id", + str(org_id), + ) + ) + + if len(org_choices) > 1: + + class OrganizationFilterCls: + title = _("organization") + choices = org_choices + + filter_specs.append(OrganizationFilterCls()) + return filter_specs def _paginate_commands(self, items, page_param, per_page=None): @@ -402,8 +506,19 @@ def change_view(self, request, object_id, form_url="", extra_context=None): if search_query: commands_qs = commands_qs.filter(device__name__icontains=search_query) current_status = request.GET.get("status", "") + current_location = request.GET.get("location_id", "") + current_group = request.GET.get("group_id", "") + current_org = request.GET.get("organization_id", "") if current_status and current_status != "skipped": commands_qs = commands_qs.filter(status=current_status) + if current_location: + commands_qs = commands_qs.filter( + device__devicelocation__location_id=current_location + ) + if current_group: + commands_qs = commands_qs.filter(device__group_id=current_group) + if current_org: + commands_qs = commands_qs.filter(device__organization_id=current_org) rows = [] for cmd in commands_qs: rows.append( @@ -419,10 +534,29 @@ def change_view(self, request, object_id, form_url="", extra_context=None): ) if obj.skipped_devices and current_status in ("", "skipped"): pks = list(obj.skipped_devices.keys()) - devices = {str(d.pk): d for d in Device.objects.filter(pk__in=pks)} + device_qs = Device.objects.filter(pk__in=pks) + if current_location: + DeviceLocation = swapper.load_model("geo", "DeviceLocation") + device_locations = set( + DeviceLocation.objects.filter( + device_id__in=pks, + location_id=current_location, + ).values_list("device_id", flat=True) + ) + else: + device_locations = None + devices = {str(d.pk): d for d in device_qs} for pk_str, errors in obj.skipped_devices.items(): device = devices.get(pk_str) - name = device.name if device else _("Deleted ({})").format(pk_str) + if not device: + continue + if current_org and str(device.organization_id) != current_org: + continue + if current_group and str(device.group_id) != current_group: + continue + if current_location and pk_str not in device_locations: + continue + name = device.name if search_query and search_query.lower() not in name.lower(): continue rows.append( @@ -442,7 +576,14 @@ def _sort_key(row): return (priority.get(row["status"], 99), row["device_name"].lower()) rows.sort(key=_sort_key) - filter_specs = self._build_filter_specs(request, obj, current_status) + filter_specs = self._build_filter_specs( + request, + obj, + current_status, + current_location=current_location, + current_group=current_group, + current_org=current_org, + ) page_obj, paginator, commands = self._paginate_commands( rows, request.GET.get("page", 1) ) diff --git a/openwisp_controller/connection/filters.py b/openwisp_controller/connection/filters.py index b0623c387..7d03b9be0 100644 --- a/openwisp_controller/connection/filters.py +++ b/openwisp_controller/connection/filters.py @@ -1,4 +1,6 @@ +from django.contrib import admin from django.utils.translation import gettext_lazy as _ +from swapper import load_model from openwisp_users.multitenancy import MultitenantRelatedOrgFilter @@ -13,3 +15,22 @@ class LocationFilter(MultitenantRelatedOrgFilter): field_name = "location" parameter_name = "location_id" title = _("location") + + +class TypeFilter(admin.SimpleListFilter): + title = _("type") + parameter_name = "type" + + def lookups(self, request, model_admin): + BatchCommand = load_model("connection", "BatchCommand") + qs = BatchCommand.objects.all() + if not request.user.is_superuser: + qs = qs.filter(organization_id__in=request.user.organizations_managed) + types = qs.values_list("type", flat=True).distinct() + choices = dict(BatchCommand._meta.get_field("type").choices) + return [(t, choices.get(t, t)) for t in types] + + def queryset(self, request, queryset): + if self.value(): + return queryset.filter(type=self.value()) + return queryset From bca0f2810223784a5e0c2c2c3dd5c626f04b606f Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 28 Jul 2026 02:42:30 +0530 Subject: [PATCH 05/13] [feature] Add new execute and confirm page --- openwisp_controller/connection/admin.py | 432 +++++++++----- openwisp_controller/connection/base/models.py | 2 +- .../static/connection/css/batch-command.css | 536 ++++++++++++++++++ .../static/connection/js/execute-command.js | 157 +++++ .../batch_command/confirm_command.html | 155 +++++ .../batch_command/execute_command.html | 159 ++++++ 6 files changed, 1287 insertions(+), 154 deletions(-) create mode 100644 openwisp_controller/connection/static/connection/js/execute-command.js create mode 100644 openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html create mode 100644 openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 970eae743..47f42d760 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,11 +1,14 @@ from datetime import timedelta +from types import SimpleNamespace import reversion import swapper from django import forms from django.contrib import admin +from django.core.exceptions import PermissionDenied from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.http import HttpResponseForbidden, JsonResponse +from django.template.response import TemplateResponse from django.urls import path, resolve from django.utils.html import format_html, format_html_join from django.utils.safestring import mark_safe @@ -39,6 +42,43 @@ class Meta: widgets = {"input": CommandSchemaWidget} +class BatchCommandExecutionForm(forms.ModelForm): + """Form layout for the mass command execution workflow. + + The execution and confirmation behavior is intentionally added separately. + Keeping the form here lets the custom admin view use the same model fields + and tenant-scoped choices as the eventual workflow. + """ + + class Meta: + model = BatchCommand + fields = [ + "organization", + "label", + "notes", + "type", + "input", + "group", + "location", + "devices", + ] + widgets = { + "notes": forms.Textarea(attrs={"rows": 3}), + "input": forms.Textarea(attrs={"rows": 5}), + "devices": forms.SelectMultiple(attrs={"size": 8}), + } + + def __init__(self, *args, request=None, **kwargs): + super().__init__(*args, **kwargs) + if request is None or request.user.is_superuser: + return + organization_ids = request.user.organizations_managed + for field_name in ("organization", "group", "location", "devices"): + self.fields[field_name].queryset = self.fields[field_name].queryset.filter( + organization_id__in=organization_ids + ) + + @admin.register(Credentials) class CredentialsAdmin(MultitenantAdminMixin, TimeReadonlyAdminMixin, admin.ModelAdmin): list_display = ( @@ -222,6 +262,8 @@ def schema_view(self, request): class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): + execute_command_template = "admin/connection/batch_command/execute_command.html" + confirm_command_template = "admin/connection/batch_command/confirm_command.html" list_display = [ "label", "organization_display", @@ -288,6 +330,108 @@ class Media: ] } + def get_urls(self): + options = self.model._meta + return [ + path( + "execute/", + self.admin_site.admin_view(self.execute_command_view), + name=f"{options.app_label}_{options.model_name}_execute", + ), + path( + "confirm/", + self.admin_site.admin_view(self.confirm_command_view), + name=f"{options.app_label}_{options.model_name}_confirm", + ), + ] + super().get_urls() + + def execute_command_view(self, request): + """Render the first step of the mass command workflow. + + This page only collects command details for now. The preview and + confirmation POST flow will be added in a later change. + """ + permission = f"{self.opts.app_label}.add_{self.opts.model_name}" + if not request.user.has_perm(permission): + raise PermissionDenied + form = BatchCommandExecutionForm(request=request) + context = { + **self.admin_site.each_context(request), + "title": _("Execute mass command"), + "opts": self.model._meta, + "form": form, + "media": self.media + form.media, + "has_view_permission": self.has_view_permission(request), + } + return TemplateResponse(request, self.execute_command_template, context) + + def confirm_command_view(self, request): + """Render the second step of the mass command workflow. + + Displays a summary of the command to be executed and provides + an Execute button to create the BatchCommand. + """ + permission = f"{self.opts.app_label}.add_{self.opts.model_name}" + if not request.user.has_perm(permission): + raise PermissionDenied + command_type = request.GET.get("type", "") + label = request.GET.get("label", "") + notes = request.GET.get("notes", "") + organization_id = request.GET.get("organization", "") + group_id = request.GET.get("group", "") + location_id = request.GET.get("location", "") + device_ids = request.GET.getlist("devices") + + command_type_display = command_type + command_description = "" + for choice_value, choice_label in BatchCommand._meta.get_field("type").choices: + if choice_value == command_type: + command_type_display = choice_label + break + + targets_parts = [] + if organization_id: + Organization = swapper.load_model("openwisp_users", "Organization") + try: + org = Organization.objects.get(pk=organization_id) + targets_parts.append(str(org)) + except Organization.DoesNotExist: + pass + if group_id: + DeviceGroup = swapper.load_model("config", "DeviceGroup") + try: + group = DeviceGroup.objects.get(pk=group_id) + targets_parts.append(str(group)) + except DeviceGroup.DoesNotExist: + pass + if location_id: + Location = swapper.load_model("geo", "Location") + try: + location = Location.objects.get(pk=location_id) + targets_parts.append(str(location)) + except Location.DoesNotExist: + pass + targets_display = ( + ", ".join(targets_parts) if targets_parts else _("All devices") + ) + + device_count = len(device_ids) if device_ids else 0 + skipped_devices_count = 0 + + context = { + **self.admin_site.each_context(request), + "title": _("Review mass command"), + "opts": self.model._meta, + "command_type_display": command_type_display, + "command_description": command_description, + "targets_display": targets_display, + "device_count": device_count, + "skipped_devices_count": skipped_devices_count, + "media": self.media, + "has_view_permission": self.has_view_permission(request), + } + return TemplateResponse(request, self.confirm_command_template, context) + def get_readonly_fields(self, request, obj=None): fields = super().get_readonly_fields(request, obj) return fields + list(self.__class__.readonly_fields) @@ -303,6 +447,8 @@ def _get_commands(self, request, obj): def organization_display(self, obj): if obj.organization: return obj.organization.name + # Will return Shared systemwide (no organization) after + # https://github.com/openwisp/openwisp-users/issues/238 return _("All") organization_display.short_description = _("organization") @@ -347,7 +493,7 @@ def display_skipped_devices(self, obj): format_html_join(mark_safe("
"), "{}", ((line,) for line in lines)), ) - display_skipped_devices.short_description = _("Skipped devices") + display_skipped_devices.short_description = _("skipped devices") def _build_filter_specs( self, @@ -376,7 +522,7 @@ def _make_choice(current_value, display, param_name, value): status_choices = [] for status_value, display_name in ( - (("", _("All")),) + Command.STATUS_CHOICES + (("skipped", _("Skipped")),) + (("", _("All")),) + Command.STATUS_CHOICES + (("skipped", _("skipped")),) ): status_choices.append( _make_choice(current_status, display_name, "status", status_value) @@ -388,104 +534,68 @@ class StatusFilter: filter_specs.append(StatusFilter()) - # Location filter Device = swapper.load_model("config", "Device") - location_qs = ( + + # Location filter + location_spec = self._build_related_filter( + _("location"), + "location_id", + current_location or "", Device.objects.filter(command__batch_command=obj) .exclude(devicelocation__location__isnull=True) .values_list( "devicelocation__location__id", "devicelocation__location__name", ) - .distinct() + .distinct(), + _make_choice, ) - location_choices = [] - location_choices.append( - _make_choice(current_location or "", _("All"), "location_id", "") - ) - for loc_id, loc_name in location_qs: - if loc_id: - location_choices.append( - _make_choice( - current_location or "", - loc_name, - "location_id", - str(loc_id), - ) - ) - - if len(location_choices) > 1: - - class LocationFilterCls: - title = _("location") - choices = location_choices - - filter_specs.append(LocationFilterCls()) + if location_spec: + filter_specs.append(location_spec) # Group filter - group_qs = ( + group_spec = self._build_related_filter( + _("device group"), + "group_id", + current_group or "", Device.objects.filter( command__batch_command=obj, group__isnull=False, ) .values_list("group__id", "group__name") - .distinct() - ) - group_choices = [] - group_choices.append( - _make_choice(current_group or "", _("All"), "group_id", "") + .distinct(), + _make_choice, ) - for grp_id, grp_name in group_qs: - if grp_id: - group_choices.append( - _make_choice( - current_group or "", - grp_name, - "group_id", - str(grp_id), - ) - ) - - if len(group_choices) > 1: - - class GroupFilterCls: - title = _("device group") - choices = group_choices - - filter_specs.append(GroupFilterCls()) + if group_spec: + filter_specs.append(group_spec) # Organization filter (superusers only) if request.user.is_superuser: - org_qs = ( + org_spec = self._build_related_filter( + _("organization"), + "organization_id", + current_org or "", Device.objects.filter(command__batch_command=obj) .values_list("organization__id", "organization__name") - .distinct() + .distinct(), + _make_choice, ) - org_choices = [] - org_choices.append( - _make_choice(current_org or "", _("All"), "organization_id", "") - ) - for org_id, org_name in org_qs: - if org_id: - org_choices.append( - _make_choice( - current_org or "", - org_name, - "organization_id", - str(org_id), - ) - ) - - if len(org_choices) > 1: - - class OrganizationFilterCls: - title = _("organization") - choices = org_choices - - filter_specs.append(OrganizationFilterCls()) + if org_spec: + filter_specs.append(org_spec) return filter_specs + def _build_related_filter(self, title, param_name, current_value, qs, make_choice): + choices = [make_choice(current_value, _("All"), param_name, "")] + for obj_id, obj_name in qs: + if obj_id: + choices.append( + make_choice(current_value, obj_name, param_name, str(obj_id)) + ) + if len(choices) <= 1: + return None + return SimpleNamespace(title=title, choices=choices) + def _paginate_commands(self, items, page_param, per_page=None): per_page = per_page or self.device_commands_per_page paginator = Paginator(list(items), per_page) @@ -496,93 +606,109 @@ def _paginate_commands(self, items, page_param, per_page=None): page_obj = paginator.page(1) return page_obj, paginator, page_obj.object_list + def _get_active_filters(self, request): + return { + "q": request.GET.get("q", ""), + "status": request.GET.get("status", ""), + "location_id": request.GET.get("location_id", ""), + "group_id": request.GET.get("group_id", ""), + "organization_id": request.GET.get("organization_id", ""), + } + + def _apply_command_filters(self, qs, filters): + if filters["q"]: + qs = qs.filter(device__name__icontains=filters["q"]) + status = filters["status"] + if status and status != "skipped": + qs = qs.filter(status=status) + if filters["location_id"]: + qs = qs.filter(device__devicelocation__location_id=filters["location_id"]) + if filters["group_id"]: + qs = qs.filter(device__group_id=filters["group_id"]) + if filters["organization_id"]: + qs = qs.filter(device__organization_id=filters["organization_id"]) + return qs + + def _get_matching_skipped_devices(self, obj, filters): + Device = swapper.load_model("config", "Device") + pks = list(obj.skipped_devices.keys()) + device_qs = Device.objects.filter(pk__in=pks) + location_id = filters["location_id"] + if location_id: + DeviceLocation = swapper.load_model("geo", "DeviceLocation") + device_locations = set( + DeviceLocation.objects.filter( + device_id__in=pks, + location_id=location_id, + ).values_list("device_id", flat=True) + ) + else: + device_locations = None + devices = {str(d.pk): d for d in device_qs} + rows = [] + for pk_str, errors in obj.skipped_devices.items(): + device = devices.get(pk_str) + if not device: + continue + if ( + filters["organization_id"] + and str(device.organization_id) != filters["organization_id"] + ): + continue + if filters["group_id"] and str(device.group_id) != filters["group_id"]: + continue + if device_locations is not None and pk_str not in device_locations: + continue + if filters["q"] and filters["q"].lower() not in device.name.lower(): + continue + rows.append( + { + "device_name": device.name, + "device_pk": pk_str, + "status": "skipped", + "status_display": _("skipped"), + "output": ", ".join(errors), + "created": None, + "is_skipped": True, + } + ) + return rows + def change_view(self, request, object_id, form_url="", extra_context=None): extra_context = extra_context or {} obj = self.get_object(request, object_id) if obj: - Device = swapper.load_model("config", "Device") commands_qs = self._get_commands(request, obj) - search_query = request.GET.get("q", "") - if search_query: - commands_qs = commands_qs.filter(device__name__icontains=search_query) - current_status = request.GET.get("status", "") - current_location = request.GET.get("location_id", "") - current_group = request.GET.get("group_id", "") - current_org = request.GET.get("organization_id", "") - if current_status and current_status != "skipped": - commands_qs = commands_qs.filter(status=current_status) - if current_location: - commands_qs = commands_qs.filter( - device__devicelocation__location_id=current_location - ) - if current_group: - commands_qs = commands_qs.filter(device__group_id=current_group) - if current_org: - commands_qs = commands_qs.filter(device__organization_id=current_org) - rows = [] - for cmd in commands_qs: - rows.append( - { - "device_name": cmd.device.name, - "device_pk": cmd.device.pk, - "status": cmd.status, - "status_display": cmd.get_status_display(), - "output": (cmd.output or "").lstrip(), - "created": cmd.created, - "is_skipped": False, - } + filters = self._get_active_filters(request) + commands_qs = self._apply_command_filters(commands_qs, filters) + rows = [ + { + "device_name": cmd.device.name, + "device_pk": cmd.device.pk, + "status": cmd.status, + "status_display": cmd.get_status_display(), + "output": (cmd.output or "").lstrip(), + "created": cmd.created, + "is_skipped": False, + } + for cmd in commands_qs + ] + if obj.skipped_devices and filters["status"] in ("", "skipped"): + rows.extend(self._get_matching_skipped_devices(obj, filters)) + # Sort by status priority: success(0) > failed(1) > skipped(2), then alphabetically + rows.sort( + key=lambda r: ( + {"success": 0, "failed": 1, "skipped": 2}.get(r["status"], 99), + r["device_name"].lower(), ) - if obj.skipped_devices and current_status in ("", "skipped"): - pks = list(obj.skipped_devices.keys()) - device_qs = Device.objects.filter(pk__in=pks) - if current_location: - DeviceLocation = swapper.load_model("geo", "DeviceLocation") - device_locations = set( - DeviceLocation.objects.filter( - device_id__in=pks, - location_id=current_location, - ).values_list("device_id", flat=True) - ) - else: - device_locations = None - devices = {str(d.pk): d for d in device_qs} - for pk_str, errors in obj.skipped_devices.items(): - device = devices.get(pk_str) - if not device: - continue - if current_org and str(device.organization_id) != current_org: - continue - if current_group and str(device.group_id) != current_group: - continue - if current_location and pk_str not in device_locations: - continue - name = device.name - if search_query and search_query.lower() not in name.lower(): - continue - rows.append( - { - "device_name": name, - "device_pk": pk_str, - "status": "skipped", - "status_display": _("Skipped"), - "output": ", ".join(errors), - "created": None, - "is_skipped": True, - } - ) - - def _sort_key(row): - priority = {"success": 0, "failed": 1, "skipped": 2} - return (priority.get(row["status"], 99), row["device_name"].lower()) - - rows.sort(key=_sort_key) + ) filter_specs = self._build_filter_specs( request, obj, - current_status, - current_location=current_location, - current_group=current_group, - current_org=current_org, + filters["status"], + current_location=filters["location_id"], + current_group=filters["group_id"], + current_org=filters["organization_id"], ) page_obj, paginator, commands = self._paginate_commands( rows, request.GET.get("page", 1) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 1037f9ece..a01d094eb 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -792,7 +792,7 @@ class AbstractBatchCommand(ValidateOrgMixin, TimeStampedEditableModel): blank=True, null=True, default=dict, - verbose_name=_("Skipped devices"), + verbose_name=_("skipped devices"), help_text=_( "Maps device UUIDs to validation error messages for devices " "that were skipped during command creation." diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index cff9510f1..3e3a4c5b4 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -129,6 +129,10 @@ padding: 0; } +#content-main { + margin: 20px; +} + /* Adjustments for list filters */ #main #content .left-arrow { left: -1.125rem; @@ -142,3 +146,535 @@ #main #content .filters-top { margin-bottom: 0.5rem; } + +/* ================================================================ + EXECUTION PAGE — Step 1 of 2 + ================================================================ */ + +.batch-command-execution { + max-width: 80rem; +} + +/* ── Heading ────────────────────────────────────────────── */ + +.batch-command-execution__heading { + margin-bottom: 1.5rem; +} + +.batch-command-execution__heading h1 { + font-size: 1.625rem; + font-weight: 600; + margin: 0 0 0.25rem; +} + +.batch-command-execution__heading p { + color: var(--body-quiet-color); + font-size: 0.9375rem; + margin: 0; +} + +/* ── Stepper ────────────────────────────────────────────── */ + +.stepper { + --stepper-bg: #ffffff; + --stepper-border: #e2e8f0; + --stepper-shadow: 0 1px 3px rgba(0, 0, 0, 0.05), 0 1px 2px rgba(0, 0, 0, 0.03); + --stepper-radius: 9999px; + + --step-active-bg: #0d7377; + --step-active-text: #ffffff; + --step-active-highlight: #d5f1ea; + --step-active-tint: #f0faf6; + --step-active-underline: #0d7377; + + --step-inactive-bg: #e8eaed; + --step-inactive-text: #9aa0a6; + + --divider-color: #e2e8f0; + --arrow-color: #9aa0a6; + display: inline-flex; + align-items: stretch; + overflow: hidden; + margin-bottom: 1.75rem; +} + +.stepper__step { + align-items: center; + cursor: pointer; + display: flex; + gap: 0.75rem; + padding: 0.875rem 1.5rem 0.875rem 0; + position: relative; +} + +/* Badge */ +.stepper__badge { + align-items: center; + border-radius: 50%; + display: flex; + flex-shrink: 0; + font-size: 0.8125rem; + font-weight: 600; + height: 2rem; + justify-content: center; + position: relative; + width: 2rem; + z-index: 1; +} + +.stepper__step--active .stepper__badge { + background-color: var(--step-active-bg); + color: var(--step-active-text); +} + +.stepper__step--active .stepper__badge::before { + background-color: var(--step-active-highlight); +} + +.stepper__step--inactive .stepper__badge { + background-color: var(--step-inactive-bg); + color: var(--step-inactive-text); +} + +.stepper__step--inactive .stepper__badge::before { + display: none; +} + +/* Label */ +.stepper__label { + display: flex; + flex-direction: column; + gap: 0.2rem; + min-width: 0; +} + +.stepper__label-text { + font-size: 0.875rem; + font-weight: 500; + line-height: 1.2; + white-space: nowrap; +} + +.stepper__step--active .stepper__label-text { + color: var(--step-active-bg); + font-weight: 600; +} + +.stepper__step--inactive .stepper__label-text { + color: var(--step-inactive-text); + font-weight: 500; +} + +/* Divider + arrow */ +.stepper__divider { + align-items: center; + display: flex; + flex-shrink: 0; + justify-content: center; + padding: 0.5rem 1rem 0.5rem 0; +} + +.stepper__arrow { + color: var(--arrow-color); + display: block; + flex-shrink: 0; + height: 1rem; + width: 1rem; +} + +/* ── Cards ──────────────────────────────────────────────── */ + +.bce-card { + background: #ffffff; + border: 1px solid #e2e8f0; + border-radius: 1rem; + box-shadow: 0 1px 3px rgba(0, 0, 0, 0.04); + margin-bottom: 1.5rem; + overflow: hidden; +} + +.bce-card__header { + padding: 1.5rem 2rem 0; +} + +.bce-card__header-row { + align-items: flex-start; + display: flex; + justify-content: space-between; +} + +.bce-card__title { + font-size: 1.125rem; + font-weight: 600; + margin: 0 0 0.2rem; +} + +.bce-card__subtitle { + color: var(--body-quiet-color); + font-size: 0.875rem; + margin: 0; +} + +.bce-card__muted { + color: var(--body-quiet-color); + font-size: 0.8125rem; + white-space: nowrap; +} + +.bce-card__body { + padding: 1.25rem 2rem 2rem; +} + +/* ── Form fields inside cards ───────────────────────────── */ + +.bce-field { + margin-bottom: 1.25rem; +} + +.bce-field:last-child { + margin-bottom: 0; +} + +.bce-field label, +.bce-field > div > label { + display: block; + font-size: 0.875rem; + font-weight: 500; + margin-bottom: 0.4rem; +} + +.bce-field select, +.bce-field textarea, +.bce-field input[type="text"], +.bce-field input[type="number"], +.bce-field input[type="password"] { + background: #ffffff; + border: 1px solid #d0d5dd; + border-radius: 0.5rem; + font-size: 0.875rem; + max-width: 100%; + padding: 0.625rem 0.875rem; + transition: + border-color 0.15s ease, + box-shadow 0.15s ease; + width: 100%; +} + +.bce-field select:focus, +.bce-field textarea:focus, +.bce-field input[type="text"]:focus, +.bce-field input[type="number"]:focus, +.bce-field input[type="password"]:focus { + border-color: var(--step-active-bg); + box-shadow: 0 0 0 3px rgba(13, 115, 119, 0.12); + outline: none; +} + +.bce-field select[multiple] { + min-height: 8rem; + padding: 0.5rem; +} + +.bce-field .help { + color: var(--body-quiet-color); + font-size: 0.8125rem; + margin-top: 0.35rem; +} + +.bce-field .errors { + list-style: none; + margin: 0.25rem 0 0; + padding: 0; +} + +.bce-field .errors li { + color: var(--error-fg); + font-size: 0.8125rem; +} + +.bce-field-grid { + display: grid; + gap: 1.25rem; + grid-template-columns: 1fr 1fr; +} + +/* ── Warning banner ─────────────────────────────────────── */ + +.bce-warning { + align-items: flex-start; + background: #fffbeb; + border: 1px solid #fde68a; + border-radius: 0.75rem; + color: #92400e; + display: flex; + font-size: 0.875rem; + gap: 0.75rem; + margin: 0 2rem 1.5rem; + padding: 1rem 1.25rem; +} + +.bce-warning svg { + flex-shrink: 0; + height: 1.25rem; + margin-top: 0.1rem; + width: 1.25rem; +} + +/* ── Device summary banner ──────────────────────────────── */ + +.bce-device-summary { + align-items: center; + background: #eff6ff; + border: 1px solid #bfdbfe; + border-radius: 0.75rem; + color: #1e40af; + display: flex; + font-size: 0.875rem; + font-weight: 500; + justify-content: space-between; + margin-top: 1.5rem; + padding: 0.875rem 1.25rem; +} + +.bce-device-summary__live { + align-items: center; + color: #6b7280; + display: flex; + font-size: 0.8125rem; + font-weight: 400; + gap: 0.4rem; +} + +.bce-device-summary__dot { + background: #22c55e; + border-radius: 50%; + display: inline-block; + height: 6px; + width: 6px; +} + +/* ── Submit row ──────────────────────────────────────────── */ + +.batch-command-execution__actions { + display: flex; + gap: 0.75rem; + justify-content: flex-end; + margin-top: 1.5rem; +} + +.batch-command-execution__actions .cancel-link { + margin: 0; +} + +.batch-command-execution__actions button[disabled] { + cursor: not-allowed; + opacity: 0.5; +} + +.batch-command-execution__form > .help { + color: var(--body-quiet-color); + font-size: 0.8125rem; + margin: 0.75rem 0 0; + text-align: right; +} + +/* ================================================================ + CONFIRM / REVIEW PAGE — Step 2 of 2 + ================================================================ */ + +/* ── Completed stepper step ─────────────────────────────────── */ + +.stepper__step--completed .stepper__badge { + background-color: #059669; + color: #ffffff; +} + +.stepper__step--completed .stepper__badge::before { + display: none; +} + +.stepper__check { + display: block; + height: 1rem; + width: 1rem; +} + +.stepper__step--completed .stepper__label-text { + color: #059669; + font-weight: 600; +} + +/* ── Edit link ──────────────────────────────────────────────── */ + +.bce-card__edit-link { + align-items: center; + color: var(--step-active-bg); + display: inline-flex; + font-size: 0.875rem; + font-weight: 500; + gap: 0.35rem; + text-decoration: none; +} + +.bce-card__edit-link:hover { + text-decoration: underline; +} + +.bce-card__edit-link svg { + flex-shrink: 0; +} + +/* ── Summary definition list ────────────────────────────────── */ + +.bcr-summary { + margin: 0; +} + +.bcr-summary__row { + align-items: baseline; + display: flex; + gap: 1rem; + padding: 0.75rem 0; +} + +.bcr-summary__row + .bcr-summary__row { + border-top: 1px solid #f1f5f9; +} + +.bcr-summary__label { + color: var(--body-quiet-color); + flex-shrink: 0; + font-size: 0.875rem; + font-weight: 400; + min-width: 8rem; +} + +.bcr-summary__value { + font-size: 0.875rem; + font-weight: 500; + margin: 0; +} + +.bcr-summary__value strong { + font-weight: 700; +} + +.bcr-summary__value--warning { + color: #d97706; + font-weight: 600; +} + +.bcr-summary__badge { + background: #f1f5f9; + border: 1px solid #e2e8f0; + border-radius: 0.375rem; + display: inline-block; + font-family: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace; + font-size: 0.8125rem; + font-weight: 500; + padding: 0.15rem 0.5rem; +} + +.bcr-summary__desc { + color: var(--body-quiet-color); + font-weight: 400; + margin-left: 0.35rem; +} + +/* ── Affected devices placeholder ───────────────────────────── */ + +.bcr-devices-placeholder { + min-height: 6rem; +} + +/* ── Sticky action bar ──────────────────────────────────────── */ + +.bcr-action-bar { + align-items: center; + background: #ffffff; + border: 1px solid #e2e8f0; + border-radius: 0.75rem; + bottom: 1.5rem; + box-shadow: + 0 4px 6px -1px rgba(0, 0, 0, 0.07), + 0 2px 4px -2px rgba(0, 0, 0, 0.05); + display: flex; + gap: 1.5rem; + justify-content: space-between; + margin-top: 1.5rem; + padding: 1rem 1.5rem; + position: sticky; + z-index: 10; +} + +.bcr-action-bar__summary { + color: var(--body-quiet-color); + font-size: 0.9375rem; +} + +.bcr-action-bar__summary strong { + color: var(--body-fg); + font-weight: 600; +} + +.bcr-action-bar__actions { + align-items: center; + display: flex; + gap: 0.75rem; + flex-shrink: 0; +} + +/* ── Responsive ─────────────────────────────────────────── */ + +@media (max-width: 767px) { + .batch-command-execution__heading h1 { + font-size: 1.375rem; + } + + .bce-card__header { + padding: 1.25rem 1.25rem 0; + } + + .bce-card__body { + padding: 1rem 1.25rem 1.5rem; + } + + .bce-field-grid { + grid-template-columns: 1fr; + } + + .stepper { + max-width: 100%; + } + + .stepper__step { + padding: 0.75rem 1rem 0.75rem 0; + } + + .stepper__label-text { + font-size: 0.8125rem; + } + + .bcr-summary__row { + flex-direction: column; + gap: 0.25rem; + } + + .bcr-summary__label { + min-width: 0; + } + + .bcr-action-bar { + flex-direction: column; + gap: 1rem; + padding: 1rem 1.25rem; + } + + .bcr-action-bar__actions { + width: 100%; + } + + .bcr-action-bar__actions .button { + flex: 1; + } +} diff --git a/openwisp_controller/connection/static/connection/js/execute-command.js b/openwisp_controller/connection/static/connection/js/execute-command.js new file mode 100644 index 000000000..b832a5009 --- /dev/null +++ b/openwisp_controller/connection/static/connection/js/execute-command.js @@ -0,0 +1,157 @@ +django.jQuery(function ($) { + "use strict"; + + var TYPE_CUSTOM = "custom"; + var TYPE_CHANGE_PASSWORD = "change_password"; + + var $typeSelect = $("#id_type"); + if (!$typeSelect.length) return; + + var $form = $typeSelect.closest("form"); + var $container = $("#command-input-container"); + var fieldName = $("#id_input").length ? $("#id_input").attr("name") : "input"; + var $hiddenInput; + + function ensureHiddenInput() { + $hiddenInput = $form.find('input[name="' + fieldName + '"][type="hidden"]'); + if (!$hiddenInput.length) { + $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); + $form.append($hiddenInput); + } + } + + function clearContainer() { + $container.empty(); + } + + function syncCustom() { + var val = $container.find("#bce-dynamic-command").val(); + val = $.trim(val); + $hiddenInput.val(val ? JSON.stringify({ command: val }) : ""); + } + + function syncPassword() { + var pw = $container.find("#bce-dynamic-password").val(); + var cp = $container.find("#bce-dynamic-confirm_password").val(); + $hiddenInput.val( + pw && cp ? JSON.stringify({ password: pw, confirm_password: cp }) : "", + ); + } + + function buildCustomField() { + var $wrapper = $('
'); + $wrapper.append(''); + $wrapper.append( + '', + ); + $wrapper.append( + '
Enter the shell command to run on all devices
', + ); + $container.append($wrapper); + } + + function buildChangePasswordField() { + var $grid = $('
'); + + var $pwField = $('
'); + $pwField.append(''); + $pwField.append( + '', + ); + $grid.append($pwField); + + var $cpField = $('
'); + $cpField.append( + '', + ); + $cpField.append( + '', + ); + $grid.append($cpField); + + $container.append($grid); + $container.append( + '
Password must be at least 6 characters long
', + ); + } + + function onTypeChange() { + var selected = $typeSelect.val(); + clearContainer(); + + if (selected === TYPE_CUSTOM) { + buildCustomField(); + syncCustom(); + } else if (selected === TYPE_CHANGE_PASSWORD) { + buildChangePasswordField(); + syncPassword(); + } else { + $hiddenInput.val(""); + } + } + + ensureHiddenInput(); + $container.on("input", "#bce-dynamic-command", syncCustom); + $container.on( + "input", + "#bce-dynamic-password, #bce-dynamic-confirm_password", + syncPassword, + ); + $typeSelect.on("change", onTypeChange); + onTypeChange(); + + var $reviewBtn = $("#review-command-btn"); + if ($reviewBtn.length) { + $typeSelect.on("change", function () { + $reviewBtn.prop("disabled", !$(this).val()); + }); + $reviewBtn.prop("disabled", !$typeSelect.val()); + + $reviewBtn.on("click", function () { + var type = $typeSelect.val(); + if (!type) return; + + var params = new URLSearchParams(); + params.append("type", type); + + var inputVal = $hiddenInput.val(); + if (inputVal) { + params.append("input", inputVal); + } + + var label = $("#id_label").val(); + if (label) { + params.append("label", label); + } + + var notes = $("#id_notes").val(); + if (notes) { + params.append("notes", notes); + } + + var org = $("#id_organization").val(); + if (org) { + params.append("organization", org); + } + + var group = $("#id_group").val(); + if (group) { + params.append("group", group); + } + + var location = $("#id_location").val(); + if (location) { + params.append("location", location); + } + + $("#id_devices option:selected").each(function () { + params.append("devices", $(this).val()); + }); + + var confirmUrl = window.location.href.replace("execute/", "confirm/"); + window.location.href = confirmUrl.split("?")[0] + "?" + params.toString(); + }); + } +}); diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html new file mode 100644 index 000000000..a6975ab80 --- /dev/null +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html @@ -0,0 +1,155 @@ +{% extends "admin/base_site.html" %} +{% load i18n admin_urls static %} + +{% block extrahead %} +{{ block.super }} +{{ media }} + +{% endblock %} + +{% block bodyclass %}app-{{ opts.app_label }} model-{{ opts.model_name }} confirm-batch-command{% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content_title %}{% endblock %} + +{% block content %} +
+
+

{% trans 'Review mass command' %}

+

{% trans 'Confirm what will run before execution.' %}

+
+ + + + {# ── Summary card ──────────────────────────────────────── #} +
+
+
+
+

{% trans 'Summary' %}

+
+ + + + + {% trans 'Edit' %} + +
+
+
+
+
+
{% trans 'Command' %}
+
+ {{ command_type_display }} + {% if command_description %}— {{ command_description }}{% endif %} +
+
+
+
{% trans 'Targets' %}
+
{{ targets_display }}
+
+
+
{% trans 'Will run on' %}
+
+ {{ device_count }} {% blocktrans count device_count=device_count %}device{% plural %}devices{% endblocktrans %} +
+
+ {% if skipped_devices_count %} +
+
{% trans 'Will skip' %}
+
+ {{ skipped_devices_count }} {% blocktrans count skipped_devices_count=skipped_devices_count %}device{% plural %}devices{% endblocktrans %} +
+
+ {% endif %} +
+
{% trans 'Triggered by' %}
+
{{ request.user }}
+
+
+
+
+ + {% if skipped_devices_count %} + {# ── Warning banner ──────────────────────────────────────── #} +
+ + + +
+ {% blocktrans count skipped_devices_count=skipped_devices_count %}{{ skipped_devices_count }} device will be skipped{% plural %}{{ skipped_devices_count }} devices will be skipped{% endblocktrans %} +

{% trans 'These devices do not match the selected filters or are not available.' %}

+
+
+ {% endif %} + + {# ── Affected devices ──────────────────────────────────── #} +
+
+
+
+

{% trans 'Affected devices' %}

+

{% trans 'Devices that will receive this command' %}

+
+
+
+
+
+
+
+ + {# ── Sticky action bar ─────────────────────────────────── #} +
+
+ {% blocktrans count device_count=device_count %} + About to run {{ command_type_display }} on {{ device_count }} device + {% plural %} + About to run {{ command_type_display }} on {{ device_count }} devices + {% endblocktrans %} +
+
+ {% trans 'Back' %} + +
+
+
+{% endblock %} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html new file mode 100644 index 000000000..851311e37 --- /dev/null +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html @@ -0,0 +1,159 @@ +{% extends "admin/base_site.html" %} +{% load i18n admin_urls static %} + +{% block extrahead %} +{{ block.super }} +{{ media }} + + +{% endblock %} + +{% block bodyclass %}app-{{ opts.app_label }} model-{{ opts.model_name }} execute-batch-command{% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content_title %}{% endblock %} + +{% block content %} +
+
+

{% trans 'Execute mass command' %}

+

{% trans 'Run a shell command across many devices at once' %}

+
+ + + +
+ + {# ── Command card ─────────────────────────────────── #} +
+
+
+
+

{% trans 'Command' %}

+

{% trans 'What to run on the selected devices' %}

+
+
+
+
+ {% with field=form.type %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} + +
+ +
+ {% with field=form.label %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} + + {% with field=form.notes %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} +
+
+
+ + {# ── Targets card ──────────────────────────────────── #} +
+
+
+
+

{% trans 'Targets' %}

+

{% trans 'Which devices receive this command' %}

+
+ {% trans 'Filters combine with AND' %} +
+
+
+
+ {% with field=form.organization %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} + + {% with field=form.location %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} + + {% with field=form.group %} +
+ {{ field.errors }} + + {{ field }} + {% if field.help_text %}
{{ field.help_text }}
{% endif %} +
+ {% endwith %} +
+ +
+ {% trans '12 devices match these filters' %} + + + {% trans 'Updated live' %} + +
+
+
+ + {# ── Hidden submit (kept for form validation) ──────── #} +
+ {% trans 'Cancel' %} + +
+
+
+{% endblock %} From c8442c1c1dd2f57cd516e2f9738a39b034cd1a67 Mon Sep 17 00:00:00 2001 From: dee077 Date: Sun, 9 Aug 2026 23:25:29 +0530 Subject: [PATCH 06/13] [fix] Test with monitoring --- openwisp_controller/connection/admin.py | 484 ++++++++++++++---- .../connection/api/serializers.py | 13 + openwisp_controller/connection/apps.py | 79 ++- openwisp_controller/connection/base/models.py | 99 ++-- .../connection/channels/consumers.py | 90 ++++ .../connection/channels/routing.py | 6 +- openwisp_controller/connection/settings.py | 8 + .../static/connection/css/batch-command.css | 459 ++--------------- .../static/connection/js/batch-command.js | 357 +++++++++++++ .../static/connection/js/execute-command.js | 460 ++++++++++++----- .../batch_command_change_form.html | 20 +- .../batch_command/confirm_command.html | 253 ++++----- .../batch_command/execute_command.html | 249 +++++---- 13 files changed, 1625 insertions(+), 952 deletions(-) create mode 100644 openwisp_controller/connection/static/connection/js/batch-command.js diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 47f42d760..9a6d70cd0 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,13 +1,15 @@ from datetime import timedelta from types import SimpleNamespace +from uuid import uuid4 import reversion import swapper from django import forms -from django.contrib import admin -from django.core.exceptions import PermissionDenied +from django.contrib import admin, messages +from django.core.exceptions import PermissionDenied, ValidationError from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.http import HttpResponseForbidden, JsonResponse +from django.shortcuts import redirect from django.template.response import TemplateResponse from django.urls import path, resolve from django.utils.html import format_html, format_html_join @@ -20,6 +22,7 @@ from ..admin import MultitenantAdminMixin from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin +from . import settings as app_settings from .filters import GroupFilter, LocationFilter, TypeFilter from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget @@ -43,13 +46,16 @@ class Meta: class BatchCommandExecutionForm(forms.ModelForm): - """Form layout for the mass command execution workflow. + """Collects the mass command details on the first step of the workflow. - The execution and confirmation behavior is intentionally added separately. - Keeping the form here lets the custom admin view use the same model fields - and tenant-scoped choices as the eventual workflow. + This form is the only place where the submitted values are validated. + Narrowing the querysets in ``__init__`` controls what the widgets offer, + it does not control what is accepted, so ``clean()`` re-checks the + submitted values against the organizations the user actually manages. """ + required_css_class = "required" + class Meta: model = BatchCommand fields = [ @@ -60,24 +66,101 @@ class Meta: "input", "group", "location", - "devices", ] widgets = { "notes": forms.Textarea(attrs={"rows": 3}), - "input": forms.Textarea(attrs={"rows": 5}), - "devices": forms.SelectMultiple(attrs={"size": 8}), + # filled in by execute-command.js, which renders the fields + # relevant to the selected command type + "input": forms.HiddenInput(), + } + + class Media: + # select2 must be loaded before jquery.init.js, which calls + # jQuery.noConflict(): same ordering as admin.widgets.AutocompleteMixin + js = [ + "admin/js/vendor/jquery/jquery.min.js", + "admin/js/vendor/select2/select2.full.min.js", + "admin/js/jquery.init.js", + "connection/js/execute-command.js", + ] + css = { + "screen": [ + "admin/css/vendor/select2/select2.min.css", + "admin/css/autocomplete.css", + ] } def __init__(self, *args, request=None, **kwargs): super().__init__(*args, **kwargs) + self.request = request if request is None or request.user.is_superuser: return organization_ids = request.user.organizations_managed - for field_name in ("organization", "group", "location", "devices"): + self.fields["organization"].queryset = self.fields[ + "organization" + ].queryset.filter(id__in=organization_ids) + for field_name in ("group", "location"): self.fields[field_name].queryset = self.fields[field_name].queryset.filter( organization_id__in=organization_ids ) + def clean(self): + cleaned_data = super().clean() + if self.request is None or self.request.user.is_superuser: + return cleaned_data + organization = cleaned_data.get("organization") + group = cleaned_data.get("group") + location = cleaned_data.get("location") + # a batch without any target would run on every device of the + # deployment, which only superusers are allowed to do + if not any([organization, group, location]): + raise ValidationError( + _( + "Please select at least one of: organization, device group," + " or location." + ) + ) + # "organizations_managed" is a list of organization UUIDs as strings + organization_ids = self.request.user.organizations_managed + related_organizations = ( + ("organization", organization.pk if organization else None), + ("group", group.organization_id if group else None), + ("location", location.organization_id if location else None), + ) + for field_name, organization_id in related_organizations: + if organization_id is None: + continue + if str(organization_id) not in organization_ids: + self.add_error(field_name, _("Select a valid choice.")) + return cleaned_data + + def to_session(self): + """Returns the cleaned values as JSON serializable primitives. + + The session uses the JSON serializer, so model instances and UUIDs + cannot be stored as they are. + """ + + def _pk(value): + return str(value.pk) if value else None + + return { + # Namespaces the device selection the confirm page keeps in + # sessionStorage, which lives as long as the browser tab: without + # it a wizard would inherit the devices unselected by a previous + # one. It has to be issued here rather than by the browser, + # because the session is shared between tabs and sessionStorage + # is not. Not a security token: it only scopes a storage key. + "token": uuid4().hex, + "type": self.cleaned_data["type"], + "label": self.cleaned_data["label"], + "notes": self.cleaned_data.get("notes") or "", + "input": self.cleaned_data.get("input"), + "organization_id": _pk(self.cleaned_data.get("organization")), + "group_id": _pk(self.cleaned_data.get("group")), + "location_id": _pk(self.cleaned_data.get("location")), + } + @admin.register(Credentials) class CredentialsAdmin(MultitenantAdminMixin, TimeReadonlyAdminMixin, admin.ModelAdmin): @@ -261,9 +344,74 @@ def schema_view(self, request): DeviceAdmin.add_reversion_following(follow=["deviceconnection_set"]) +class BatchCommandDeviceAdminMixin: + """Turns the device changelist into the selection table of the confirm page. + + Applied on top of whichever ModelAdmin is registered for Device rather + than on top of this module's DeviceAdmin, because other modules replace + that registration: openwisp-monitoring unregisters Device and registers + its own subclass, which adds the health status column. Building on the + registered class means those columns appear here too, along with the + select_related and the media they need, without this module knowing + which ones exist. See BatchCommandAdmin.get_device_admin(). + + Filters and search are removed on purpose: the devices are already + determined by the targets chosen on the execute page, this table only + allows excluding individual devices from that set. Emptying + ``list_filter`` and ``search_fields`` is enough for the stock changelist + template to render neither, so it can be reused as it is. + """ + + # DeviceAdmin leaves this as an empty tuple, which ModelAdmin reads as + # "link the first column": that would wrap the checkbox in an and + # navigate to the device instead of ticking it. Name is the column the + # device changelist links anyway. + list_display_links = ["name"] + list_filter = [] + search_fields = [] + actions = None + list_per_page = 20 + ordering = ["name"] + change_list_template = "admin/connection/batch_command/confirm_command.html" + # django-import-export replaces change_list_template on the instance with + # a template of its own, which redefines the object-tools block and so + # brings back the "Import", "Export" and "Add device" buttons this page + # suppresses. Setting this to None is its documented way of opting out: + # ImportExportMixinBase.init_change_list_template() then falls back to + # the template set above. Unused when import-export is not installed. + import_export_change_list_template = None + + def __init__(self, model, admin_site, devices=None): + super().__init__(model, admin_site) + self.devices = devices + + def get_list_display(self, request): + # resolved per request instead of being a class attribute: the + # attribute would be a snapshot taken when this module is imported, + # which can be before another module has replaced the registration + return ["select_device"] + list(super().get_list_display(request)) + + def get_queryset(self, request): + # MultitenantAdminMixin.get_queryset() scopes this to the + # organizations managed by the user, independently of list_filter + return super().get_queryset(request).filter(pk__in=self.devices) + + @admin.display(description="") + def select_device(self, obj): + # deliberately without a "name": these checkboxes are never + # submitted, execute-command.js mirrors them into the hidden + # "excluded" field of the form holding the execute button + return format_html( + '', + obj.pk, + ) + + class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): execute_command_template = "admin/connection/batch_command/execute_command.html" + # rendered through BatchCommandDeviceAdmin.change_list_template confirm_command_template = "admin/connection/batch_command/confirm_command.html" + session_key = "batch_command_wizard" list_display = [ "label", "organization_display", @@ -292,7 +440,7 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): change_form_template = ( "admin/connection/batch_command/batch_command_change_form.html" ) - device_commands_per_page = 20 + device_commands_per_page = app_settings.BATCH_COMMAND_PAGE_SIZE exclude = ("devices",) fields = [ "organization_display", @@ -345,92 +493,205 @@ def get_urls(self): ), ] + super().get_urls() - def execute_command_view(self, request): - """Render the first step of the mass command workflow. - - This page only collects command details for now. The preview and - confirmation POST flow will be added in a later change. - """ + def _check_add_permission(self, request): permission = f"{self.opts.app_label}.add_{self.opts.model_name}" if not request.user.has_perm(permission): raise PermissionDenied - form = BatchCommandExecutionForm(request=request) + + def execute_command_view(self, request): + """First step of the mass command workflow: collect the details. + + A valid submission is stored in the session and the user is + redirected to the confirm page (Post/Redirect/Get), so that the + device table there can be paginated with ordinary GET requests: a + pagination link cannot carry the contents of a form. + """ + self._check_add_permission(request) + if request.method == "POST": + form = BatchCommandExecutionForm(request.POST, request=request) + if form.is_valid(): + request.session[self.session_key] = form.to_session() + return redirect( + f"admin:{self.opts.app_label}_{self.opts.model_name}_confirm" + ) + else: + form = BatchCommandExecutionForm(request=request) context = { **self.admin_site.each_context(request), "title": _("Execute mass command"), - "opts": self.model._meta, + "opts": self.opts, "form": form, - "media": self.media + form.media, + # not combined with self.media: ModelAdmin.media loads + # jquery.init.js before select2, the opposite of what select2 + # needs (see BatchCommandExecutionForm.Media) + "media": form.media, "has_view_permission": self.has_view_permission(request), } return TemplateResponse(request, self.execute_command_template, context) def confirm_command_view(self, request): - """Render the second step of the mass command workflow. + """Second step: review the targeted devices and dispatch the command. - Displays a summary of the command to be executed and provides - an Execute button to create the BatchCommand. + Dispatching is decided by the HTTP method alone, never by looking + for a field in the request body. """ - permission = f"{self.opts.app_label}.add_{self.opts.model_name}" - if not request.user.has_perm(permission): - raise PermissionDenied - command_type = request.GET.get("type", "") - label = request.GET.get("label", "") - notes = request.GET.get("notes", "") - organization_id = request.GET.get("organization", "") - group_id = request.GET.get("group", "") - location_id = request.GET.get("location", "") - device_ids = request.GET.getlist("devices") - - command_type_display = command_type - command_description = "" - for choice_value, choice_label in BatchCommand._meta.get_field("type").choices: - if choice_value == command_type: - command_type_display = choice_label - break - - targets_parts = [] - if organization_id: - Organization = swapper.load_model("openwisp_users", "Organization") - try: - org = Organization.objects.get(pk=organization_id) - targets_parts.append(str(org)) - except Organization.DoesNotExist: - pass - if group_id: - DeviceGroup = swapper.load_model("config", "DeviceGroup") - try: - group = DeviceGroup.objects.get(pk=group_id) - targets_parts.append(str(group)) - except DeviceGroup.DoesNotExist: - pass - if location_id: - Location = swapper.load_model("geo", "Location") - try: - location = Location.objects.get(pk=location_id) - targets_parts.append(str(location)) - except Location.DoesNotExist: - pass - targets_display = ( - ", ".join(targets_parts) if targets_parts else _("All devices") + self._check_add_permission(request) + if request.method == "POST": + return self._execute_batch_command(request) + wizard = request.session.get(self.session_key) + if not wizard: + return self._restart(request) + devices = self._resolve_target_queryset(request, wizard) + device_admin = self.get_device_admin(devices) + # changelist_view() assembles the whole changelist context (cl, + # media, pagination) and renders the change_list_template of the + # mixin, which is the confirm page extending the stock changelist + # template + return device_admin.changelist_view( + request, extra_context=self._confirm_context(request, wizard, devices) ) - device_count = len(device_ids) if device_ids else 0 - skipped_devices_count = 0 + def get_device_admin(self, devices): + """Builds the ModelAdmin rendering the device table of the confirm page. + + Composed with the ModelAdmin currently registered for Device instead + of a named class, so that the table shows the columns of the device + changelist as it actually is. Modules layered on top of the + controller replace that registration rather than extending the class + this module imports: openwisp-monitoring, for one, unregisters Device + and registers a subclass adding the health status column. + + Resolved here, per request, rather than at import time: every app has + finished loading by now, so the registration is final. Nothing is + imported from those modules and none of them needs to know about this + page; with none of them installed this returns the controller's own + Device admin and the table is unchanged. + """ + Device = swapper.load_model("config", "Device") + registered = self.admin_site.get_model_admin(Device).__class__ + # the mixin comes first so that its attributes win over the + # registered admin's + device_admin_class = type( + "BatchCommandDeviceAdmin", + (BatchCommandDeviceAdminMixin, registered), + {}, + ) + return device_admin_class(Device, self.admin_site, devices=devices) - context = { - **self.admin_site.each_context(request), + def get_device_changelist_template(self): + """The template the registered Device admin renders its changelist with. + + The confirm page extends it instead of the stock changelist template, + because that is where other modules load the assets their columns + need: openwisp-monitoring pulls in the stylesheet drawing the health + status accordion, and the script expanding it, from there. + + Read from the class rather than from an instance, since + django-import-export rewrites the attribute on the instance. + """ + Device = swapper.load_model("config", "Device") + registered = self.admin_site.get_model_admin(Device).__class__ + return getattr(registered, "change_list_template", None) or ( + "admin/change_list.html" + ) + + def _restart(self, request): + """Sends the user back to step one when there is no wizard to show.""" + messages.warning( + request, _("Please fill in the mass command details to continue.") + ) + return redirect(f"admin:{self.opts.app_label}_{self.opts.model_name}_execute") + + def _resolve_target_queryset(self, request, wizard): + """Devices matched by the organization, group and location chosen. + + ``distinct()`` and an explicit ordering are required because this + queryset is paginated: the devicelocation join can return the same + device more than once, and page boundaries are undefined without an + ordering. + """ + Device = swapper.load_model("config", "Device") + qs = Device.objects.all() + if not request.user.is_superuser: + qs = qs.filter(organization_id__in=request.user.organizations_managed) + if wizard.get("organization_id"): + qs = qs.filter(organization_id=wizard["organization_id"]) + if wizard.get("group_id"): + qs = qs.filter(group_id=wizard["group_id"]) + if wizard.get("location_id"): + qs = qs.filter(devicelocation__location_id=wizard["location_id"]) + return qs.distinct().order_by("name") + + def _confirm_context(self, request, wizard, devices): + targets = [] + for app_label, model_name, key in ( + ("openwisp_users", "Organization", "organization_id"), + ("config", "DeviceGroup", "group_id"), + ("geo", "Location", "location_id"), + ): + if not wizard.get(key): + continue + model = swapper.load_model(app_label, model_name) + target = model.objects.filter(pk=wizard[key]).first() + if target: + targets.append(str(target)) + command_types = dict(BatchCommand._meta.get_field("type").choices) + return { "title": _("Review mass command"), - "opts": self.model._meta, - "command_type_display": command_type_display, - "command_description": command_description, - "targets_display": targets_display, - "device_count": device_count, - "skipped_devices_count": skipped_devices_count, - "media": self.media, + "batch_opts": self.opts, + # the template this page extends, see get_device_changelist_template() + "device_changelist_template": self.get_device_changelist_template(), + "wizard": wizard, + "command_type_display": command_types.get(wizard["type"], wizard["type"]), + "command_description": (wizard.get("input") or {}).get("command", ""), + "targets_display": ", ".join(targets) if targets else _("All devices"), + "device_count": devices.count(), "has_view_permission": self.has_view_permission(request), } - return TemplateResponse(request, self.confirm_command_template, context) + + def _execute_batch_command(self, request): + """Applies the device selection and dispatches the mass command. + + The wizard is popped from the session before anything else happens, + so that a double submit cannot create the batch twice: the second + request finds nothing and is sent back to step one. + """ + wizard = request.session.pop(self.session_key, None) + if not wizard: + return self._restart(request) + devices = self._resolve_target_queryset(request, wizard) + # The confirm page only lists the devices matched on the execute + # page, so the selection can only ever remove from that set: the + # browser never supplies a device to add. + excluded = self._get_pk_list(request.POST, "excluded") + selection = devices.exclude(pk__in=excluded) + kwargs = { + "type": wizard["type"], + "label": wizard["label"], + "input": wizard.get("input"), + "notes": wizard.get("notes") or "", + "organization_id": wizard.get("organization_id"), + "group_id": wizard.get("group_id"), + "location_id": wizard.get("location_id"), + "devices": list(selection.distinct()), + } + try: + batch = BatchCommand.execute(**kwargs) + except ValidationError as error: + # put the wizard back so the user can correct the selection + request.session[self.session_key] = wizard + messages.error(request, error.messages[0]) + return redirect( + f"admin:{self.opts.app_label}_{self.opts.model_name}_confirm" + ) + messages.success(request, _("Mass command executed successfully.")) + return redirect( + f"admin:{self.opts.app_label}_{self.opts.model_name}_change", batch.pk + ) + + @staticmethod + def _get_pk_list(source, name): + return [pk for pk in source.get(name, "").split(",") if pk] def get_readonly_fields(self, request, obj=None): fields = super().get_readonly_fields(request, obj) @@ -596,15 +857,48 @@ def _build_related_filter(self, title, param_name, current_value, qs, make_choic return None return SimpleNamespace(title=title, choices=choices) - def _paginate_commands(self, items, page_param, per_page=None): + @staticmethod + def _command_row(command): + return { + "device_name": command.device.name, + "device_pk": command.device.pk, + "status": command.status, + "status_display": command.get_status_display(), + "output": (command.output or "").lstrip(), + "created": command.created, + "is_skipped": False, + } + + def _paginate_commands(self, commands_qs, skipped_rows, page_param, per_page=None): + """Returns one page of rows without loading the whole batch in memory. + + Commands keep the ordering of ``AbstractCommand.Meta`` ("created"), + which is the order they were fanned out in: the newest one is always + last. That is what lets the change page append results live without + re-fetching, because a new result always belongs on the last page. + + Skipped devices are not Command rows, they are entries of the + ``skipped_devices`` field, so they are kept as a (normally short) + list and follow the commands. + """ per_page = per_page or self.device_commands_per_page - paginator = Paginator(list(items), per_page) - page_number = page_param or 1 + commands_count = commands_qs.count() + total = commands_count + len(skipped_rows) + paginator = Paginator(range(total), per_page) try: - page_obj = paginator.page(page_number) + page_obj = paginator.page(page_param or 1) except (PageNotAnInteger, EmptyPage): page_obj = paginator.page(1) - return page_obj, paginator, page_obj.object_list + start = (page_obj.number - 1) * per_page + end = start + per_page + commands_end = min(end, commands_count) + rows = [ + self._command_row(command) for command in commands_qs[start:commands_end] + ] + skipped_start = max(0, start - commands_count) + skipped_end = max(0, end - commands_count) + rows += skipped_rows[skipped_start:skipped_end] + return page_obj, paginator, rows def _get_active_filters(self, request): return { @@ -681,26 +975,11 @@ def change_view(self, request, object_id, form_url="", extra_context=None): commands_qs = self._get_commands(request, obj) filters = self._get_active_filters(request) commands_qs = self._apply_command_filters(commands_qs, filters) - rows = [ - { - "device_name": cmd.device.name, - "device_pk": cmd.device.pk, - "status": cmd.status, - "status_display": cmd.get_status_display(), - "output": (cmd.output or "").lstrip(), - "created": cmd.created, - "is_skipped": False, - } - for cmd in commands_qs - ] + skipped_rows = [] if obj.skipped_devices and filters["status"] in ("", "skipped"): - rows.extend(self._get_matching_skipped_devices(obj, filters)) - # Sort by status priority: success(0) > failed(1) > skipped(2), then alphabetically - rows.sort( - key=lambda r: ( - {"success": 0, "failed": 1, "skipped": 2}.get(r["status"], 99), - r["device_name"].lower(), - ) + skipped_rows = self._get_matching_skipped_devices(obj, filters) + page_obj, paginator, commands = self._paginate_commands( + commands_qs, skipped_rows, request.GET.get("page", 1) ) filter_specs = self._build_filter_specs( request, @@ -710,9 +989,6 @@ def change_view(self, request, object_id, form_url="", extra_context=None): current_group=filters["group_id"], current_org=filters["organization_id"], ) - page_obj, paginator, commands = self._paginate_commands( - rows, request.GET.get("page", 1) - ) extra_context.update( { "commands": commands, diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index e776def03..37c4280cd 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -15,6 +15,19 @@ BatchCommand = load_model("connection", "BatchCommand") +def command_to_batch_payload(command): + """Serialize a Command into the payload used for batch-command websocket messages. + + Shared by the batch-command signal receiver and the batch-command consumer so + real-time messages and the initial ``request_current_state`` reply use the same + shape (including the extra fields the admin table needs to render a row). + """ + data = CommandSerializer(command).data + data["device_name"] = command.device.name + data["status_display"] = command.get_status_display() + return data + + class ValidatedDeviceFieldSerializer(ValidatedModelSerializer): def validate(self, data): # Add "device_id" to the data for validation diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index f67e326d0..1abc5cca9 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -8,9 +8,10 @@ from openwisp_notifications.types import register_notification_type from swapper import get_model_name, load_model -from openwisp_utils.admin_theme.menu import register_menu_subitem +from openwisp_utils.admin_theme.menu import register_menu_group, register_menu_subitem from ..config.signals import config_deactivating, config_modified +from .settings import BATCH_COMMAND_PAGE_SIZE from .signals import is_working_changed @@ -37,6 +38,7 @@ def ready(self): Config = load_model("config", "Config") Credentials = load_model("connection", "Credentials") Command = load_model("connection", "Command") + BatchCommand = load_model("connection", "BatchCommand") config_modified.connect( self.config_modified_receiver, dispatch_uid="connection.update_config" @@ -61,6 +63,11 @@ def ready(self): sender=Command, dispatch_uid="command_save_handler", ) + post_save.connect( + self.batch_command_save_receiver, + sender=BatchCommand, + dispatch_uid="batch_command_save_handler", + ) @classmethod def config_modified_receiver(cls, **kwargs): @@ -68,16 +75,53 @@ def config_modified_receiver(cls, **kwargs): @classmethod def command_save_receiver(cls, sender, created, instance, **kwargs): - from .api.serializers import CommandSerializer + from .api.serializers import CommandSerializer, command_to_batch_payload channel_layer = layers.get_channel_layer() - if created: - # Trigger websocket message only when command status is updated - return serialized_data = CommandSerializer(instance).data + if not created: + # Trigger websocket message only when command status is updated + async_to_sync(channel_layer.group_send)( + f"config.device-{instance.device_id}", + {"type": "send.update", "model": "Command", "data": serialized_data}, + ) + if instance.batch_command_id: + batch_data = command_to_batch_payload(instance) + # Authoritative counts, recomputed fresh on every send rather than + # relying on the client to increment a running total (a missed + # or duplicate message would otherwise desync it permanently). + batch = instance.batch_command + affected_devices = batch.batch_commands.count() + batch_data["affected_devices"] = affected_devices + # the table also paginates the skipped devices, which are not + # Command rows: without them the client computes too few pages + # and the last one becomes unreachable + batch_data["total_rows"] = affected_devices + len( + batch.skipped_devices or {} + ) + if created: + # Results are ordered by creation, so a new one is always the + # last: its index is the count minus one. Only new results + # carry a page, a status change is not a new row and must not + # be drawn anywhere it is not already displayed. + batch_data["page"] = ( + affected_devices - 1 + ) // BATCH_COMMAND_PAGE_SIZE + 1 + async_to_sync(channel_layer.group_send)( + f"config.batchcommand-{instance.batch_command_id}", + {"type": "send.update", "model": "Command", "data": batch_data}, + ) + + @classmethod + def batch_command_save_receiver(cls, sender, instance, **kwargs): + from .api.serializers import BatchCommandSerializer + + channel_layer = layers.get_channel_layer() + serialized_data = BatchCommandSerializer(instance).data + serialized_data["status_display"] = instance.get_status_display() async_to_sync(channel_layer.group_send)( - f"config.device-{instance.device_id}", - {"type": "send.update", "model": "Command", "data": serialized_data}, + f"config.batchcommand-{instance.pk}", + {"type": "send.update", "model": "BatchCommand", "data": serialized_data}, ) @classmethod @@ -188,3 +232,24 @@ def register_menu_groups(self): "icon": "ow-access-credential", }, ) + register_menu_group( + position=35, + config={ + "label": _("Network Operations"), + "icon": "ow-build", + "items": { + 1: { + "label": _("Mass command admin"), + "model": get_model_name("connection", "BatchCommand"), + "name": "changelist", + "icon": "ow-mass-upgrade", + }, + 2: { + "label": _("Mass command execute"), + "model": get_model_name("connection", "BatchCommand"), + "name": "execute", + "icon": "ow-mass-upgrade", + }, + }, + }, + ) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index a01d094eb..1db8afa8e 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -982,11 +982,8 @@ def create_commands(self): batch_command=self, ) try: - # Validate before the atomic block so errors like - # ValidationError don't create/rollback command.full_clean() - with transaction.atomic(): - command.save() + command.save() except ValidationError as e: self.skipped_devices[str(device.pk)] = ( e.messages if hasattr(e, "messages") else [str(e)] @@ -1016,52 +1013,50 @@ def calculate_and_update_status(self): - All commands completed successfully: status set to "success". - Status unchanged: no database write performed. """ - with transaction.atomic(): - batch = self.__class__.objects.select_for_update().get(pk=self.pk) - stats = batch.batch_commands.aggregate( - total_operations=models.Count("id"), - in_progress=models.Count( - models.Case( - models.When(status="in-progress", then=1), - output_field=models.IntegerField(), - ) - ), - completed=models.Count( - models.Case( - models.When(~models.Q(status="in-progress"), then=1), - output_field=models.IntegerField(), - ) - ), - successful=models.Count( - models.Case( - models.When(status="success", then=1), - output_field=models.IntegerField(), - ) - ), - failed=models.Count( - models.Case( - models.When(status="failed", then=1), - output_field=models.IntegerField(), - ) - ), - ) - if stats["total_operations"] == 0: - if batch.skipped_devices: - new_status = "failed" - else: - new_status = "idle" - elif stats["in_progress"] > 0: - new_status = "in-progress" - elif stats["failed"] > 0: + batch = self.__class__.objects.get(pk=self.pk) + stats = batch.batch_commands.aggregate( + total_operations=models.Count("id"), + in_progress=models.Count( + models.Case( + models.When(status="in-progress", then=1), + output_field=models.IntegerField(), + ) + ), + completed=models.Count( + models.Case( + models.When(~models.Q(status="in-progress"), then=1), + output_field=models.IntegerField(), + ) + ), + successful=models.Count( + models.Case( + models.When(status="success", then=1), + output_field=models.IntegerField(), + ) + ), + failed=models.Count( + models.Case( + models.When(status="failed", then=1), + output_field=models.IntegerField(), + ) + ), + ) + if stats["total_operations"] == 0: + if batch.skipped_devices: new_status = "failed" - elif ( - stats["successful"] > 0 - and stats["completed"] == stats["total_operations"] - ): - if batch.skipped_devices: - new_status = "failed" - else: - new_status = "success" - if batch.status != new_status: - batch.status = new_status - batch.save(update_fields=["status"]) + else: + new_status = "idle" + elif stats["in_progress"] > 0: + new_status = "in-progress" + elif stats["failed"] > 0: + new_status = "failed" + elif ( + stats["successful"] > 0 and stats["completed"] == stats["total_operations"] + ): + if batch.skipped_devices: + new_status = "failed" + else: + new_status = "success" + if batch.status != new_status: + batch.status = new_status + batch.save(update_fields=["status"]) diff --git a/openwisp_controller/connection/channels/consumers.py b/openwisp_controller/connection/channels/consumers.py index 7b4955c47..a695494ab 100644 --- a/openwisp_controller/connection/channels/consumers.py +++ b/openwisp_controller/connection/channels/consumers.py @@ -4,8 +4,10 @@ from swapper import load_model from ...config.base.channels_consumer import BaseDeviceConsumer +from .. import settings as app_settings Device = load_model("config", "Device") +BatchCommand = load_model("connection", "BatchCommand") class CommandConsumer(BaseDeviceConsumer): @@ -13,3 +15,91 @@ def send_update(self, event): data = deepcopy(event) data.pop("type") self.send(json.dumps(data)) + + +class BatchCommandConsumer(BaseDeviceConsumer): + model = BatchCommand + channel_layer_group = "config.batchcommand" + + def connect(self): + # ensure the user can only access the batch command if they + # can view the organization it belongs to + pk = self.scope["url_route"]["kwargs"]["pk"] + user = self.scope["user"] + batch = ( + BatchCommand.objects.select_related("organization").filter(pk=pk).first() + ) + if not batch: + self.close() + return + if not user.is_superuser and not ( + batch.organization_id + and user.organizations_managed.filter(pk=batch.organization_id).exists() + ): + self.close() + return + super().connect() + + def send_update(self, event): + data = deepcopy(event) + data.pop("type") + self.send(json.dumps(data)) + + per_page = app_settings.BATCH_COMMAND_PAGE_SIZE + + def receive(self, text_data): + try: + content = json.loads(text_data) + except ValueError: + return + if content.get("type") == "request_current_state": + self._handle_current_state_request(content.get("page")) + + def _handle_current_state_request(self, page=None): + """Reply with the state of the page the client is showing. + + The client requests this once on websocket open (and on every + reconnect) so the table can be reconciled even for commands created + while the page was closed or before the socket connected. + + Only the requested page is sent: a mass command can target thousands + of devices, and serializing all of them (including their output) on + every connect would make the payload grow without bound. + """ + # Imported here instead of at module import time to avoid + # AppRegistryNotReady errors. + from ..api.serializers import BatchCommandSerializer, command_to_batch_payload + + batch = BatchCommand.objects.filter( + pk=self.scope["url_route"]["kwargs"]["pk"] + ).first() + if not batch: + return + affected_devices = batch.batch_commands.count() + batch_data = BatchCommandSerializer(batch).data + batch_data["status_display"] = batch.get_status_display() + batch_data["affected_devices"] = affected_devices + try: + page = max(int(page), 1) + except (TypeError, ValueError): + page = 1 + start = (page - 1) * self.per_page + end = start + self.per_page + page_commands = batch.batch_commands.select_related("device")[start:end] + commands = [command_to_batch_payload(command) for command in page_commands] + self.send( + json.dumps( + { + "model": "BatchState", + "data": { + "batch_status": batch_data, + "commands": commands, + "page": page, + # the table paginates the skipped devices too, they + # are not Command rows + "total_rows": affected_devices + + len(batch.skipped_devices or {}), + }, + } + ) + ) diff --git a/openwisp_controller/connection/channels/routing.py b/openwisp_controller/connection/channels/routing.py index 2012b86b2..7b8afad04 100644 --- a/openwisp_controller/connection/channels/routing.py +++ b/openwisp_controller/connection/channels/routing.py @@ -8,5 +8,9 @@ def get_routes(consumer=ow_consumer): path( "ws/controller/device//command", consumer.CommandConsumer.as_asgi(), - ) + ), + path( + "ws/controller/batch-command/", + consumer.BatchCommandConsumer.as_asgi(), + ), ] diff --git a/openwisp_controller/connection/settings.py b/openwisp_controller/connection/settings.py index 50223a137..d28cfdc5d 100644 --- a/openwisp_controller/connection/settings.py +++ b/openwisp_controller/connection/settings.py @@ -35,6 +35,14 @@ }, ) +# How many results are listed per page on the mass command change page. +# Shared by the admin, which paginates with it, and by the websocket layer, +# which tells the browser the page a new result belongs to: the two have to +# agree or results are drawn on the wrong page. +BATCH_COMMAND_PAGE_SIZE = getattr( + settings, "OPENWISP_CONTROLLER_BATCH_COMMAND_PAGE_SIZE", 20 +) + SSH_AUTH_TIMEOUT = getattr(settings, "OPENWISP_SSH_AUTH_TIMEOUT", 2) SSH_BANNER_TIMEOUT = getattr(settings, "OPENWISP_SSH_BANNER_TIMEOUT", 60) SSH_COMMAND_TIMEOUT = getattr(settings, "OPENWISP_SSH_COMMAND_TIMEOUT", 30) diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index 3e3a4c5b4..749070dd7 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -129,10 +129,6 @@ padding: 0; } -#content-main { - margin: 20px; -} - /* Adjustments for list filters */ #main #content .left-arrow { left: -1.125rem; @@ -148,50 +144,21 @@ } /* ================================================================ - EXECUTION PAGE — Step 1 of 2 + STEPPER ================================================================ */ -.batch-command-execution { - max-width: 80rem; -} - -/* ── Heading ────────────────────────────────────────────── */ - -.batch-command-execution__heading { - margin-bottom: 1.5rem; -} - -.batch-command-execution__heading h1 { - font-size: 1.625rem; - font-weight: 600; - margin: 0 0 0.25rem; -} - -.batch-command-execution__heading p { - color: var(--body-quiet-color); - font-size: 0.9375rem; - margin: 0; -} +.stepper { + --step-active-bg: var(--ow-color-primary); + --step-active-text: var(--ow-color-white); + --step-active-highlight: var(--ow-color-primary-light); + --step-active-tint: var(--ow-color-primary-lighter); + --step-active-underline: var(--ow-color-primary); -/* ── Stepper ────────────────────────────────────────────── */ + --step-inactive-bg: var(--ow-color-fg-light); + --step-inactive-text: var(--ow-color-fg-dark); -.stepper { - --stepper-bg: #ffffff; - --stepper-border: #e2e8f0; - --stepper-shadow: 0 1px 3px rgba(0, 0, 0, 0.05), 0 1px 2px rgba(0, 0, 0, 0.03); - --stepper-radius: 9999px; - - --step-active-bg: #0d7377; - --step-active-text: #ffffff; - --step-active-highlight: #d5f1ea; - --step-active-tint: #f0faf6; - --step-active-underline: #0d7377; - - --step-inactive-bg: #e8eaed; - --step-inactive-text: #9aa0a6; - - --divider-color: #e2e8f0; - --arrow-color: #9aa0a6; + --divider-color: var(--ow-color-fg-light); + --arrow-color: var(--ow-color-fg-dark); display: inline-flex; align-items: stretch; overflow: hidden; @@ -282,399 +249,51 @@ width: 1rem; } -/* ── Cards ──────────────────────────────────────────────── */ - -.bce-card { - background: #ffffff; - border: 1px solid #e2e8f0; - border-radius: 1rem; - box-shadow: 0 1px 3px rgba(0, 0, 0, 0.04); - margin-bottom: 1.5rem; - overflow: hidden; -} - -.bce-card__header { - padding: 1.5rem 2rem 0; -} - -.bce-card__header-row { - align-items: flex-start; - display: flex; - justify-content: space-between; -} - -.bce-card__title { - font-size: 1.125rem; - font-weight: 600; - margin: 0 0 0.2rem; -} - -.bce-card__subtitle { - color: var(--body-quiet-color); - font-size: 0.875rem; - margin: 0; -} - -.bce-card__muted { - color: var(--body-quiet-color); - font-size: 0.8125rem; - white-space: nowrap; -} - -.bce-card__body { - padding: 1.25rem 2rem 2rem; -} - -/* ── Form fields inside cards ───────────────────────────── */ - -.bce-field { - margin-bottom: 1.25rem; -} - -.bce-field:last-child { - margin-bottom: 0; -} - -.bce-field label, -.bce-field > div > label { - display: block; - font-size: 0.875rem; - font-weight: 500; - margin-bottom: 0.4rem; -} - -.bce-field select, -.bce-field textarea, -.bce-field input[type="text"], -.bce-field input[type="number"], -.bce-field input[type="password"] { - background: #ffffff; - border: 1px solid #d0d5dd; - border-radius: 0.5rem; - font-size: 0.875rem; - max-width: 100%; - padding: 0.625rem 0.875rem; - transition: - border-color 0.15s ease, - box-shadow 0.15s ease; - width: 100%; -} - -.bce-field select:focus, -.bce-field textarea:focus, -.bce-field input[type="text"]:focus, -.bce-field input[type="number"]:focus, -.bce-field input[type="password"]:focus { - border-color: var(--step-active-bg); - box-shadow: 0 0 0 3px rgba(13, 115, 119, 0.12); - outline: none; -} - -.bce-field select[multiple] { - min-height: 8rem; - padding: 0.5rem; -} - -.bce-field .help { - color: var(--body-quiet-color); - font-size: 0.8125rem; - margin-top: 0.35rem; -} - -.bce-field .errors { - list-style: none; - margin: 0.25rem 0 0; - padding: 0; -} - -.bce-field .errors li { - color: var(--error-fg); - font-size: 0.8125rem; -} - -.bce-field-grid { - display: grid; - gap: 1.25rem; - grid-template-columns: 1fr 1fr; -} - -/* ── Warning banner ─────────────────────────────────────── */ - -.bce-warning { - align-items: flex-start; - background: #fffbeb; - border: 1px solid #fde68a; - border-radius: 0.75rem; - color: #92400e; - display: flex; - font-size: 0.875rem; - gap: 0.75rem; - margin: 0 2rem 1.5rem; - padding: 1rem 1.25rem; -} - -.bce-warning svg { - flex-shrink: 0; - height: 1.25rem; - margin-top: 0.1rem; - width: 1.25rem; -} - -/* ── Device summary banner ──────────────────────────────── */ - -.bce-device-summary { - align-items: center; - background: #eff6ff; - border: 1px solid #bfdbfe; - border-radius: 0.75rem; - color: #1e40af; - display: flex; - font-size: 0.875rem; - font-weight: 500; - justify-content: space-between; - margin-top: 1.5rem; - padding: 0.875rem 1.25rem; -} - -.bce-device-summary__live { - align-items: center; - color: #6b7280; - display: flex; - font-size: 0.8125rem; - font-weight: 400; - gap: 0.4rem; -} - -.bce-device-summary__dot { - background: #22c55e; - border-radius: 50%; - display: inline-block; - height: 6px; - width: 6px; -} - -/* ── Submit row ──────────────────────────────────────────── */ - -.batch-command-execution__actions { - display: flex; - gap: 0.75rem; - justify-content: flex-end; - margin-top: 1.5rem; -} - -.batch-command-execution__actions .cancel-link { - margin: 0; -} - -.batch-command-execution__actions button[disabled] { - cursor: not-allowed; - opacity: 0.5; -} - -.batch-command-execution__form > .help { - color: var(--body-quiet-color); - font-size: 0.8125rem; - margin: 0.75rem 0 0; - text-align: right; -} - /* ================================================================ - CONFIRM / REVIEW PAGE — Step 2 of 2 + CONFIRM PAGE ================================================================ */ -/* ── Completed stepper step ─────────────────────────────────── */ - -.stepper__step--completed .stepper__badge { - background-color: #059669; - color: #ffffff; -} - -.stepper__step--completed .stepper__badge::before { - display: none; -} - -.stepper__check { - display: block; - height: 1rem; - width: 1rem; -} - -.stepper__step--completed .stepper__label-text { - color: #059669; - font-weight: 600; -} - -/* ── Edit link ──────────────────────────────────────────────── */ - -.bce-card__edit-link { - align-items: center; - color: var(--step-active-bg); - display: inline-flex; - font-size: 0.875rem; - font-weight: 500; - gap: 0.35rem; - text-decoration: none; -} - -.bce-card__edit-link:hover { - text-decoration: underline; -} - -.bce-card__edit-link svg { - flex-shrink: 0; -} - -/* ── Summary definition list ────────────────────────────────── */ - -.bcr-summary { - margin: 0; -} - -.bcr-summary__row { - align-items: baseline; - display: flex; - gap: 1rem; - padding: 0.75rem 0; -} +/* The device table on the confirm page is the stock admin changelist, + only the surrounding chrome is styled here. */ -.bcr-summary__row + .bcr-summary__row { - border-top: 1px solid #f1f5f9; -} - -.bcr-summary__label { - color: var(--body-quiet-color); - flex-shrink: 0; - font-size: 0.875rem; - font-weight: 400; - min-width: 8rem; -} - -.bcr-summary__value { - font-size: 0.875rem; - font-weight: 500; - margin: 0; -} - -.bcr-summary__value strong { - font-weight: 700; -} - -.bcr-summary__value--warning { - color: #d97706; - font-weight: 600; +/* the stepper and the summary sit outside #content-main, so they do not + inherit its spacing */ +.confirm-batch-command .stepper { + margin-bottom: 1.5rem; } -.bcr-summary__badge { - background: #f1f5f9; - border: 1px solid #e2e8f0; - border-radius: 0.375rem; - display: inline-block; - font-family: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace; - font-size: 0.8125rem; - font-weight: 500; - padding: 0.15rem 0.5rem; +.bc-summary { + margin-bottom: 1.5rem; } -.bcr-summary__desc { - color: var(--body-quiet-color); - font-weight: 400; - margin-left: 0.35rem; +.bc-summary .form-row { + padding: 8px 12px; } -/* ── Affected devices placeholder ───────────────────────────── */ - -.bcr-devices-placeholder { - min-height: 6rem; +/* heading above the device table: only the caption bar, the table follows it + as a separate block */ +.bc-devices-heading { + margin-bottom: 1rem; } -/* ── Sticky action bar ──────────────────────────────────────── */ - -.bcr-action-bar { - align-items: center; - background: #ffffff; - border: 1px solid #e2e8f0; - border-radius: 0.75rem; - bottom: 1.5rem; - box-shadow: - 0 4px 6px -1px rgba(0, 0, 0, 0.07), - 0 2px 4px -2px rgba(0, 0, 0, 0.05); - display: flex; - gap: 1.5rem; - justify-content: space-between; - margin-top: 1.5rem; - padding: 1rem 1.5rem; - position: sticky; - z-index: 10; +.confirm-batch-command #changelist { + margin-top: 0; } -.bcr-action-bar__summary { - color: var(--body-quiet-color); - font-size: 0.9375rem; +/* no admin actions on this changelist, so the row would be empty */ +.confirm-batch-command #changelist .actions { + display: none; } -.bcr-action-bar__summary strong { - color: var(--body-fg); - font-weight: 600; +/* the checkbox column: not a link, so it renders as a plain cell */ +.confirm-batch-command #result_list th.column-select_device, +.confirm-batch-command #result_list td.field-select_device { + text-align: center; + width: 2rem; } -.bcr-action-bar__actions { - align-items: center; +.bc-execute-form .submit-row { display: flex; - gap: 0.75rem; - flex-shrink: 0; -} - -/* ── Responsive ─────────────────────────────────────────── */ - -@media (max-width: 767px) { - .batch-command-execution__heading h1 { - font-size: 1.375rem; - } - - .bce-card__header { - padding: 1.25rem 1.25rem 0; - } - - .bce-card__body { - padding: 1rem 1.25rem 1.5rem; - } - - .bce-field-grid { - grid-template-columns: 1fr; - } - - .stepper { - max-width: 100%; - } - - .stepper__step { - padding: 0.75rem 1rem 0.75rem 0; - } - - .stepper__label-text { - font-size: 0.8125rem; - } - - .bcr-summary__row { - flex-direction: column; - gap: 0.25rem; - } - - .bcr-summary__label { - min-width: 0; - } - - .bcr-action-bar { - flex-direction: column; - gap: 1rem; - padding: 1rem 1.25rem; - } - - .bcr-action-bar__actions { - width: 100%; - } - - .bcr-action-bar__actions .button { - flex: 1; - } + gap: 0.5rem; + justify-content: flex-end; } diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js new file mode 100644 index 000000000..65febc09d --- /dev/null +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -0,0 +1,357 @@ +"use strict"; + +// admin/change_form.html loads the translation catalog, these fallbacks only +// keep the page working if it ever fails to load +var gettext = + window.gettext || + function (word) { + return word; + }; +var ngettext = + window.ngettext || + function (singular, plural, count) { + return count === 1 ? singular : plural; + }; +var interpolate = + window.interpolate || + function (fmt, args) { + return fmt.replace(/%s/g, function () { + return args.shift(); + }); + }; + +django.jQuery(function ($) { + if ( + typeof owControllerApiHost === "undefined" || + typeof batchCommandId === "undefined" + ) { + return; + } + const batchCommandWebSocket = new ReconnectingWebSocket( + getWebSocketUrl(), + null, + { + debug: false, + automaticOpen: false, + // The library re-connects if it fails to establish a connection in "timeoutInterval". + // On slow internet connections, the default value of "timeoutInterval" will + // keep terminating and re-establishing the connection. + timeoutInterval: 7000, + }, + ); + batchCommandWebSocket.addEventListener("open", function () { + requestCurrentState(batchCommandWebSocket); + }); + + batchCommandWebSocket.addEventListener("message", function (e) { + let data = JSON.parse(e.data); + if (data.model === "Command") { + handleCommandMessage($, data.data); + } else if (data.model === "BatchCommand") { + handleBatchCommandMessage($, data.data); + } else if (data.model === "BatchState") { + handleBatchStateMessage($, data.data); + } + }); + + // "automaticOpen: false" above means the socket never connects unless + // .open() is called explicitly (mirrors commands.js's initCommandWebSockets). + batchCommandWebSocket.open(); + + function getWebSocketUrl() { + let protocol = getWebSocketProtocol(); + return `${protocol}${owControllerApiHost.host}/ws/controller/batch-command/${batchCommandId}`; + } + + function getWebSocketProtocol() { + let protocol = "ws://"; + if (window.location.protocol === "https:") { + protocol = "wss://"; + } + return protocol; + } + + function requestCurrentState(websocket) { + if (websocket.readyState === WebSocket.OPEN) { + try { + websocket.send( + JSON.stringify({ + type: "request_current_state", + batch_id: batchCommandId, + // only the page being shown is sent back, a mass command can + // target thousands of devices + page: getCurrentPage(), + }), + ); + } catch (error) { + console.error("Error requesting current batch state:", error); + } + } + } + + function handleBatchStateMessage($, data) { + if (data.batch_status) { + handleBatchCommandMessage($, data.batch_status); + } + updateTotals( + $, + data.batch_status ? data.batch_status.affected_devices : null, + data.total_rows, + ); + if (data.commands && Array.isArray(data.commands)) { + // These are the results of the page being shown, selected as such by + // the server, so they are drawn unconditionally: running them through + // the eligibility test used for live messages would reject them, since + // an individual result carries no page of its own. + data.commands.forEach(function (command) { + let $row = $("#batch-command-row-" + command.device); + if ($row.length) { + updateRow($, $row, command); + } else { + insertRow($, command); + } + }); + } + } + + function getActiveStatusFilter() { + return $("#result_list").attr("data-active-status") || ""; + } + + function getCurrentPage() { + return parseInt($("#result_list").attr("data-current-page"), 10) || 1; + } + + function getPerPage() { + return parseInt($("#result_list").attr("data-per-page"), 10) || 20; + } + + function handleCommandMessage($, data) { + // The totals are updated on every message, whatever happens to the DOM + // afterwards. They used to be updated at the end of insertRow(), which + // returns early once the page is full, so the counter and the paginator + // silently froze as soon as the first page filled up. + updateTotals($, data.affected_devices, data.total_rows); + renderCommand($, data); + } + + function renderCommand($, data) { + let $row = $("#batch-command-row-" + data.device); + if ($row.length) { + updateRow($, $row, data); + } else if (belongsOnCurrentPage($, data)) { + insertRow($, data); + } + // otherwise the row belongs to another page and is left alone: it will + // be rendered by the server when that page is opened + } + + /* + * The server states the page a result belongs to, and only does so for + * results it has just created. Draw it when that is the page being shown + * and it still has room, which is what makes the first page stop at "per + * page" rows while the paginator keeps growing, without moving the user. + * + * The page cannot be derived here from the total number of results: the + * total describes the whole batch, not the position of this result. A + * status change on the third result still arrives with the total of the + * batch, and would be placed on the last page instead of being left alone. + */ + function belongsOnCurrentPage($, data) { + // with a filter on, the totals pushed over the websocket are unfiltered + // and cannot be used to work out page boundaries + if (getActiveStatusFilter()) { + return false; + } + if (data.page == null) { + // a status change, not a new result: it is either already displayed + // or it lives on another page + return false; + } + let renderedRows = $("#result_list tbody tr").not( + ":has(td.empty-results)", + ).length; + if (renderedRows >= getPerPage()) { + return false; + } + return data.page === getCurrentPage(); + } + + function updateRow($, $row, data) { + let activeFilter = getActiveStatusFilter(); + if (activeFilter && activeFilter !== data.status) { + // the row no longer matches the filter the page was rendered with + $row.remove(); + return; + } + let $status = $row.find(".command-status"); + $status + .removeClass() + .addClass("command-status " + data.status) + .text(data.status_display); + $row.find(".command-output pre").text(data.output || "-"); + $row.find("td:last-child").text(formatTimestamp(data.created)); + } + + // Only draws the row: whether it should be drawn at all is decided by + // belongsOnCurrentPage(), and the totals are updated independently. + function insertRow($, data) { + // remove the "No commands found." empty state + $("#result_list td.empty-results").closest("tr").remove(); + let $tableBody = $("#result_list tbody"); + let rowClass = $tableBody.find("tr").length % 2 === 0 ? "row1" : "row2"; + let $row = $("").attr({ + id: "batch-command-row-" + data.device, + "data-device-pk": data.device, + class: rowClass, + }); + let $deviceTd = $("").append( + $("") + .attr({ href: getDeviceChangeUrl(data.device), class: "device-link" }) + .text(data.device_name), + ); + $row.append($deviceTd); + $row.append( + $("").append( + $("") + .addClass("command-status " + data.status) + .text(data.status_display), + ), + ); + $row.append( + $("") + .addClass("command-output") + .append($("
").text(data.output || "-")),
+    );
+    $row.append($("").text(formatTimestamp(data.created)));
+    $tableBody.append($row);
+  }
+
+  function getDeviceChangeUrl(devicePk) {
+    let template = $("#result_list").attr("data-device-url");
+    if (!template) {
+      return "#";
+    }
+    return template.replace("00000000-0000-0000-0000-000000000000", devicePk);
+  }
+
+  /*
+   * "affected_devices" counts Command rows, "total_rows" also counts the
+   * skipped devices the table paginates alongside them. They are two
+   * different numbers and drive two different things: passing one for both
+   * makes the page count too small and the last page unreachable whenever a
+   * device was skipped.
+   *
+   * Both are authoritative values recomputed server side on every send,
+   * never a client tracked delta, so a missed or duplicate message cannot
+   * desync them permanently.
+   */
+  function updateTotals($, affectedDevices, totalRows) {
+    if (affectedDevices != null) {
+      let $affected = $(".field-affected_devices .readonly");
+      if ($affected.length) {
+        $affected.text(String(affectedDevices));
+      }
+    }
+    if (totalRows == null) {
+      return;
+    }
+    // counts are filtered server side, the totals pushed here are not
+    if (getActiveStatusFilter()) {
+      return;
+    }
+    let $paginator = $(".results-container .paginator");
+    if ($paginator.length) {
+      $paginator.text(
+        interpolate(ngettext("%s command", "%s commands", totalRows), [
+          totalRows,
+        ]),
+      );
+    }
+    renderPagination($, totalRows);
+  }
+
+  /*
+   * Rebuilt from scratch rather than patched, so there is a single code
+   * path whether or not the widget was rendered by the server. Patching
+   * only the "Page X of Y" label used to leave the last page without a
+   * "Next" link: at "3 of 3" growing to "3 of 5" the label changed but
+   * there was still no way to move forward.
+   *
+   * This only touches the pagination widget, never the rows: the user is
+   * never navigated automatically, and no page is ever re-fetched.
+   */
+  function renderPagination($, totalRows) {
+    let currentPage = getCurrentPage();
+    let perPage = getPerPage();
+    let totalPages = Math.max(1, Math.ceil(totalRows / perPage));
+    $(".results-container .pagination").remove();
+    if (totalPages <= 1) {
+      return;
+    }
+    let pageLabel =
+      gettext("Page") +
+      " " +
+      currentPage +
+      " " +
+      gettext("of") +
+      " " +
+      totalPages;
+    let params = new URLSearchParams(window.location.search);
+    params.delete("page");
+    let baseQuery = params.toString();
+    let buildHref = function (page) {
+      return "?" + (baseQuery ? baseQuery + "&page=" + page : "page=" + page);
+    };
+    let $stepLinks = $("").addClass("step-links");
+    if (currentPage > 1) {
+      $stepLinks.append(
+        $("")
+          .attr("href", buildHref(currentPage - 1))
+          .text(gettext("Previous")),
+      );
+    }
+    $stepLinks.append($("").addClass("current-page").text(pageLabel));
+    if (currentPage < totalPages) {
+      $stepLinks.append(
+        $("")
+          .attr("href", buildHref(currentPage + 1))
+          .text(gettext("Next")),
+      );
+    }
+    $("
") + .addClass("pagination") + .append($stepLinks) + .appendTo(".results-container"); + } + + function handleBatchCommandMessage($, data) { + let $status = $(".field-colored_status .readonly .command-status"); + if ($status.length && data.status && data.status_display) { + $status + .removeClass() + .addClass("command-status " + data.status) + .text(data.status_display); + } + if (data.skipped_devices && Object.keys(data.skipped_devices).length) { + let $list = $(".field-display_skipped_devices .skipped-devices-list"); + if ($list.length) { + let $first = $list.contents().first(); + if ($first.length && $first[0].nodeType === 3) { + $first[0].textContent = Object.keys(data.skipped_devices).length; + } + } + } + } + + function formatTimestamp(iso) { + if (!iso) { + return "-"; + } + let date = new Date(iso); + if (isNaN(date.getTime())) { + return "-"; + } + return date.toLocaleString(); + } +}); diff --git a/openwisp_controller/connection/static/connection/js/execute-command.js b/openwisp_controller/connection/static/connection/js/execute-command.js index b832a5009..320f88f32 100644 --- a/openwisp_controller/connection/static/connection/js/execute-command.js +++ b/openwisp_controller/connection/static/connection/js/execute-command.js @@ -1,157 +1,381 @@ django.jQuery(function ($) { "use strict"; - var TYPE_CUSTOM = "custom"; - var TYPE_CHANGE_PASSWORD = "change_password"; + // Both steps of the mass command workflow load this file. Each section + // returns early when the element it is anchored to is missing, so only + // the one belonging to the current page does anything. + initExecuteCommandForm($); + initConfirmCommandSelection($); - var $typeSelect = $("#id_type"); - if (!$typeSelect.length) return; + //////////////////////////////////////////////////////////////////////// + // Execute command js + //////////////////////////////////////////////////////////////////////// - var $form = $typeSelect.closest("form"); - var $container = $("#command-input-container"); - var fieldName = $("#id_input").length ? $("#id_input").attr("name") : "input"; - var $hiddenInput; + function initExecuteCommandForm($) { + var TYPE_CUSTOM = "custom"; + var TYPE_CHANGE_PASSWORD = "change_password"; - function ensureHiddenInput() { - $hiddenInput = $form.find('input[name="' + fieldName + '"][type="hidden"]'); - if (!$hiddenInput.length) { - $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); - $form.append($hiddenInput); + var $typeSelect = $("#id_type"); + if (!$typeSelect.length) return; + + var $form = $typeSelect.closest("form"); + var $container = $("#command-input-container"); + var fieldName = $("#id_input").length + ? $("#id_input").attr("name") + : "input"; + var $hiddenInput; + + function ensureHiddenInput() { + $hiddenInput = $form.find( + 'input[name="' + fieldName + '"][type="hidden"]', + ); + if (!$hiddenInput.length) { + $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); + $form.append($hiddenInput); + } } - } - function clearContainer() { - $container.empty(); - } + function clearContainer() { + $container.empty(); + } - function syncCustom() { - var val = $container.find("#bce-dynamic-command").val(); - val = $.trim(val); - $hiddenInput.val(val ? JSON.stringify({ command: val }) : ""); - } + function syncCustom() { + var val = $container.find("#bce-dynamic-command").val(); + val = $.trim(val); + $hiddenInput.val(val ? JSON.stringify({ command: val }) : ""); + } - function syncPassword() { - var pw = $container.find("#bce-dynamic-password").val(); - var cp = $container.find("#bce-dynamic-confirm_password").val(); - $hiddenInput.val( - pw && cp ? JSON.stringify({ password: pw, confirm_password: cp }) : "", - ); - } + function syncPassword() { + var pw = $container.find("#bce-dynamic-password").val(); + var cp = $container.find("#bce-dynamic-confirm_password").val(); + $hiddenInput.val( + pw && cp ? JSON.stringify({ password: pw, confirm_password: cp }) : "", + ); + } - function buildCustomField() { - var $wrapper = $('
'); - $wrapper.append(''); - $wrapper.append( - '', - ); - $wrapper.append( - '
Enter the shell command to run on all devices
', - ); - $container.append($wrapper); - } + function buildCustomField() { + var $wrapper = $('
'); + var $fc = $('
'); + $fc.append( + '", + ); + $fc.append( + '', + ); + $wrapper.append($fc); + $wrapper.append( + '
' + + gettext("Enter the shell command to run on all devices") + + "
", + ); + $container.append($wrapper); + } - function buildChangePasswordField() { - var $grid = $('
'); + function buildChangePasswordField() { + var $pwRow = $('
'); + var $pwFc = $('
'); + $pwFc.append( + '", + ); + $pwFc.append( + '', + ); + $pwRow.append($pwFc); + $pwRow.append( + '
' + + gettext("Password must be at least 6 characters long") + + "
", + ); + $container.append($pwRow); - var $pwField = $('
'); - $pwField.append(''); - $pwField.append( - '', - ); - $grid.append($pwField); + var $cpRow = $('
'); + var $cpFc = $('
'); + $cpFc.append( + '", + ); + $cpFc.append( + '', + ); + $cpRow.append($cpFc); + $container.append($cpRow); + } - var $cpField = $('
'); - $cpField.append( - '', - ); - $cpField.append( - '', - ); - $grid.append($cpField); + function onTypeChange() { + var selected = $typeSelect.val(); + clearContainer(); + + if (selected === TYPE_CUSTOM) { + buildCustomField(); + syncCustom(); + } else if (selected === TYPE_CHANGE_PASSWORD) { + buildChangePasswordField(); + syncPassword(); + } else { + $hiddenInput.val(""); + } + } + + // Reaching this page starts a new mass command, so drop the device + // selections of any earlier one the user configured but never executed: + // they are namespaced per command and would otherwise pile up for as + // long as the browser tab lives. + discardAbandonedSelections(); - $container.append($grid); - $container.append( - '
Password must be at least 6 characters long
', + ensureHiddenInput(); + $container.on("input", "#bce-dynamic-command", syncCustom); + $container.on( + "input", + "#bce-dynamic-password, #bce-dynamic-confirm_password", + syncPassword, ); - } + $typeSelect.on("change", onTypeChange); + onTypeChange(); - function onTypeChange() { - var selected = $typeSelect.val(); - clearContainer(); + $("#id_type, #id_organization, #id_group, #id_location").select2({ + theme: "default", + placeholder: gettext("Select an option"), + allowClear: true, + width: "resolve", + }); - if (selected === TYPE_CUSTOM) { - buildCustomField(); - syncCustom(); - } else if (selected === TYPE_CHANGE_PASSWORD) { - buildChangePasswordField(); - syncPassword(); - } else { - $hiddenInput.val(""); + // Admin pages are served with Cache-Control: no-store, so going back to + // this page re-fetches it and the browser restores the previous form + // values after select2 has already been initialized, leaving the rendered + // labels stale. Re-sync the select2 display on every pageshow event. + $(window).on("pageshow", function () { + $("#id_type, #id_organization, #id_group, #id_location").each( + function () { + var $field = $(this); + if ($field.data("select2")) $field.trigger("change.select2"); + }, + ); + if ($typeSelect.val()) { + onTypeChange(); + var data = null; + try { + data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; + } catch (e) { + data = null; + } + if (data && data.command) { + $container.find("#bce-dynamic-command").val(data.command); + } + } + }); + + function clearAllErrors() { + $(".form-row.errors").removeClass("errors"); + $(".form-row .errorlist").remove(); } - } - ensureHiddenInput(); - $container.on("input", "#bce-dynamic-command", syncCustom); - $container.on( - "input", - "#bce-dynamic-password, #bce-dynamic-confirm_password", - syncPassword, - ); - $typeSelect.on("change", onTypeChange); - onTypeChange(); - - var $reviewBtn = $("#review-command-btn"); - if ($reviewBtn.length) { - $typeSelect.on("change", function () { - $reviewBtn.prop("disabled", !$(this).val()); - }); - $reviewBtn.prop("disabled", !$typeSelect.val()); + function showFieldError($row, message) { + $row.addClass("errors"); + $row.prepend('
  • ' + message + "
"); + } - $reviewBtn.on("click", function () { - var type = $typeSelect.val(); - if (!type) return; + var $reviewBtn = $("#review-command-btn"); + if ($reviewBtn.length) { + $reviewBtn.on("click", function () { + clearAllErrors(); - var params = new URLSearchParams(); - params.append("type", type); + var type = $typeSelect.val(); + var $typeRow = $typeSelect.closest(".form-row"); + var hasError = false; - var inputVal = $hiddenInput.val(); - if (inputVal) { - params.append("input", inputVal); - } + if (!type) { + showFieldError($typeRow, gettext("This field is required.")); + hasError = true; + } + + var label = $("#id_label").val(); + if (!label || !$.trim(label)) { + showFieldError( + $("#id_label").closest(".form-row"), + gettext("This field is required."), + ); + hasError = true; + } + + if (type === TYPE_CUSTOM) { + var cmd = $container.find("#bce-dynamic-command").val(); + if (!cmd || !$.trim(cmd)) { + showFieldError( + $container.find(".form-row").first(), + gettext("This field is required."), + ); + hasError = true; + } + } - var label = $("#id_label").val(); - if (label) { - params.append("label", label); + if (hasError) return; + + $form.submit(); + }); + } + } + + //////////////////////////////////////////////////////////////////////// + // Confirm command js + //////////////////////////////////////////////////////////////////////// + + /* + * Device selection on the confirm page. + * + * Every device matched by the targets chosen on the first step starts + * selected, unselecting one adds it to the "excluded" list. That list is + * kept both in a hidden field, submitted when the command is executed, and + * in sessionStorage, because turning the page of the device table is an + * ordinary page load: without it, unselecting a device on the first page + * would be forgotten as soon as the second page is opened. + */ + var STORAGE_PREFIX = "ow-batch-command-excluded:"; + + function discardAbandonedSelections() { + try { + var storage = window.sessionStorage; + for (var i = storage.length - 1; i >= 0; i--) { + var key = storage.key(i); + if (key && key.indexOf(STORAGE_PREFIX) === 0) { + storage.removeItem(key); + } } + } catch (e) { + // private browsing modes can make sessionStorage unavailable + } + } + + function initConfirmCommandSelection($) { + var $form = $("#bc-execute-form"); + if (!$form.length) return; + + // Namespaced by the token the server issues for this mass command: + // sessionStorage lives as long as the browser tab, so a shared key would + // make a new command inherit the devices unselected by the previous one. + var STORAGE_KEY = STORAGE_PREFIX + ($form.data("wizard-token") || ""); + var $table = $("#result_list"); + var $excludedField = $("#id_excluded"); + var $count = $("#bc-selected-count"); + var $button = $("#bc-execute-button"); + var totalDevices = parseInt($form.data("total-devices"), 10) || 0; + var excluded = readStoredExclusions(); - var notes = $("#id_notes").val(); - if (notes) { - params.append("notes", notes); + function readStoredExclusions() { + var stored = {}; + try { + var raw = window.sessionStorage.getItem(STORAGE_KEY); + $.each(raw ? JSON.parse(raw) : [], function (index, pk) { + stored[pk] = true; + }); + } catch (e) { + // private browsing modes can make sessionStorage unavailable: + // the selection is then simply not carried across pages } + return stored; + } - var org = $("#id_organization").val(); - if (org) { - params.append("organization", org); + function storeExclusions(pks) { + try { + window.sessionStorage.setItem(STORAGE_KEY, JSON.stringify(pks)); + } catch (e) { + // see readStoredExclusions() } + } - var group = $("#id_group").val(); - if (group) { - params.append("group", group); + function clearExclusions() { + try { + window.sessionStorage.removeItem(STORAGE_KEY); + } catch (e) { + // see readStoredExclusions() } + } + + // rows are rendered selected by the server, restore the ones which were + // unselected on a previously visited page + function restoreCheckboxes() { + $table.find(".bc-select-device").each(function () { + var $checkbox = $(this); + $checkbox.prop("checked", !excluded[$checkbox.val()]); + }); + } + + function refresh() { + var pks = Object.keys(excluded); + var selected = Math.max(totalDevices - pks.length, 0); + $excludedField.val(pks.join(",")); + storeExclusions(pks); + $count.text(selected); + $button.text( + interpolate( + ngettext("Execute on %s device", "Execute on %s devices", selected), + [selected], + ), + ); + $button.prop("disabled", selected === 0); + refreshSelectAll(); + } - var location = $("#id_location").val(); - if (location) { - params.append("location", location); + function refreshSelectAll() { + var $checkboxes = $table.find(".bc-select-device"); + var $checked = $checkboxes.filter(":checked"); + $("#bc-select-all").prop( + "checked", + $checkboxes.length > 0 && $checked.length === $checkboxes.length, + ); + } + + // the changelist has no header checkbox of its own once the admin + // actions are disabled, so add one for the current page + function addSelectAllCheckbox() { + var $header = $table.find("thead th").first(); + if (!$header.length || $header.find("#bc-select-all").length) return; + $header.append( + $("").attr({ + type: "checkbox", + id: "bc-select-all", + title: gettext("Select all devices on this page"), + }), + ); + } + + $table.on("change", ".bc-select-device", function () { + var pk = $(this).val(); + if (this.checked) { + delete excluded[pk]; + } else { + excluded[pk] = true; } + refresh(); + }); - $("#id_devices option:selected").each(function () { - params.append("devices", $(this).val()); + // only the devices listed on the current page are affected: devices the + // user cannot see are never selected or unselected implicitly + $table.on("change", "#bc-select-all", function () { + var checked = this.checked; + $table.find(".bc-select-device").each(function () { + var $checkbox = $(this); + if ($checkbox.prop("checked") !== checked) { + $checkbox.prop("checked", checked).trigger("change"); + } }); + }); - var confirmUrl = window.location.href.replace("execute/", "confirm/"); - window.location.href = confirmUrl.split("?")[0] + "?" + params.toString(); + $form.on("submit", function () { + clearExclusions(); + // guards against a double click creating two mass commands, the + // server discards the second request as well + $button.prop("disabled", true); }); + + addSelectAllCheckbox(); + restoreCheckboxes(); + refresh(); } }); diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html index 43ae2a83c..b432c5dee 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -6,6 +6,14 @@ + {% endblock %} {% block content %} @@ -96,7 +104,11 @@

- +
@@ -107,7 +119,9 @@

{% for command in commands %} - + - + @@ -138,7 +138,7 @@

- + {% empty %} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html index 4105befd7..ec92e55e9 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html @@ -1,30 +1,12 @@ {% extends device_changelist_template|default:"admin/change_list.html" %} {% load i18n admin_urls static %} -{% comment %} -Second step of the mass command workflow. - -This extends the changelist template of whichever ModelAdmin is registered for -Device, not the stock one, because that is where other modules load the assets -their columns need: openwisp-monitoring pulls the stylesheet and the script of -the health status accordion in from there. The device table, its pagination and -its styling are then reused as they are, and only the surrounding chrome is -added here. BatchCommandDeviceAdminMixin empties list_filter and search_fields, -which is enough for the parent template to render neither. -See BatchCommandAdmin.get_device_changelist_template(). - -Note the two forms on this page are siblings, never nested: the changelist -brings its own (which is never submitted, its checkboxes have no name) -and the execute button lives in a separate one below it. -{% endcomment %} +{# step two: extends the changelist template of the registered Device admin, #} +{# so the assets of columns added by other modules load too #} +{# see BatchCommandAdmin.get_device_changelist_template() #} {% block extrastyle %} {{ block.super }} -{% comment %} -The changelist only loads forms.css when it has a formset, but the summary -below is built out of the same .module.aligned rows the execute page uses and -needs it for its spacing. -{% endcomment %} {% endblock %} @@ -35,14 +17,9 @@ {% endblock %} -{% block bodyclass %}{{ block.super }} confirm-batch-command{% endblock %} +{% block bodyclass %}{{ block.super }} confirm-command{% endblock %} -{% comment %} -Suppresses the changelist's "Add Device" button, which has no place on a -confirmation screen. Overriding the block empty also removes the duplicate: the -theme declares object-tools twice, in .title-wrapper (admin/base.html) and in -#content-main (admin/change_list.html), so the default renders at both. -{% endcomment %} +{# hides the changelist's "Add device" button #} {% block object-tools %}{% endblock %} {% block breadcrumbs %} @@ -54,40 +31,28 @@ {% endblock %} -{% comment %} -Everything goes in a single "content" block. - -Not "object-tools": the openwisp theme declares that block twice, once inside -.title-wrapper in admin/base.html and once inside #content-main in -admin/change_list.html, so overriding it renders the content in both places. -The same is true of "filters". And a template may only declare a given block -once, so the stepper, the summary, the device table and the execute button all -live in this one block. -{% endcomment %} {% block content %} -
+

{% trans 'Summary' %}

@@ -119,7 +84,7 @@

{% trans 'Summary' %}

- {{ device_count }} {% trans 'devices' %} + {{ device_count }} {% trans 'devices' %}
@@ -131,28 +96,23 @@

{% trans 'Summary' %}

-{# same .module h2 caption bar as the Summary heading above #} -
+

{% trans 'Affected devices' %}

-{# the device table, its pagination and its own #} +{# renders the changelist: the device table, its pagination, and the wrapping both #} +{# HTML does not allow nested forms, so the execute form below is a sibling of that one: #} +{# nesting it made the browser drop the opening tag, leaving the button outside any form #} {{ block.super }} -{# sibling of the changelist's form, never nested inside it #} -{% comment %} -"data-wizard-token" namespaces the sessionStorage entry holding the unselected -devices. sessionStorage lives as long as the browser tab, so without it a new -mass command would inherit the devices unselected by the previous one. -{% endcomment %} - {% csrf_token %}
{% trans 'Back' %} -
{% endif %} - {# execute-command.js renders the fields of the selected command type here #}
{{ form.input }} {% if form.input.errors %} From 68d60a835540956111140a4e1607c91114a00d7b Mon Sep 17 00:00:00 2001 From: dee077 Date: Sat, 15 Aug 2026 02:58:07 +0530 Subject: [PATCH 08/13] [feature] Finalize mass command admin workflow #1345 Aligns the admin workflow with the patterns used by the batch upgrade of openwisp-firmware-upgrader and fixes the issues found while reviewing the whole feature. - Reuse BatchCommand.dry_run() for the confirm page target queryset instead of duplicating the targeting rule in the admin - Return querysets from resolve_devices() and dry_run(), consuming them with iterator() where the whole result is walked - Restore the live counters: affected_devices and total_devices were cached properties, which froze the websocket payload at the value computed for the first command of the batch - Truncate the command output of the results table to its last line - Show date and time in the "Modified" column, formatted server side so that live rows and reloaded rows are identical - Fix the location filter of the skipped devices, which used a non existing device_id field of DeviceLocation and raised a 500 - Show the "Clear all filters" link for the location, group and organization filters too - Remove one COUNT query per changelist row by annotating the affected devices, and fetch the batch and the skipped devices only once per request - Use message_user(), load the swappable models at module level and drop the duplicated readonly fields for consistency with the other admin classes - Restructure batch-command.js and execute-command.js to module level functions, dropping the dead gettext fallbacks and guards - Sync the verbose name of skipped_devices in the migrations, which was left unmigrated and failed checkmigrations - Update the query count of the estimated location tests, the location foreign key of BatchCommand adds a SET NULL cascade Closes #1345 --- openwisp_controller/connection/admin.py | 242 +++---- openwisp_controller/connection/apps.py | 6 + openwisp_controller/connection/base/models.py | 29 +- .../connection/channels/consumers.py | 6 + ...0011_batchcommand_command_batch_command.py | 2 +- .../static/connection/css/batch-command.css | 6 +- .../static/connection/js/batch-command.js | 531 ++++++-------- .../static/connection/js/execute-command.js | 661 ++++++++---------- .../batch_command_change_form.html | 2 +- .../geo/estimated_location/tests/tests.py | 5 +- ...0005_batchcommand_command_batch_command.py | 2 +- 11 files changed, 668 insertions(+), 824 deletions(-) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 9a6d70cd0..782cb13f4 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -8,6 +8,7 @@ from django.contrib import admin, messages from django.core.exceptions import PermissionDenied, ValidationError from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator +from django.db.models import Count from django.http import HttpResponseForbidden, JsonResponse from django.shortcuts import redirect from django.template.response import TemplateResponse @@ -22,7 +23,6 @@ from ..admin import MultitenantAdminMixin from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin -from . import settings as app_settings from .filters import GroupFilter, LocationFilter, TypeFilter from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget @@ -31,6 +31,11 @@ DeviceConnection = swapper.load_model("connection", "DeviceConnection") Command = swapper.load_model("connection", "Command") BatchCommand = swapper.load_model("connection", "BatchCommand") +Device = swapper.load_model("config", "Device") +DeviceGroup = swapper.load_model("config", "DeviceGroup") +DeviceLocation = swapper.load_model("geo", "DeviceLocation") +Location = swapper.load_model("geo", "Location") +Organization = swapper.load_model("openwisp_users", "Organization") class CredentialsForm(forms.ModelForm): @@ -46,14 +51,6 @@ class Meta: class BatchCommandExecutionForm(forms.ModelForm): - """Collects the mass command details on the first step of the workflow. - - This form is the only place where the submitted values are validated. - Narrowing the querysets in ``__init__`` controls what the widgets offer, - it does not control what is accepted, so ``clean()`` re-checks the - submitted values against the organizations the user actually manages. - """ - required_css_class = "required" class Meta: @@ -69,14 +66,10 @@ class Meta: ] widgets = { "notes": forms.Textarea(attrs={"rows": 3}), - # filled in by execute-command.js, which renders the fields - # relevant to the selected command type "input": forms.HiddenInput(), } class Media: - # select2 must be loaded before jquery.init.js, which calls - # jQuery.noConflict(): same ordering as admin.widgets.AutocompleteMixin js = [ "admin/js/vendor/jquery/jquery.min.js", "admin/js/vendor/select2/select2.full.min.js", @@ -345,27 +338,14 @@ def schema_view(self, request): class BatchCommandDeviceAdminMixin: - """Turns the device changelist into the selection table of the confirm page. - - Applied on top of whichever ModelAdmin is registered for Device rather - than on top of this module's DeviceAdmin, because other modules replace - that registration: openwisp-monitoring unregisters Device and registers - its own subclass, which adds the health status column. Building on the - registered class means those columns appear here too, along with the - select_related and the media they need, without this module knowing - which ones exist. See BatchCommandAdmin.get_device_admin(). - - Filters and search are removed on purpose: the devices are already - determined by the targets chosen on the execute page, this table only - allows excluding individual devices from that set. Emptying - ``list_filter`` and ``search_fields`` is enough for the stock changelist - template to render neither, so it can be reused as it is. + """Applied on top of the ModelAdmin registered for Device, for + openwisp-monitoring which replaces that registration and + its extra columns must appear. + + Filters and search are emptied because the devices are already chosen on + the execute page, this table only excludes some of them. """ - # DeviceAdmin leaves this as an empty tuple, which ModelAdmin reads as - # "link the first column": that would wrap the checkbox in an and - # navigate to the device instead of ticking it. Name is the column the - # device changelist links anyway. list_display_links = ["name"] list_filter = [] search_fields = [] @@ -373,12 +353,6 @@ class BatchCommandDeviceAdminMixin: list_per_page = 20 ordering = ["name"] change_list_template = "admin/connection/batch_command/confirm_command.html" - # django-import-export replaces change_list_template on the instance with - # a template of its own, which redefines the object-tools block and so - # brings back the "Import", "Export" and "Add device" buttons this page - # suppresses. Setting this to None is its documented way of opting out: - # ImportExportMixinBase.init_change_list_template() then falls back to - # the template set above. Unused when import-export is not installed. import_export_change_list_template = None def __init__(self, model, admin_site, devices=None): @@ -386,30 +360,21 @@ def __init__(self, model, admin_site, devices=None): self.devices = devices def get_list_display(self, request): - # resolved per request instead of being a class attribute: the - # attribute would be a snapshot taken when this module is imported, - # which can be before another module has replaced the registration return ["select_device"] + list(super().get_list_display(request)) def get_queryset(self, request): - # MultitenantAdminMixin.get_queryset() scopes this to the - # organizations managed by the user, independently of list_filter return super().get_queryset(request).filter(pk__in=self.devices) @admin.display(description="") def select_device(self, obj): - # deliberately without a "name": these checkboxes are never - # submitted, execute-command.js mirrors them into the hidden - # "excluded" field of the form holding the execute button return format_html( - '', + '', obj.pk, ) class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): execute_command_template = "admin/connection/batch_command/execute_command.html" - # rendered through BatchCommandDeviceAdmin.change_list_template confirm_command_template = "admin/connection/batch_command/confirm_command.html" session_key = "batch_command_wizard" list_display = [ @@ -440,7 +405,7 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): change_form_template = ( "admin/connection/batch_command/batch_command_change_form.html" ) - device_commands_per_page = app_settings.BATCH_COMMAND_PAGE_SIZE + device_commands_per_page = 20 exclude = ("devices",) fields = [ "organization_display", @@ -459,14 +424,9 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): readonly_fields = [ "organization_display", "colored_status", - "type", "formatted_input", "affected_devices", "display_skipped_devices", - "group", - "location", - "created", - "modified", ] class Media: @@ -500,11 +460,8 @@ def _check_add_permission(self, request): def execute_command_view(self, request): """First step of the mass command workflow: collect the details. - - A valid submission is stored in the session and the user is - redirected to the confirm page (Post/Redirect/Get), so that the - device table there can be paginated with ordinary GET requests: a - pagination link cannot carry the contents of a form. + A valid submission goes to the session and redirects to the confirm + page, so that its device table can be paginated with plain GETs. """ self._check_add_permission(request) if request.method == "POST": @@ -531,9 +488,7 @@ def execute_command_view(self, request): def confirm_command_view(self, request): """Second step: review the targeted devices and dispatch the command. - - Dispatching is decided by the HTTP method alone, never by looking - for a field in the request body. + Dispatching is decided by the HTTP method alone. """ self._check_add_permission(request) if request.method == "POST": @@ -553,21 +508,11 @@ def confirm_command_view(self, request): def get_device_admin(self, devices): """Builds the ModelAdmin rendering the device table of the confirm page. - - Composed with the ModelAdmin currently registered for Device instead - of a named class, so that the table shows the columns of the device - changelist as it actually is. Modules layered on top of the - controller replace that registration rather than extending the class - this module imports: openwisp-monitoring, for one, unregisters Device - and registers a subclass adding the health status column. - - Resolved here, per request, rather than at import time: every app has - finished loading by now, so the registration is final. Nothing is - imported from those modules and none of them needs to know about this - page; with none of them installed this returns the controller's own - Device admin and the table is unchanged. + Composed with whichever ModelAdmin is registered for Device, so the + table shows the columns of the device changelist as it actually is: + openwisp-monitoring replaces that registration to add its own. + Resolved per request, when the registration is final. """ - Device = swapper.load_model("config", "Device") registered = self.admin_site.get_model_admin(Device).__class__ # the mixin comes first so that its attributes win over the # registered admin's @@ -580,16 +525,10 @@ def get_device_admin(self, devices): def get_device_changelist_template(self): """The template the registered Device admin renders its changelist with. - - The confirm page extends it instead of the stock changelist template, - because that is where other modules load the assets their columns - need: openwisp-monitoring pulls in the stylesheet drawing the health - status accordion, and the script expanding it, from there. - - Read from the class rather than from an instance, since - django-import-export rewrites the attribute on the instance. + The confirm page extends it rather than the stock one, because that + is where other modules load the assets their columns need. Read from + the class: django-import-export rewrites it on the instance. """ - Device = swapper.load_model("config", "Device") registered = self.admin_site.get_model_admin(Device).__class__ return getattr(registered, "change_list_template", None) or ( "admin/change_list.html" @@ -597,41 +536,45 @@ def get_device_changelist_template(self): def _restart(self, request): """Sends the user back to step one when there is no wizard to show.""" - messages.warning( - request, _("Please fill in the mass command details to continue.") + self.message_user( + request, + _("Please fill in the mass command details to continue."), + messages.WARNING, ) return redirect(f"admin:{self.opts.app_label}_{self.opts.model_name}_execute") def _resolve_target_queryset(self, request, wizard): """Devices matched by the organization, group and location chosen. - - ``distinct()`` and an explicit ordering are required because this - queryset is paginated: the devicelocation join can return the same - device more than once, and page boundaries are undefined without an - ordering. + The targeting rule lives on the model so this page and the execution + cannot drift apart; the multitenancy scope and the ordering the + pagination needs are admin concerns, applied on top. """ - Device = swapper.load_model("config", "Device") - qs = Device.objects.all() + try: + devices = BatchCommand.dry_run( + organization_id=wizard.get("organization_id"), + group_id=wizard.get("group_id"), + location_id=wizard.get("location_id"), + )["devices"] + except ValidationError: + # a wizard left in the session while its group or location moved + # to another organization: nothing matches, and execute() reports + # it through its usual error path + return Device.objects.none() if not request.user.is_superuser: - qs = qs.filter(organization_id__in=request.user.organizations_managed) - if wizard.get("organization_id"): - qs = qs.filter(organization_id=wizard["organization_id"]) - if wizard.get("group_id"): - qs = qs.filter(group_id=wizard["group_id"]) - if wizard.get("location_id"): - qs = qs.filter(devicelocation__location_id=wizard["location_id"]) - return qs.distinct().order_by("name") + devices = devices.filter( + organization_id__in=request.user.organizations_managed + ) + return devices.distinct().order_by("name") def _confirm_context(self, request, wizard, devices): targets = [] - for app_label, model_name, key in ( - ("openwisp_users", "Organization", "organization_id"), - ("config", "DeviceGroup", "group_id"), - ("geo", "Location", "location_id"), + for model, key in ( + (Organization, "organization_id"), + (DeviceGroup, "group_id"), + (Location, "location_id"), ): if not wizard.get(key): continue - model = swapper.load_model(app_label, model_name) target = model.objects.filter(pk=wizard[key]).first() if target: targets.append(str(target)) @@ -651,10 +594,8 @@ def _confirm_context(self, request, wizard, devices): def _execute_batch_command(self, request): """Applies the device selection and dispatches the mass command. - - The wizard is popped from the session before anything else happens, - so that a double submit cannot create the batch twice: the second - request finds nothing and is sent back to step one. + The wizard is popped first, so a double submit cannot create the + batch twice: the second request finds nothing and restarts. """ wizard = request.session.pop(self.session_key, None) if not wizard: @@ -680,11 +621,13 @@ def _execute_batch_command(self, request): except ValidationError as error: # put the wizard back so the user can correct the selection request.session[self.session_key] = wizard - messages.error(request, error.messages[0]) + self.message_user(request, error.messages[0], messages.ERROR) return redirect( f"admin:{self.opts.app_label}_{self.opts.model_name}_confirm" ) - messages.success(request, _("Mass command executed successfully.")) + self.message_user( + request, _("Mass command executed successfully."), messages.SUCCESS + ) return redirect( f"admin:{self.opts.app_label}_{self.opts.model_name}_change", batch.pk ) @@ -697,6 +640,22 @@ def get_readonly_fields(self, request, obj=None): fields = super().get_readonly_fields(request, obj) return fields + list(self.__class__.readonly_fields) + def get_queryset(self, request): + return ( + super() + .get_queryset(request) + .annotate(_affected_devices=Count("batch_commands", distinct=True)) + ) + + def get_object(self, request, object_id, from_field=None): + """Avoids duplicating queries in change_view custom logic""" + cache_attr = f"_cached_object_{object_id}_{from_field}" + if not hasattr(request, cache_attr): + setattr( + request, cache_attr, super().get_object(request, object_id, from_field) + ) + return getattr(request, cache_attr) + def _get_commands(self, request, obj): qs = Command.objects.filter(batch_command=obj).select_related("device") if not request.user.is_superuser: @@ -733,17 +692,27 @@ def formatted_input(self, obj): formatted_input.short_description = _("input") def affected_devices(self, obj): - return obj.affected_devices + count = getattr(obj, "_affected_devices", None) + if count is None: + count = obj.affected_devices + return count affected_devices.short_description = _("affected devices") + affected_devices.admin_order_field = "_affected_devices" + + def _get_skipped_devices(self, obj): + if not hasattr(obj, "_skipped_devices_cache"): + obj._skipped_devices_cache = { + str(device.pk): device + for device in Device.objects.filter(pk__in=obj.skipped_devices.keys()) + } + return obj._skipped_devices_cache def display_skipped_devices(self, obj): if not obj.skipped_devices: return "-" - Device = swapper.load_model("config", "Device") - pks = list(obj.skipped_devices.keys()) - devices = {str(d.pk): d for d in Device.objects.filter(pk__in=pks)} - count = len(pks) + devices = self._get_skipped_devices(obj) + count = len(obj.skipped_devices) lines = [str(count)] for pk_str, errors in obj.skipped_devices.items(): device = devices.get(pk_str) @@ -795,8 +764,6 @@ class StatusFilter: filter_specs.append(StatusFilter()) - Device = swapper.load_model("config", "Device") - # Location filter location_spec = self._build_related_filter( _("location"), @@ -864,22 +831,17 @@ def _command_row(command): "device_pk": command.device.pk, "status": command.status, "status_display": command.get_status_display(), - "output": (command.output or "").lstrip(), - "created": command.created, + "output": command.output_preview, + "modified": command.modified, "is_skipped": False, } def _paginate_commands(self, commands_qs, skipped_rows, page_param, per_page=None): """Returns one page of rows without loading the whole batch in memory. - Commands keep the ordering of ``AbstractCommand.Meta`` ("created"), - which is the order they were fanned out in: the newest one is always - last. That is what lets the change page append results live without - re-fetching, because a new result always belongs on the last page. - - Skipped devices are not Command rows, they are entries of the - ``skipped_devices`` field, so they are kept as a (normally short) - list and follow the commands. + so the newest is always last and the change page can append results + live. Skipped devices are not Command rows, they come as a list and + follow the commands. """ per_page = per_page or self.device_commands_per_page commands_count = commands_qs.count() @@ -924,21 +886,19 @@ def _apply_command_filters(self, qs, filters): return qs def _get_matching_skipped_devices(self, obj, filters): - Device = swapper.load_model("config", "Device") pks = list(obj.skipped_devices.keys()) - device_qs = Device.objects.filter(pk__in=pks) location_id = filters["location_id"] if location_id: - DeviceLocation = swapper.load_model("geo", "DeviceLocation") - device_locations = set( - DeviceLocation.objects.filter( - device_id__in=pks, + device_locations = { + str(pk) + for pk in DeviceLocation.objects.filter( + content_object_id__in=pks, location_id=location_id, - ).values_list("device_id", flat=True) - ) + ).values_list("content_object_id", flat=True) + } else: device_locations = None - devices = {str(d.pk): d for d in device_qs} + devices = self._get_skipped_devices(obj) rows = [] for pk_str, errors in obj.skipped_devices.items(): device = devices.get(pk_str) @@ -962,7 +922,7 @@ def _get_matching_skipped_devices(self, obj, filters): "status": "skipped", "status_display": _("skipped"), "output": ", ".join(errors), - "created": None, + "modified": None, "is_skipped": True, } ) @@ -996,7 +956,7 @@ def change_view(self, request, object_id, form_url="", extra_context=None): "paginator": paginator, "filter_specs": filter_specs, "has_active_filters": any( - request.GET.get(param) for param in ["status"] + value for key, value in filters.items() if key != "q" ), } ) diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index b739d6513..5fb187972 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -3,6 +3,8 @@ from django.apps import AppConfig from django.db import transaction from django.db.models.signals import post_save +from django.utils.formats import date_format +from django.utils.timezone import localtime from django.utils.translation import gettext_lazy as _ from openwisp_notifications.signals import notify from openwisp_notifications.types import register_notification_type @@ -88,6 +90,10 @@ def command_save_receiver(cls, sender, created, instance, **kwargs): batch_data = CommandSerializer(instance).data batch_data["device_name"] = instance.device.name batch_data["status_display"] = instance.get_status_display() + batch_data["output"] = instance.output_preview + batch_data["modified"] = date_format( + localtime(instance.modified), "DATETIME_FORMAT" + ) batch_data["type"] = "command_update" batch = instance.batch_command affected_devices = batch.affected_devices diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index ef868ac03..0fab4bd63 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -550,6 +550,16 @@ def _verify_command_type_allowed(self): } ) + @property + def output_preview(self): + """Last line of the output, for tables which list many commands.""" + lines = (self.output or "").strip().splitlines() + if not lines: + return "" + if len(lines) == 1: + return lines[0] + return "… " + lines[-1] + @property def is_custom(self): return self.type == "custom" @@ -807,11 +817,11 @@ class Meta: def __str__(self): return self.label - @cached_property + @property def total_devices(self): return self.affected_devices + len(self.skipped_devices or {}) - @cached_property + @property def affected_devices(self): return self.batch_commands.count() @@ -888,12 +898,13 @@ def clean(self): def resolve_devices(self): """ - Returns an iterator of devices targeted by this batch command, + Returns a queryset of devices targeted by this batch command, resolved from explicit M2M devices or filtered by organization, - group, and location. Returns an empty iterator if no devices match. + group, and location. Callers which walk the whole result should + consume it with iterator(). """ if self.pk and self.devices.exists(): - return self.devices.select_related("config").iterator() + return self.devices.select_related("config") Device = load_model("config", "Device") qs = Device.objects.select_related("config") if self.organization_id: @@ -902,7 +913,7 @@ def resolve_devices(self): qs = qs.filter(group=self.group) if self.location: qs = qs.filter(devicelocation__location=self.location) - return qs.iterator() + return qs @classmethod def execute(cls, **kwargs): @@ -920,7 +931,7 @@ def execute(cls, **kwargs): batch.devices.set(devices_list) batch._validate_org_relations() else: - batch.devices.set(list(batch.resolve_devices())) + batch.devices.set(batch.resolve_devices()) if not batch.devices.exists(): raise ValidationError( _("No devices match the specified criteria."), @@ -950,7 +961,7 @@ def dry_run(cls, **kwargs): cls._validate_devices_org(devices_list, batch.organization_id) if devices_list is not None: return {"devices": list(devices_list)} - return {"devices": list(batch.resolve_devices())} + return {"devices": batch.resolve_devices()} def _clean_sensitive_info(self): if self.type == "change_password": @@ -973,7 +984,7 @@ def create_commands(self): Device = load_model("config", "Device") self.skipped_devices = {} device_pks = [] - for device in self.resolve_devices(): + for device in self.resolve_devices().iterator(): device_pks.append(device.pk) command = Command( device=device, diff --git a/openwisp_controller/connection/channels/consumers.py b/openwisp_controller/connection/channels/consumers.py index 73e3e760b..08562ff33 100644 --- a/openwisp_controller/connection/channels/consumers.py +++ b/openwisp_controller/connection/channels/consumers.py @@ -2,6 +2,8 @@ import logging from copy import deepcopy +from django.utils.formats import date_format +from django.utils.timezone import localtime from swapper import load_model from ...config.base.channels_consumer import BaseDeviceConsumer @@ -81,6 +83,10 @@ def _handle_current_state_request(self, page=None): row = CommandSerializer(command).data row["device_name"] = command.device.name row["status_display"] = command.get_status_display() + row["output"] = command.output_preview + row["modified"] = date_format( + localtime(command.modified), "DATETIME_FORMAT" + ) commands.append(row) self.send( json.dumps( diff --git a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py index 8059c39c4..5205fef92 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -98,7 +98,7 @@ class Migration(migrations.Migration): "devices that were skipped during command creation." ), null=True, - verbose_name="Skipped devices", + verbose_name="skipped devices", ), ), ( diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index b3aa607a5..c77bfe3fa 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -36,9 +36,9 @@ width: 100%; border-collapse: collapse; /* the output column takes whatever these three leave over */ - --device-column: 22%; - --status-column: 10%; - --modified-column: 12%; + --device-column: 23%; + --status-column: 13%; + --modified-column: 15%; } .results-table th:nth-child(1), .results-table td:nth-child(1):not(.empty-results) { diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js index 65febc09d..453ef4eca 100644 --- a/openwisp_controller/connection/static/connection/js/batch-command.js +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -1,357 +1,260 @@ "use strict"; -// admin/change_form.html loads the translation catalog, these fallbacks only -// keep the page working if it ever fails to load -var gettext = - window.gettext || - function (word) { - return word; - }; -var ngettext = - window.ngettext || - function (singular, plural, count) { - return count === 1 ? singular : plural; - }; -var interpolate = - window.interpolate || - function (fmt, args) { - return fmt.replace(/%s/g, function () { - return args.shift(); - }); - }; +const DEFAULT_PER_PAGE = 20; +const DEVICE_URL_PLACEHOLDER = "00000000-0000-0000-0000-000000000000"; django.jQuery(function ($) { - if ( - typeof owControllerApiHost === "undefined" || - typeof batchCommandId === "undefined" - ) { - return; - } - const batchCommandWebSocket = new ReconnectingWebSocket( - getWebSocketUrl(), - null, - { - debug: false, - automaticOpen: false, - // The library re-connects if it fails to establish a connection in "timeoutInterval". - // On slow internet connections, the default value of "timeoutInterval" will - // keep terminating and re-establishing the connection. - timeoutInterval: 7000, - }, - ); + const batchCommandWebSocket = new ReconnectingWebSocket(getWebSocketUrl(), null, { + debug: false, + automaticOpen: false, + timeoutInterval: 7000, + }); batchCommandWebSocket.addEventListener("open", function () { - requestCurrentState(batchCommandWebSocket); + requestCurrentState($, batchCommandWebSocket); }); - batchCommandWebSocket.addEventListener("message", function (e) { - let data = JSON.parse(e.data); - if (data.model === "Command") { - handleCommandMessage($, data.data); - } else if (data.model === "BatchCommand") { - handleBatchCommandMessage($, data.data); - } else if (data.model === "BatchState") { - handleBatchStateMessage($, data.data); + const data = JSON.parse(e.data); + if (data.type === "command_update") { + handleCommandMessage($, data); + } else if (data.type === "batch_status") { + handleBatchStatusMessage($, data); + } else if (data.type === "batch_state") { + handleBatchStateMessage($, data); } }); - - // "automaticOpen: false" above means the socket never connects unless - // .open() is called explicitly (mirrors commands.js's initCommandWebSockets). batchCommandWebSocket.open(); +}); + +function getWebSocketUrl() { + return `${getWebSocketProtocol()}${owControllerApiHost.host}/ws/controller/batch-command/${batchCommandId}`; +} - function getWebSocketUrl() { - let protocol = getWebSocketProtocol(); - return `${protocol}${owControllerApiHost.host}/ws/controller/batch-command/${batchCommandId}`; +function getWebSocketProtocol() { + let protocol = "ws://"; + if (window.location.protocol === "https:") { + protocol = "wss://"; } + return protocol; +} - function getWebSocketProtocol() { - let protocol = "ws://"; - if (window.location.protocol === "https:") { - protocol = "wss://"; - } - return protocol; +function requestCurrentState($, websocket) { + if (websocket.readyState !== WebSocket.OPEN) { + return; } + try { + websocket.send( + JSON.stringify({ + type: "request_current_state", + batch_id: batchCommandId, + page: getCurrentPage($), + }), + ); + } catch (error) { + console.error("Error requesting current batch state:", error); + } +} + +function handleCommandMessage($, data) { + updateTotals($, data.affected_devices, data.total_rows); + renderCommand($, data); +} - function requestCurrentState(websocket) { - if (websocket.readyState === WebSocket.OPEN) { - try { - websocket.send( - JSON.stringify({ - type: "request_current_state", - batch_id: batchCommandId, - // only the page being shown is sent back, a mass command can - // target thousands of devices - page: getCurrentPage(), - }), - ); - } catch (error) { - console.error("Error requesting current batch state:", error); +function handleBatchStatusMessage($, data) { + const $status = $(".field-colored_status .readonly .command-status"); + if ($status.length && data.status && data.status_display) { + $status + .removeClass() + .addClass("command-status " + data.status) + .text(data.status_display); + } + if (data.skipped_devices && Object.keys(data.skipped_devices).length) { + const $list = $(".field-display_skipped_devices .skipped-devices-list"); + if ($list.length) { + const $first = $list.contents().first(); + if ($first.length && $first[0].nodeType === 3) { + $first[0].textContent = Object.keys(data.skipped_devices).length; } } } +} - function handleBatchStateMessage($, data) { - if (data.batch_status) { - handleBatchCommandMessage($, data.batch_status); - } - updateTotals( - $, - data.batch_status ? data.batch_status.affected_devices : null, - data.total_rows, - ); - if (data.commands && Array.isArray(data.commands)) { - // These are the results of the page being shown, selected as such by - // the server, so they are drawn unconditionally: running them through - // the eligibility test used for live messages would reject them, since - // an individual result carries no page of its own. - data.commands.forEach(function (command) { - let $row = $("#batch-command-row-" + command.device); - if ($row.length) { - updateRow($, $row, command); - } else { - insertRow($, command); - } - }); - } +function handleBatchStateMessage($, data) { + if (data.batch_status) { + handleBatchStatusMessage($, data.batch_status); } - - function getActiveStatusFilter() { - return $("#result_list").attr("data-active-status") || ""; + updateTotals( + $, + data.batch_status ? data.batch_status.affected_devices : null, + data.total_rows, + ); + if (!data.commands || !Array.isArray(data.commands)) { + return; } + data.commands.forEach(function (command) { + const $row = $("#batch-command-row-" + command.device); + if ($row.length) { + updateRow($, $row, command); + } else { + insertRow($, command); + } + }); +} - function getCurrentPage() { - return parseInt($("#result_list").attr("data-current-page"), 10) || 1; +function renderCommand($, data) { + const $row = $("#batch-command-row-" + data.device); + if ($row.length) { + updateRow($, $row, data); + } else if (belongsOnCurrentPage($, data)) { + insertRow($, data); } + // otherwise the row is on another page, the server renders it there +} - function getPerPage() { - return parseInt($("#result_list").attr("data-per-page"), 10) || 20; +// The server sends the position of newly created results only, the page is +// worked out here from the size the table was rendered with: this keeps the +// first page at "per page" rows while the paginator keeps growing. +function belongsOnCurrentPage($, data) { + // with a filter on, the pushed totals are unfiltered and page boundaries + // cannot be worked out + if (getActiveStatusFilter($)) { + return false; } - - function handleCommandMessage($, data) { - // The totals are updated on every message, whatever happens to the DOM - // afterwards. They used to be updated at the end of insertRow(), which - // returns early once the page is full, so the counter and the paginator - // silently froze as soon as the first page filled up. - updateTotals($, data.affected_devices, data.total_rows); - renderCommand($, data); + if (data.index == null) { + return false; } - - function renderCommand($, data) { - let $row = $("#batch-command-row-" + data.device); - if ($row.length) { - updateRow($, $row, data); - } else if (belongsOnCurrentPage($, data)) { - insertRow($, data); - } - // otherwise the row belongs to another page and is left alone: it will - // be rendered by the server when that page is opened + const renderedRows = $("#result_list tbody tr").not(":has(td.empty-results)").length; + if (renderedRows >= getPerPage($)) { + return false; } + // the paginator is 1-based, so position 0 is on page 1 + return Math.floor(data.index / getPerPage($)) + 1 === getCurrentPage($); +} - /* - * The server states the page a result belongs to, and only does so for - * results it has just created. Draw it when that is the page being shown - * and it still has room, which is what makes the first page stop at "per - * page" rows while the paginator keeps growing, without moving the user. - * - * The page cannot be derived here from the total number of results: the - * total describes the whole batch, not the position of this result. A - * status change on the third result still arrives with the total of the - * batch, and would be placed on the last page instead of being left alone. - */ - function belongsOnCurrentPage($, data) { - // with a filter on, the totals pushed over the websocket are unfiltered - // and cannot be used to work out page boundaries - if (getActiveStatusFilter()) { - return false; - } - if (data.page == null) { - // a status change, not a new result: it is either already displayed - // or it lives on another page - return false; - } - let renderedRows = $("#result_list tbody tr").not( - ":has(td.empty-results)", - ).length; - if (renderedRows >= getPerPage()) { - return false; - } - return data.page === getCurrentPage(); +function updateRow($, $row, data) { + const activeFilter = getActiveStatusFilter($); + if (activeFilter && activeFilter !== data.status) { + $row.remove(); + return; } + $row + .find(".command-status") + .removeClass() + .addClass("command-status " + data.status) + .text(data.status_display); + $row.find(".command-output pre").text(data.output || "-"); + $row.find("td:last-child").text(data.modified || "-"); +} - function updateRow($, $row, data) { - let activeFilter = getActiveStatusFilter(); - if (activeFilter && activeFilter !== data.status) { - // the row no longer matches the filter the page was rendered with - $row.remove(); - return; +function insertRow($, data) { + $("#result_list td.empty-results").closest("tr").remove(); + const $tableBody = $("#result_list tbody"); + const rowClass = $tableBody.find("tr").length % 2 === 0 ? "row1" : "row2"; + const $row = $("
").attr({ + id: "batch-command-row-" + data.device, + "data-device-pk": data.device, + class: rowClass, + }); + $row.append( + $("").attr({ - id: "batch-command-row-" + data.device, - "data-device-pk": data.device, - class: rowClass, - }); - let $deviceTd = $(" - + {% empty %} diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index b3f0d3772..f253aeea8 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -727,9 +727,10 @@ def _verify_location_details(device, mocked_response): old_location = device2.devicelocation.location device2.last_ip = "172.217.22.10" device2.save() - # 3 queries related to notifications cleanup + # 3 queries related to notifications cleanup, + # 1 to set BatchCommand.location to NULL when the location is deleted device2.refresh_from_db() - with self.assertNumQueries(16): + with self.assertNumQueries(17): manage_estimated_locations(device2.pk, device2.last_ip) mock_info.assert_called_once_with( f"Estimated location saved successfully for {device2.pk}" diff --git a/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py b/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py index c0f67afd1..675ddac9e 100644 --- a/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py +++ b/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py @@ -98,7 +98,7 @@ class Migration(migrations.Migration): blank=True, null=True, default=dict, - verbose_name="Skipped devices", + verbose_name="skipped devices", help_text=( "Maps device UUIDs to validation error messages for " "devices that were skipped during command creation." From 08c740aa601b869a583f4bc6fa71c4a8e82e935b Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 18 Aug 2026 19:56:29 +0530 Subject: [PATCH 09/13] [fix] Addressed coderabbit comments - Use _registry instead of get_model_admin(), which is Django 5.0+ while the CI matrix still runs Django 4.2 - Validate the UUID request parameters before they reach the queryset filters, a malformed id returned a 500 - Log the ValidationError swallowed when resolving the wizard targets - Store the device name and error in skipped_devices and cap the admin field to a count, a per reason breakdown and ten devices, a batch skipping thousands of devices rendered one line each - Render the skipped devices live: send bounded counts and previews on batch_status and window the skipped rows into the paginated page of the websocket resync - Use gettext instead of gettext_lazy in the websocket payload, the lazy proxy could not be serialized by the channel layer - Drop the page parameter from the change page filter links so that filtering restarts from the first page - Keep deleted devices in the skipped rows of the unfiltered table, the field and the table disagreed on the count - Add an accessible label to the device checkboxes of the confirm page - Validate the change password fields inline, the form is submitted with novalidate so the length was never checked - Restore the wizard values when going back from the confirm page - Hide the command types the organization is not allowed to run from non superusers, every other entry point already filtered them - Extract the repeated field markup of the execute page into an include and use SimpleNamespace for the status filter spec - Use the locale aware format for the "Triggered by" timestamp - Drop the full stop from the two validation messages shown in the skipped devices list - Remove three redundant queries from the execute endpoint: the devices check of an unsaved batch, the second count of the websocket payload and the emptiness check after devices.set() - Return an empty command queryset for the "skipped" status filter, it listed every command of the batch on top of the skipped devices - Drop the command input from the batch websocket payloads and mask it in the admin, the change_password plaintext was exposed until the celery task cleaned it - Reuse the affected devices count for the total rows, total_devices ran the same COUNT a second time on every command save - Submit the execute form from its submit event so that pressing Enter runs the same validation as the button --- openwisp_controller/connection/admin.py | 125 ++++++++++++------ openwisp_controller/connection/apps.py | 16 ++- openwisp_controller/connection/base/models.py | 52 ++++++-- .../connection/channels/consumers.py | 24 +++- ...0011_batchcommand_command_batch_command.py | 5 +- .../static/connection/css/batch-command.css | 5 +- .../static/connection/js/batch-command.js | 84 +++++++++--- .../static/connection/js/execute-command.js | 66 +++++---- .../batch_command_change_form.html | 14 +- .../batch_command/confirm_command.html | 4 +- .../batch_command/execute_command.html | 80 +---------- .../connection/batch_command/form_row.html | 12 ++ .../connection/tests/test_api.py | 14 +- .../connection/tests/test_models.py | 18 +-- ...0005_batchcommand_command_batch_command.py | 5 +- 15 files changed, 309 insertions(+), 215 deletions(-) create mode 100644 openwisp_controller/connection/templates/admin/connection/batch_command/form_row.html diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 782cb13f4..2862b8844 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,12 +1,13 @@ +import logging from datetime import timedelta from types import SimpleNamespace -from uuid import uuid4 +from uuid import UUID, uuid4 import reversion import swapper from django import forms from django.contrib import admin, messages -from django.core.exceptions import PermissionDenied, ValidationError +from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.db.models import Count from django.http import HttpResponseForbidden, JsonResponse @@ -27,6 +28,8 @@ from .schema import schema from .widgets import CommandSchemaWidget, CredentialsSchemaWidget +logger = logging.getLogger(__name__) + Credentials = swapper.load_model("connection", "Credentials") DeviceConnection = swapper.load_model("connection", "DeviceConnection") Command = swapper.load_model("connection", "Command") @@ -96,6 +99,15 @@ def __init__(self, *args, request=None, **kwargs): self.fields[field_name].queryset = self.fields[field_name].queryset.filter( organization_id__in=organization_ids ) + allowed_commands = {} + for organization_id in organization_ids: + allowed_commands.update( + dict(Command.get_org_allowed_commands(organization_id=organization_id)) + ) + empty_choices = [ + choice for choice in self.fields["type"].choices if not choice[0] + ] + self.fields["type"].choices = empty_choices + list(allowed_commands.items()) def clean(self): cleaned_data = super().clean() @@ -368,8 +380,10 @@ def get_queryset(self, request): @admin.display(description="") def select_device(self, obj): return format_html( - '', + '', obj.pk, + _("Include {}").format(obj.name), ) @@ -415,9 +429,9 @@ class BatchCommandAdmin(MultitenantAdminMixin, ReadOnlyAdmin): "type", "formatted_input", "affected_devices", + "display_skipped_devices", "group", "location", - "display_skipped_devices", "created", "modified", ] @@ -472,6 +486,7 @@ def execute_command_view(self, request): f"admin:{self.opts.app_label}_{self.opts.model_name}_confirm" ) else: + request.session.pop(self.session_key, None) form = BatchCommandExecutionForm(request=request) context = { **self.admin_site.each_context(request), @@ -513,7 +528,8 @@ def get_device_admin(self, devices): openwisp-monitoring replaces that registration to add its own. Resolved per request, when the registration is final. """ - registered = self.admin_site.get_model_admin(Device).__class__ + # TODO: replace _registry with get_model_admin once Django 4.2 is dropped + registered = self.admin_site._registry[Device].__class__ # the mixin comes first so that its attributes win over the # registered admin's device_admin_class = type( @@ -529,7 +545,8 @@ def get_device_changelist_template(self): is where other modules load the assets their columns need. Read from the class: django-import-export rewrites it on the instance. """ - registered = self.admin_site.get_model_admin(Device).__class__ + # TODO: replace _registry with get_model_admin once Django 4.2 is dropped + registered = self.admin_site._registry[Device].__class__ return getattr(registered, "change_list_template", None) or ( "admin/change_list.html" ) @@ -555,10 +572,15 @@ def _resolve_target_queryset(self, request, wizard): group_id=wizard.get("group_id"), location_id=wizard.get("location_id"), )["devices"] - except ValidationError: - # a wizard left in the session while its group or location moved - # to another organization: nothing matches, and execute() reports - # it through its usual error path + except (ObjectDoesNotExist, ValidationError) as error: + logger.warning( + "Failed to resolve devices for mass command wizard" + " (organization_id=%s, group_id=%s, location_id=%s): %s", + wizard.get("organization_id"), + wizard.get("group_id"), + wizard.get("location_id"), + error, + ) return Device.objects.none() if not request.user.is_superuser: devices = devices.filter( @@ -618,6 +640,8 @@ def _execute_batch_command(self, request): } try: batch = BatchCommand.execute(**kwargs) + except ObjectDoesNotExist: + return self._restart(request) except ValidationError as error: # put the wizard back so the user can correct the selection request.session[self.session_key] = wizard @@ -632,9 +656,19 @@ def _execute_batch_command(self, request): f"admin:{self.opts.app_label}_{self.opts.model_name}_change", batch.pk ) + @staticmethod + def _get_uuid(value): + try: + return str(UUID(str(value))) + except (AttributeError, TypeError, ValueError): + return "" + @staticmethod def _get_pk_list(source, name): - return [pk for pk in source.get(name, "").split(",") if pk] + pks = ( + BatchCommandAdmin._get_uuid(pk) for pk in source.get(name, "").split(",") + ) + return [pk for pk in pks if pk] def get_readonly_fields(self, request, obj=None): fields = super().get_readonly_fields(request, obj) @@ -687,7 +721,9 @@ def colored_status(self, obj): def formatted_input(self, obj): if not obj.input: return "-" - return obj.input.get("command", obj.input) + if obj.type == "change_password": + return "********" + return self._describe_input(obj.input) or "-" formatted_input.short_description = _("input") @@ -711,16 +747,18 @@ def _get_skipped_devices(self, obj): def display_skipped_devices(self, obj): if not obj.skipped_devices: return "-" - devices = self._get_skipped_devices(obj) - count = len(obj.skipped_devices) - lines = [str(count)] - for pk_str, errors in obj.skipped_devices.items(): - device = devices.get(pk_str) - name = device.name if device else _("Deleted ({})").format(pk_str) - lines.append(format_html("{}: {}", name, ", ".join(errors))) + rows = obj.get_skipped_preview() + lines = [str(len(obj.skipped_devices))] + lines += [ + format_html("{}: {}", row["device_name"], row["output"]) for row in rows + ] + if len(rows) < len(obj.skipped_devices): + lines.insert(-1, "\u2026") return format_html( - '
{}
', + '
{}' + '

{}

', format_html_join(mark_safe("
"), "{}", ((line,) for line in lines)), + _("Refer to the table below to see what happened to each device."), ) display_skipped_devices.short_description = _("skipped devices") @@ -736,6 +774,7 @@ def _build_filter_specs( ): filter_specs = [] params = request.GET.copy() + params.pop("page", None) def _make_choice(current_value, display, param_name, value): q = params.copy() @@ -758,11 +797,7 @@ def _make_choice(current_value, display, param_name, value): _make_choice(current_status, display_name, "status", status_value) ) - class StatusFilter: - title = _("status") - choices = status_choices - - filter_specs.append(StatusFilter()) + filter_specs.append(SimpleNamespace(title=_("status"), choices=status_choices)) # Location filter location_spec = self._build_related_filter( @@ -828,7 +863,7 @@ def _build_related_filter(self, title, param_name, current_value, qs, make_choic def _command_row(command): return { "device_name": command.device.name, - "device_pk": command.device.pk, + "device": command.device.pk, "status": command.status, "status_display": command.get_status_display(), "output": command.output_preview, @@ -866,16 +901,18 @@ def _get_active_filters(self, request): return { "q": request.GET.get("q", ""), "status": request.GET.get("status", ""), - "location_id": request.GET.get("location_id", ""), - "group_id": request.GET.get("group_id", ""), - "organization_id": request.GET.get("organization_id", ""), + "location_id": self._get_uuid(request.GET.get("location_id", "")), + "group_id": self._get_uuid(request.GET.get("group_id", "")), + "organization_id": self._get_uuid(request.GET.get("organization_id", "")), } def _apply_command_filters(self, qs, filters): + status = filters["status"] + if status == "skipped": + return qs.none() if filters["q"]: qs = qs.filter(device__name__icontains=filters["q"]) - status = filters["status"] - if status and status != "skipped": + if status: qs = qs.filter(status=status) if filters["location_id"]: qs = qs.filter(device__devicelocation__location_id=filters["location_id"]) @@ -900,9 +937,19 @@ def _get_matching_skipped_devices(self, obj, filters): device_locations = None devices = self._get_skipped_devices(obj) rows = [] - for pk_str, errors in obj.skipped_devices.items(): + for pk_str, skipped in obj.skipped_devices.items(): device = devices.get(pk_str) if not device: + if not any( + ( + filters["organization_id"], + filters["group_id"], + location_id, + ) + ) and ( + not filters["q"] or filters["q"].lower() in skipped["name"].lower() + ): + rows.append(BatchCommand.build_skipped_row(pk_str, skipped)) continue if ( filters["organization_id"] @@ -913,19 +960,9 @@ def _get_matching_skipped_devices(self, obj, filters): continue if device_locations is not None and pk_str not in device_locations: continue - if filters["q"] and filters["q"].lower() not in device.name.lower(): + if filters["q"] and filters["q"].lower() not in skipped["name"].lower(): continue - rows.append( - { - "device_name": device.name, - "device_pk": pk_str, - "status": "skipped", - "status_display": _("skipped"), - "output": ", ".join(errors), - "modified": None, - "is_skipped": True, - } - ) + rows.append(BatchCommand.build_skipped_row(pk_str, skipped)) return rows def change_view(self, request, object_id, form_url="", extra_context=None): diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index 5fb187972..37dbcfc7c 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -78,16 +78,18 @@ def config_modified_receiver(cls, **kwargs): def command_save_receiver(cls, sender, created, instance, **kwargs): from .api.serializers import CommandSerializer + if created and not instance.batch_command_id: + return channel_layer = layers.get_channel_layer() serialized_data = CommandSerializer(instance).data if not created: - # Trigger websocket message only when command status is updated async_to_sync(channel_layer.group_send)( f"config.device-{instance.device_id}", {"type": "send.update", "model": "Command", "data": serialized_data}, ) if instance.batch_command_id: - batch_data = CommandSerializer(instance).data + batch_data = dict(serialized_data) + batch_data.pop("input", None) batch_data["device_name"] = instance.device.name batch_data["status_display"] = instance.get_status_display() batch_data["output"] = instance.output_preview @@ -98,7 +100,9 @@ def command_save_receiver(cls, sender, created, instance, **kwargs): batch = instance.batch_command affected_devices = batch.affected_devices batch_data["affected_devices"] = affected_devices - batch_data["total_rows"] = batch.total_devices + batch_data["total_rows"] = affected_devices + len( + batch.skipped_devices or {} + ) if created: batch_data["index"] = affected_devices - 1 async_to_sync(channel_layer.group_send)( @@ -114,6 +118,12 @@ def batch_command_save_receiver(cls, sender, instance, **kwargs): batch_data = BatchCommandSerializer(instance).data batch_data["status_display"] = instance.get_status_display() batch_data["type"] = "batch_status" + affected_devices = instance.affected_devices + skipped_count = len(batch_data.pop("skipped_devices", None) or {}) + batch_data["affected_devices"] = affected_devices + batch_data["total_rows"] = affected_devices + skipped_count + batch_data["skipped_count"] = skipped_count + batch_data["skipped_preview"] = instance.get_skipped_preview() async_to_sync(channel_layer.group_send)( f"config.batchcommand-{instance.pk}", {"type": "send.update", "data": batch_data}, diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 0fab4bd63..65916dbac 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -516,7 +516,7 @@ def __str__(self): def clean(self): if self.device.is_fully_deactivated(): - raise ValidationError({"device": _("Device is deactivated.")}) + raise ValidationError({"device": _("Device is deactivated")}) self._verify_command_type_allowed() self._verify_connection() try: @@ -527,7 +527,7 @@ def clean(self): def _verify_connection(self): """Raises validation error if device has no connection and credentials.""" if self.device and not self.device.deviceconnection_set.exists(): - raise ValidationError({"device": _("Device has no credentials assigned.")}) + raise ValidationError({"device": _("Device has no credentials assigned")}) def _verify_command_type_allowed(self): """Raises validation error if command type is not allowed.""" @@ -804,8 +804,8 @@ class AbstractBatchCommand(ValidateOrgMixin, TimeStampedEditableModel): default=dict, verbose_name=_("skipped devices"), help_text=_( - "Maps device UUIDs to validation error messages for devices " - "that were skipped during command creation." + "Maps device UUIDs to the name of the device and the validation " + "error that caused it to be skipped during command creation." ), ) @@ -821,6 +821,27 @@ def __str__(self): def total_devices(self): return self.affected_devices + len(self.skipped_devices or {}) + @staticmethod + def build_skipped_row(device_pk, skipped): + return { + "device": str(device_pk), + "device_name": skipped["name"], + "status": "skipped", + "status_display": gettext("skipped"), + "output": skipped["error"], + "modified": None, + "is_skipped": True, + } + + def get_skipped_rows(self, start=0, end=None): + items = list((self.skipped_devices or {}).items())[start:end] + return [self.build_skipped_row(pk, skipped) for pk, skipped in items] + + def get_skipped_preview(self, limit=10): + if len(self.skipped_devices or {}) <= limit: + return self.get_skipped_rows() + return self.get_skipped_rows(end=2) + self.get_skipped_rows(start=-1) + @property def affected_devices(self): return self.batch_commands.count() @@ -857,7 +878,7 @@ def _validate_org_relations(self): return self._validate_org_relation("group", field_error="group") self._validate_org_relation("location", field_error="location") - if self.pk and self.devices.exists(): + if not self._state.adding and self.devices.exists(): org_mismatch = self.devices.exclude(organization=self.organization).exists() if org_mismatch: raise ValidationError( @@ -927,12 +948,13 @@ def execute(cls, **kwargs): with transaction.atomic(): batch.full_clean() batch.save() - if devices_list is not None: + if devices_list is None: + devices_list = list(batch.resolve_devices()) batch.devices.set(devices_list) - batch._validate_org_relations() else: - batch.devices.set(batch.resolve_devices()) - if not batch.devices.exists(): + batch.devices.set(devices_list) + batch._validate_org_relations() + if not devices_list: raise ValidationError( _("No devices match the specified criteria."), ) @@ -979,7 +1001,8 @@ def create_commands(self): ) if not updated: return - self.refresh_from_db(fields=["status"]) + self.status = "in-progress" + self.save(update_fields=["status"]) Command = load_model("connection", "Command") Device = load_model("config", "Device") self.skipped_devices = {} @@ -996,9 +1019,12 @@ def create_commands(self): command.full_clean() command.save() except ValidationError as e: - self.skipped_devices[str(device.pk)] = ( - e.messages if hasattr(e, "messages") else [str(e)] - ) + self.skipped_devices[str(device.pk)] = { + "name": device.name, + "error": ( + ", ".join(e.messages) if hasattr(e, "messages") else str(e) + ), + } logger.warning( "Skipping device %s for batch %s: %s", device.pk, diff --git a/openwisp_controller/connection/channels/consumers.py b/openwisp_controller/connection/channels/consumers.py index 08562ff33..9b2ac78f6 100644 --- a/openwisp_controller/connection/channels/consumers.py +++ b/openwisp_controller/connection/channels/consumers.py @@ -51,7 +51,11 @@ def receive(self, text_data): try: content = json.loads(text_data) except ValueError: - logger.warning("Received a websocket message which is not valid JSON") + content = None + if not isinstance(content, dict): + logger.warning( + "Received a websocket message which is not a valid JSON object" + ) return message_type = content.get("type") if message_type == self.current_state_message: @@ -70,17 +74,26 @@ def _handle_current_state_request(self, page=None): return batch_status = BatchCommandSerializer(batch).data batch_status["status_display"] = batch.get_status_display() - batch_status["affected_devices"] = batch.affected_devices + commands_count = batch.batch_commands.count() + batch_status["affected_devices"] = commands_count + batch_status["skipped_count"] = len( + batch_status.pop("skipped_devices", None) or {} + ) + batch_status["skipped_preview"] = batch.get_skipped_preview() try: page = max(int(page), 1) except (TypeError, ValueError): page = 1 start = (page - 1) * self.per_page end = start + self.per_page - page_commands = batch.batch_commands.select_related("device")[start:end] + commands_end = min(end, commands_count) + page_commands = batch.batch_commands.select_related("device")[ + start:commands_end + ] commands = [] for command in page_commands: row = CommandSerializer(command).data + row.pop("input", None) row["device_name"] = command.device.name row["status_display"] = command.get_status_display() row["output"] = command.output_preview @@ -88,13 +101,16 @@ def _handle_current_state_request(self, page=None): localtime(command.modified), "DATETIME_FORMAT" ) commands.append(row) + commands += batch.get_skipped_rows( + max(0, start - commands_count), max(0, end - commands_count) + ) self.send( json.dumps( { "type": "batch_state", "batch_status": batch_status, "commands": commands, - "total_rows": batch.total_devices, + "total_rows": commands_count + batch_status["skipped_count"], } ) ) diff --git a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py index 5205fef92..e63a48708 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -94,8 +94,9 @@ class Migration(migrations.Migration): blank=True, default=dict, help_text=( - "Maps device UUIDs to validation error messages for " - "devices that were skipped during command creation." + "Maps device UUIDs to the name of the device and the " + "validation error that caused it to be skipped during " + "command creation." ), null=True, verbose_name="skipped devices", diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index c77bfe3fa..369101406 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -37,7 +37,7 @@ border-collapse: collapse; /* the output column takes whatever these three leave over */ --device-column: 23%; - --status-column: 13%; + --status-column: 15%; --modified-column: 15%; } .results-table th:nth-child(1), @@ -117,6 +117,9 @@ .skipped-devices-list { line-height: 1.7; } +.skipped-devices-note { + margin: 1em 0 0; +} .field-display_skipped_devices .readonly.readonly { padding: 0; } diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js index 453ef4eca..b5ae5046b 100644 --- a/openwisp_controller/connection/static/connection/js/batch-command.js +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -17,7 +17,7 @@ django.jQuery(function ($) { if (data.type === "command_update") { handleCommandMessage($, data); } else if (data.type === "batch_status") { - handleBatchStatusMessage($, data); + handleBatchStatusMessage($, data, batchCommandWebSocket); } else if (data.type === "batch_state") { handleBatchStateMessage($, data); } @@ -59,7 +59,7 @@ function handleCommandMessage($, data) { renderCommand($, data); } -function handleBatchStatusMessage($, data) { +function handleBatchStatusMessage($, data, websocket) { const $status = $(".field-colored_status .readonly .command-status"); if ($status.length && data.status && data.status_display) { $status @@ -67,15 +67,47 @@ function handleBatchStatusMessage($, data) { .addClass("command-status " + data.status) .text(data.status_display); } - if (data.skipped_devices && Object.keys(data.skipped_devices).length) { - const $list = $(".field-display_skipped_devices .skipped-devices-list"); - if ($list.length) { - const $first = $list.contents().first(); - if ($first.length && $first[0].nodeType === 3) { - $first[0].textContent = Object.keys(data.skipped_devices).length; - } + updateSkippedDevices($, data); + updateTotals($, data.affected_devices, data.total_rows); + const $table = $("#result_list"); + if ( + websocket && + data.skipped_count && + data.skipped_count !== $table.data("skippedCount") + ) { + $table.data("skippedCount", data.skipped_count); + requestCurrentState($, websocket); + } +} + +function updateSkippedDevices($, data) { + if (!data.skipped_count) { + return; + } + let $list = $(".field-display_skipped_devices .skipped-devices-list"); + if (!$list.length) { + const $readonly = $(".field-display_skipped_devices .readonly"); + if (!$readonly.length) { + return; } + $list = $("
").addClass("skipped-devices-list"); + $readonly.empty().append($list); } + $list.empty().append(document.createTextNode(String(data.skipped_count))); + const rows = data.skipped_preview || []; + rows.forEach(function (row, index) { + if (index === rows.length - 1 && rows.length < data.skipped_count) { + $list.append($("
")).append(document.createTextNode("\u2026")); + } + $list + .append($("
")) + .append(document.createTextNode(row.device_name + ": " + row.output)); + }); + $list.append( + $("

") + .addClass("skipped-devices-note") + .text(gettext("Refer to the table below to see what happened to each device.")), + ); } function handleBatchStateMessage($, data) { @@ -154,16 +186,24 @@ function insertRow($, data) { "data-device-pk": data.device, class: rowClass, }); - $row.append( - $("

{% trans "Device" %}
{% if command.is_skipped %} {{ command.device_name }} @@ -170,4 +184,6 @@

{% block footer %} {{ block.super }} + + {% endblock %} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html index a6975ab80..4105befd7 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html @@ -1,155 +1,164 @@ -{% extends "admin/base_site.html" %} +{% extends device_changelist_template|default:"admin/change_list.html" %} {% load i18n admin_urls static %} +{% comment %} +Second step of the mass command workflow. + +This extends the changelist template of whichever ModelAdmin is registered for +Device, not the stock one, because that is where other modules load the assets +their columns need: openwisp-monitoring pulls the stylesheet and the script of +the health status accordion in from there. The device table, its pagination and +its styling are then reused as they are, and only the surrounding chrome is +added here. BatchCommandDeviceAdminMixin empties list_filter and search_fields, +which is enough for the parent template to render neither. +See BatchCommandAdmin.get_device_changelist_template(). + +Note the two forms on this page are siblings, never nested: the changelist +brings its own
(which is never submitted, its checkboxes have no name) +and the execute button lives in a separate one below it. +{% endcomment %} + +{% block extrastyle %} +{{ block.super }} +{% comment %} +The changelist only loads forms.css when it has a formset, but the summary +below is built out of the same .module.aligned rows the execute page uses and +needs it for its spacing. +{% endcomment %} + + +{% endblock %} + {% block extrahead %} {{ block.super }} -{{ media }} - + + {% endblock %} -{% block bodyclass %}app-{{ opts.app_label }} model-{{ opts.model_name }} confirm-batch-command{% endblock %} +{% block bodyclass %}{{ block.super }} confirm-batch-command{% endblock %} + +{% comment %} +Suppresses the changelist's "Add Device" button, which has no place on a +confirmation screen. Overriding the block empty also removes the duplicate: the +theme declares object-tools twice, in .title-wrapper (admin/base.html) and in +#content-main (admin/change_list.html), so the default renders at both. +{% endcomment %} +{% block object-tools %}{% endblock %} {% block breadcrumbs %} {% endblock %} -{% block content_title %}{% endblock %} +{% comment %} +Everything goes in a single "content" block. +Not "object-tools": the openwisp theme declares that block twice, once inside +.title-wrapper in admin/base.html and once inside #content-main in +admin/change_list.html, so overriding it renders the content in both places. +The same is true of "filters". And a template may only declare a given block +once, so the stepper, the summary, the device table and the execute button all +live in this one block. +{% endcomment %} {% block content %} -
-
-

{% trans 'Review mass command' %}

-

{% trans 'Confirm what will run before execution.' %}

+ - - - {# ── Summary card ──────────────────────────────────────── #} -
-
-
-
-

{% trans 'Summary' %}

-
- - - - - {% trans 'Edit' %} - -
+
+ {% if command_description %} +
+
+ +
{{ command_description }}
-
-
-
-
{% trans 'Command' %}
-
- {{ command_type_display }} - {% if command_description %}— {{ command_description }}{% endif %} -
-
-
-
{% trans 'Targets' %}
-
{{ targets_display }}
-
-
-
{% trans 'Will run on' %}
-
- {{ device_count }} {% blocktrans count device_count=device_count %}device{% plural %}devices{% endblocktrans %} -
-
- {% if skipped_devices_count %} -
-
{% trans 'Will skip' %}
-
- {{ skipped_devices_count }} {% blocktrans count skipped_devices_count=skipped_devices_count %}device{% plural %}devices{% endblocktrans %} -
-
- {% endif %} -
-
{% trans 'Triggered by' %}
-
{{ request.user }}
-
-
+
+ {% endif %} +
+
+ +
{{ wizard.label }}
- - {% if skipped_devices_count %} - {# ── Warning banner ──────────────────────────────────────── #} -
- - - -
- {% blocktrans count skipped_devices_count=skipped_devices_count %}{{ skipped_devices_count }} device will be skipped{% plural %}{{ skipped_devices_count }} devices will be skipped{% endblocktrans %} -

{% trans 'These devices do not match the selected filters or are not available.' %}

+
+
+ +
{{ targets_display }}
- {% endif %} - - {# ── Affected devices ──────────────────────────────────── #} -
-
-
-
-

{% trans 'Affected devices' %}

-

{% trans 'Devices that will receive this command' %}

-
+
+
+ +
+ {{ device_count }} {% trans 'devices' %}
-
-
+
+
+
+ +
{{ request.user }} — {% now "F j, Y, P" %}
+ + +{# same .module h2 caption bar as the Summary heading above #} +
+

{% trans 'Affected devices' %}

+
+ +{# the device table, its pagination and its own #} +{{ block.super }} - {# ── Sticky action bar ─────────────────────────────────── #} -
-
- {% blocktrans count device_count=device_count %} - About to run {{ command_type_display }} on {{ device_count }} device +{# sibling of the changelist's form, never nested inside it #} +{% comment %} +"data-wizard-token" namespaces the sessionStorage entry holding the unselected +devices. sessionStorage lives as long as the browser tab, so without it a new +mass command would inherit the devices unselected by the previous one. +{% endcomment %} + + {% csrf_token %} + +
+ {% trans 'Back' %} +
-
- {% trans 'Back' %} - -
+
-
+ {% endblock %} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html index 851311e37..a0da29fe5 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/execute_command.html @@ -1,14 +1,26 @@ {% extends "admin/base_site.html" %} {% load i18n admin_urls static %} +{% comment %} +First step of the mass command workflow: collects the command details and the +targets. A valid submission is stored in the session and redirects to the +confirm page, where the matched devices can be reviewed. +{% endcomment %} + +{% block extrastyle %} +{{ block.super }} + + +{{ media.css }} +{% endblock %} + {% block extrahead %} {{ block.super }} -{{ media }} - - + +{{ media.js }} {% endblock %} -{% block bodyclass %}app-{{ opts.app_label }} model-{{ opts.model_name }} execute-batch-command{% endblock %} +{% block bodyclass %}{{ block.super }} app-{{ opts.app_label }} model-{{ opts.model_name }} execute-batch-command{% endblock %} {% block breadcrumbs %} {% endblock %} -{% block content_title %}{% endblock %} - {% block content %} -
-
-

{% trans 'Execute mass command' %}

-

{% trans 'Run a shell command across many devices at once' %}

-
- - - -
- - {# ── Command card ─────────────────────────────────── #} -
-
-
-
-

{% trans 'Command' %}

-

{% trans 'What to run on the selected devices' %}

-
+
+ + {% csrf_token %} + + + {% if form.non_field_errors %} +

{{ form.non_field_errors }}

+ {% endif %} + +
+

{% trans "Command" %}

+
+ {{ form.type.errors }} +
+ {{ form.type.label_tag }} + {{ form.type }}
+ {% if form.type.help_text %} +
+
{{ form.type.help_text }}
+
+ {% endif %}
-
- {% with field=form.type %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} + {# execute-command.js renders the fields of the selected command type here #} +
+ {{ form.input }} + {% if form.input.errors %} +
{{ form.input.errors }}
+ {% endif %} +
+ {{ form.label.errors }} +
+ {{ form.label.label_tag }} + {{ form.label }}
- {% endwith %} - -
- -
- {% with field=form.label %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} -
- {% endwith %} - - {% with field=form.notes %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} -
- {% endwith %} + {% if form.label.help_text %} +
+
{{ form.label.help_text }}
+ {% endif %}
-
- - {# ── Targets card ──────────────────────────────────── #} -
-
-
-
-

{% trans 'Targets' %}

-

{% trans 'Which devices receive this command' %}

-
- {% trans 'Filters combine with AND' %} +
+ {{ form.notes.errors }} +
+ {{ form.notes.label_tag }} + {{ form.notes }}
+ {% if form.notes.help_text %} +
+
{{ form.notes.help_text }}
+
+ {% endif %}
-
-
- {% with field=form.organization %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} -
- {% endwith %} - - {% with field=form.location %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} -
- {% endwith %} - - {% with field=form.group %} -
- {{ field.errors }} - - {{ field }} - {% if field.help_text %}
{{ field.help_text }}
{% endif %} -
- {% endwith %} +
+ +
+

{% trans "Targets" %}

+
+ {{ form.organization.errors }} +
+ {{ form.organization.label_tag }} + {{ form.organization }}
- -
- {% trans '12 devices match these filters' %} - - - {% trans 'Updated live' %} - + {% if form.organization.help_text %} +
+
{{ form.organization.help_text }}
+ {% endif %}
-
+
+ {{ form.location.errors }} +
+ {{ form.location.label_tag }} + {{ form.location }} +
+ {% if form.location.help_text %} +
+
{{ form.location.help_text }}
+
+ {% endif %} +
+
+ {{ form.group.errors }} +
+ {{ form.group.label_tag }} + {{ form.group }} +
+ {% if form.group.help_text %} +
+
{{ form.group.help_text }}
+
+ {% endif %} +
+
- {# ── Hidden submit (kept for form validation) ──────── #} -
- {% trans 'Cancel' %} - +
+ {% trans "Cancel" %} +
From a167425ba29694deff2bd6d6dadb5e1efd904db8 Mon Sep 17 00:00:00 2001 From: dee077 Date: Fri, 14 Aug 2026 07:19:35 +0530 Subject: [PATCH 07/13] [fix] Refactoring --- .../connection/api/serializers.py | 13 -- openwisp_controller/connection/apps.py | 37 ++--- openwisp_controller/connection/base/models.py | 2 +- .../connection/channels/consumers.py | 95 +++++------ openwisp_controller/connection/settings.py | 8 - .../static/connection/css/batch-command.css | 147 ++++++------------ .../batch_command_change_form.html | 4 +- .../batch_command/confirm_command.html | 86 +++------- .../batch_command/execute_command.html | 32 ++-- 9 files changed, 144 insertions(+), 280 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 37c4280cd..e776def03 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -15,19 +15,6 @@ BatchCommand = load_model("connection", "BatchCommand") -def command_to_batch_payload(command): - """Serialize a Command into the payload used for batch-command websocket messages. - - Shared by the batch-command signal receiver and the batch-command consumer so - real-time messages and the initial ``request_current_state`` reply use the same - shape (including the extra fields the admin table needs to render a row). - """ - data = CommandSerializer(command).data - data["device_name"] = command.device.name - data["status_display"] = command.get_status_display() - return data - - class ValidatedDeviceFieldSerializer(ValidatedModelSerializer): def validate(self, data): # Add "device_id" to the data for validation diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index 1abc5cca9..b739d6513 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -11,7 +11,6 @@ from openwisp_utils.admin_theme.menu import register_menu_group, register_menu_subitem from ..config.signals import config_deactivating, config_modified -from .settings import BATCH_COMMAND_PAGE_SIZE from .signals import is_working_changed @@ -75,7 +74,7 @@ def config_modified_receiver(cls, **kwargs): @classmethod def command_save_receiver(cls, sender, created, instance, **kwargs): - from .api.serializers import CommandSerializer, command_to_batch_payload + from .api.serializers import CommandSerializer channel_layer = layers.get_channel_layer() serialized_data = CommandSerializer(instance).data @@ -86,30 +85,19 @@ def command_save_receiver(cls, sender, created, instance, **kwargs): {"type": "send.update", "model": "Command", "data": serialized_data}, ) if instance.batch_command_id: - batch_data = command_to_batch_payload(instance) - # Authoritative counts, recomputed fresh on every send rather than - # relying on the client to increment a running total (a missed - # or duplicate message would otherwise desync it permanently). + batch_data = CommandSerializer(instance).data + batch_data["device_name"] = instance.device.name + batch_data["status_display"] = instance.get_status_display() + batch_data["type"] = "command_update" batch = instance.batch_command - affected_devices = batch.batch_commands.count() + affected_devices = batch.affected_devices batch_data["affected_devices"] = affected_devices - # the table also paginates the skipped devices, which are not - # Command rows: without them the client computes too few pages - # and the last one becomes unreachable - batch_data["total_rows"] = affected_devices + len( - batch.skipped_devices or {} - ) + batch_data["total_rows"] = batch.total_devices if created: - # Results are ordered by creation, so a new one is always the - # last: its index is the count minus one. Only new results - # carry a page, a status change is not a new row and must not - # be drawn anywhere it is not already displayed. - batch_data["page"] = ( - affected_devices - 1 - ) // BATCH_COMMAND_PAGE_SIZE + 1 + batch_data["index"] = affected_devices - 1 async_to_sync(channel_layer.group_send)( f"config.batchcommand-{instance.batch_command_id}", - {"type": "send.update", "model": "Command", "data": batch_data}, + {"type": "send.update", "data": batch_data}, ) @classmethod @@ -117,11 +105,12 @@ def batch_command_save_receiver(cls, sender, instance, **kwargs): from .api.serializers import BatchCommandSerializer channel_layer = layers.get_channel_layer() - serialized_data = BatchCommandSerializer(instance).data - serialized_data["status_display"] = instance.get_status_display() + batch_data = BatchCommandSerializer(instance).data + batch_data["status_display"] = instance.get_status_display() + batch_data["type"] = "batch_status" async_to_sync(channel_layer.group_send)( f"config.batchcommand-{instance.pk}", - {"type": "send.update", "model": "BatchCommand", "data": serialized_data}, + {"type": "send.update", "data": batch_data}, ) @classmethod diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 1db8afa8e..ef868ac03 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -809,7 +809,7 @@ def __str__(self): @cached_property def total_devices(self): - return self.batch_commands.count() + len(self.skipped_devices or {}) + return self.affected_devices + len(self.skipped_devices or {}) @cached_property def affected_devices(self): diff --git a/openwisp_controller/connection/channels/consumers.py b/openwisp_controller/connection/channels/consumers.py index a695494ab..73e3e760b 100644 --- a/openwisp_controller/connection/channels/consumers.py +++ b/openwisp_controller/connection/channels/consumers.py @@ -1,10 +1,13 @@ import json +import logging from copy import deepcopy from swapper import load_model from ...config.base.channels_consumer import BaseDeviceConsumer -from .. import settings as app_settings +from ..api.serializers import BatchCommandSerializer, CommandSerializer + +logger = logging.getLogger(__name__) Device = load_model("config", "Device") BatchCommand = load_model("connection", "BatchCommand") @@ -20,65 +23,52 @@ def send_update(self, event): class BatchCommandConsumer(BaseDeviceConsumer): model = BatchCommand channel_layer_group = "config.batchcommand" - - def connect(self): - # ensure the user can only access the batch command if they - # can view the organization it belongs to - pk = self.scope["url_route"]["kwargs"]["pk"] - user = self.scope["user"] - batch = ( - BatchCommand.objects.select_related("organization").filter(pk=pk).first() - ) - if not batch: - self.close() - return - if not user.is_superuser and not ( - batch.organization_id - and user.organizations_managed.filter(pk=batch.organization_id).exists() - ): - self.close() - return - super().connect() + per_page = 20 + current_state_message = "request_current_state" def send_update(self, event): - data = deepcopy(event) - data.pop("type") - self.send(json.dumps(data)) + self.send(json.dumps(event["data"])) - per_page = app_settings.BATCH_COMMAND_PAGE_SIZE + def is_user_authorized(self): + user = self.scope["user"] + if user.is_superuser: + return True + # a mass command cannot be changed or deleted from the admin + if not ( + user.is_staff and self._user_has_permissions(change=False, delete=False) + ): + return False + organization_id = ( + self.model.objects.filter(pk=self.scope["url_route"]["kwargs"]["pk"]) + .values_list("organization_id", flat=True) + .first() + ) + return bool(organization_id) and user.is_manager(str(organization_id)) def receive(self, text_data): try: content = json.loads(text_data) except ValueError: + logger.warning("Received a websocket message which is not valid JSON") return - if content.get("type") == "request_current_state": + message_type = content.get("type") + if message_type == self.current_state_message: self._handle_current_state_request(content.get("page")) + else: + logger.warning(f"Unknown websocket message type received: {message_type}") def _handle_current_state_request(self, page=None): - """Reply with the state of the page the client is showing. - - The client requests this once on websocket open (and on every - reconnect) so the table can be reconciled even for commands created - while the page was closed or before the socket connected. - - Only the requested page is sent: a mass command can target thousands - of devices, and serializing all of them (including their output) on - every connect would make the payload grow without bound. - """ - # Imported here instead of at module import time to avoid - # AppRegistryNotReady errors. - from ..api.serializers import BatchCommandSerializer, command_to_batch_payload + """Handle request for current state of the operation""" batch = BatchCommand.objects.filter( pk=self.scope["url_route"]["kwargs"]["pk"] ).first() if not batch: + # deleted after the connection was accepted return - affected_devices = batch.batch_commands.count() - batch_data = BatchCommandSerializer(batch).data - batch_data["status_display"] = batch.get_status_display() - batch_data["affected_devices"] = affected_devices + batch_status = BatchCommandSerializer(batch).data + batch_status["status_display"] = batch.get_status_display() + batch_status["affected_devices"] = batch.affected_devices try: page = max(int(page), 1) except (TypeError, ValueError): @@ -86,20 +76,19 @@ def _handle_current_state_request(self, page=None): start = (page - 1) * self.per_page end = start + self.per_page page_commands = batch.batch_commands.select_related("device")[start:end] - commands = [command_to_batch_payload(command) for command in page_commands] + commands = [] + for command in page_commands: + row = CommandSerializer(command).data + row["device_name"] = command.device.name + row["status_display"] = command.get_status_display() + commands.append(row) self.send( json.dumps( { - "model": "BatchState", - "data": { - "batch_status": batch_data, - "commands": commands, - "page": page, - # the table paginates the skipped devices too, they - # are not Command rows - "total_rows": affected_devices - + len(batch.skipped_devices or {}), - }, + "type": "batch_state", + "batch_status": batch_status, + "commands": commands, + "total_rows": batch.total_devices, } ) ) diff --git a/openwisp_controller/connection/settings.py b/openwisp_controller/connection/settings.py index d28cfdc5d..50223a137 100644 --- a/openwisp_controller/connection/settings.py +++ b/openwisp_controller/connection/settings.py @@ -35,14 +35,6 @@ }, ) -# How many results are listed per page on the mass command change page. -# Shared by the admin, which paginates with it, and by the websocket layer, -# which tells the browser the page a new result belongs to: the two have to -# agree or results are drawn on the wrong page. -BATCH_COMMAND_PAGE_SIZE = getattr( - settings, "OPENWISP_CONTROLLER_BATCH_COMMAND_PAGE_SIZE", 20 -) - SSH_AUTH_TIMEOUT = getattr(settings, "OPENWISP_SSH_AUTH_TIMEOUT", 2) SSH_BANNER_TIMEOUT = getattr(settings, "OPENWISP_SSH_BANNER_TIMEOUT", 60) SSH_COMMAND_TIMEOUT = getattr(settings, "OPENWISP_SSH_COMMAND_TIMEOUT", 30) diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index 749070dd7..b3aa607a5 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -1,23 +1,20 @@ +/* ==== Mass Command Change Page CSS ==== */ #batchcommand_form .submit-row { display: none; } - .commands-title { font-size: 22px; font-weight: 300; margin: 0; padding: 0; } - .search-section { padding: 20px; } - .search-form { display: flex; align-items: center; } - #main #content .search-icon { width: 20px; height: 20px; @@ -25,92 +22,89 @@ font-size: 16px; margin-top: -3px; } - #main #content .search-input { padding: 10px 15px; } - #main #content .search-button { padding: 10px 20px; margin-left: 15px; } - .filter-clear-link:hover { color: var(--ow-color-fg-darker); } - .results-table { width: 100%; border-collapse: collapse; + /* the output column takes whatever these three leave over */ + --device-column: 22%; + --status-column: 10%; + --modified-column: 12%; +} +.results-table th:nth-child(1), +.results-table td:nth-child(1):not(.empty-results) { + width: var(--device-column); +} +.results-table th:nth-child(2), +.results-table td:nth-child(2) { + width: var(--status-column); + white-space: nowrap; +} +.results-table th:nth-child(4), +.results-table td:nth-child(4) { + width: var(--modified-column); + white-space: nowrap; } - #main #content .device-link { color: var(--ow-color-primary); font-weight: bold; } - .device-name-disabled { color: var(--body-quiet-color); font-style: italic; } - .empty-results { padding: 40px; text-align: center; color: var(--body-quiet-color); font-style: italic; } - .pagination { padding: 15px 20px; text-align: right; border-top: 2px solid var(--hairline-color); background: var(--darkened-bg); } - .pagination a { color: var(--body-fg); text-decoration: none; margin: 0 5px; } - .pagination .current-page { margin: 0 10px; color: var(--body-quiet-color); } - .paginator { color: var(--body-quiet-color); padding: 10px 20px; border-bottom: 1px solid var(--hairline-color); margin: 0; } - .command-status { font-weight: bold; } - .command-status.success { color: var(--ow-color-success); } - .command-status.failed { color: var(--error-fg); } - .command-status.in-progress { color: var(--body-quiet-color); } - .command-status.skipped { color: var(--body-quiet-color); opacity: 0.7; } - -.command-output { - padding: 0; -} - .command-output pre { white-space: pre-wrap; word-wrap: break-word; @@ -120,16 +114,14 @@ color: inherit; background: transparent; } - .skipped-devices-list { line-height: 1.7; } - .field-display_skipped_devices .readonly.readonly { padding: 0; } -/* Adjustments for list filters */ +/* List Filters */ #main #content .left-arrow { left: -1.125rem; } @@ -143,29 +135,19 @@ margin-bottom: 0.5rem; } -/* ================================================================ - STEPPER - ================================================================ */ - +/* ==== Stepper CSS ==== */ .stepper { --step-active-bg: var(--ow-color-primary); --step-active-text: var(--ow-color-white); - --step-active-highlight: var(--ow-color-primary-light); - --step-active-tint: var(--ow-color-primary-lighter); - --step-active-underline: var(--ow-color-primary); - --step-inactive-bg: var(--ow-color-fg-light); --step-inactive-text: var(--ow-color-fg-dark); - - --divider-color: var(--ow-color-fg-light); --arrow-color: var(--ow-color-fg-dark); display: inline-flex; align-items: stretch; overflow: hidden; margin-bottom: 1.75rem; } - -.stepper__step { +.stepper-step { align-items: center; cursor: pointer; display: flex; @@ -173,9 +155,7 @@ padding: 0.875rem 1.5rem 0.875rem 0; position: relative; } - -/* Badge */ -.stepper__badge { +.stepper-badge { align-items: center; border-radius: 50%; display: flex; @@ -188,60 +168,42 @@ width: 2rem; z-index: 1; } - -.stepper__step--active .stepper__badge { +.stepper-step.active .stepper-badge { background-color: var(--step-active-bg); color: var(--step-active-text); } - -.stepper__step--active .stepper__badge::before { - background-color: var(--step-active-highlight); -} - -.stepper__step--inactive .stepper__badge { +.stepper-step.inactive .stepper-badge { background-color: var(--step-inactive-bg); color: var(--step-inactive-text); } - -.stepper__step--inactive .stepper__badge::before { - display: none; -} - -/* Label */ -.stepper__label { +.stepper-label { display: flex; flex-direction: column; gap: 0.2rem; min-width: 0; } - -.stepper__label-text { +.stepper-label-text { font-size: 0.875rem; font-weight: 500; line-height: 1.2; white-space: nowrap; } - -.stepper__step--active .stepper__label-text { +.stepper-step.active .stepper-label-text { color: var(--step-active-bg); font-weight: 600; } - -.stepper__step--inactive .stepper__label-text { +.stepper-step.inactive .stepper-label-text { color: var(--step-inactive-text); font-weight: 500; } - -/* Divider + arrow */ -.stepper__divider { +.stepper-divider { align-items: center; display: flex; flex-shrink: 0; justify-content: center; padding: 0.5rem 1rem 0.5rem 0; } - -.stepper__arrow { +.stepper-arrow { color: var(--arrow-color); display: block; flex-shrink: 0; @@ -249,50 +211,41 @@ width: 1rem; } -/* ================================================================ - CONFIRM PAGE - ================================================================ */ - -/* The device table on the confirm page is the stock admin changelist, - only the surrounding chrome is styled here. */ - -/* the stepper and the summary sit outside #content-main, so they do not - inherit its spacing */ -.confirm-batch-command .stepper { +/* ==== Confirm Page CSS ==== */ +.confirm-command .stepper { margin-bottom: 1.5rem; } - -.bc-summary { +.command-summary { margin-bottom: 1.5rem; } - -.bc-summary .form-row { +.command-summary .form-row { padding: 8px 12px; } - -/* heading above the device table: only the caption bar, the table follows it - as a separate block */ -.bc-devices-heading { +.devices-heading { margin-bottom: 1rem; } - -.confirm-batch-command #changelist { +.confirm-command #changelist { margin-top: 0; } - /* no admin actions on this changelist, so the row would be empty */ -.confirm-batch-command #changelist .actions { +.confirm-command #changelist .actions { display: none; } - -/* the checkbox column: not a link, so it renders as a plain cell */ -.confirm-batch-command #result_list th.column-select_device, -.confirm-batch-command #result_list td.field-select_device { +/* Django styles its own rows with "tr:has(.action-select:checked)", which cannot match here */ +.confirm-command #changelist tbody tr:has(.device-checkbox:checked) { + background-color: var(--selected-row); +} +@media (forced-colors: active) { + .confirm-command #changelist tbody tr:has(.device-checkbox:checked) { + background-color: SelectedItem; + } +} +.confirm-command #result_list th.column-select_device, +.confirm-command #result_list td.field-select_device { text-align: center; width: 2rem; } - -.bc-execute-form .submit-row { +.execute-form .submit-row { display: flex; gap: 0.5rem; justify-content: flex-end; diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html index b432c5dee..d3bb2c54a 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -114,7 +114,7 @@

{% trans "Device" %} {% trans "Status" %} {% trans "Output" %}{% trans "Timestamp" %}{% trans "Modified" %}
{{ command.output|default:"-" }}
{{ command.created|date|default:"-" }}{{ command.modified|date|default:"-" }}
").append( + $("") + .attr({ + href: getDeviceChangeUrl($, data.device), + class: "device-link", + }) + .text(data.device_name), + ), + ); + $row.append( + $("").append( + $("") + .addClass("command-status " + data.status) + .text(data.status_display), + ), + ); + $row.append( + $("") + .addClass("command-output") + .append($("
").text(data.output || "-")),
+  );
+  $row.append($("
").text(data.modified || "-")); + $tableBody.append($row); +} + +function updateTotals($, affectedDevices, totalRows) { + if (affectedDevices != null) { + const $affected = $(".field-affected_devices .readonly"); + if ($affected.length) { + $affected.text(String(affectedDevices)); } - let $status = $row.find(".command-status"); - $status - .removeClass() - .addClass("command-status " + data.status) - .text(data.status_display); - $row.find(".command-output pre").text(data.output || "-"); - $row.find("td:last-child").text(formatTimestamp(data.created)); } + // counts are filtered server side, the totals pushed here are not + if (totalRows == null || getActiveStatusFilter($)) { + return; + } + const $paginator = $(".results-container .paginator"); + if ($paginator.length) { + $paginator.text( + interpolate(ngettext("%s command", "%s commands", totalRows), [totalRows]), + ); + } + renderPagination($, totalRows); +} - // Only draws the row: whether it should be drawn at all is decided by - // belongsOnCurrentPage(), and the totals are updated independently. - function insertRow($, data) { - // remove the "No commands found." empty state - $("#result_list td.empty-results").closest("tr").remove(); - let $tableBody = $("#result_list tbody"); - let rowClass = $tableBody.find("tr").length % 2 === 0 ? "row1" : "row2"; - let $row = $("
").append( +function renderPagination($, totalRows) { + const currentPage = getCurrentPage($); + const perPage = getPerPage($); + const totalPages = Math.max(1, Math.ceil(totalRows / perPage)); + $(".results-container .pagination").remove(); + if (totalPages <= 1) { + return; + } + const params = new URLSearchParams(window.location.search); + params.delete("page"); + const baseQuery = params.toString(); + const buildHref = function (page) { + return "?" + (baseQuery ? baseQuery + "&page=" + page : "page=" + page); + }; + const $stepLinks = $("").addClass("step-links"); + if (currentPage > 1) { + $stepLinks.append( $("") - .attr({ href: getDeviceChangeUrl(data.device), class: "device-link" }) - .text(data.device_name), + .attr("href", buildHref(currentPage - 1)) + .text(gettext("Previous")), ); - $row.append($deviceTd); - $row.append( - $("").append( - $("") - .addClass("command-status " + data.status) - .text(data.status_display), + } + $stepLinks.append( + $("") + .addClass("current-page") + .text( + gettext("Page") + " " + currentPage + " " + gettext("of") + " " + totalPages, ), + ); + if (currentPage < totalPages) { + $stepLinks.append( + $("") + .attr("href", buildHref(currentPage + 1)) + .text(gettext("Next")), ); - $row.append( - $("") - .addClass("command-output") - .append($("
").text(data.output || "-")),
-    );
-    $row.append($("
").text(formatTimestamp(data.created))); - $tableBody.append($row); } + $("
").addClass("pagination").append($stepLinks).appendTo(".results-container"); +} - function getDeviceChangeUrl(devicePk) { - let template = $("#result_list").attr("data-device-url"); - if (!template) { - return "#"; - } - return template.replace("00000000-0000-0000-0000-000000000000", devicePk); +function getDeviceChangeUrl($, devicePk) { + const template = $("#result_list").attr("data-device-url"); + if (!template) { + return "#"; } + return template.replace(DEVICE_URL_PLACEHOLDER, devicePk); +} - /* - * "affected_devices" counts Command rows, "total_rows" also counts the - * skipped devices the table paginates alongside them. They are two - * different numbers and drive two different things: passing one for both - * makes the page count too small and the last page unreachable whenever a - * device was skipped. - * - * Both are authoritative values recomputed server side on every send, - * never a client tracked delta, so a missed or duplicate message cannot - * desync them permanently. - */ - function updateTotals($, affectedDevices, totalRows) { - if (affectedDevices != null) { - let $affected = $(".field-affected_devices .readonly"); - if ($affected.length) { - $affected.text(String(affectedDevices)); - } - } - if (totalRows == null) { - return; - } - // counts are filtered server side, the totals pushed here are not - if (getActiveStatusFilter()) { - return; - } - let $paginator = $(".results-container .paginator"); - if ($paginator.length) { - $paginator.text( - interpolate(ngettext("%s command", "%s commands", totalRows), [ - totalRows, - ]), - ); - } - renderPagination($, totalRows); - } - - /* - * Rebuilt from scratch rather than patched, so there is a single code - * path whether or not the widget was rendered by the server. Patching - * only the "Page X of Y" label used to leave the last page without a - * "Next" link: at "3 of 3" growing to "3 of 5" the label changed but - * there was still no way to move forward. - * - * This only touches the pagination widget, never the rows: the user is - * never navigated automatically, and no page is ever re-fetched. - */ - function renderPagination($, totalRows) { - let currentPage = getCurrentPage(); - let perPage = getPerPage(); - let totalPages = Math.max(1, Math.ceil(totalRows / perPage)); - $(".results-container .pagination").remove(); - if (totalPages <= 1) { - return; - } - let pageLabel = - gettext("Page") + - " " + - currentPage + - " " + - gettext("of") + - " " + - totalPages; - let params = new URLSearchParams(window.location.search); - params.delete("page"); - let baseQuery = params.toString(); - let buildHref = function (page) { - return "?" + (baseQuery ? baseQuery + "&page=" + page : "page=" + page); - }; - let $stepLinks = $("").addClass("step-links"); - if (currentPage > 1) { - $stepLinks.append( - $("") - .attr("href", buildHref(currentPage - 1)) - .text(gettext("Previous")), - ); - } - $stepLinks.append($("").addClass("current-page").text(pageLabel)); - if (currentPage < totalPages) { - $stepLinks.append( - $("") - .attr("href", buildHref(currentPage + 1)) - .text(gettext("Next")), - ); - } - $("
") - .addClass("pagination") - .append($stepLinks) - .appendTo(".results-container"); - } +function getActiveStatusFilter($) { + return $("#result_list").attr("data-active-status") || ""; +} - function handleBatchCommandMessage($, data) { - let $status = $(".field-colored_status .readonly .command-status"); - if ($status.length && data.status && data.status_display) { - $status - .removeClass() - .addClass("command-status " + data.status) - .text(data.status_display); - } - if (data.skipped_devices && Object.keys(data.skipped_devices).length) { - let $list = $(".field-display_skipped_devices .skipped-devices-list"); - if ($list.length) { - let $first = $list.contents().first(); - if ($first.length && $first[0].nodeType === 3) { - $first[0].textContent = Object.keys(data.skipped_devices).length; - } - } - } - } +function getCurrentPage($) { + return parseInt($("#result_list").attr("data-current-page"), 10) || 1; +} - function formatTimestamp(iso) { - if (!iso) { - return "-"; - } - let date = new Date(iso); - if (isNaN(date.getTime())) { - return "-"; - } - return date.toLocaleString(); - } -}); +function getPerPage($) { + return parseInt($("#result_list").attr("data-per-page"), 10) || DEFAULT_PER_PAGE; +} diff --git a/openwisp_controller/connection/static/connection/js/execute-command.js b/openwisp_controller/connection/static/connection/js/execute-command.js index 320f88f32..6ac818c27 100644 --- a/openwisp_controller/connection/static/connection/js/execute-command.js +++ b/openwisp_controller/connection/static/connection/js/execute-command.js @@ -1,381 +1,338 @@ -django.jQuery(function ($) { - "use strict"; +"use strict"; - // Both steps of the mass command workflow load this file. Each section - // returns early when the element it is anchored to is missing, so only - // the one belonging to the current page does anything. - initExecuteCommandForm($); - initConfirmCommandSelection($); +const COMMAND_TYPE_CUSTOM = "custom"; +const COMMAND_TYPE_CHANGE_PASSWORD = "change_password"; +const EXCLUDED_STORAGE_PREFIX = "ow-batch-command-excluded:"; +const WIZARD_SELECTS = "#id_type, #id_organization, #id_group, #id_location"; - //////////////////////////////////////////////////////////////////////// - // Execute command js - //////////////////////////////////////////////////////////////////////// +django.jQuery(function ($) { + initExecuteCommandForm($); + initDeviceSelection($); +}); - function initExecuteCommandForm($) { - var TYPE_CUSTOM = "custom"; - var TYPE_CHANGE_PASSWORD = "change_password"; +function initExecuteCommandForm($) { + const $typeSelect = $("#id_type"); + if (!$typeSelect.length) { + return; + } + const $form = $typeSelect.closest("form"); + const $container = $("#command-input-container"); + const fieldName = $("#id_input").length ? $("#id_input").attr("name") : "input"; + const $hiddenInput = getHiddenInput($, $form, fieldName); + + function updateCustomCommandInput() { + const command = $.trim($container.find("#id_command").val()); + $hiddenInput.val(command ? JSON.stringify({ command: command }) : ""); + } - var $typeSelect = $("#id_type"); - if (!$typeSelect.length) return; + function updateChangePasswordInput() { + const password = $container.find("#id_password").val(); + const confirmPassword = $container.find("#id_confirm_password").val(); + $hiddenInput.val( + password && confirmPassword + ? JSON.stringify({ + password: password, + confirm_password: confirmPassword, + }) + : "", + ); + } - var $form = $typeSelect.closest("form"); - var $container = $("#command-input-container"); - var fieldName = $("#id_input").length - ? $("#id_input").attr("name") - : "input"; - var $hiddenInput; + function handleTypeChange() { + const selected = $typeSelect.val(); + $container.empty(); + if (selected === COMMAND_TYPE_CUSTOM) { + renderCustomCommandField($, $container, fieldName); + updateCustomCommandInput(); + } else if (selected === COMMAND_TYPE_CHANGE_PASSWORD) { + renderChangePasswordFields($, $container); + updateChangePasswordInput(); + } else { + $hiddenInput.val(""); + } + } - function ensureHiddenInput() { - $hiddenInput = $form.find( - 'input[name="' + fieldName + '"][type="hidden"]', - ); - if (!$hiddenInput.length) { - $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); - $form.append($hiddenInput); + // a new mass command starts here, so the devices unselected in an earlier + // one which was configured but never executed are dropped + clearAbandonedExclusions(); + + $container.on("input", "#id_command", updateCustomCommandInput); + $container.on( + "input", + "#id_password, #id_confirm_password", + updateChangePasswordInput, + ); + $typeSelect.on("change", handleTypeChange); + handleTypeChange(); + + $(WIZARD_SELECTS).select2({ + theme: "default", + placeholder: gettext("Select an option"), + allowClear: true, + width: "resolve", + }); + + // admin pages are served with Cache-Control: no-store, so going back + // restores the form values after select2 has rendered its labels + $(window).on("pageshow", function () { + $(WIZARD_SELECTS).each(function () { + const $field = $(this); + if ($field.data("select2")) { + $field.trigger("change.select2"); } + }); + if (!$typeSelect.val()) { + return; } - - function clearContainer() { - $container.empty(); + handleTypeChange(); + let data = null; + try { + data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; + } catch (error) { + data = null; } - - function syncCustom() { - var val = $container.find("#bce-dynamic-command").val(); - val = $.trim(val); - $hiddenInput.val(val ? JSON.stringify({ command: val }) : ""); + if (data && data.command) { + $container.find("#id_command").val(data.command); } - - function syncPassword() { - var pw = $container.find("#bce-dynamic-password").val(); - var cp = $container.find("#bce-dynamic-confirm_password").val(); - $hiddenInput.val( - pw && cp ? JSON.stringify({ password: pw, confirm_password: cp }) : "", + }); + + $("#review-command-btn").on("click", function () { + clearFieldErrors($); + const type = $typeSelect.val(); + let hasError = false; + if (!type) { + showFieldError( + $typeSelect.closest(".form-row"), + gettext("This field is required."), ); + hasError = true; } - - function buildCustomField() { - var $wrapper = $('
'); - var $fc = $('
'); - $fc.append( - '", + if (!$.trim($("#id_label").val() || "")) { + showFieldError( + $("#id_label").closest(".form-row"), + gettext("This field is required."), ); - $fc.append( - '', - ); - $wrapper.append($fc); - $wrapper.append( - '
' + - gettext("Enter the shell command to run on all devices") + - "
", - ); - $container.append($wrapper); + hasError = true; } - - function buildChangePasswordField() { - var $pwRow = $('
'); - var $pwFc = $('
'); - $pwFc.append( - '", - ); - $pwFc.append( - '', - ); - $pwRow.append($pwFc); - $pwRow.append( - '
' + - gettext("Password must be at least 6 characters long") + - "
", - ); - $container.append($pwRow); - - var $cpRow = $('
'); - var $cpFc = $('
'); - $cpFc.append( - '", - ); - $cpFc.append( - '', - ); - $cpRow.append($cpFc); - $container.append($cpRow); - } - - function onTypeChange() { - var selected = $typeSelect.val(); - clearContainer(); - - if (selected === TYPE_CUSTOM) { - buildCustomField(); - syncCustom(); - } else if (selected === TYPE_CHANGE_PASSWORD) { - buildChangePasswordField(); - syncPassword(); - } else { - $hiddenInput.val(""); - } - } - - // Reaching this page starts a new mass command, so drop the device - // selections of any earlier one the user configured but never executed: - // they are namespaced per command and would otherwise pile up for as - // long as the browser tab lives. - discardAbandonedSelections(); - - ensureHiddenInput(); - $container.on("input", "#bce-dynamic-command", syncCustom); - $container.on( - "input", - "#bce-dynamic-password, #bce-dynamic-confirm_password", - syncPassword, - ); - $typeSelect.on("change", onTypeChange); - onTypeChange(); - - $("#id_type, #id_organization, #id_group, #id_location").select2({ - theme: "default", - placeholder: gettext("Select an option"), - allowClear: true, - width: "resolve", - }); - - // Admin pages are served with Cache-Control: no-store, so going back to - // this page re-fetches it and the browser restores the previous form - // values after select2 has already been initialized, leaving the rendered - // labels stale. Re-sync the select2 display on every pageshow event. - $(window).on("pageshow", function () { - $("#id_type, #id_organization, #id_group, #id_location").each( - function () { - var $field = $(this); - if ($field.data("select2")) $field.trigger("change.select2"); - }, - ); - if ($typeSelect.val()) { - onTypeChange(); - var data = null; - try { - data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; - } catch (e) { - data = null; - } - if (data && data.command) { - $container.find("#bce-dynamic-command").val(data.command); - } + if (type === COMMAND_TYPE_CUSTOM) { + const command = $container.find("#id_command").val(); + if (!$.trim(command || "")) { + showFieldError( + $container.find(".form-row").first(), + gettext("This field is required."), + ); + hasError = true; } - }); - - function clearAllErrors() { - $(".form-row.errors").removeClass("errors"); - $(".form-row .errorlist").remove(); } - - function showFieldError($row, message) { - $row.addClass("errors"); - $row.prepend('
  • ' + message + "
"); + if (!hasError) { + $form.submit(); } + }); +} - var $reviewBtn = $("#review-command-btn"); - if ($reviewBtn.length) { - $reviewBtn.on("click", function () { - clearAllErrors(); - - var type = $typeSelect.val(); - var $typeRow = $typeSelect.closest(".form-row"); - var hasError = false; - - if (!type) { - showFieldError($typeRow, gettext("This field is required.")); - hasError = true; - } - - var label = $("#id_label").val(); - if (!label || !$.trim(label)) { - showFieldError( - $("#id_label").closest(".form-row"), - gettext("This field is required."), - ); - hasError = true; - } - - if (type === TYPE_CUSTOM) { - var cmd = $container.find("#bce-dynamic-command").val(); - if (!cmd || !$.trim(cmd)) { - showFieldError( - $container.find(".form-row").first(), - gettext("This field is required."), - ); - hasError = true; - } - } - - if (hasError) return; - - $form.submit(); - }); - } +function initDeviceSelection($) { + const $form = $("#execute-form"); + if (!$form.length) { + return; } - - //////////////////////////////////////////////////////////////////////// - // Confirm command js - //////////////////////////////////////////////////////////////////////// - - /* - * Device selection on the confirm page. - * - * Every device matched by the targets chosen on the first step starts - * selected, unselecting one adds it to the "excluded" list. That list is - * kept both in a hidden field, submitted when the command is executed, and - * in sessionStorage, because turning the page of the device table is an - * ordinary page load: without it, unselecting a device on the first page - * would be forgotten as soon as the second page is opened. - */ - var STORAGE_PREFIX = "ow-batch-command-excluded:"; - - function discardAbandonedSelections() { - try { - var storage = window.sessionStorage; - for (var i = storage.length - 1; i >= 0; i--) { - var key = storage.key(i); - if (key && key.indexOf(STORAGE_PREFIX) === 0) { - storage.removeItem(key); - } - } - } catch (e) { - // private browsing modes can make sessionStorage unavailable - } + // sessionStorage lives as long as the browser tab, so the key carries the + // token of this mass command to stop a new one inheriting its exclusions + const storageKey = EXCLUDED_STORAGE_PREFIX + ($form.data("wizard-token") || ""); + const $table = $("#result_list"); + const $excludedField = $("#id_excluded"); + const $count = $("#selected-count"); + const $button = $("#execute-button"); + const totalDevices = parseInt($form.data("total-devices"), 10) || 0; + const excluded = getStoredExclusions($, storageKey); + + function updateSelectionSummary() { + const pks = Object.keys(excluded); + const selected = Math.max(totalDevices - pks.length, 0); + $excludedField.val(pks.join(",")); + setStoredExclusions(storageKey, pks); + $count.text(selected); + $button.text( + interpolate(ngettext("Execute on %s device", "Execute on %s devices", selected), [ + selected, + ]), + ); + $button.prop("disabled", selected === 0); + updateSelectAllCheckbox(); } - function initConfirmCommandSelection($) { - var $form = $("#bc-execute-form"); - if (!$form.length) return; - - // Namespaced by the token the server issues for this mass command: - // sessionStorage lives as long as the browser tab, so a shared key would - // make a new command inherit the devices unselected by the previous one. - var STORAGE_KEY = STORAGE_PREFIX + ($form.data("wizard-token") || ""); - var $table = $("#result_list"); - var $excludedField = $("#id_excluded"); - var $count = $("#bc-selected-count"); - var $button = $("#bc-execute-button"); - var totalDevices = parseInt($form.data("total-devices"), 10) || 0; - var excluded = readStoredExclusions(); + function updateSelectAllCheckbox() { + const $checkboxes = $table.find(".device-checkbox"); + $("#select-all-devices").prop( + "checked", + $checkboxes.length > 0 && + $checkboxes.filter(":checked").length === $checkboxes.length, + ); + } - function readStoredExclusions() { - var stored = {}; - try { - var raw = window.sessionStorage.getItem(STORAGE_KEY); - $.each(raw ? JSON.parse(raw) : [], function (index, pk) { - stored[pk] = true; - }); - } catch (e) { - // private browsing modes can make sessionStorage unavailable: - // the selection is then simply not carried across pages - } - return stored; + $table.on("change", ".device-checkbox", function () { + const pk = $(this).val(); + if (this.checked) { + delete excluded[pk]; + } else { + excluded[pk] = true; } - - function storeExclusions(pks) { - try { - window.sessionStorage.setItem(STORAGE_KEY, JSON.stringify(pks)); - } catch (e) { - // see readStoredExclusions() + updateSelectionSummary(); + }); + + // only the devices listed on the current page are toggled, the ones the + // user cannot see are never selected or unselected implicitly + $table.on("change", "#select-all-devices", function () { + const checked = this.checked; + $table.find(".device-checkbox").each(function () { + const $checkbox = $(this); + if ($checkbox.prop("checked") !== checked) { + $checkbox.prop("checked", checked).trigger("change"); } - } + }); + }); + + $form.on("submit", function () { + removeStoredExclusions(storageKey); + // guards against a double click creating two mass commands + $button.prop("disabled", true); + }); + + renderSelectAllCheckbox($, $table); + restoreDeviceCheckboxes($, $table, excluded); + updateSelectionSummary(); +} + +function getHiddenInput($, $form, fieldName) { + let $hiddenInput = $form.find('input[name="' + fieldName + '"][type="hidden"]'); + if (!$hiddenInput.length) { + $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); + $form.append($hiddenInput); + } + return $hiddenInput; +} + +function renderCustomCommandField($, $container, fieldName) { + const $row = $('
'); + const $flexContainer = $('
'); + $flexContainer.append( + '", + ); + $flexContainer.append( + '', + ); + $row.append($flexContainer); + $row.append( + '
' + + gettext("Enter the shell command to run on all devices") + + "
", + ); + $container.append($row); +} + +function renderChangePasswordFields($, $container) { + const $passwordRow = $('
'); + const $passwordContainer = $('
'); + $passwordContainer.append( + '", + ); + $passwordContainer.append( + '', + ); + $passwordRow.append($passwordContainer); + $passwordRow.append( + '
' + + gettext("Password must be at least 6 characters long") + + "
", + ); + $container.append($passwordRow); + + const $confirmRow = $('
'); + const $confirmContainer = $('
'); + $confirmContainer.append( + '", + ); + $confirmContainer.append( + '', + ); + $confirmRow.append($confirmContainer); + $container.append($confirmRow); +} + +function renderSelectAllCheckbox($, $table) { + const $header = $table.find("thead th").first(); + if (!$header.length || $header.find("#select-all-devices").length) { + return; + } + $header.append( + $("").attr({ + type: "checkbox", + id: "select-all-devices", + title: gettext("Select all devices on this page"), + }), + ); +} + +// rows are rendered selected by the server, untick the ones excluded earlier +function restoreDeviceCheckboxes($, $table, excluded) { + $table.find(".device-checkbox").each(function () { + const $checkbox = $(this); + $checkbox.prop("checked", !excluded[$checkbox.val()]); + }); +} + +// exclusions are kept in sessionStorage because turning a page of the device +// table is an ordinary page load, which would otherwise forget them +function getStoredExclusions($, storageKey) { + const stored = {}; + try { + const raw = window.sessionStorage.getItem(storageKey); + $.each(raw ? JSON.parse(raw) : [], function (index, pk) { + stored[pk] = true; + }); + } catch (error) { + // private browsing modes can make sessionStorage unavailable + } + return stored; +} + +function setStoredExclusions(storageKey, pks) { + try { + window.sessionStorage.setItem(storageKey, JSON.stringify(pks)); + } catch (error) { + // see getStoredExclusions() + } +} - function clearExclusions() { - try { - window.sessionStorage.removeItem(STORAGE_KEY); - } catch (e) { - // see readStoredExclusions() +function removeStoredExclusions(storageKey) { + try { + window.sessionStorage.removeItem(storageKey); + } catch (error) { + // see getStoredExclusions() + } +} + +function clearAbandonedExclusions() { + try { + const storage = window.sessionStorage; + for (let i = storage.length - 1; i >= 0; i--) { + const key = storage.key(i); + if (key && key.indexOf(EXCLUDED_STORAGE_PREFIX) === 0) { + storage.removeItem(key); } } + } catch (error) { + // see getStoredExclusions() + } +} - // rows are rendered selected by the server, restore the ones which were - // unselected on a previously visited page - function restoreCheckboxes() { - $table.find(".bc-select-device").each(function () { - var $checkbox = $(this); - $checkbox.prop("checked", !excluded[$checkbox.val()]); - }); - } - - function refresh() { - var pks = Object.keys(excluded); - var selected = Math.max(totalDevices - pks.length, 0); - $excludedField.val(pks.join(",")); - storeExclusions(pks); - $count.text(selected); - $button.text( - interpolate( - ngettext("Execute on %s device", "Execute on %s devices", selected), - [selected], - ), - ); - $button.prop("disabled", selected === 0); - refreshSelectAll(); - } - - function refreshSelectAll() { - var $checkboxes = $table.find(".bc-select-device"); - var $checked = $checkboxes.filter(":checked"); - $("#bc-select-all").prop( - "checked", - $checkboxes.length > 0 && $checked.length === $checkboxes.length, - ); - } - - // the changelist has no header checkbox of its own once the admin - // actions are disabled, so add one for the current page - function addSelectAllCheckbox() { - var $header = $table.find("thead th").first(); - if (!$header.length || $header.find("#bc-select-all").length) return; - $header.append( - $("").attr({ - type: "checkbox", - id: "bc-select-all", - title: gettext("Select all devices on this page"), - }), - ); - } - - $table.on("change", ".bc-select-device", function () { - var pk = $(this).val(); - if (this.checked) { - delete excluded[pk]; - } else { - excluded[pk] = true; - } - refresh(); - }); - - // only the devices listed on the current page are affected: devices the - // user cannot see are never selected or unselected implicitly - $table.on("change", "#bc-select-all", function () { - var checked = this.checked; - $table.find(".bc-select-device").each(function () { - var $checkbox = $(this); - if ($checkbox.prop("checked") !== checked) { - $checkbox.prop("checked", checked).trigger("change"); - } - }); - }); - - $form.on("submit", function () { - clearExclusions(); - // guards against a double click creating two mass commands, the - // server discards the second request as well - $button.prop("disabled", true); - }); +function clearFieldErrors($) { + $(".form-row.errors").removeClass("errors"); + $(".form-row .errorlist").remove(); +} - addSelectAllCheckbox(); - restoreCheckboxes(); - refresh(); - } -}); +function showFieldError($row, message) { + $row.addClass("errors"); + $row.prepend('
  • ' + message + "
"); +} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html index d3bb2c54a..32c8f9441 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -138,7 +138,7 @@

{{ command.output|default:"-" }}
{{ command.modified|date|default:"-" }}{{ command.modified|date:"DATETIME_FORMAT"|default:"-" }}
").append( - $("") - .attr({ - href: getDeviceChangeUrl($, data.device), - class: "device-link", - }) - .text(data.device_name), - ), - ); + if (data.is_skipped) { + $row.append( + $("").append( + $("").addClass("device-name-disabled").text(data.device_name), + ), + ); + } else { + $row.append( + $("").append( + $("") + .attr({ + href: getDeviceChangeUrl($, data.device), + class: "device-link", + }) + .text(data.device_name), + ), + ); + } $row.append( $("").append( $("") @@ -226,7 +266,11 @@ function renderPagination($, totalRows) { $("") .addClass("current-page") .text( - gettext("Page") + " " + currentPage + " " + gettext("of") + " " + totalPages, + interpolate( + gettext("Page %(current)s of %(total)s"), + { current: currentPage, total: totalPages }, + true, + ), ), ); if (currentPage < totalPages) { diff --git a/openwisp_controller/connection/static/connection/js/execute-command.js b/openwisp_controller/connection/static/connection/js/execute-command.js index 6ac818c27..70e1f5e19 100644 --- a/openwisp_controller/connection/static/connection/js/execute-command.js +++ b/openwisp_controller/connection/static/connection/js/execute-command.js @@ -40,9 +40,18 @@ function initExecuteCommandForm($) { function handleTypeChange() { const selected = $typeSelect.val(); + let data = null; + try { + data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; + } catch (error) { + data = null; + } $container.empty(); if (selected === COMMAND_TYPE_CUSTOM) { - renderCustomCommandField($, $container, fieldName); + renderCustomCommandField($, $container); + if (data && data.command) { + $container.find("#id_command").val(data.command); + } updateCustomCommandInput(); } else if (selected === COMMAND_TYPE_CHANGE_PASSWORD) { renderChangePasswordFields($, $container); @@ -85,18 +94,9 @@ function initExecuteCommandForm($) { return; } handleTypeChange(); - let data = null; - try { - data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; - } catch (error) { - data = null; - } - if (data && data.command) { - $container.find("#id_command").val(data.command); - } }); - $("#review-command-btn").on("click", function () { + $form.on("submit", function (event) { clearFieldErrors($); const type = $typeSelect.val(); let hasError = false; @@ -124,8 +124,33 @@ function initExecuteCommandForm($) { hasError = true; } } - if (!hasError) { - $form.submit(); + if (type === COMMAND_TYPE_CHANGE_PASSWORD) { + const $password = $container.find("#id_password"); + const $confirmPassword = $container.find("#id_confirm_password"); + const password = $password.val() || ""; + const confirmPassword = $confirmPassword.val() || ""; + if (!password || !confirmPassword) { + showFieldError( + (password ? $confirmPassword : $password).closest(".form-row"), + gettext("This field is required."), + ); + hasError = true; + } else if (password.length < 6 || !$.trim(password)) { + showFieldError( + $password.closest(".form-row"), + gettext("Your password must be at least 6 characters long"), + ); + hasError = true; + } else if (password !== confirmPassword) { + showFieldError( + $confirmPassword.closest(".form-row"), + gettext("The two password fields didn't match."), + ); + hasError = true; + } + } + if (hasError) { + event.preventDefault(); } }); } @@ -192,7 +217,6 @@ function initDeviceSelection($) { }); $form.on("submit", function () { - removeStoredExclusions(storageKey); // guards against a double click creating two mass commands $button.prop("disabled", true); }); @@ -211,15 +235,13 @@ function getHiddenInput($, $form, fieldName) { return $hiddenInput; } -function renderCustomCommandField($, $container, fieldName) { +function renderCustomCommandField($, $container) { const $row = $('
'); const $flexContainer = $('
'); $flexContainer.append( '", ); - $flexContainer.append( - '', - ); + $flexContainer.append(''); $row.append($flexContainer); $row.append( '
' + @@ -305,14 +327,6 @@ function setStoredExclusions(storageKey, pks) { } } -function removeStoredExclusions(storageKey) { - try { - window.sessionStorage.removeItem(storageKey); - } catch (error) { - // see getStoredExclusions() - } -} - function clearAbandonedExclusions() { try { const storage = window.sessionStorage; diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html index 32c8f9441..a07f2fba4 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/batch_command_change_form.html @@ -19,10 +19,8 @@ {% block content %} {{ block.super }} -

{% trans "Commands" %}

- {% if filter_specs %}
@@ -84,7 +82,6 @@

{% endif %} -
@@ -102,7 +99,6 @@

-
{% for command in commands %} -
{% if command.is_skipped %} {{ command.device_name }} {% else %} - {{ command.device_name }} @@ -148,7 +144,6 @@

- {% if paginator %}

{% blocktrans count counter=paginator.count %} @@ -158,7 +153,6 @@

{% endif %} - {% if page_obj.has_other_pages %} diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/form_row.html b/openwisp_controller/connection/templates/admin/connection/batch_command/form_row.html new file mode 100644 index 000000000..bec22c341 --- /dev/null +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/form_row.html @@ -0,0 +1,12 @@ +
+ {{ field.errors }} +
+ {{ field.label_tag }} + {{ field }} +
+ {% if field.help_text %} +
+
{{ field.help_text }}
+
+ {% endif %} +
diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index ef46278f2..01351230e 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -423,7 +423,7 @@ def test_create_command_without_connection(self): ) self.assertEqual(response.status_code, 400) self.assertIn( - "Device has no credentials assigned.", + "Device has no credentials assigned", response.data["device"][0], ) @@ -1065,7 +1065,7 @@ def test_batch_command_endpoints_no_of_queries(self): "devices": [str(d.pk) for d in devices], } url = reverse("connection_api:batch_command_execute") - with self.assertNumQueries(16): + with self.assertNumQueries(15): response = self.client.post( url, data=json.dumps(payload), @@ -1102,7 +1102,7 @@ def test_batch_command_endpoints_no_of_queries(self): "group": str(group.pk), } url = reverse("connection_api:batch_command_execute") - with self.assertNumQueries(15): + with self.assertNumQueries(14): response = self.client.post( url, data=json.dumps(payload), @@ -1128,7 +1128,7 @@ def test_batch_command_endpoints_no_of_queries(self): "label": "test-label", } url = reverse("connection_api:batch_command_execute") - with self.assertNumQueries(13): + with self.assertNumQueries(12): response = self.client.post( url, data=json.dumps(payload), @@ -2258,7 +2258,7 @@ def test_batch_command_execute_skipped_devices(self): self.assertIn(str(device_b.pk), batch.skipped_devices) self.assertIn( '"custom" command is not available for this organization', - batch.skipped_devices[str(device_b.pk)][0], + batch.skipped_devices[str(device_b.pk)]["error"], ) command_qs = Command.objects.filter(batch_command=batch) self.assertTrue(command_qs.filter(device=device_a).exists()) @@ -2318,7 +2318,7 @@ def test_batch_command_execute_skipped_devices(self): self.assertIn(str(device.pk), batch.skipped_devices) self.assertIn( "Device has no credentials assigned", - batch.skipped_devices[str(device.pk)][0], + batch.skipped_devices[str(device.pk)]["error"], ) detail_url = reverse( "connection_api:batch_command_detail", @@ -2360,7 +2360,7 @@ def test_batch_command_execute_skipped_devices(self): self.assertIn(str(device.pk), batch.skipped_devices) self.assertIn( "Device is deactivated", - batch.skipped_devices[str(device.pk)][0], + batch.skipped_devices[str(device.pk)]["error"], ) detail_url = reverse( "connection_api:batch_command_detail", diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index fc45ff2d6..963f625b9 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -614,7 +614,7 @@ def test_command_validation(self): self.assertIn("device", exception.message_dict) self.assertEqual( exception.message_dict["device"], - ["Device has no credentials assigned."], + ["Device has no credentials assigned"], ) def test_command_validation_deactivated_device(self): @@ -646,7 +646,7 @@ def test_command_validation_deactivated_device(self): command.clean() self.assertIn("device", ctx.exception.message_dict) self.assertEqual( - ctx.exception.message_dict["device"], ["Device is deactivated."] + ctx.exception.message_dict["device"], ["Device is deactivated"] ) @tag("skip_prod") @@ -1141,7 +1141,7 @@ def test_batch_command_create_commands_deactivated_device(self): self.assertIn(str(device.pk), batch.skipped_devices) self.assertIn( "Device is deactivated", - batch.skipped_devices[str(device.pk)][0], + batch.skipped_devices[str(device.pk)]["error"], ) def test_batch_command_create_commands_no_credentials(self): @@ -1157,7 +1157,7 @@ def test_batch_command_create_commands_no_credentials(self): self.assertIn(str(device.pk), batch.skipped_devices) self.assertIn( "Device has no credentials assigned", - batch.skipped_devices[str(device.pk)][0], + batch.skipped_devices[str(device.pk)]["error"], ) def test_batch_command_create_commands_skip_scenarios(self): @@ -1192,7 +1192,7 @@ def test_batch_command_create_commands_skip_scenarios(self): self.assertIn(str(device_b.pk), batch.skipped_devices) self.assertIn( '"custom" command is not available for this organization', - batch.skipped_devices[str(device_b.pk)][0], + batch.skipped_devices[str(device_b.pk)]["error"], ) db_batch = BatchCommand.objects.get(pk=batch.pk) self.assertEqual(batch.skipped_devices, db_batch.skipped_devices) @@ -1238,12 +1238,12 @@ def test_batch_command_create_commands_skip_scenarios(self): self.assertIn(str(device_no_creds.pk), batch.skipped_devices) self.assertIn( "Device has no credentials assigned", - batch.skipped_devices[str(device_no_creds.pk)][0], + batch.skipped_devices[str(device_no_creds.pk)]["error"], ) self.assertIn(str(device_deactivated.pk), batch.skipped_devices) self.assertIn( "Device is deactivated", - batch.skipped_devices[str(device_deactivated.pk)][0], + batch.skipped_devices[str(device_deactivated.pk)]["error"], ) self.assertNotIn(str(device_ok.pk), batch.skipped_devices) db_batch = BatchCommand.objects.get(pk=batch.pk) @@ -1720,7 +1720,9 @@ def test_batch_command_calculate_and_update_status(self): with self.subTest("all success with skipped shows failed"): batch3 = self._create_batch_command(organization=org) - batch3.skipped_devices = {str(device.pk): ["no credentials"]} + batch3.skipped_devices = { + str(device.pk): {"name": device.name, "error": "no credentials"} + } batch3.save(update_fields=["skipped_devices"]) Command.objects.create( batch_command=batch3, diff --git a/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py b/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py index 675ddac9e..19896a465 100644 --- a/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py +++ b/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py @@ -100,8 +100,9 @@ class Migration(migrations.Migration): default=dict, verbose_name="skipped devices", help_text=( - "Maps device UUIDs to validation error messages for " - "devices that were skipped during command creation." + "Maps device UUIDs to the name of the device and the " + "validation error that caused it to be skipped during " + "command creation." ), ), ), From ff7a90316a1ad0a87858b274d671a3b00e837221 Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 18 Aug 2026 19:55:20 +0530 Subject: [PATCH 10/13] [fix] Made skipped devices show first 2 and the last device erros only --- openwisp_controller/connection/admin.py | 4 +--- .../connection/static/connection/css/batch-command.css | 3 --- .../connection/static/connection/js/batch-command.js | 5 ----- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 2862b8844..31537082e 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -755,10 +755,8 @@ def display_skipped_devices(self, obj): if len(rows) < len(obj.skipped_devices): lines.insert(-1, "\u2026") return format_html( - '
{}' - '

{}

', + '
{}
', format_html_join(mark_safe("
"), "{}", ((line,) for line in lines)), - _("Refer to the table below to see what happened to each device."), ) display_skipped_devices.short_description = _("skipped devices") diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index 369101406..1661ee6bb 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -117,9 +117,6 @@ .skipped-devices-list { line-height: 1.7; } -.skipped-devices-note { - margin: 1em 0 0; -} .field-display_skipped_devices .readonly.readonly { padding: 0; } diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js index b5ae5046b..66668e3ff 100644 --- a/openwisp_controller/connection/static/connection/js/batch-command.js +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -103,11 +103,6 @@ function updateSkippedDevices($, data) { .append($("
")) .append(document.createTextNode(row.device_name + ": " + row.output)); }); - $list.append( - $("

") - .addClass("skipped-devices-note") - .text(gettext("Refer to the table below to see what happened to each device.")), - ); } function handleBatchStateMessage($, data) { From b8538986e44bb37a5fea3827992456fd17809d88 Mon Sep 17 00:00:00 2001 From: dee077 Date: Thu, 20 Aug 2026 07:40:16 +0530 Subject: [PATCH 11/13] [fix] Addressed review comments - give the confirm page device admin its own readonly_fields copy - reject an execution whose wizard token or device set no longer matches - check the view permission in the batch websocket consumer instead of add - defer batch websocket broadcasts to transaction commit and log failures - take the affected devices count from the creating loop instead of a query - cap the command output preview to the last 100 characters - build the batch filters from skipped devices too and page them lazily - preserve server totals on the change page whenever a filter is active - link live rows to the device recent commands section - scope the group and location choices to the selected organization - use the command schema widget for the mass command input, so any registered command type can be configured, reviewed and executed - keep its generated fields and validation errors consistent with the rest of the admin form --- openwisp_controller/connection/admin.py | 193 +++++++++------ openwisp_controller/connection/apps.py | 47 ++-- openwisp_controller/connection/base/models.py | 13 +- .../connection/channels/consumers.py | 7 +- .../static/connection/css/batch-command.css | 56 +++++ .../static/connection/js/batch-command.js | 17 +- .../static/connection/js/execute-command.js | 233 ++++++------------ .../batch_command/confirm_command.html | 1 + .../batch_command/execute_command.html | 6 +- openwisp_controller/connection/widgets.py | 28 +++ 10 files changed, 333 insertions(+), 268 deletions(-) diff --git a/openwisp_controller/connection/admin.py b/openwisp_controller/connection/admin.py index 31537082e..c321f92e1 100644 --- a/openwisp_controller/connection/admin.py +++ b/openwisp_controller/connection/admin.py @@ -1,3 +1,4 @@ +import hashlib import logging from datetime import timedelta from types import SimpleNamespace @@ -9,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 +from django.db.models import Count, Q from django.http import HttpResponseForbidden, JsonResponse from django.shortcuts import redirect from django.template.response import TemplateResponse @@ -26,7 +27,12 @@ from ..config.admin import DeactivatedDeviceReadOnlyMixin, DeviceAdmin from .filters import GroupFilter, LocationFilter, TypeFilter from .schema import schema -from .widgets import CommandSchemaWidget, CredentialsSchemaWidget +from .widgets import ( + BatchCommandSchemaWidget, + CommandSchemaWidget, + CredentialsSchemaWidget, + OrganizationScopedSelect, +) logger = logging.getLogger(__name__) @@ -36,7 +42,6 @@ BatchCommand = swapper.load_model("connection", "BatchCommand") Device = swapper.load_model("config", "Device") DeviceGroup = swapper.load_model("config", "DeviceGroup") -DeviceLocation = swapper.load_model("geo", "DeviceLocation") Location = swapper.load_model("geo", "Location") Organization = swapper.load_model("openwisp_users", "Organization") @@ -68,8 +73,11 @@ class Meta: "location", ] widgets = { + "label": forms.TextInput(attrs={"class": "vTextField"}), "notes": forms.Textarea(attrs={"rows": 3}), - "input": forms.HiddenInput(), + "input": BatchCommandSchemaWidget, + "group": OrganizationScopedSelect, + "location": OrganizationScopedSelect, } class Media: @@ -155,7 +163,8 @@ def _pk(value): # it a wizard would inherit the devices unselected by a previous # one. It has to be issued here rather than by the browser, # because the session is shared between tabs and sessionStorage - # is not. Not a security token: it only scopes a storage key. + # is not. It is also submitted back on execution, which runs + # only the wizard the confirm page was rendered with. "token": uuid4().hex, "type": self.cleaned_data["type"], "label": self.cleaned_data["label"], @@ -465,6 +474,11 @@ def get_urls(self): self.admin_site.admin_view(self.confirm_command_view), name=f"{options.app_label}_{options.model_name}_confirm", ), + path( + "ui/schema.json", + self.admin_site.admin_view(self.schema_view), + name=f"{options.app_label}_{options.model_name}_schema", + ), ] + super().get_urls() def _check_add_permission(self, request): @@ -472,6 +486,23 @@ def _check_add_permission(self, request): if not request.user.has_perm(permission): raise PermissionDenied + def schema_view(self, request): + """Returns the schemas of the command types the wizard offers. + + The type choices are the union over the organizations the user + manages, so the schemas are too: the editor fetches them once on + page load, before an organization has been picked. + """ + self._check_add_permission(request) + if request.user.is_superuser: + return JsonResponse(Command.get_org_schema()) + schemas = {} + for organization_id in request.user.organizations_managed: + schemas.update( + Command.get_org_schema(organization_id=organization_id) or {} + ) + return JsonResponse(schemas) + def execute_command_view(self, request): """First step of the mass command workflow: collect the details. A valid submission goes to the session and redirects to the confirm @@ -512,6 +543,8 @@ def confirm_command_view(self, request): if not wizard: return self._restart(request) devices = self._resolve_target_queryset(request, wizard) + wizard["devices_digest"] = self._devices_digest(devices) + request.session[self.session_key] = wizard device_admin = self.get_device_admin(devices) # changelist_view() assembles the whole changelist context (cl, # media, pagination) and renders the change_list_template of the @@ -535,7 +568,7 @@ def get_device_admin(self, devices): device_admin_class = type( "BatchCommandDeviceAdmin", (BatchCommandDeviceAdminMixin, registered), - {}, + {"readonly_fields": list(registered.readonly_fields)}, ) return device_admin_class(Device, self.admin_site, devices=devices) @@ -588,6 +621,13 @@ def _resolve_target_queryset(self, request, wizard): ) return devices.distinct().order_by("name") + def _devices_digest(self, devices): + """Identifies the set of devices a confirm page was rendered with, + so that only the reviewed set is executed. + """ + pks = sorted(str(pk) for pk in devices.values_list("pk", flat=True)) + return hashlib.sha256(",".join(pks).encode()).hexdigest() + def _confirm_context(self, request, wizard, devices): targets = [] for model, key in ( @@ -608,21 +648,47 @@ def _confirm_context(self, request, wizard, devices): "device_changelist_template": self.get_device_changelist_template(), "wizard": wizard, "command_type_display": command_types.get(wizard["type"], wizard["type"]), - "command_description": (wizard.get("input") or {}).get("command", ""), + "command_description": self._describe_input(wizard.get("input")), "targets_display": ", ".join(targets) if targets else _("All devices"), "device_count": devices.count(), "has_view_permission": self.has_view_permission(request), } + def _describe_input(self, command_input): + """Renders the submitted input for the review step, so that every + registered command type is shown and not only custom ones. + """ + if not isinstance(command_input, dict): + return "" + if "command" in command_input: + return command_input["command"] + return ", ".join( + f"{key}: {value}" + for key, value in command_input.items() + if "password" not in key + ) + def _execute_batch_command(self, request): """Applies the device selection and dispatches the mass command. - The wizard is popped first, so a double submit cannot create the - batch twice: the second request finds nothing and restarts. + Only the wizard the confirm page was rendered with is executed, and + it is removed before the batch is created, so a double submit finds + nothing and restarts. """ - wizard = request.session.pop(self.session_key, None) - if not wizard: + wizard = request.session.get(self.session_key) + if not wizard or request.POST.get("token") != wizard.get("token"): return self._restart(request) + del request.session[self.session_key] devices = self._resolve_target_queryset(request, wizard) + if self._devices_digest(devices) != wizard.get("devices_digest"): + request.session[self.session_key] = wizard + self.message_user( + request, + _("The targeted devices changed, please review them again."), + messages.WARNING, + ) + return redirect( + f"admin:{self.opts.app_label}_{self.opts.model_name}_confirm" + ) # The confirm page only lists the devices matched on the execute # page, so the selection can only ever remove from that set: the # browser never supplies a device to add. @@ -736,14 +802,6 @@ def affected_devices(self, obj): affected_devices.short_description = _("affected devices") affected_devices.admin_order_field = "_affected_devices" - def _get_skipped_devices(self, obj): - if not hasattr(obj, "_skipped_devices_cache"): - obj._skipped_devices_cache = { - str(device.pk): device - for device in Device.objects.filter(pk__in=obj.skipped_devices.keys()) - } - return obj._skipped_devices_cache - def display_skipped_devices(self, obj): if not obj.skipped_devices: return "-" @@ -797,13 +855,16 @@ 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_devices.keys()) + ) + # Location filter location_spec = self._build_related_filter( _("location"), "location_id", current_location or "", - Device.objects.filter(command__batch_command=obj) - .exclude(devicelocation__location__isnull=True) + batch_devices.exclude(devicelocation__location__isnull=True) .values_list( "devicelocation__location__id", "devicelocation__location__name", @@ -819,10 +880,7 @@ def _make_choice(current_value, display, param_name, value): _("device group"), "group_id", current_group or "", - Device.objects.filter( - command__batch_command=obj, - group__isnull=False, - ) + batch_devices.filter(group__isnull=False) .values_list("group__id", "group__name") .distinct(), _make_choice, @@ -836,9 +894,9 @@ def _make_choice(current_value, display, param_name, value): _("organization"), "organization_id", current_org or "", - Device.objects.filter(command__batch_command=obj) - .values_list("organization__id", "organization__name") - .distinct(), + batch_devices.values_list( + "organization__id", "organization__name" + ).distinct(), _make_choice, ) if org_spec: @@ -869,7 +927,7 @@ def _command_row(command): "is_skipped": False, } - def _paginate_commands(self, commands_qs, skipped_rows, page_param, per_page=None): + def _paginate_commands(self, commands_qs, skipped_items, page_param, per_page=None): """Returns one page of rows without loading the whole batch in memory. Commands keep the ordering of ``AbstractCommand.Meta`` ("created"), so the newest is always last and the change page can append results @@ -878,7 +936,7 @@ def _paginate_commands(self, commands_qs, skipped_rows, page_param, per_page=Non """ per_page = per_page or self.device_commands_per_page commands_count = commands_qs.count() - total = commands_count + len(skipped_rows) + total = commands_count + len(skipped_items) paginator = Paginator(range(total), per_page) try: page_obj = paginator.page(page_param or 1) @@ -892,7 +950,10 @@ def _paginate_commands(self, commands_qs, skipped_rows, page_param, per_page=Non ] skipped_start = max(0, start - commands_count) skipped_end = max(0, end - commands_count) - rows += skipped_rows[skipped_start:skipped_end] + rows += [ + BatchCommand.build_skipped_row(pk, skipped) + for pk, skipped in skipped_items[skipped_start:skipped_end] + ] return page_obj, paginator, rows def _get_active_filters(self, request): @@ -921,47 +982,29 @@ def _apply_command_filters(self, qs, filters): return qs def _get_matching_skipped_devices(self, obj, filters): - pks = list(obj.skipped_devices.keys()) - location_id = filters["location_id"] - if location_id: - device_locations = { - str(pk) - for pk in DeviceLocation.objects.filter( - content_object_id__in=pks, - location_id=location_id, - ).values_list("content_object_id", flat=True) - } - else: - device_locations = None - devices = self._get_skipped_devices(obj) - rows = [] - for pk_str, skipped in obj.skipped_devices.items(): - device = devices.get(pk_str) - if not device: - if not any( - ( - filters["organization_id"], - filters["group_id"], - location_id, - ) - ) and ( - not filters["q"] or filters["q"].lower() in skipped["name"].lower() - ): - rows.append(BatchCommand.build_skipped_row(pk_str, skipped)) - continue - if ( - filters["organization_id"] - and str(device.organization_id) != filters["organization_id"] - ): - continue - if filters["group_id"] and str(device.group_id) != filters["group_id"]: - continue - if device_locations is not None and pk_str not in device_locations: - continue - if filters["q"] and filters["q"].lower() not in skipped["name"].lower(): - continue - rows.append(BatchCommand.build_skipped_row(pk_str, skipped)) - return rows + related = ( + filters["organization_id"], + filters["group_id"], + filters["location_id"], + ) + matching = None + if any(related): + organization_id, group_id, location_id = related + devices = Device.objects.filter(pk__in=obj.skipped_devices.keys()) + if organization_id: + devices = devices.filter(organization_id=organization_id) + if group_id: + devices = devices.filter(group_id=group_id) + if location_id: + devices = devices.filter(devicelocation__location_id=location_id) + matching = {str(pk) for pk in devices.values_list("pk", flat=True)} + query = filters["q"].lower() if filters["q"] else "" + return [ + (pk, skipped) + for pk, skipped in obj.skipped_devices.items() + if (matching is None or pk in matching) + and (not query or query in skipped["name"].lower()) + ] def change_view(self, request, object_id, form_url="", extra_context=None): extra_context = extra_context or {} @@ -970,11 +1013,11 @@ def change_view(self, request, object_id, form_url="", extra_context=None): commands_qs = self._get_commands(request, obj) filters = self._get_active_filters(request) commands_qs = self._apply_command_filters(commands_qs, filters) - skipped_rows = [] + skipped_items = [] if obj.skipped_devices and filters["status"] in ("", "skipped"): - skipped_rows = self._get_matching_skipped_devices(obj, filters) + skipped_items = self._get_matching_skipped_devices(obj, filters) page_obj, paginator, commands = self._paginate_commands( - commands_qs, skipped_rows, request.GET.get("page", 1) + commands_qs, skipped_items, request.GET.get("page", 1) ) filter_specs = self._build_filter_specs( request, diff --git a/openwisp_controller/connection/apps.py b/openwisp_controller/connection/apps.py index 37dbcfc7c..7c2485527 100644 --- a/openwisp_controller/connection/apps.py +++ b/openwisp_controller/connection/apps.py @@ -1,3 +1,5 @@ +import logging + from asgiref.sync import async_to_sync from channels import layers from django.apps import AppConfig @@ -15,6 +17,8 @@ from ..config.signals import config_deactivating, config_modified from .signals import is_working_changed +logger = logging.getLogger(__name__) + class ConnectionConfig(AppConfig): name = "openwisp_controller.connection" @@ -74,16 +78,27 @@ def ready(self): def config_modified_receiver(cls, **kwargs): transaction.on_commit(lambda: cls._launch_update_config(kwargs["device"])) + @classmethod + def _send_batch_update(cls, group, data): + def send(): + try: + async_to_sync(layers.get_channel_layer().group_send)( + group, {"type": "send.update", "data": data} + ) + except Exception: + logger.exception("Failed to send update to %s", group) + + transaction.on_commit(send) + @classmethod def command_save_receiver(cls, sender, created, instance, **kwargs): from .api.serializers import CommandSerializer if created and not instance.batch_command_id: return - channel_layer = layers.get_channel_layer() serialized_data = CommandSerializer(instance).data if not created: - async_to_sync(channel_layer.group_send)( + async_to_sync(layers.get_channel_layer().group_send)( f"config.device-{instance.device_id}", {"type": "send.update", "model": "Command", "data": serialized_data}, ) @@ -97,24 +112,25 @@ def command_save_receiver(cls, sender, created, instance, **kwargs): localtime(instance.modified), "DATETIME_FORMAT" ) batch_data["type"] = "command_update" - batch = instance.batch_command - affected_devices = batch.affected_devices - batch_data["affected_devices"] = affected_devices - batch_data["total_rows"] = affected_devices + len( - batch.skipped_devices or {} - ) if created: - batch_data["index"] = affected_devices - 1 - async_to_sync(channel_layer.group_send)( - f"config.batchcommand-{instance.batch_command_id}", - {"type": "send.update", "data": batch_data}, + batch = instance.batch_command + index = getattr(instance, "_batch_index", None) + if index is None: + index = batch.affected_devices - 1 + affected_devices = index + 1 + batch_data["index"] = index + batch_data["affected_devices"] = affected_devices + batch_data["total_rows"] = affected_devices + len( + batch.skipped_devices or {} + ) + cls._send_batch_update( + f"config.batchcommand-{instance.batch_command_id}", batch_data ) @classmethod def batch_command_save_receiver(cls, sender, instance, **kwargs): from .api.serializers import BatchCommandSerializer - channel_layer = layers.get_channel_layer() batch_data = BatchCommandSerializer(instance).data batch_data["status_display"] = instance.get_status_display() batch_data["type"] = "batch_status" @@ -124,10 +140,7 @@ def batch_command_save_receiver(cls, sender, instance, **kwargs): batch_data["total_rows"] = affected_devices + skipped_count batch_data["skipped_count"] = skipped_count batch_data["skipped_preview"] = instance.get_skipped_preview() - async_to_sync(channel_layer.group_send)( - f"config.batchcommand-{instance.pk}", - {"type": "send.update", "data": batch_data}, - ) + cls._send_batch_update(f"config.batchcommand-{instance.pk}", batch_data) @classmethod def _launch_update_config(cls, device): diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 65916dbac..67d195936 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -556,9 +556,13 @@ def output_preview(self): lines = (self.output or "").strip().splitlines() if not lines: return "" - if len(lines) == 1: - return lines[0] - return "… " + lines[-1] + max_length = 100 + line = lines[-1] + truncated = len(lines) > 1 + if len(line) > max_length: + line = line[-max_length:] + truncated = True + return f"… {line}" if truncated else line @property def is_custom(self): @@ -1007,6 +1011,7 @@ def create_commands(self): Device = load_model("config", "Device") self.skipped_devices = {} device_pks = [] + created_count = 0 for device in self.resolve_devices().iterator(): device_pks.append(device.pk) command = Command( @@ -1017,7 +1022,9 @@ def create_commands(self): ) try: command.full_clean() + command._batch_index = created_count command.save() + created_count += 1 except ValidationError as e: self.skipped_devices[str(device.pk)] = { "name": device.name, diff --git a/openwisp_controller/connection/channels/consumers.py b/openwisp_controller/connection/channels/consumers.py index 9b2ac78f6..8fc6b34d4 100644 --- a/openwisp_controller/connection/channels/consumers.py +++ b/openwisp_controller/connection/channels/consumers.py @@ -35,9 +35,10 @@ def is_user_authorized(self): user = self.scope["user"] if user.is_superuser: return True - # a mass command cannot be changed or deleted from the admin - if not ( - user.is_staff and self._user_has_permissions(change=False, delete=False) + opts = self.model._meta + if not user.is_staff or not any( + user.has_perm(f"{opts.app_label}.{action}_{opts.model_name}") + for action in ("view", "change") ): return False organization_id = ( diff --git a/openwisp_controller/connection/static/connection/css/batch-command.css b/openwisp_controller/connection/static/connection/css/batch-command.css index 1661ee6bb..bf5307b85 100644 --- a/openwisp_controller/connection/static/connection/css/batch-command.css +++ b/openwisp_controller/connection/static/connection/css/batch-command.css @@ -250,3 +250,59 @@ gap: 0.5rem; justify-content: flex-end; } +.execute-batch-command .jsoneditor-wrapper > fieldset.module { + background: none; + border: none; + margin: 0; + padding: 0; +} +.execute-batch-command .jsoneditor-wrapper > fieldset > h2, +.execute-batch-command .jsoneditor h3 { + display: none !important; +} +.execute-batch-command.no-command-type .jsoneditor-wrapper { + display: none; +} +.execute-batch-command #main .jsoneditor-wrapper div.jsoneditor .form-row { + display: flex; + flex-wrap: wrap; + padding: 15px; +} +.execute-batch-command .jsoneditor-wrapper div.jsoneditor label { + font-weight: bold; + margin-left: 0; +} +.execute-batch-command .jsoneditor .form-row > .help { + flex-basis: 100%; +} +.execute-batch-command .jsoneditor .errorlist { + display: none; +} +.execute-batch-command .jsoneditor.command-errors .errorlist { + display: block; +} +.execute-batch-command .jsoneditor .form-row.errors input, +.execute-batch-command .jsoneditor .form-row.errors select, +.execute-batch-command .jsoneditor .form-row.errors textarea { + border-color: var(--border-color); +} +.execute-batch-command .jsoneditor.command-errors .form-row.errors input, +.execute-batch-command .jsoneditor.command-errors .form-row.errors select, +.execute-batch-command .jsoneditor.command-errors .form-row.errors textarea { + border-color: var(--ow-color-danger); +} +.execute-batch-command .jsoneditor .form-row > ul.errorlist { + order: -1; + flex-basis: calc(100% - 160px); + margin-left: 160px; + padding-left: 10px; +} +.execute-batch-command .form-row.field-input { + display: none; +} +.execute-batch-command .form-row.field-input.errors { + display: block !important; +} +.execute-batch-command .form-row.field-input.errors .flex-container { + display: none; +} diff --git a/openwisp_controller/connection/static/connection/js/batch-command.js b/openwisp_controller/connection/static/connection/js/batch-command.js index 66668e3ff..e63da7d31 100644 --- a/openwisp_controller/connection/static/connection/js/batch-command.js +++ b/openwisp_controller/connection/static/connection/js/batch-command.js @@ -121,7 +121,7 @@ function handleBatchStateMessage($, data) { const $row = $("#batch-command-row-" + command.device); if ($row.length) { updateRow($, $row, command); - } else { + } else if (!hasActiveFilters()) { insertRow($, command); } }); @@ -143,7 +143,7 @@ function renderCommand($, data) { function belongsOnCurrentPage($, data) { // with a filter on, the pushed totals are unfiltered and page boundaries // cannot be worked out - if (getActiveStatusFilter($)) { + if (hasActiveFilters()) { return false; } if (data.index == null) { @@ -223,7 +223,7 @@ function updateTotals($, affectedDevices, totalRows) { } } // counts are filtered server side, the totals pushed here are not - if (totalRows == null || getActiveStatusFilter($)) { + if (totalRows == null || hasActiveFilters()) { return; } const $paginator = $(".results-container .paginator"); @@ -283,13 +283,22 @@ function getDeviceChangeUrl($, devicePk) { if (!template) { return "#"; } - return template.replace(DEVICE_URL_PLACEHOLDER, devicePk); + return template.replace(DEVICE_URL_PLACEHOLDER, devicePk) + "#command_set-2-group"; } function getActiveStatusFilter($) { return $("#result_list").attr("data-active-status") || ""; } +function hasActiveFilters() { + const params = new URLSearchParams(window.location.search); + return ["q", "status", "location_id", "group_id", "organization_id"].some( + function (name) { + return !!params.get(name); + }, + ); +} + function getCurrentPage($) { return parseInt($("#result_list").attr("data-current-page"), 10) || 1; } diff --git a/openwisp_controller/connection/static/connection/js/execute-command.js b/openwisp_controller/connection/static/connection/js/execute-command.js index 70e1f5e19..b52577c2c 100644 --- a/openwisp_controller/connection/static/connection/js/execute-command.js +++ b/openwisp_controller/connection/static/connection/js/execute-command.js @@ -1,9 +1,8 @@ "use strict"; -const COMMAND_TYPE_CUSTOM = "custom"; -const COMMAND_TYPE_CHANGE_PASSWORD = "change_password"; const EXCLUDED_STORAGE_PREFIX = "ow-batch-command-excluded:"; const WIZARD_SELECTS = "#id_type, #id_organization, #id_group, #id_location"; +const COMMAND_EDITOR_ID = "id_input_jsoneditor"; django.jQuery(function ($) { initExecuteCommandForm($); @@ -16,64 +15,9 @@ function initExecuteCommandForm($) { return; } const $form = $typeSelect.closest("form"); - const $container = $("#command-input-container"); - const fieldName = $("#id_input").length ? $("#id_input").attr("name") : "input"; - const $hiddenInput = getHiddenInput($, $form, fieldName); - function updateCustomCommandInput() { - const command = $.trim($container.find("#id_command").val()); - $hiddenInput.val(command ? JSON.stringify({ command: command }) : ""); - } - - function updateChangePasswordInput() { - const password = $container.find("#id_password").val(); - const confirmPassword = $container.find("#id_confirm_password").val(); - $hiddenInput.val( - password && confirmPassword - ? JSON.stringify({ - password: password, - confirm_password: confirmPassword, - }) - : "", - ); - } - - function handleTypeChange() { - const selected = $typeSelect.val(); - let data = null; - try { - data = $hiddenInput.val() ? JSON.parse($hiddenInput.val()) : null; - } catch (error) { - data = null; - } - $container.empty(); - if (selected === COMMAND_TYPE_CUSTOM) { - renderCustomCommandField($, $container); - if (data && data.command) { - $container.find("#id_command").val(data.command); - } - updateCustomCommandInput(); - } else if (selected === COMMAND_TYPE_CHANGE_PASSWORD) { - renderChangePasswordFields($, $container); - updateChangePasswordInput(); - } else { - $hiddenInput.val(""); - } - } - - // a new mass command starts here, so the devices unselected in an earlier - // one which was configured but never executed are dropped clearAbandonedExclusions(); - $container.on("input", "#id_command", updateCustomCommandInput); - $container.on( - "input", - "#id_password, #id_confirm_password", - updateChangePasswordInput, - ); - $typeSelect.on("change", handleTypeChange); - handleTypeChange(); - $(WIZARD_SELECTS).select2({ theme: "default", placeholder: gettext("Select an option"), @@ -81,8 +25,11 @@ function initExecuteCommandForm($) { width: "resolve", }); - // admin pages are served with Cache-Control: no-store, so going back - // restores the form values after select2 has rendered its labels + initOrganizationScope($); + initCommandInput($, $typeSelect); + + // admin pages are served no-store, so a back navigation restores the field + // values after select2 has already rendered its labels $(window).on("pageshow", function () { $(WIZARD_SELECTS).each(function () { const $field = $(this); @@ -90,10 +37,6 @@ function initExecuteCommandForm($) { $field.trigger("change.select2"); } }); - if (!$typeSelect.val()) { - return; - } - handleTypeChange(); }); $form.on("submit", function (event) { @@ -114,54 +57,81 @@ function initExecuteCommandForm($) { ); hasError = true; } - if (type === COMMAND_TYPE_CUSTOM) { - const command = $container.find("#id_command").val(); - if (!$.trim(command || "")) { - showFieldError( - $container.find(".form-row").first(), - gettext("This field is required."), - ); - hasError = true; - } - } - if (type === COMMAND_TYPE_CHANGE_PASSWORD) { - const $password = $container.find("#id_password"); - const $confirmPassword = $container.find("#id_confirm_password"); - const password = $password.val() || ""; - const confirmPassword = $confirmPassword.val() || ""; - if (!password || !confirmPassword) { - showFieldError( - (password ? $confirmPassword : $password).closest(".form-row"), - gettext("This field is required."), - ); - hasError = true; - } else if (password.length < 6 || !$.trim(password)) { - showFieldError( - $password.closest(".form-row"), - gettext("Your password must be at least 6 characters long"), - ); - hasError = true; - } else if (password !== confirmPassword) { - showFieldError( - $confirmPassword.closest(".form-row"), - gettext("The two password fields didn't match."), - ); - hasError = true; - } + if (showCommandErrors($)) { + hasError = true; } if (hasError) { event.preventDefault(); + event.stopImmediatePropagation(); } }); } +// mirrors checkInputIsValid() in commands.js: the editor renders its errors as +// soon as it is built, so they are kept hidden until the form is submitted +function showCommandErrors($) { + const editor = (django._jsonEditors || {})[COMMAND_EDITOR_ID]; + if (!editor) { + return false; + } + const errors = editor.validate(); + // a field is redisplayed only when it was edited or when "show_errors" + // changed since the last call, "always" skips both checks + editor.options.show_errors = "always"; + editor.root.showValidationErrors(errors); + $("#" + COMMAND_EDITOR_ID).addClass("command-errors"); + return errors.length > 0; +} + +// with no type the editor is handed the whole schema map and renders it as a +// generic root object +function initCommandInput($, $typeSelect) { + function toggle() { + $(document.body).toggleClass("no-command-type", !$typeSelect.val()); + // the container is reused, its errors belong to the previous type + $("#" + COMMAND_EDITOR_ID).removeClass("command-errors"); + } + + $typeSelect.on("change", toggle); + toggle(); +} + +function initOrganizationScope($) { + const $organization = $("#id_organization"); + const fields = ["#id_group", "#id_location"]; + fields.forEach(function (selector) { + $(selector).data("allOptions", $(selector).find("option").clone()); + }); + + function applyScope() { + const organizationId = $organization.val() || ""; + fields.forEach(function (selector) { + const $field = $(selector); + const current = $field.val(); + const $options = $field.data("allOptions").filter(function () { + const value = $(this).attr("value"); + return ( + !value || + !organizationId || + $(this).attr("data-organization-id") === organizationId + ); + }); + $field.empty().append($options.clone()); + $field.val($options.filter('[value="' + current + '"]').length ? current : ""); + $field.trigger("change.select2"); + }); + } + + $organization.on("change", applyScope); + applyScope(); +} + function initDeviceSelection($) { const $form = $("#execute-form"); if (!$form.length) { return; } - // sessionStorage lives as long as the browser tab, so the key carries the - // token of this mass command to stop a new one inheriting its exclusions + // sessionStorage outlives the wizard, so the key is namespaced by its token const storageKey = EXCLUDED_STORAGE_PREFIX + ($form.data("wizard-token") || ""); const $table = $("#result_list"); const $excludedField = $("#id_excluded"); @@ -204,8 +174,7 @@ function initDeviceSelection($) { updateSelectionSummary(); }); - // only the devices listed on the current page are toggled, the ones the - // user cannot see are never selected or unselected implicitly + // devices the user cannot see are never toggled implicitly $table.on("change", "#select-all-devices", function () { const checked = this.checked; $table.find(".device-checkbox").each(function () { @@ -226,62 +195,6 @@ function initDeviceSelection($) { updateSelectionSummary(); } -function getHiddenInput($, $form, fieldName) { - let $hiddenInput = $form.find('input[name="' + fieldName + '"][type="hidden"]'); - if (!$hiddenInput.length) { - $hiddenInput = $("").attr({ type: "hidden", name: fieldName }); - $form.append($hiddenInput); - } - return $hiddenInput; -} - -function renderCustomCommandField($, $container) { - const $row = $('

'); - const $flexContainer = $('
'); - $flexContainer.append( - '", - ); - $flexContainer.append(''); - $row.append($flexContainer); - $row.append( - '
' + - gettext("Enter the shell command to run on all devices") + - "
", - ); - $container.append($row); -} - -function renderChangePasswordFields($, $container) { - const $passwordRow = $('
'); - const $passwordContainer = $('
'); - $passwordContainer.append( - '", - ); - $passwordContainer.append( - '', - ); - $passwordRow.append($passwordContainer); - $passwordRow.append( - '
' + - gettext("Password must be at least 6 characters long") + - "
", - ); - $container.append($passwordRow); - - const $confirmRow = $('
'); - const $confirmContainer = $('
'); - $confirmContainer.append( - '", - ); - $confirmContainer.append( - '', - ); - $confirmRow.append($confirmContainer); - $container.append($confirmRow); -} - function renderSelectAllCheckbox($, $table) { const $header = $table.find("thead th").first(); if (!$header.length || $header.find("#select-all-devices").length) { @@ -296,7 +209,6 @@ function renderSelectAllCheckbox($, $table) { ); } -// rows are rendered selected by the server, untick the ones excluded earlier function restoreDeviceCheckboxes($, $table, excluded) { $table.find(".device-checkbox").each(function () { const $checkbox = $(this); @@ -304,8 +216,7 @@ function restoreDeviceCheckboxes($, $table, excluded) { }); } -// exclusions are kept in sessionStorage because turning a page of the device -// table is an ordinary page load, which would otherwise forget them +// paging the device table is an ordinary page load, which would forget them function getStoredExclusions($, storageKey) { const stored = {}; try { @@ -343,7 +254,7 @@ function clearAbandonedExclusions() { function clearFieldErrors($) { $(".form-row.errors").removeClass("errors"); - $(".form-row .errorlist").remove(); + $(".form-row .errorlist").not(".jsoneditor .errorlist").remove(); } function showFieldError($row, message) { diff --git a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html index 040d826f6..7401ec413 100644 --- a/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html +++ b/openwisp_controller/connection/templates/admin/connection/batch_command/confirm_command.html @@ -110,6 +110,7 @@

{% trans 'Affected devices' %}

data-wizard-token="{{ wizard.token }}"> {% csrf_token %} +
{% trans 'Back' %}