From 872fd14adae7279dc0c18569940a8c635d2e85a5 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 4 Jun 2026 19:52:58 -0300 Subject: [PATCH 01/20] [ci] Fixed ci YAML inconsistencies (for coveralls, etc.) --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 062131170..1aae2d006 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,13 +5,13 @@ on: push: branches: - master + - "gsoc26-*" pull_request: branches: - master - "1.1" - "1.2" - - "gsoc26-x509-certificate-generator-templates" - - "gsoc26-mass-commands" + - "gsoc26-*" jobs: build: @@ -100,7 +100,7 @@ jobs: with: parallel: true format: cobertura - flag-name: python-${{ matrix.env.env }} + flag-name: python-${{ matrix.python-version }}-${{ matrix.django-version }} github-token: ${{ secrets.GITHUB_TOKEN }} fail-on-error: false From d04280ede7cd4c752bd8a4b4df820cb53860fb31 Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 9 Jun 2026 18:28:35 +0530 Subject: [PATCH 02/20] [feature] Added BatchCommand model for mass command execution #1344 Introduced AbstractBatchCommand model with calculate_and_update_status() and launch() methods to support batch command execution on multiple devices, following the pattern of BatchUpgradeOperation in openwisp-firmware-upgrader. Added batch_command FK to the existing Command model to link individual commands to their parent batch. Closes #1344 --- openwisp_controller/connection/base/models.py | 174 ++++++++++++++++++ openwisp_controller/connection/models.py | 13 +- openwisp_controller/connection/tasks.py | 10 + 3 files changed, 196 insertions(+), 1 deletion(-) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 17a06f7bb..04f86ad41 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -467,6 +467,12 @@ class AbstractCommand(TimeStampedEditableModel): encoder=DjangoJSONEncoder, ) output = models.TextField(blank=True) + batch_command = models.ForeignKey( + get_model_name("connection", "BatchCommand"), + on_delete=models.SET_NULL, + blank=True, + null=True, + ) class Meta: verbose_name = _("Command") @@ -719,3 +725,171 @@ def _enforce_not_custom(self): f"arguments property is not applicable in " f'command instance of type "{self.type}"' ) + + +class AbstractBatchCommand(TimeStampedEditableModel): + STATUS_CHOICES = ( + ("idle", _("idle")), + ("in-progress", _("in progress")), + ("success", _("completed successfully")), + ("failed", _("completed with some failures")), + ("cancelled", _("completed with some cancellations")), + ) + organization = models.ForeignKey( + get_model_name("openwisp_users", "Organization"), + on_delete=models.CASCADE, + ) + status = models.CharField( + max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0] + ) + command_type = models.CharField( + max_length=16, + choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices), + ) + command_input = JSONField(blank=True, null=True, encoder=DjangoJSONEncoder) + group = models.ForeignKey( + get_model_name("config", "DeviceGroup"), + on_delete=models.SET_NULL, + blank=True, + null=True, + verbose_name=_("device group"), + ) + location = models.ForeignKey( + get_model_name("geo", "Location"), + on_delete=models.SET_NULL, + blank=True, + null=True, + verbose_name=_("location"), + ) + include_all_devices = models.BooleanField(default=False) + total_devices = models.PositiveIntegerField(default=0) + successful = models.PositiveIntegerField(default=0) + failed = models.PositiveIntegerField(default=0) + cancelled = models.PositiveIntegerField(default=0) + + class Meta: + abstract = True + verbose_name = _("Batch command operation") + verbose_name_plural = _("Batch command operations") + + def clean(self): + super().clean() + if self.group and self.group.organization != self.organization: + raise ValidationError( + { + "group": _( + "The organization of the group doesn't match " + "the organization of the batch command operation" + ) + } + ) + if self.location and self.location.organization != self.organization: + raise ValidationError( + { + "location": _( + "The organization of the location doesn't match " + "the organization of the batch command operation" + ) + } + ) + allowed = dict( + AbstractCommand.get_org_allowed_commands( + organization_id=self.organization_id + ) + ) + if self.command_type not in allowed: + raise ValidationError( + { + "command_type": _( + '"{command}" command is not available ' "for this organization" + ).format(command=self.command_type) + } + ) + try: + jsonschema.Draft4Validator(get_command_schema(self.command_type)).validate( + self.command_input + ) + except SchemaError as e: + raise ValidationError({"command_input": e.message}) + + def resolve_devices(self): + Device = load_model("config", "Device") + qs = Device.objects.filter(organization=self.organization) + if self.group: + qs = qs.filter(group=self.group) + if self.location: + qs = qs.filter(location=self.location) + if not self.include_all_devices and not self.group and not self.location: + qs = qs.none() + return qs + + def launch(self): + self.status = "in-progress" + self.save() + devices = self.resolve_devices() + Command = load_model("connection", "Command") + count = 0 + for device in devices.iterator(): + cmd = Command( + device=device, + type=self.command_type, + input=self.command_input, + batch_command=self, + ) + cmd.full_clean() + cmd.save() + count += 1 + self.total_devices = count + self.save(update_fields=["total_devices"]) + self.calculate_and_update_status() + + def launch_async(self): + self.status = "in-progress" + self.save(update_fields=["status"]) + from ..tasks import launch_batch_command + + transaction.on_commit(lambda: launch_batch_command.delay(self.pk)) + + def calculate_and_update_status(self): + Command = load_model("connection", "Command") + operations = Command.objects.filter(batch_command=self) + stats = operations.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(), + ) + ), + ) + self.successful = stats["successful"] + self.failed = stats["failed"] + if stats["total_operations"] == 0: + self.status = "idle" + elif stats["in_progress"] > 0: + self.status = "in-progress" + elif stats["failed"] > 0: + self.status = "failed" + elif ( + stats["successful"] > 0 and stats["completed"] == stats["total_operations"] + ): + self.status = "success" + self.save(update_fields=["status", "successful", "failed"]) diff --git a/openwisp_controller/connection/models.py b/openwisp_controller/connection/models.py index 39d485b57..533ad6c4d 100644 --- a/openwisp_controller/connection/models.py +++ b/openwisp_controller/connection/models.py @@ -1,6 +1,11 @@ import swapper -from .base.models import AbstractCommand, AbstractCredentials, AbstractDeviceConnection +from .base.models import ( + AbstractBatchCommand, + AbstractCommand, + AbstractCredentials, + AbstractDeviceConnection, +) class Credentials(AbstractCredentials): @@ -19,3 +24,9 @@ class Command(AbstractCommand): class Meta(AbstractCommand.Meta): abstract = False swappable = swapper.swappable_setting("connection", "Command") + + +class BatchCommand(AbstractBatchCommand): + class Meta(AbstractBatchCommand.Meta): + abstract = False + swappable = swapper.swappable_setting("connection", "BatchCommand") diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index d6ea2828b..7d65a7038 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -98,6 +98,16 @@ def launch_command(command_id): command._save_without_resurrecting() +@shared_task(bind=True, soft_time_limit=3600) +def launch_batch_command(self, batch_id): + BatchCommand = load_model("connection", "BatchCommand") + try: + batch = BatchCommand.objects.get(pk=batch_id) + batch.launch() + except ObjectDoesNotExist: + logger.warning(f"The BatchCommand object with id {batch_id} has been deleted") + + @shared_task(soft_time_limit=3600) def auto_add_credentials_to_devices(credential_id, organization_id): Credentials = load_model("connection", "Credentials") From 901be90a80b9d047046675906281be12c0babb36 Mon Sep 17 00:00:00 2001 From: dee077 Date: Sat, 13 Jun 2026 01:42:46 +0530 Subject: [PATCH 03/20] [feature] Proof of concept need some code refinement --- .../connection/api/serializers.py | 56 +++++++ openwisp_controller/connection/api/urls.py | 5 + openwisp_controller/connection/api/views.py | 34 +++++ openwisp_controller/connection/base/models.py | 70 ++++++--- ...0011_batchcommand_command_batch_command.py | 142 ++++++++++++++++++ openwisp_controller/connection/tasks.py | 10 +- 6 files changed, 293 insertions(+), 24 deletions(-) create mode 100644 openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 142c1c30a..32af819b0 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -12,6 +12,7 @@ DeviceConnection = load_model("connection", "DeviceConnection") Credentials = load_model("connection", "Credentials") Device = load_model("config", "Device") +BatchCommand = load_model("connection", "BatchCommand") class ValidatedDeviceFieldSerializer(ValidatedModelSerializer): @@ -43,6 +44,10 @@ class CommandSerializer(ValidatedDeviceFieldSerializer): required=False, pk_field=serializers.UUIDField(format="hex_verbose"), ) + batch_command = serializers.PrimaryKeyRelatedField( + read_only=True, + pk_field=serializers.UUIDField(format="hex_verbose"), + ) def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) @@ -115,3 +120,54 @@ class Meta: "is_working": {"read_only": True}, } read_only_fields = ("created", "modified") + + +class BatchCommandExecuteSerializer( + FilterSerializerByOrgManaged, serializers.ModelSerializer +): + type = serializers.CharField(source="command_type") + input = serializers.JSONField( + source="command_input", allow_null=True, required=False + ) + devices = serializers.PrimaryKeyRelatedField( + many=True, + queryset=Device.objects.all(), + required=False, + allow_empty=True, + pk_field=serializers.UUIDField(format="hex_verbose"), + ) + + class Meta: + model = BatchCommand + fields = ( + "organization", + "type", + "input", + "devices", + "group", + "location", + ) + extra_kwargs = { + "organization": {"required": False, "allow_null": True}, + } + + def validate(self, data): + if ( + not data.get("organization") + and not self.context["request"].user.is_superuser + ): + raise serializers.ValidationError( + _("Only superusers can execute batch commands without an organization.") + ) + if devices := data.get("devices"): + org = data.get("organization") + for device in devices: + if org and device.organization_id != org.id: + raise serializers.ValidationError( + { + "devices": _( + "All devices must belong to the same organization." + ) + } + ) + return data diff --git a/openwisp_controller/connection/api/urls.py b/openwisp_controller/connection/api/urls.py index 4ec3e70ab..76a30c64a 100644 --- a/openwisp_controller/connection/api/urls.py +++ b/openwisp_controller/connection/api/urls.py @@ -40,6 +40,11 @@ def get_api_urls(api_views): api_views.deviceconnection_detail_view, name="deviceconnection_detail", ), + path( + "api/v1/controller/batch-command/execute/", + api_views.batch_command_execute_view, + name="batch_command_execute", + ), ] diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index 6af1270c7..ca88355d0 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -1,12 +1,15 @@ from django.utils.translation import gettext_lazy as _ from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema +from rest_framework import status from rest_framework.generics import ( + GenericAPIView, ListCreateAPIView, RetrieveAPIView, RetrieveUpdateDestroyAPIView, get_object_or_404, ) +from rest_framework.response import Response from swapper import load_model from openwisp_utils.api.pagination import OpenWispPagination @@ -17,6 +20,7 @@ RelatedDeviceProtectedAPIMixin, ) from .serializers import ( + BatchCommandExecuteSerializer, CommandSerializer, CredentialSerializer, DeviceConnectionSerializer, @@ -26,6 +30,7 @@ Device = load_model("config", "Device") Credentials = load_model("connection", "Credentials") DeviceConnection = load_model("connection", "DeviceConnection") +BatchCommand = load_model("connection", "BatchCommand") class BaseCommandView(RelatedDeviceProtectedAPIMixin): @@ -138,6 +143,33 @@ class DeviceConnectionListCreateView(BaseDeviceConnection, ListCreateAPIView): DeviceConnenctionListCreateView = DeviceConnectionListCreateView +class BatchCommandExecuteView(ProtectedAPIMixin, GenericAPIView): + model = BatchCommand + queryset = BatchCommand.objects.all() + serializer_class = BatchCommandExecuteSerializer + + def post(self, request): + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + batch = serializer.save() + batch.launch_async() + return Response({"batch": str(batch.pk)}, status=status.HTTP_201_CREATED) + + def get(self, request): + serializer = self.get_serializer(data=request.query_params) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + device_pks = [] + devices_list = data.pop("devices", None) + if devices_list: + device_pks = [str(d.pk) for d in devices_list] + batch = BatchCommand(**data) + if not device_pks: + resolved = batch.resolve_devices() + device_pks = [str(d.pk) for d in resolved] + return Response({"devices": device_pks}) + + class DeviceConnectionDetailView(BaseDeviceConnection, RetrieveUpdateDestroyAPIView): def get_object(self): queryset = self.filter_queryset(self.get_queryset()) @@ -158,3 +190,5 @@ def get_object(self): # TODO: remove in version 1.4 deviceconnection_details_view = deviceconnection_detail_view + +batch_command_execute_view = BatchCommandExecuteView.as_view() diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 04f86ad41..a6c48c8c8 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -563,6 +563,8 @@ def save(self, *args, **kwargs): output = super().save(*args, **kwargs) if adding: self._schedule_command() + if self.batch_command_id and self.status != "in-progress": + self.batch_command.calculate_and_update_status() return output def _save_without_resurrecting(self): @@ -738,6 +740,8 @@ class AbstractBatchCommand(TimeStampedEditableModel): organization = models.ForeignKey( get_model_name("openwisp_users", "Organization"), on_delete=models.CASCADE, + blank=True, + null=True, ) status = models.CharField( max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0] @@ -761,7 +765,11 @@ class AbstractBatchCommand(TimeStampedEditableModel): null=True, verbose_name=_("location"), ) - include_all_devices = models.BooleanField(default=False) + devices = models.ManyToManyField( + get_model_name("config", "Device"), + blank=True, + verbose_name=_("devices"), + ) total_devices = models.PositiveIntegerField(default=0) successful = models.PositiveIntegerField(default=0) failed = models.PositiveIntegerField(default=0) @@ -769,29 +777,43 @@ class AbstractBatchCommand(TimeStampedEditableModel): class Meta: abstract = True - verbose_name = _("Batch command operation") - verbose_name_plural = _("Batch command operations") + verbose_name = _("Batch command") + verbose_name_plural = _("Batch commands") def clean(self): super().clean() - if self.group and self.group.organization != self.organization: - raise ValidationError( - { - "group": _( - "The organization of the group doesn't match " - "the organization of the batch command operation" - ) - } - ) - if self.location and self.location.organization != self.organization: - raise ValidationError( - { - "location": _( - "The organization of the location doesn't match " - "the organization of the batch command operation" + if self.organization_id: + if self.group and self.group.organization != self.organization: + raise ValidationError( + { + "group": _( + "The organization of the group doesn't match " + "the organization of the batch command operation" + ) + } + ) + if self.location and self.location.organization != self.organization: + raise ValidationError( + { + "location": _( + "The organization of the location doesn't match " + "the organization of the batch command operation" + ) + } + ) + if self.pk and self.devices.exists(): + org_mismatch = self.devices.exclude( + organization=self.organization + ).exists() + if org_mismatch: + raise ValidationError( + { + "devices": _( + "All devices must belong to the same " + "organization as the batch command." + ) + } ) - } - ) allowed = dict( AbstractCommand.get_org_allowed_commands( organization_id=self.organization_id @@ -813,14 +835,16 @@ def clean(self): raise ValidationError({"command_input": e.message}) def resolve_devices(self): + if self.pk and self.devices.exists(): + return self.devices.all() Device = load_model("config", "Device") - qs = Device.objects.filter(organization=self.organization) + qs = Device.objects.all() + if self.organization_id: + qs = qs.filter(organization=self.organization) if self.group: qs = qs.filter(group=self.group) if self.location: qs = qs.filter(location=self.location) - if not self.include_all_devices and not self.group and not self.location: - qs = qs.none() return qs def launch(self): diff --git a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py new file mode 100644 index 000000000..a0dad8b92 --- /dev/null +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -0,0 +1,142 @@ +# Generated by Django 5.2.15 on 2026-06-09 22:36 + +import uuid + +import django.core.serializers.json +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields +from django.conf import settings +from django.db import migrations, models + +import openwisp_controller.connection.commands + + +class Migration(migrations.Migration): + + dependencies = [ + ("connection", "0010_replace_jsonfield_with_django_builtin"), + ("openwisp_users", "0022_user_expiration_date"), + migrations.swappable_dependency(settings.CONFIG_DEVICEGROUP_MODEL), + migrations.swappable_dependency(settings.CONFIG_DEVICE_MODEL), + migrations.swappable_dependency(settings.GEO_LOCATION_MODEL), + ] + + operations = [ + migrations.CreateModel( + name="BatchCommand", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ( + "created", + model_utils.fields.AutoCreatedField( + default=django.utils.timezone.now, + editable=False, + verbose_name="created", + ), + ), + ( + "modified", + model_utils.fields.AutoLastModifiedField( + default=django.utils.timezone.now, + editable=False, + verbose_name="modified", + ), + ), + ( + "status", + models.CharField( + choices=[ + ("idle", "idle"), + ("in-progress", "in progress"), + ("success", "completed successfully"), + ("failed", "completed with some failures"), + ("cancelled", "completed with some cancellations"), + ], + default="idle", + max_length=12, + ), + ), + ( + "command_type", + models.CharField( + choices=openwisp_controller.connection.commands.get_command_choices, + max_length=16, + ), + ), + ( + "command_input", + models.JSONField( + blank=True, + encoder=django.core.serializers.json.DjangoJSONEncoder, + null=True, + ), + ), + ("total_devices", models.PositiveIntegerField(default=0)), + ("successful", models.PositiveIntegerField(default=0)), + ("failed", models.PositiveIntegerField(default=0)), + ("cancelled", models.PositiveIntegerField(default=0)), + ( + "devices", + models.ManyToManyField( + blank=True, + to=settings.CONFIG_DEVICE_MODEL, + verbose_name="devices", + ), + ), + ( + "group", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.CONFIG_DEVICEGROUP_MODEL, + verbose_name="device group", + ), + ), + ( + "location", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.GEO_LOCATION_MODEL, + verbose_name="location", + ), + ), + ( + "organization", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="openwisp_users.organization", + ), + ), + ], + options={ + "verbose_name": "Batch command operation", + "verbose_name_plural": "Batch command operations", + "abstract": False, + "swappable": "CONNECTION_BATCHCOMMAND_MODEL", + }, + ), + migrations.AddField( + model_name="command", + name="batch_command", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.CONNECTION_BATCHCOMMAND_MODEL, + ), + ), + ] diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 7d65a7038..b3b169c40 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -81,6 +81,14 @@ def launch_command(command_id): return try: command.execute() + # Todo: Remove once demo is completed + if command.batch_command_id: + print("****************************") + print(f"Device: {command.device.name}") + print(f"Status: {command.status}") + print("") + print(command.output) + print("****************************") except SoftTimeLimitExceeded: command.status = "failed" command._add_output(_("Background task time limit exceeded.")) @@ -105,7 +113,7 @@ def launch_batch_command(self, batch_id): batch = BatchCommand.objects.get(pk=batch_id) batch.launch() except ObjectDoesNotExist: - logger.warning(f"The BatchCommand object with id {batch_id} has been deleted") + logger.warning(f"The BatchCommand object with id {batch_id} not foound") @shared_task(soft_time_limit=3600) From 7b8b7a4fa9a25fe9e9e467b064806e64c7931f23 Mon Sep 17 00:00:00 2001 From: dee077 Date: Mon, 15 Jun 2026 00:06:51 +0530 Subject: [PATCH 04/20] [feature] Refactored BatchCommand model, API, and task for mass command execution - Removed counter DB fields - Added computed properties via aggregation - Added execute_all boolean field - Renamed launch() to create_commands() - Added execute() and dry_run() classmethods - Updated calculate_and_update_status() - Made organization FK nullable - Updated views with execute/dry_run - Updated serializer with execute_all and type/input aliases - Updated migration and celery task --- .../connection/api/serializers.py | 22 +++-- openwisp_controller/connection/api/views.py | 29 +++--- openwisp_controller/connection/base/models.py | 93 +++++++++++++------ ...0011_batchcommand_command_batch_command.py | 18 ++-- openwisp_controller/connection/tasks.py | 2 +- 5 files changed, 102 insertions(+), 62 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 32af819b0..77f9a5968 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -136,6 +136,7 @@ class BatchCommandExecuteSerializer( allow_empty=True, pk_field=serializers.UUIDField(format="hex_verbose"), ) + execute_all = serializers.BooleanField(required=False, default=False) class Meta: model = BatchCommand @@ -146,21 +147,30 @@ class Meta: "devices", "group", "location", + "execute_all", ) extra_kwargs = { "organization": {"required": False, "allow_null": True}, } def validate(self, data): - if ( - not data.get("organization") - and not self.context["request"].user.is_superuser - ): + org = data.get("organization") + execute_all = data.get("execute_all", False) + devices = data.get("devices") + group = data.get("group") + location = data.get("location") + if not org and not self.context["request"].user.is_superuser: raise serializers.ValidationError( _("Only superusers can execute batch commands without an organization.") ) - if devices := data.get("devices"): - org = data.get("organization") + if not execute_all and not org and not devices and not group and not location: + raise serializers.ValidationError( + _( + "Specify at least one targeting option " + "or set execute_all to true." + ) + ) + if devices: for device in devices: if org and device.organization_id != org.id: raise serializers.ValidationError( diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index ca88355d0..8628eeb4d 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -1,3 +1,4 @@ +from django.core.exceptions import ValidationError from django.utils.translation import gettext_lazy as _ from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema @@ -151,23 +152,25 @@ class BatchCommandExecuteView(ProtectedAPIMixin, GenericAPIView): def post(self, request): serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) - batch = serializer.save() - batch.launch_async() - return Response({"batch": str(batch.pk)}, status=status.HTTP_201_CREATED) + try: + batch = BatchCommand.execute(**serializer.validated_data) + except ValidationError as e: + return Response( + {"error": str(e.messages[0])}, status=status.HTTP_400_BAD_REQUEST + ) + return Response({"batch": str(batch.pk)}, status=201) def get(self, request): serializer = self.get_serializer(data=request.query_params) serializer.is_valid(raise_exception=True) - data = serializer.validated_data - device_pks = [] - devices_list = data.pop("devices", None) - if devices_list: - device_pks = [str(d.pk) for d in devices_list] - batch = BatchCommand(**data) - if not device_pks: - resolved = batch.resolve_devices() - device_pks = [str(d.pk) for d in resolved] - return Response({"devices": device_pks}) + try: + data = BatchCommand.dry_run(**serializer.validated_data) + except ValidationError as e: + return Response( + {"error": str(e.messages[0])}, status=status.HTTP_400_BAD_REQUEST + ) + data["devices"] = [str(d.pk) for d in data["devices"]] + return Response(data) class DeviceConnectionDetailView(BaseDeviceConnection, RetrieveUpdateDestroyAPIView): diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index a6c48c8c8..2ce23f676 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -30,7 +30,11 @@ ) from ..exceptions import NoWorkingDeviceConnectionError from ..signals import is_working_changed -from ..tasks import auto_add_credentials_to_devices, launch_command +from ..tasks import ( + auto_add_credentials_to_devices, + launch_batch_command, + launch_command, +) logger = logging.getLogger(__name__) @@ -735,8 +739,8 @@ class AbstractBatchCommand(TimeStampedEditableModel): ("in-progress", _("in progress")), ("success", _("completed successfully")), ("failed", _("completed with some failures")), - ("cancelled", _("completed with some cancellations")), ) + organization = models.ForeignKey( get_model_name("openwisp_users", "Organization"), on_delete=models.CASCADE, @@ -770,16 +774,28 @@ class AbstractBatchCommand(TimeStampedEditableModel): blank=True, verbose_name=_("devices"), ) - total_devices = models.PositiveIntegerField(default=0) - successful = models.PositiveIntegerField(default=0) - failed = models.PositiveIntegerField(default=0) - cancelled = models.PositiveIntegerField(default=0) + execute_all = models.BooleanField(default=False) class Meta: abstract = True verbose_name = _("Batch command") verbose_name_plural = _("Batch commands") + @cached_property + def total_devices(self): + Command = load_model("connection", "Command") + return Command.objects.filter(batch_command=self).count() + + @property + def successful(self): + Command = load_model("connection", "Command") + return Command.objects.filter(batch_command=self, status="success").count() + + @property + def failed(self): + Command = load_model("connection", "Command") + return Command.objects.filter(batch_command=self, status="failed").count() + def clean(self): super().clean() if self.organization_id: @@ -841,39 +857,54 @@ def resolve_devices(self): qs = Device.objects.all() if self.organization_id: qs = qs.filter(organization=self.organization) + if self.execute_all: + return qs if self.group: qs = qs.filter(group=self.group) if self.location: qs = qs.filter(location=self.location) return qs - def launch(self): + @classmethod + def execute(cls, **kwargs): + devices_list = kwargs.pop("devices", None) + batch = cls(**kwargs) + batch.full_clean() + batch.save() + if devices_list: + batch.devices.set(devices_list) + batch.status = "in-progress" + batch.save(update_fields=["status"]) + transaction.on_commit(lambda: launch_batch_command.delay(batch.pk)) + return batch + + @classmethod + def dry_run(cls, **kwargs): + devices_list = kwargs.pop("devices", None) + batch = cls(**kwargs) + batch.full_clean() + if devices_list: + return {"devices": list(devices_list)} + return {"devices": list(batch.resolve_devices())} + + def create_commands(self): self.status = "in-progress" self.save() - devices = self.resolve_devices() Command = load_model("connection", "Command") - count = 0 - for device in devices.iterator(): - cmd = Command( + for device in self.resolve_devices().iterator(): + command = Command( device=device, type=self.command_type, input=self.command_input, batch_command=self, ) - cmd.full_clean() - cmd.save() - count += 1 - self.total_devices = count - self.save(update_fields=["total_devices"]) + try: + command.full_clean() + command.save() + except ValidationError as e: + logger.warning(f"Skipping device {device.pk} for batch {self.pk}: {e}") self.calculate_and_update_status() - def launch_async(self): - self.status = "in-progress" - self.save(update_fields=["status"]) - from ..tasks import launch_batch_command - - transaction.on_commit(lambda: launch_batch_command.delay(self.pk)) - def calculate_and_update_status(self): Command = load_model("connection", "Command") operations = Command.objects.filter(batch_command=self) @@ -904,16 +935,18 @@ def calculate_and_update_status(self): ) ), ) - self.successful = stats["successful"] - self.failed = stats["failed"] if stats["total_operations"] == 0: - self.status = "idle" + new_status = "idle" elif stats["in_progress"] > 0: - self.status = "in-progress" + new_status = "in-progress" elif stats["failed"] > 0: - self.status = "failed" + new_status = "failed" elif ( stats["successful"] > 0 and stats["completed"] == stats["total_operations"] ): - self.status = "success" - self.save(update_fields=["status", "successful", "failed"]) + new_status = "success" + else: + new_status = self.status + if self.status != new_status: + self.status = new_status + self.save(update_fields=["status"]) 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 a0dad8b92..8b6e02d47 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -1,16 +1,14 @@ -# Generated by Django 5.2.15 on 2026-06-09 22:36 - -import uuid +# Generated by Django 5.2.15 on 2026-06-14 18:00 import django.core.serializers.json import django.db.models.deletion import django.utils.timezone import model_utils.fields +import openwisp_controller.connection.commands +import uuid from django.conf import settings from django.db import migrations, models -import openwisp_controller.connection.commands - class Migration(migrations.Migration): @@ -59,7 +57,6 @@ class Migration(migrations.Migration): ("in-progress", "in progress"), ("success", "completed successfully"), ("failed", "completed with some failures"), - ("cancelled", "completed with some cancellations"), ], default="idle", max_length=12, @@ -80,10 +77,7 @@ class Migration(migrations.Migration): null=True, ), ), - ("total_devices", models.PositiveIntegerField(default=0)), - ("successful", models.PositiveIntegerField(default=0)), - ("failed", models.PositiveIntegerField(default=0)), - ("cancelled", models.PositiveIntegerField(default=0)), + ("execute_all", models.BooleanField(default=False)), ( "devices", models.ManyToManyField( @@ -123,8 +117,8 @@ class Migration(migrations.Migration): ), ], options={ - "verbose_name": "Batch command operation", - "verbose_name_plural": "Batch command operations", + "verbose_name": "Batch command", + "verbose_name_plural": "Batch commands", "abstract": False, "swappable": "CONNECTION_BATCHCOMMAND_MODEL", }, diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index b3b169c40..e90ba2edb 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -111,7 +111,7 @@ def launch_batch_command(self, batch_id): BatchCommand = load_model("connection", "BatchCommand") try: batch = BatchCommand.objects.get(pk=batch_id) - batch.launch() + batch.create_commands() except ObjectDoesNotExist: logger.warning(f"The BatchCommand object with id {batch_id} not foound") From 5c8547a2f743085f2eacf410f8637af577f2edf1 Mon Sep 17 00:00:00 2001 From: dee077 Date: Mon, 15 Jun 2026 20:22:14 +0530 Subject: [PATCH 05/20] [feature] Added model-level validation, failed Command records, and idempotency guard - Added full_clean() to dry_run() for model-level validation - Create failed Command records instead of skipping on validation error - Added idempotency guard to create_commands() via Command existence check - Narrowed ObjectDoesNotExist handler in launch_batch_command task - Return full message_dict instead of first message on ValidationError --- openwisp_controller/connection/api/views.py | 6 ++++-- openwisp_controller/connection/base/models.py | 9 +++++++-- .../0011_batchcommand_command_batch_command.py | 6 ++++-- openwisp_controller/connection/tasks.py | 7 ++++--- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index 8628eeb4d..df87f9cce 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -156,7 +156,8 @@ def post(self, request): batch = BatchCommand.execute(**serializer.validated_data) except ValidationError as e: return Response( - {"error": str(e.messages[0])}, status=status.HTTP_400_BAD_REQUEST + getattr(e, "message_dict", e.messages), + status=status.HTTP_400_BAD_REQUEST, ) return Response({"batch": str(batch.pk)}, status=201) @@ -167,7 +168,8 @@ def get(self, request): data = BatchCommand.dry_run(**serializer.validated_data) except ValidationError as e: return Response( - {"error": str(e.messages[0])}, status=status.HTTP_400_BAD_REQUEST + getattr(e, "message_dict", e.messages), + status=status.HTTP_400_BAD_REQUEST, ) data["devices"] = [str(d.pk) for d in data["devices"]] return Response(data) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 2ce23f676..510c04027 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -888,9 +888,11 @@ def dry_run(cls, **kwargs): return {"devices": list(batch.resolve_devices())} def create_commands(self): + Command = load_model("connection", "Command") + if Command.objects.filter(batch_command=self).exists(): + return self.status = "in-progress" self.save() - Command = load_model("connection", "Command") for device in self.resolve_devices().iterator(): command = Command( device=device, @@ -902,7 +904,10 @@ def create_commands(self): command.full_clean() command.save() except ValidationError as e: - logger.warning(f"Skipping device {device.pk} for batch {self.pk}: {e}") + command.status = "failed" + command.output = str(e) + models.Model.save(command) + logger.warning(f"Device {device.pk} failed for batch {self.pk}: {e}") self.calculate_and_update_status() def calculate_and_update_status(self): 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 8b6e02d47..c91b62d35 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -1,14 +1,16 @@ # Generated by Django 5.2.15 on 2026-06-14 18:00 +import uuid + import django.core.serializers.json import django.db.models.deletion import django.utils.timezone import model_utils.fields -import openwisp_controller.connection.commands -import uuid from django.conf import settings from django.db import migrations, models +import openwisp_controller.connection.commands + class Migration(migrations.Migration): diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index e90ba2edb..53f39e1d8 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -111,9 +111,10 @@ def launch_batch_command(self, batch_id): BatchCommand = load_model("connection", "BatchCommand") try: batch = BatchCommand.objects.get(pk=batch_id) - batch.create_commands() - except ObjectDoesNotExist: - logger.warning(f"The BatchCommand object with id {batch_id} not foound") + except BatchCommand.DoesNotExist: + logger.warning(f"The BatchCommand object with id {batch_id} has been deleted") + return + batch.create_commands() @shared_task(soft_time_limit=3600) From cf971df7f74112c4c07d8556b9b0cc38208c227e Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 16 Jun 2026 01:34:53 +0530 Subject: [PATCH 06/20] [feature] Finalized BatchCommand migration and QA fixes - Fixed migration swappable dependency for config/geo apps - Log only field names (not values) in create_commands error handler - Added batch_command field to expected websocket response - Added sample_connection BatchCommand model, migration, view, and settings - Updated geo test query count assertions --- openwisp_controller/connection/base/models.py | 8 +- ...0011_batchcommand_command_batch_command.py | 26 ++-- .../connection/tests/pytest.py | 1 + .../geo/estimated_location/tests/tests.py | 2 +- openwisp_controller/geo/tests/test_api.py | 2 +- .../openwisp2/sample_connection/api/views.py | 8 + ...0005_batchcommand_command_batch_command.py | 138 ++++++++++++++++++ tests/openwisp2/sample_connection/models.py | 6 + tests/openwisp2/settings.py | 1 + 9 files changed, 178 insertions(+), 14 deletions(-) create mode 100644 tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 510c04027..0d095bf87 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -907,7 +907,13 @@ def create_commands(self): command.status = "failed" command.output = str(e) models.Model.save(command) - logger.warning(f"Device {device.pk} failed for batch {self.pk}: {e}") + fields = list(getattr(e, "message_dict", {}).keys()) or ["__all__"] + logger.warning( + "Command validation failed for batch=%s device=%s fields=%s", + self.pk, + device.pk, + ",".join(fields), + ) self.calculate_and_update_status() def calculate_and_update_status(self): 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 c91b62d35..62fe4eef7 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -2,14 +2,15 @@ import uuid +import django import django.core.serializers.json import django.db.models.deletion import django.utils.timezone import model_utils.fields -from django.conf import settings +import swapper from django.db import migrations, models -import openwisp_controller.connection.commands +from openwisp_controller import connection as connection_config class Migration(migrations.Migration): @@ -17,9 +18,8 @@ class Migration(migrations.Migration): dependencies = [ ("connection", "0010_replace_jsonfield_with_django_builtin"), ("openwisp_users", "0022_user_expiration_date"), - migrations.swappable_dependency(settings.CONFIG_DEVICEGROUP_MODEL), - migrations.swappable_dependency(settings.CONFIG_DEVICE_MODEL), - migrations.swappable_dependency(settings.GEO_LOCATION_MODEL), + ("config", "0063_replace_jsonfield_with_django_builtin"), + ("geo", "0006_create_geo_settings_for_existing_orgs"), ] operations = [ @@ -67,8 +67,12 @@ class Migration(migrations.Migration): ( "command_type", models.CharField( - choices=openwisp_controller.connection.commands.get_command_choices, max_length=16, + choices=( + connection_config.commands.COMMAND_CHOICES + if django.VERSION < (5, 0) + else connection_config.commands.get_command_choices + ), ), ), ( @@ -84,7 +88,7 @@ class Migration(migrations.Migration): "devices", models.ManyToManyField( blank=True, - to=settings.CONFIG_DEVICE_MODEL, + to=swapper.get_model_name("config", "Device"), verbose_name="devices", ), ), @@ -94,7 +98,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to=settings.CONFIG_DEVICEGROUP_MODEL, + to=swapper.get_model_name("config", "DeviceGroup"), verbose_name="device group", ), ), @@ -104,7 +108,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to=settings.GEO_LOCATION_MODEL, + to=swapper.get_model_name("geo", "Location"), verbose_name="location", ), ), @@ -114,7 +118,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, - to="openwisp_users.organization", + to=swapper.get_model_name("openwisp_users", "Organization"), ), ), ], @@ -132,7 +136,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to=settings.CONNECTION_BATCHCOMMAND_MODEL, + to=swapper.get_model_name("connection", "BatchCommand"), ), ), ] diff --git a/openwisp_controller/connection/tests/pytest.py b/openwisp_controller/connection/tests/pytest.py index 220604cef..d7f7f5659 100644 --- a/openwisp_controller/connection/tests/pytest.py +++ b/openwisp_controller/connection/tests/pytest.py @@ -65,6 +65,7 @@ def _get_expected_response(self, command): "output": command.output, "device": str(command.device_id), "connection": str(command.connection_id), + "batch_command": None, }, } diff --git a/openwisp_controller/geo/estimated_location/tests/tests.py b/openwisp_controller/geo/estimated_location/tests/tests.py index c037106dd..fee63ae94 100644 --- a/openwisp_controller/geo/estimated_location/tests/tests.py +++ b/openwisp_controller/geo/estimated_location/tests/tests.py @@ -566,7 +566,7 @@ def _verify_location_details(device, mocked_response): device2.save() # 3 queries related to notifications cleanup device2.refresh_from_db() - with self.assertNumQueries(15): + with self.assertNumQueries(16): 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/openwisp_controller/geo/tests/test_api.py b/openwisp_controller/geo/tests/test_api.py index 156550692..df46460cf 100644 --- a/openwisp_controller/geo/tests/test_api.py +++ b/openwisp_controller/geo/tests/test_api.py @@ -694,7 +694,7 @@ def test_change_location_type_to_outdoor_api(self): def test_delete_location_detail(self): l1 = self._create_location() path = reverse("geo_api:detail_location", args=[l1.pk]) - with self.assertNumQueries(5): + with self.assertNumQueries(6): response = self.client.delete(path) self.assertEqual(response.status_code, 204) diff --git a/tests/openwisp2/sample_connection/api/views.py b/tests/openwisp2/sample_connection/api/views.py index fd2207cbc..66510f22a 100644 --- a/tests/openwisp2/sample_connection/api/views.py +++ b/tests/openwisp2/sample_connection/api/views.py @@ -1,3 +1,6 @@ +from openwisp_controller.connection.api.views import ( + BatchCommandExecuteView as BaseBatchCommandExecuteView, +) from openwisp_controller.connection.api.views import ( CommandDetailsView as BaseCommandDetailsView, ) @@ -42,9 +45,14 @@ class DeviceConnectionDetailView(BaseDeviceConnectionDetailView): pass +class BatchCommandExecuteView(BaseBatchCommandExecuteView): + pass + + command_list_create_view = CommandListCreateView.as_view() command_details_view = CommandDetailsView.as_view() credential_list_create_view = CredentialListCreateView.as_view() credential_detail_view = CredentialDetailView.as_view() deviceconnection_list_create_view = DeviceConnectionListCreateView.as_view() deviceconnection_detail_view = DeviceConnectionDetailView.as_view() +batch_command_execute_view = BatchCommandExecuteView.as_view() 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 new file mode 100644 index 000000000..593a9f6b2 --- /dev/null +++ b/tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py @@ -0,0 +1,138 @@ +# Generated by Django 5.2.15 on 2026-06-15 18:15 + +import uuid + +import django +import django.core.serializers.json +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields +from django.db import migrations, models + +from openwisp_controller import connection as connection_config + + +class Migration(migrations.Migration): + + dependencies = [ + ("sample_config", "0009_replace_jsonfield_with_django_builtin"), + ("sample_connection", "0004_replace_jsonfield_with_django_builtin"), + ("sample_geo", "0005_organizationgeosettings"), + ("sample_users", "0005_user_expiration_date_user_user_active_expiry_idx"), + ] + + operations = [ + migrations.CreateModel( + name="BatchCommand", + fields=[ + ( + "id", + models.UUIDField( + default=uuid.uuid4, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ( + "created", + model_utils.fields.AutoCreatedField( + default=django.utils.timezone.now, + editable=False, + verbose_name="created", + ), + ), + ( + "modified", + model_utils.fields.AutoLastModifiedField( + default=django.utils.timezone.now, + editable=False, + verbose_name="modified", + ), + ), + ( + "status", + models.CharField( + choices=[ + ("idle", "idle"), + ("in-progress", "in progress"), + ("success", "completed successfully"), + ("failed", "completed with some failures"), + ], + default="idle", + max_length=12, + ), + ), + ( + "command_type", + models.CharField( + max_length=16, + choices=( + connection_config.commands.COMMAND_CHOICES + if django.VERSION < (5, 0) + else connection_config.commands.get_command_choices + ), + ), + ), + ( + "command_input", + models.JSONField( + blank=True, + encoder=django.core.serializers.json.DjangoJSONEncoder, + null=True, + ), + ), + ("execute_all", models.BooleanField(default=False)), + ( + "devices", + models.ManyToManyField( + blank=True, to="sample_config.device", verbose_name="devices" + ), + ), + ( + "group", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="sample_config.devicegroup", + verbose_name="device group", + ), + ), + ( + "location", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="sample_geo.location", + verbose_name="location", + ), + ), + ( + "organization", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="sample_users.organization", + ), + ), + ], + options={ + "verbose_name": "Batch command", + "verbose_name_plural": "Batch commands", + "abstract": False, + }, + ), + migrations.AddField( + model_name="command", + name="batch_command", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="sample_connection.batchcommand", + ), + ), + ] diff --git a/tests/openwisp2/sample_connection/models.py b/tests/openwisp2/sample_connection/models.py index 7964c5471..4e96065e8 100644 --- a/tests/openwisp2/sample_connection/models.py +++ b/tests/openwisp2/sample_connection/models.py @@ -1,6 +1,7 @@ from django.db import models from openwisp_controller.connection.base.models import ( + AbstractBatchCommand, AbstractCommand, AbstractCredentials, AbstractDeviceConnection, @@ -27,3 +28,8 @@ class Meta(AbstractDeviceConnection.Meta): class Command(AbstractCommand): class Meta(AbstractCommand.Meta): abstract = False + + +class BatchCommand(AbstractBatchCommand): + class Meta(AbstractBatchCommand.Meta): + abstract = False diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index c45eb5537..0e27adfde 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -293,6 +293,7 @@ CONNECTION_CREDENTIALS_MODEL = "sample_connection.Credentials" CONNECTION_DEVICECONNECTION_MODEL = "sample_connection.DeviceConnection" CONNECTION_COMMAND_MODEL = "sample_connection.Command" + CONNECTION_BATCHCOMMAND_MODEL = "sample_connection.BatchCommand" SUBNET_DIVISION_SUBNETDIVISIONRULE_MODEL = ( "sample_subnet_division.SubnetDivisionRule" ) From d65dc5e755d4fc763c34870c276b8a2fb42ec84d Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 16 Jun 2026 20:16:23 +0530 Subject: [PATCH 07/20] [fix] Made GET dry-run work without type param, default execute_all to true - Make type optional on GET requests via serializer __init__ - Skip full_clean() in dry_run when command_type is not provided - Default execute_all to True for both GET and POST --- openwisp_controller/connection/api/serializers.py | 8 +++++++- openwisp_controller/connection/base/models.py | 5 ++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 77f9a5968..c25f27812 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -136,7 +136,13 @@ class BatchCommandExecuteSerializer( allow_empty=True, pk_field=serializers.UUIDField(format="hex_verbose"), ) - execute_all = serializers.BooleanField(required=False, default=False) + execute_all = serializers.BooleanField(required=False, default=True) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + request = self.context.get("request") + if request and request.method == "GET": + self.fields["type"].required = False class Meta: model = BatchCommand diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 0d095bf87..592835070 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -881,8 +881,11 @@ def execute(cls, **kwargs): @classmethod def dry_run(cls, **kwargs): devices_list = kwargs.pop("devices", None) + command_type = kwargs.pop("command_type", None) batch = cls(**kwargs) - batch.full_clean() + if command_type: + batch.command_type = command_type + batch.full_clean() if devices_list: return {"devices": list(devices_list)} return {"devices": list(batch.resolve_devices())} From 2c5602fa051c91019cf9f2a9fb5a999d58b7a769 Mon Sep 17 00:00:00 2001 From: dee077 Date: Fri, 19 Jun 2026 06:32:46 +0530 Subject: [PATCH 08/20] [fix] Add docstrings and minor fixes --- openwisp_controller/connection/base/models.py | 156 +++++++++++------- ...0011_batchcommand_command_batch_command.py | 2 +- openwisp_controller/connection/tasks.py | 10 +- ...0005_batchcommand_command_batch_command.py | 2 +- 4 files changed, 101 insertions(+), 69 deletions(-) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 592835070..0564c3dea 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -476,6 +476,7 @@ class AbstractCommand(TimeStampedEditableModel): on_delete=models.SET_NULL, blank=True, null=True, + related_name="batch_commands", ) class Meta: @@ -774,7 +775,6 @@ class AbstractBatchCommand(TimeStampedEditableModel): blank=True, verbose_name=_("devices"), ) - execute_all = models.BooleanField(default=False) class Meta: abstract = True @@ -783,63 +783,61 @@ class Meta: @cached_property def total_devices(self): - Command = load_model("connection", "Command") - return Command.objects.filter(batch_command=self).count() + return self.batch_commands.count() @property def successful(self): - Command = load_model("connection", "Command") - return Command.objects.filter(batch_command=self, status="success").count() + return self.batch_commands.filter(status="success").count() @property def failed(self): - Command = load_model("connection", "Command") - return Command.objects.filter(batch_command=self, status="failed").count() + return self.batch_commands.filter(status="failed").count() - def clean(self): - super().clean() - if self.organization_id: - if self.group and self.group.organization != self.organization: - raise ValidationError( - { - "group": _( - "The organization of the group doesn't match " - "the organization of the batch command operation" - ) - } - ) - if self.location and self.location.organization != self.organization: + def _validate_org_relations(self): + if not self.organization_id: + return + if self.group and self.group.organization != self.organization: + raise ValidationError( + { + "group": _( + "The organization of the group doesn't match " + "the organization of the batch command operation" + ) + } + ) + if self.location and self.location.organization != self.organization: + raise ValidationError( + { + "location": _( + "The organization of the location doesn't match " + "the organization of the batch command operation" + ) + } + ) + if self.pk and self.devices.exists(): + org_mismatch = self.devices.exclude(organization=self.organization).exists() + if org_mismatch: raise ValidationError( { - "location": _( - "The organization of the location doesn't match " - "the organization of the batch command operation" + "devices": _( + "All devices must belong to the same " + "organization as the batch command." ) } ) - if self.pk and self.devices.exists(): - org_mismatch = self.devices.exclude( - organization=self.organization - ).exists() - if org_mismatch: - raise ValidationError( - { - "devices": _( - "All devices must belong to the same " - "organization as the batch command." - ) - } - ) + + def clean(self): + super().clean() + self._validate_org_relations() + Command = load_model("connection", "Command") allowed = dict( - AbstractCommand.get_org_allowed_commands( - organization_id=self.organization_id - ) + Command.get_org_allowed_commands(organization_id=self.organization_id) ) if self.command_type not in allowed: raise ValidationError( { "command_type": _( - '"{command}" command is not available ' "for this organization" + '"{command}" command is not available for this organization' ).format(command=self.command_type) } ) @@ -851,28 +849,51 @@ def clean(self): raise ValidationError({"command_input": e.message}) def resolve_devices(self): + """ + Returns the queryset of devices targeted by this batch command. + - Devices explicitly set via M2M relation: returns those directly. + - No explicit devices: resolves via organization, group, and location filters. + - No filters set: returns all devices (superuser targeting all orgs). + """ if self.pk and self.devices.exists(): - return self.devices.all() + return self.devices.iterator() Device = load_model("config", "Device") qs = Device.objects.all() if self.organization_id: qs = qs.filter(organization=self.organization) - if self.execute_all: - return qs if self.group: qs = qs.filter(group=self.group) if self.location: - qs = qs.filter(location=self.location) + qs = qs.filter(devicelocation__location=self.location) return qs @classmethod def execute(cls, **kwargs): + """ + Creates, validates, and persists the batch command, then schedules + execution via a background task. + - No devices match the specified criteria: batch is deleted and + ValidationError is raised to avoid orphan records. + - Explicit device list provided: uses device PKs directly instead + of resolving via organization/group/location filters. + - Superuser with no organization: targets devices across all orgs. + """ devices_list = kwargs.pop("devices", None) batch = cls(**kwargs) batch.full_clean() batch.save() if devices_list: batch.devices.set(devices_list) + if not batch.devices.exists(): + batch.delete() + raise ValidationError( + _("No devices match the specified criteria."), + ) + elif not batch.resolve_devices().exists(): + batch.delete() + raise ValidationError( + _("No devices match the specified criteria."), + ) batch.status = "in-progress" batch.save(update_fields=["status"]) transaction.on_commit(lambda: launch_batch_command.delay(batch.pk)) @@ -880,23 +901,40 @@ def execute(cls, **kwargs): @classmethod def dry_run(cls, **kwargs): + """ + Returns the devices that would be targeted by this batch command without + executing it. + - Command type not provided (GET request): skips full clean, only + validates organization relations. + - Explicit device list provided: returns it directly without resolving + via organization/group/location filters. + """ devices_list = kwargs.pop("devices", None) command_type = kwargs.pop("command_type", None) batch = cls(**kwargs) if command_type: batch.command_type = command_type batch.full_clean() + else: + batch._validate_org_relations() if devices_list: return {"devices": list(devices_list)} return {"devices": list(batch.resolve_devices())} def create_commands(self): - Command = load_model("connection", "Command") - if Command.objects.filter(batch_command=self).exists(): + """ + Creates individual Command instances for each device targeted by this batch + command. + - Commands already exist for this batch: returns early (idempotent guard). + - Device validation fails (deactivated, no credentials, wrong org): + device is skipped and logged — no Command record is created. + """ + if self.batch_commands.exists(): return + Command = load_model("connection", "Command") self.status = "in-progress" self.save() - for device in self.resolve_devices().iterator(): + for device in self.resolve_devices(): command = Command( device=device, type=self.command_type, @@ -904,25 +942,27 @@ def create_commands(self): batch_command=self, ) try: - command.full_clean() command.save() except ValidationError as e: - command.status = "failed" - command.output = str(e) - models.Model.save(command) - fields = list(getattr(e, "message_dict", {}).keys()) or ["__all__"] logger.warning( - "Command validation failed for batch=%s device=%s fields=%s", - self.pk, + "Skipping device %s for batch %s: %s", device.pk, - ",".join(fields), + self.pk, + e, ) self.calculate_and_update_status() def calculate_and_update_status(self): - Command = load_model("connection", "Command") - operations = Command.objects.filter(batch_command=self) - stats = operations.aggregate( + """ + Calculate batch status based on individual command statuses and update if + changed. + - No commands exist: status set to "idle". + - Commands still running: status set to "in-progress". + - Some commands failed: status set to "failed". + - All commands completed successfully: status set to "success". + - Status unchanged: no database write performed. + """ + stats = self.batch_commands.aggregate( total_operations=models.Count("id"), in_progress=models.Count( models.Case( 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 62fe4eef7..9ade089f7 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -83,7 +83,6 @@ class Migration(migrations.Migration): null=True, ), ), - ("execute_all", models.BooleanField(default=False)), ( "devices", models.ManyToManyField( @@ -136,6 +135,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, + related_name="batch_commands", to=swapper.get_model_name("connection", "BatchCommand"), ), ), diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 53f39e1d8..2ad4b153a 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -81,14 +81,6 @@ def launch_command(command_id): return try: command.execute() - # Todo: Remove once demo is completed - if command.batch_command_id: - print("****************************") - print(f"Device: {command.device.name}") - print(f"Status: {command.status}") - print("") - print(command.output) - print("****************************") except SoftTimeLimitExceeded: command.status = "failed" command._add_output(_("Background task time limit exceeded.")) @@ -106,7 +98,7 @@ def launch_command(command_id): command._save_without_resurrecting() -@shared_task(bind=True, soft_time_limit=3600) +@shared_task(bind=True, soft_time_limit=app_settings.SSH_COMMAND_TIMEOUT * 1.2) def launch_batch_command(self, batch_id): BatchCommand = load_model("connection", "BatchCommand") try: 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 593a9f6b2..5f361d5e5 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 @@ -82,7 +82,6 @@ class Migration(migrations.Migration): null=True, ), ), - ("execute_all", models.BooleanField(default=False)), ( "devices", models.ManyToManyField( @@ -132,6 +131,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, + related_name="batch_commands", to="sample_connection.batchcommand", ), ), From 908dea1657025e8934f2a7209339615b7fc22492 Mon Sep 17 00:00:00 2001 From: dee077 Date: Sun, 21 Jun 2026 04:26:42 +0530 Subject: [PATCH 09/20] [fix] Restructure BatchCommand fields and refine test fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Renamed BatchCommand fields across model, serializer, migrations, and tests - Moved test fixtures from setUp into individual test methods - _create_batch_command now requires organization as positional arg - Added test_batch_command_execute_queries with assertNumQueries(13) - Added test_batch_command_cross_org_restrictions for org-level command restrictions - Added device ID assertions in execute tests - Renamed tests: list_filter_org→list_organization_scoped, execute_no_devices→execute_org_has_no_devices --- .../connection/api/serializers.py | 40 ++- openwisp_controller/connection/api/urls.py | 10 + openwisp_controller/connection/api/views.py | 19 +- openwisp_controller/connection/base/models.py | 67 ++--- ...0011_batchcommand_command_batch_command.py | 8 +- .../connection/tests/test_api.py | 264 ++++++++++++++++++ .../openwisp2/sample_connection/api/views.py | 16 ++ ...0005_batchcommand_command_batch_command.py | 8 +- 8 files changed, 381 insertions(+), 51 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index c25f27812..5d2a6c681 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -125,10 +125,8 @@ class Meta: class BatchCommandExecuteSerializer( FilterSerializerByOrgManaged, serializers.ModelSerializer ): - type = serializers.CharField(source="command_type") - input = serializers.JSONField( - source="command_input", allow_null=True, required=False - ) + type = serializers.CharField() + input = serializers.JSONField(allow_null=True, required=False) devices = serializers.PrimaryKeyRelatedField( many=True, queryset=Device.objects.all(), @@ -187,3 +185,37 @@ def validate(self, data): } ) return data + + +class BatchCommandSerializer(BaseSerializer): + device_count = serializers.IntegerField(source="devices.count", read_only=True) + + class Meta: + model = BatchCommand + fields = ( + "id", + "organization", + "status", + "type", + "input", + "group", + "location", + "device_count", + "created", + "modified", + ) + read_only_fields = ( + "created", + "modified", + ) + + +class BatchCommandDetailSerializer(BatchCommandSerializer): + devices = serializers.PrimaryKeyRelatedField( + many=True, + read_only=True, + pk_field=serializers.UUIDField(format="hex_verbose"), + ) + + class Meta(BatchCommandSerializer.Meta): + fields = BatchCommandSerializer.Meta.fields + ("devices",) diff --git a/openwisp_controller/connection/api/urls.py b/openwisp_controller/connection/api/urls.py index 76a30c64a..4a94d171d 100644 --- a/openwisp_controller/connection/api/urls.py +++ b/openwisp_controller/connection/api/urls.py @@ -40,6 +40,16 @@ def get_api_urls(api_views): api_views.deviceconnection_detail_view, name="deviceconnection_detail", ), + path( + "api/v1/controller/batch-command/", + api_views.batch_command_list_view, + name="batch_command_list", + ), + path( + "api/v1/controller/batch-command//", + api_views.batch_command_detail_view, + name="batch_command_detail", + ), path( "api/v1/controller/batch-command/execute/", api_views.batch_command_execute_view, diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index df87f9cce..0a4912d76 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -5,6 +5,7 @@ from rest_framework import status from rest_framework.generics import ( GenericAPIView, + ListAPIView, ListCreateAPIView, RetrieveAPIView, RetrieveUpdateDestroyAPIView, @@ -21,7 +22,9 @@ RelatedDeviceProtectedAPIMixin, ) from .serializers import ( + BatchCommandDetailSerializer, BatchCommandExecuteSerializer, + BatchCommandSerializer, CommandSerializer, CredentialSerializer, DeviceConnectionSerializer, @@ -152,6 +155,7 @@ class BatchCommandExecuteView(ProtectedAPIMixin, GenericAPIView): def post(self, request): serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) + serializer.validated_data.pop("execute_all", None) try: batch = BatchCommand.execute(**serializer.validated_data) except ValidationError as e: @@ -164,6 +168,7 @@ def post(self, request): def get(self, request): serializer = self.get_serializer(data=request.query_params) serializer.is_valid(raise_exception=True) + serializer.validated_data.pop("execute_all", None) try: data = BatchCommand.dry_run(**serializer.validated_data) except ValidationError as e: @@ -175,6 +180,17 @@ def get(self, request): return Response(data) +class BatchCommandListView(ProtectedAPIMixin, ListAPIView): + queryset = BatchCommand.objects.all().order_by("-created") + serializer_class = BatchCommandSerializer + pagination_class = OpenWispPagination + + +class BatchCommandDetailView(ProtectedAPIMixin, RetrieveAPIView): + queryset = BatchCommand.objects.all() + serializer_class = BatchCommandDetailSerializer + + class DeviceConnectionDetailView(BaseDeviceConnection, RetrieveUpdateDestroyAPIView): def get_object(self): queryset = self.filter_queryset(self.get_queryset()) @@ -195,5 +211,6 @@ def get_object(self): # TODO: remove in version 1.4 deviceconnection_details_view = deviceconnection_detail_view - batch_command_execute_view = BatchCommandExecuteView.as_view() +batch_command_list_view = BatchCommandListView.as_view() +batch_command_detail_view = BatchCommandDetailView.as_view() diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 0564c3dea..e92888474 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -738,8 +738,8 @@ class AbstractBatchCommand(TimeStampedEditableModel): STATUS_CHOICES = ( ("idle", _("idle")), ("in-progress", _("in progress")), - ("success", _("completed successfully")), - ("failed", _("completed with some failures")), + ("success", _("success")), + ("failed", _("failed")), ) organization = models.ForeignKey( @@ -751,11 +751,11 @@ class AbstractBatchCommand(TimeStampedEditableModel): status = models.CharField( max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0] ) - command_type = models.CharField( + type = models.CharField( max_length=16, choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices), ) - command_input = JSONField(blank=True, null=True, encoder=DjangoJSONEncoder) + input = JSONField(blank=True, null=True, encoder=DjangoJSONEncoder) group = models.ForeignKey( get_model_name("config", "DeviceGroup"), on_delete=models.SET_NULL, @@ -833,27 +833,26 @@ def clean(self): allowed = dict( Command.get_org_allowed_commands(organization_id=self.organization_id) ) - if self.command_type not in allowed: + if self.type not in allowed: raise ValidationError( { - "command_type": _( + "type": _( '"{command}" command is not available for this organization' - ).format(command=self.command_type) + ).format(command=self.type) } ) try: - jsonschema.Draft4Validator(get_command_schema(self.command_type)).validate( - self.command_input + jsonschema.Draft4Validator(get_command_schema(self.type)).validate( + self.input ) except SchemaError as e: - raise ValidationError({"command_input": e.message}) + raise ValidationError({"input": e.message}) def resolve_devices(self): """ - Returns the queryset of devices targeted by this batch command. - - Devices explicitly set via M2M relation: returns those directly. - - No explicit devices: resolves via organization, group, and location filters. - - No filters set: returns all devices (superuser targeting all orgs). + Returns an iterator 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. """ if self.pk and self.devices.exists(): return self.devices.iterator() @@ -865,18 +864,14 @@ def resolve_devices(self): qs = qs.filter(group=self.group) if self.location: qs = qs.filter(devicelocation__location=self.location) - return qs + return qs.iterator() @classmethod def execute(cls, **kwargs): """ Creates, validates, and persists the batch command, then schedules - execution via a background task. - - No devices match the specified criteria: batch is deleted and - ValidationError is raised to avoid orphan records. - - Explicit device list provided: uses device PKs directly instead - of resolving via organization/group/location filters. - - Superuser with no organization: targets devices across all orgs. + execution via a background task. Raises ValidationError and deletes + the batch if no devices match the criteria. """ devices_list = kwargs.pop("devices", None) batch = cls(**kwargs) @@ -889,7 +884,7 @@ def execute(cls, **kwargs): raise ValidationError( _("No devices match the specified criteria."), ) - elif not batch.resolve_devices().exists(): + elif not any(batch.resolve_devices()): batch.delete() raise ValidationError( _("No devices match the specified criteria."), @@ -902,18 +897,15 @@ def execute(cls, **kwargs): @classmethod def dry_run(cls, **kwargs): """ - Returns the devices that would be targeted by this batch command without - executing it. - - Command type not provided (GET request): skips full clean, only - validates organization relations. - - Explicit device list provided: returns it directly without resolving - via organization/group/location filters. + Returns the devices that would be targeted by this batch command + without executing it. Skips full validation when command type is + not provided case for GET request. """ devices_list = kwargs.pop("devices", None) - command_type = kwargs.pop("command_type", None) + cmd_type = kwargs.pop("type", None) batch = cls(**kwargs) - if command_type: - batch.command_type = command_type + if cmd_type: + batch.type = cmd_type batch.full_clean() else: batch._validate_org_relations() @@ -923,11 +915,10 @@ def dry_run(cls, **kwargs): def create_commands(self): """ - Creates individual Command instances for each device targeted by this batch - command. - - Commands already exist for this batch: returns early (idempotent guard). - - Device validation fails (deactivated, no credentials, wrong org): - device is skipped and logged — no Command record is created. + Creates individual Command instances for each device targeted by + this batch command. Returns early if commands already exist + (idempotent guard). Devices that fail validation are silently + skipped. """ if self.batch_commands.exists(): return @@ -937,8 +928,8 @@ def create_commands(self): for device in self.resolve_devices(): command = Command( device=device, - type=self.command_type, - input=self.command_input, + type=self.type, + input=self.input, batch_command=self, ) try: 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 9ade089f7..5f5b7f590 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -57,15 +57,15 @@ class Migration(migrations.Migration): choices=[ ("idle", "idle"), ("in-progress", "in progress"), - ("success", "completed successfully"), - ("failed", "completed with some failures"), + ("success", "success"), + ("failed", "failed"), ], default="idle", max_length=12, ), ), ( - "command_type", + "type", models.CharField( max_length=16, choices=( @@ -76,7 +76,7 @@ class Migration(migrations.Migration): ), ), ( - "command_input", + "input", models.JSONField( blank=True, encoder=django.core.serializers.json.DjangoJSONEncoder, diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 6e3a827ab..8ab0bb5d0 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -21,6 +21,7 @@ Command = load_model("connection", "Command") DeviceConnection = load_model("connection", "DeviceConnection") +BatchCommand = load_model("connection", "BatchCommand") command_qs = Command.objects.order_by("-created") OrganizationUser = load_model("openwisp_users", "OrganizationUser") Group = load_model("openwisp_users", "Group") @@ -860,3 +861,266 @@ def test_deviceconnection_unauthenticated_user(self): "delete": 401, }, ) + + +class TestBatchCommandsAPI( + TestAdminMixin, AuthenticationMixin, TestCase, CreateConnectionsMixin +): + url_namespace = "connection_api" + + def setUp(self): + super().setUp() + self._login() + + def _create_batch_command(self, organization, **kwargs): + opts = dict( + organization=organization, + type="custom", + input={"command": "echo test"}, + ) + devices = kwargs.pop("devices", None) + opts.update(kwargs) + batch = BatchCommand(**opts) + batch.full_clean() + batch.save() + if devices is not None: + if not isinstance(devices, (list, tuple)): + devices = [devices] + batch.devices.set(devices) + return batch + + def test_batch_command_list(self): + org = self._get_org() + url = reverse("connection_api:batch_command_list") + for _ in range(3): + self._create_batch_command(organization=org) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["count"], 3) + self.assertEqual(len(response.data["results"]), 3) + created_list = [cmd["created"] for cmd in response.data["results"]] + sorted_created = sorted(created_list, reverse=True) + self.assertEqual(created_list, sorted_created) + result = response.data["results"][0] + self.assertIn("id", result) + self.assertIn("status", result) + self.assertIn("type", result) + self.assertIn("input", result) + self.assertIn("device_count", result) + self.assertIn("created", result) + self.assertEqual(result["device_count"], 0) + + def test_batch_command_detail(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + batch = self._create_batch_command(organization=org, devices=[device]) + url = reverse("connection_api:batch_command_detail", args=[batch.pk]) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["id"], str(batch.pk)) + self.assertEqual(response.data["status"], batch.status) + self.assertEqual(response.data["type"], batch.type) + self.assertEqual(response.data["input"], batch.input) + self.assertIn("devices", response.data) + self.assertEqual(response.data["devices"], [str(device.pk)]) + self.assertEqual(response.data["device_count"], 1) + + def test_batch_command_execute(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + self._create_device_connection(device=device) + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + # transaction.on_commit doesn't fire in TestCase; trigger manually + batch.create_commands() + command = Command.objects.get(batch_command=batch) + self.assertEqual(command.device.pk, device.pk) + + def test_batch_command_execute_queries(self): + org = self._get_org() + devices = [] + for i in range(3): + d = self._create_device( + name=f"q-dev-{i}", + mac_address=f"00:11:22:33:44:{i:02x}", + organization=org, + ) + self._create_config(device=d) + devices.append(d) + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(d.pk) for d in devices], + } + with self.assertNumQueries(13): + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + self.assertEqual(batch.devices.count(), 3) + self.assertCountEqual( + batch.devices.values_list("pk", flat=True), + [d.pk for d in devices], + ) + + def test_batch_command_execute_org_has_no_devices(self): + org = self._get_org() + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + + def test_batch_command_execute_no_org(self): + org = self._get_org() + self.client.logout() + operator = self._create_operator(organizations=[org]) + add_perm = Permission.objects.get(codename="add_batchcommand") + operator.user_permissions.add(add_perm) + self.client.force_login(operator) + payload = { + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn( + "Only superusers", + str(response.data), + ) + + def test_batch_command_dry_run(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + url = "{0}?organization={1}".format( + reverse("connection_api:batch_command_execute"), str(org.pk) + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertIn("devices", response.data) + self.assertIn(str(device.pk), response.data["devices"]) + + def test_batch_command_list_organization_scoped(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + self._create_batch_command(organization=org) + self._create_batch_command(organization=org2) + self.client.logout() + operator = self._create_operator(organizations=[org]) + view_perm = Permission.objects.get(codename="view_batchcommand") + operator.user_permissions.add(view_perm) + self.client.force_login(operator) + response = self.client.get(reverse("connection_api:batch_command_list")) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["count"], 1) + + def test_batch_command_unauthorized(self): + self.client.logout() + with self.subTest("List"): + response = self.client.get(reverse("connection_api:batch_command_list")) + self.assertEqual(response.status_code, 401) + + with self.subTest("Detail"): + response = self.client.get( + reverse( + "connection_api:batch_command_detail", + args=[uuid.uuid4()], + ) + ) + self.assertEqual(response.status_code, 401) + + with self.subTest("Dry run"): + response = self.client.get(reverse("connection_api:batch_command_execute")) + self.assertEqual(response.status_code, 401) + + with self.subTest("Execute"): + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps({"type": "custom"}), + content_type="application/json", + ) + self.assertEqual(response.status_code, 401) + + def test_batch_command_cross_org_restrictions(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_a = self._create_device( + name="device-a", + mac_address="00:11:22:33:44:aa", + organization=org, + ) + self._create_config(device=device_a) + self._create_device_connection(device=device_a) + device_b = self._create_device( + name="device-b", + mac_address="00:11:22:33:44:bb", + organization=org2, + ) + self._create_config(device=device_b) + self._create_device_connection(device=device_b) + + with patch.dict( + ORGANIZATION_ENABLED_COMMANDS, + {str(org2.pk): ("reboot",)}, + ): + payload = { + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device_a.pk), str(device_b.pk)], + } + response = self.client.post( + reverse("connection_api:batch_command_execute"), + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + # transaction.on_commit doesn't fire in TestCase; + # trigger create_commands() manually + batch.create_commands() + batch.refresh_from_db() + command_qs = Command.objects.filter(batch_command=batch) + self.assertTrue(command_qs.filter(device=device_a).exists()) + self.assertFalse(command_qs.filter(device=device_b).exists()) + # Verify rendering works for created commands + url = reverse( + "connection_api:device_command_list", + args=[device_a.pk], + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + cmd_data = response.data["results"][0] + self.assertIn("type", cmd_data) + self.assertIn("input", cmd_data) diff --git a/tests/openwisp2/sample_connection/api/views.py b/tests/openwisp2/sample_connection/api/views.py index 66510f22a..22d04a924 100644 --- a/tests/openwisp2/sample_connection/api/views.py +++ b/tests/openwisp2/sample_connection/api/views.py @@ -1,6 +1,12 @@ +from openwisp_controller.connection.api.views import ( + BatchCommandDetailView as BaseBatchCommandDetailView, +) from openwisp_controller.connection.api.views import ( BatchCommandExecuteView as BaseBatchCommandExecuteView, ) +from openwisp_controller.connection.api.views import ( + BatchCommandListView as BaseBatchCommandListView, +) from openwisp_controller.connection.api.views import ( CommandDetailsView as BaseCommandDetailsView, ) @@ -49,6 +55,14 @@ class BatchCommandExecuteView(BaseBatchCommandExecuteView): pass +class BatchCommandListView(BaseBatchCommandListView): + pass + + +class BatchCommandDetailView(BaseBatchCommandDetailView): + pass + + command_list_create_view = CommandListCreateView.as_view() command_details_view = CommandDetailsView.as_view() credential_list_create_view = CredentialListCreateView.as_view() @@ -56,3 +70,5 @@ class BatchCommandExecuteView(BaseBatchCommandExecuteView): deviceconnection_list_create_view = DeviceConnectionListCreateView.as_view() deviceconnection_detail_view = DeviceConnectionDetailView.as_view() batch_command_execute_view = BatchCommandExecuteView.as_view() +batch_command_list_view = BatchCommandListView.as_view() +batch_command_detail_view = BatchCommandDetailView.as_view() 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 5f361d5e5..de9613fc0 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 @@ -56,15 +56,15 @@ class Migration(migrations.Migration): choices=[ ("idle", "idle"), ("in-progress", "in progress"), - ("success", "completed successfully"), - ("failed", "completed with some failures"), + ("success", "success"), + ("failed", "failed"), ], default="idle", max_length=12, ), ), ( - "command_type", + "type", models.CharField( max_length=16, choices=( @@ -75,7 +75,7 @@ class Migration(migrations.Migration): ), ), ( - "command_input", + "input", models.JSONField( blank=True, encoder=django.core.serializers.json.DjangoJSONEncoder, From d00094e02fb7fc7302aa6d57e1ed7ede6a1557cb Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 23 Jun 2026 09:54:26 +0530 Subject: [PATCH 10/20] [feature] Added skipped_devices model field and comprehensive batch command tests - Added skipped_devices JSONField to track devices skipped during batch command creation, along with API, model, and task-level tests. - Added skipped_devices JSONField to AbstractBatchCommand model - Added API tests: execute, dry-run, skipped devices, org mismatch, authorization, query counts, device targeting by group/location - Added model tests: str, total_devices/successful/failed, validation, create_commands (deactivated device, no credentials), resolve_devices, dry_run, execute, org mismatch, idempotency, status calculation, permissions - Added task tests: deleted batch resilience, single device command creation, multiple device command creation --- .../connection/api/serializers.py | 6 +- openwisp_controller/connection/base/models.py | 37 +- ...0011_batchcommand_command_batch_command.py | 20 + .../connection/migrations/__init__.py | 27 + .../connection/tests/test_api.py | 660 ++++++++++++++--- .../connection/tests/test_models.py | 694 +++++++++++++++++- .../connection/tests/test_tasks.py | 65 ++ ...0005_batchcommand_command_batch_command.py | 21 + 8 files changed, 1436 insertions(+), 94 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 5d2a6c681..869868180 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -216,6 +216,10 @@ class BatchCommandDetailSerializer(BatchCommandSerializer): read_only=True, pk_field=serializers.UUIDField(format="hex_verbose"), ) + skipped_devices = serializers.JSONField(read_only=True) class Meta(BatchCommandSerializer.Meta): - fields = BatchCommandSerializer.Meta.fields + ("devices",) + fields = BatchCommandSerializer.Meta.fields + ( + "devices", + "skipped_devices", + ) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index e92888474..50feba84d 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -775,12 +775,28 @@ class AbstractBatchCommand(TimeStampedEditableModel): blank=True, verbose_name=_("devices"), ) + skipped_devices = models.JSONField( + blank=True, + null=True, + default=dict, + verbose_name=_("Skipped devices"), + help_text=_( + "Maps device UUIDs to validation error messages for devices " + "that were skipped during command creation." + ), + ) class Meta: abstract = True verbose_name = _("Batch command") verbose_name_plural = _("Batch commands") + def __str__(self): + return "{0} ({1})".format( + self.type, + timezone.localtime(self.created).strftime("%Y-%m-%d %H:%M:%S"), + ) + @cached_property def total_devices(self): return self.batch_commands.count() @@ -879,6 +895,7 @@ def execute(cls, **kwargs): batch.save() if devices_list: batch.devices.set(devices_list) + batch._validate_org_relations() if not batch.devices.exists(): batch.delete() raise ValidationError( @@ -910,6 +927,18 @@ def dry_run(cls, **kwargs): else: batch._validate_org_relations() if devices_list: + for device in devices_list: + if ( + batch.organization_id + and device.organization_id != batch.organization_id + ): + raise ValidationError( + { + "devices": _( + "All devices must belong to the same organization" + ) + } + ) return {"devices": list(devices_list)} return {"devices": list(batch.resolve_devices())} @@ -917,12 +946,13 @@ def create_commands(self): """ Creates individual Command instances for each device targeted by this batch command. Returns early if commands already exist - (idempotent guard). Devices that fail validation are silently - skipped. + (idempotent guard). Devices that fail validation are recorded + in skipped_devices and logged. """ if self.batch_commands.exists(): return Command = load_model("connection", "Command") + self.skipped_devices = {} self.status = "in-progress" self.save() for device in self.resolve_devices(): @@ -935,12 +965,15 @@ def create_commands(self): try: command.save() except ValidationError as e: + self.skipped_devices[str(device.pk)] = e.messages logger.warning( "Skipping device %s for batch %s: %s", device.pk, self.pk, e, ) + if self.skipped_devices: + self.save(update_fields=["skipped_devices"]) self.calculate_and_update_status() def calculate_and_update_status(self): 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 5f5b7f590..41120aad2 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -4,6 +4,7 @@ import django import django.core.serializers.json +import django.db.migrations.operations.special import django.db.models.deletion import django.utils.timezone import model_utils.fields @@ -12,6 +13,8 @@ from openwisp_controller import connection as connection_config +from . import assign_batchcommand_permissions_to_groups + class Migration(migrations.Migration): @@ -91,6 +94,19 @@ class Migration(migrations.Migration): verbose_name="devices", ), ), + ( + "skipped_devices", + models.JSONField( + blank=True, + null=True, + default=dict, + verbose_name="Skipped devices", + help_text=( + "Maps device UUIDs to validation error messages for " + "devices that were skipped during command creation." + ), + ), + ), ( "group", models.ForeignKey( @@ -139,4 +155,8 @@ class Migration(migrations.Migration): to=swapper.get_model_name("connection", "BatchCommand"), ), ), + migrations.RunPython( + code=assign_batchcommand_permissions_to_groups, + reverse_code=django.db.migrations.operations.special.RunPython.noop, + ), ] diff --git a/openwisp_controller/connection/migrations/__init__.py b/openwisp_controller/connection/migrations/__init__.py index 5b5853d2b..67a2c20e7 100644 --- a/openwisp_controller/connection/migrations/__init__.py +++ b/openwisp_controller/connection/migrations/__init__.py @@ -60,3 +60,30 @@ def assign_command_permissions_to_groups(apps, schema_editor): admin.permissions.add( Permission.objects.get(codename="{}_{}".format(operation, "command")).pk ) + + +def assign_batchcommand_permissions_to_groups(apps, schema_editor): + create_default_permissions(apps, schema_editor) + admin_operations = ["add", "change", "delete", "view"] + operator_operations = ["add", "view"] + Group = get_swapped_model(apps, "openwisp_users", "Group") + + try: + admin = Group.objects.get(name="Administrator") + operator = Group.objects.get(name="Operator") + except Group.DoesNotExist: + return + + for operation in operator_operations: + permission = Permission.objects.get( + codename="{}_{}".format(operation, "batchcommand") + ) + admin.permissions.add(permission.pk) + operator.permissions.add(permission.pk) + + for operation in admin_operations: + admin.permissions.add( + Permission.objects.get( + codename="{}_{}".format(operation, "batchcommand") + ).pk + ) diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 8ab0bb5d0..4df565d53 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -25,6 +25,9 @@ command_qs = Command.objects.order_by("-created") OrganizationUser = load_model("openwisp_users", "OrganizationUser") Group = load_model("openwisp_users", "Group") +DeviceGroup = load_model("config", "DeviceGroup") +Location = load_model("geo", "Location") +DeviceLocation = load_model("geo", "DeviceLocation") class TestCommandsAPI(TestCase, AuthenticationMixin, CreateCommandMixin): @@ -926,61 +929,271 @@ def test_batch_command_detail(self): self.assertEqual(response.data["devices"], [str(device.pk)]) self.assertEqual(response.data["device_count"], 1) - def test_batch_command_execute(self): + def test_batch_command_dry_run_endpoint(self): org = self._get_org() - device = self._create_device(organization=org) - self._create_config(device=device) - self._create_device_connection(device=device) - payload = { - "organization": str(org.pk), - "type": "custom", - "input": {"command": "echo test"}, - "devices": [str(device.pk)], - } - response = self.client.post( - reverse("connection_api:batch_command_execute"), - data=json.dumps(payload), - content_type="application/json", + device1 = self._create_device( + name="dryf-dev1", + mac_address="00:11:22:33:44:d1", + organization=org, ) - self.assertEqual(response.status_code, 201) - self.assertIn("batch", response.data) - batch = BatchCommand.objects.get(pk=response.data["batch"]) - # transaction.on_commit doesn't fire in TestCase; trigger manually - batch.create_commands() - command = Command.objects.get(batch_command=batch) - self.assertEqual(command.device.pk, device.pk) - - def test_batch_command_execute_queries(self): + self._create_config(device=device1) + device2 = self._create_device( + name="dryf-dev2", + mac_address="00:11:22:33:44:d2", + organization=org, + ) + self._create_config(device=device2) + group = DeviceGroup.objects.create(name="dryf-group", organization=org) + device1.group = group + device1.save() + location = Location.objects.create( + name="dryf-loc", + type="indoor", + organization=org, + ) + DeviceLocation.objects.create(content_object=device2, location=location) + + base_url = reverse("connection_api:batch_command_execute") + + with self.subTest("dry run with explicit devices"): + url = "{0}?organization={1}&devices={2}".format( + base_url, + str(org.pk), + str(device1.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["devices"], [str(device1.pk)]) + + with self.subTest("dry run with group"): + url = "{0}?organization={1}&group={2}".format( + base_url, + str(org.pk), + str(group.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertIn(str(device1.pk), response.data["devices"]) + self.assertNotIn(str(device2.pk), response.data["devices"]) + + with self.subTest("dry run with location"): + url = "{0}?organization={1}&location={2}".format( + base_url, + str(org.pk), + str(location.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertIn(str(device2.pk), response.data["devices"]) + self.assertNotIn(str(device1.pk), response.data["devices"]) + + with self.subTest("dry run with group and location"): + DeviceLocation.objects.create(content_object=device1, location=location) + url = "{0}?organization={1}&group={2}&location={3}".format( + base_url, + str(org.pk), + str(group.pk), + str(location.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertIn(str(device1.pk), response.data["devices"]) + self.assertNotIn(str(device2.pk), response.data["devices"]) + + with self.subTest("dry run org-wide"): + url = "{0}?organization={1}".format(base_url, str(org.pk)) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertIn(str(device1.pk), response.data["devices"]) + self.assertIn(str(device2.pk), response.data["devices"]) + + def test_batch_command_execute(self): org = self._get_org() - devices = [] - for i in range(3): - d = self._create_device( - name=f"q-dev-{i}", - mac_address=f"00:11:22:33:44:{i:02x}", - organization=org, + cred = self._create_credentials(name="exec-cred", organization=org) + device1 = self._create_device( + name="exec-dev1", + mac_address="00:11:22:33:44:e1", + organization=org, + ) + self._create_config(device=device1) + self._create_device_connection(device=device1, credentials=cred) + device2 = self._create_device( + name="exec-dev2", + mac_address="00:11:22:33:44:e2", + organization=org, + ) + self._create_config(device=device2) + self._create_device_connection(device=device2, credentials=cred) + group = DeviceGroup.objects.create(name="exec-group", organization=org) + device1.group = group + device1.save() + location = Location.objects.create( + name="exec-loc", + type="indoor", + organization=org, + ) + DeviceLocation.objects.create(content_object=device2, location=location) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("execute with explicit devices"): + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device1.pk)], + } + ), + content_type="application/json", ) - self._create_config(device=d) - devices.append(d) - payload = { - "organization": str(org.pk), - "type": "custom", - "input": {"command": "echo test"}, - "devices": [str(d.pk) for d in devices], - } - with self.assertNumQueries(13): + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + command = Command.objects.get(batch_command=batch, device=device1) + self.assertEqual(command.device.pk, device1.pk) + + with self.subTest("execute with group"): response = self.client.post( - reverse("connection_api:batch_command_execute"), - data=json.dumps(payload), + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "group": str(group.pk), + } + ), content_type="application/json", ) - self.assertEqual(response.status_code, 201) - self.assertIn("batch", response.data) - batch = BatchCommand.objects.get(pk=response.data["batch"]) - self.assertEqual(batch.devices.count(), 3) - self.assertCountEqual( - batch.devices.values_list("pk", flat=True), - [d.pk for d in devices], - ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + command = Command.objects.get(batch_command=batch, device=device1) + self.assertEqual(command.device.pk, device1.pk) + + with self.subTest("execute with location"): + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "location": str(location.pk), + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + command = Command.objects.get(batch_command=batch, device=device2) + self.assertEqual(command.device.pk, device2.pk) + + with self.subTest("execute with group and location"): + DeviceLocation.objects.create(content_object=device1, location=location) + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "group": str(group.pk), + "location": str(location.pk), + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + command = Command.objects.get(batch_command=batch, device=device1) + self.assertEqual(command.device.pk, device1.pk) + + with self.subTest("execute org-wide"): + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + self.assertEqual( + Command.objects.filter(batch_command=batch).count(), + 2, + ) + + def test_batch_command_endpoints_no_of_queries(self): + with self.subTest("list queries"): + org = self._get_org() + devices = [] + for i in range(2): + d = self._create_device( + name=f"q-dev-{i}", + mac_address=f"00:11:22:33:44:{i:02x}", + organization=org, + ) + self._create_config(device=d) + devices.append(d) + self._create_batch_command(organization=org, devices=devices) + url = reverse("connection_api:batch_command_list") + with self.assertNumQueries(4): + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["count"], 1) + + with self.subTest("detail queries"): + url = reverse( + "connection_api:batch_command_detail", + args=[response.data["results"][0]["id"]], + ) + with self.assertNumQueries(4): + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + with self.subTest("execute queries"): + org = self._get_org() + devices = [] + for i in range(3): + d = self._create_device( + name=f"q-exec-{i}", + mac_address=f"00:11:22:33:44:{i+0x10:02x}", + organization=org, + ) + self._create_config(device=d) + devices.append(d) + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(d.pk) for d in devices], + } + url = reverse("connection_api:batch_command_execute") + with self.assertNumQueries(15): + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + self.assertEqual(batch.devices.count(), 3) + self.assertCountEqual( + batch.devices.values_list("pk", flat=True), + [d.pk for d in devices], + ) def test_batch_command_execute_org_has_no_devices(self): org = self._get_org() @@ -990,14 +1203,19 @@ def test_batch_command_execute_org_has_no_devices(self): "input": {"command": "echo test"}, "execute_all": True, } + url = reverse("connection_api:batch_command_execute") response = self.client.post( - reverse("connection_api:batch_command_execute"), + url, data=json.dumps(payload), content_type="application/json", ) self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + ["No devices match the specified criteria."], + ) - def test_batch_command_execute_no_org(self): + def test_batch_command_no_org_only_allowed_to_superuser(self): org = self._get_org() self.client.logout() operator = self._create_operator(organizations=[org]) @@ -1009,71 +1227,90 @@ def test_batch_command_execute_no_org(self): "input": {"command": "echo test"}, "execute_all": True, } - response = self.client.post( - reverse("connection_api:batch_command_execute"), - data=json.dumps(payload), - content_type="application/json", - ) - self.assertEqual(response.status_code, 400) - self.assertIn( - "Only superusers", - str(response.data), - ) + url = reverse("connection_api:batch_command_execute") + with self.subTest("POST execute without org"): + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn( + "Only superusers", + str(response.data), + ) - def test_batch_command_dry_run(self): - org = self._get_org() - device = self._create_device(organization=org) - self._create_config(device=device) - url = "{0}?organization={1}".format( - reverse("connection_api:batch_command_execute"), str(org.pk) - ) - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - self.assertIn("devices", response.data) - self.assertIn(str(device.pk), response.data["devices"]) + with self.subTest("GET dry-run without org"): + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + self.assertIn( + "Only superusers", + str(response.data), + ) - def test_batch_command_list_organization_scoped(self): + def test_batch_command_endpoints_organization_scoped(self): org = self._get_org() org2 = self._create_org(name="org2", slug="org2") - self._create_batch_command(organization=org) - self._create_batch_command(organization=org2) + batch_org1 = self._create_batch_command(organization=org) + batch_org2 = self._create_batch_command(organization=org2) self.client.logout() operator = self._create_operator(organizations=[org]) view_perm = Permission.objects.get(codename="view_batchcommand") operator.user_permissions.add(view_perm) self.client.force_login(operator) - response = self.client.get(reverse("connection_api:batch_command_list")) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.data["count"], 1) - def test_batch_command_unauthorized(self): + with self.subTest("list scoped to own org"): + url = reverse("connection_api:batch_command_list") + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["count"], 1) + + with self.subTest("detail cross-org"): + url = reverse( + "connection_api:batch_command_detail", + args=[batch_org2.pk], + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + with self.subTest("detail own org"): + url = reverse( + "connection_api:batch_command_detail", + args=[batch_org1.pk], + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + def test_batch_command_endpoints_unauthorized(self): self.client.logout() + execute_url = reverse("connection_api:batch_command_execute") + with self.subTest("List"): - response = self.client.get(reverse("connection_api:batch_command_list")) + url = reverse("connection_api:batch_command_list") + response = self.client.get(url) self.assertEqual(response.status_code, 401) with self.subTest("Detail"): - response = self.client.get( - reverse( - "connection_api:batch_command_detail", - args=[uuid.uuid4()], - ) + url = reverse( + "connection_api:batch_command_detail", + args=[uuid.uuid4()], ) + response = self.client.get(url) self.assertEqual(response.status_code, 401) with self.subTest("Dry run"): - response = self.client.get(reverse("connection_api:batch_command_execute")) + response = self.client.get(execute_url) self.assertEqual(response.status_code, 401) with self.subTest("Execute"): response = self.client.post( - reverse("connection_api:batch_command_execute"), + execute_url, data=json.dumps({"type": "custom"}), content_type="application/json", ) self.assertEqual(response.status_code, 401) - def test_batch_command_cross_org_restrictions(self): + def test_batch_command_execute_skipped_devices(self): org = self._get_org() org2 = self._create_org(name="org2", slug="org2") device_a = self._create_device( @@ -1090,6 +1327,7 @@ def test_batch_command_cross_org_restrictions(self): ) self._create_config(device=device_b) self._create_device_connection(device=device_b) + execute_url = reverse("connection_api:batch_command_execute") with patch.dict( ORGANIZATION_ENABLED_COMMANDS, @@ -1101,7 +1339,7 @@ def test_batch_command_cross_org_restrictions(self): "devices": [str(device_a.pk), str(device_b.pk)], } response = self.client.post( - reverse("connection_api:batch_command_execute"), + execute_url, data=json.dumps(payload), content_type="application/json", ) @@ -1111,10 +1349,14 @@ def test_batch_command_cross_org_restrictions(self): # trigger create_commands() manually batch.create_commands() batch.refresh_from_db() + 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], + ) command_qs = Command.objects.filter(batch_command=batch) self.assertTrue(command_qs.filter(device=device_a).exists()) self.assertFalse(command_qs.filter(device=device_b).exists()) - # Verify rendering works for created commands url = reverse( "connection_api:device_command_list", args=[device_a.pk], @@ -1124,3 +1366,243 @@ def test_batch_command_cross_org_restrictions(self): cmd_data = response.data["results"][0] self.assertIn("type", cmd_data) self.assertIn("input", cmd_data) + + with self.subTest("skipped: no credentials"): + device = self._create_device( + name="skip-nc-dev", + mac_address="00:11:22:33:44:a1", + organization=org, + ) + self._create_config(device=device) + response = self.client.post( + execute_url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + batch.refresh_from_db() + self.assertIn(str(device.pk), batch.skipped_devices) + self.assertIn( + "Device has no credentials assigned", + batch.skipped_devices[str(device.pk)][0], + ) + detail_url = reverse( + "connection_api:batch_command_detail", + args=[batch.pk], + ) + detail_response = self.client.get(detail_url) + self.assertEqual(detail_response.status_code, 200) + self.assertIn(str(device.pk), detail_response.data["skipped_devices"]) + + with self.subTest("skipped: deactivated device"): + device = self._create_device( + name="skip-dd-dev", + mac_address="00:11:22:33:44:a2", + organization=org, + ) + self._create_config(device=device) + dd_cred = self._create_credentials( + name="skip-dd-cred", + organization=org, + ) + self._create_device_connection(device=device, credentials=dd_cred) + device.deactivate() + response = self.client.post( + execute_url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + batch = BatchCommand.objects.get(pk=response.data["batch"]) + batch.create_commands() + batch.refresh_from_db() + self.assertIn(str(device.pk), batch.skipped_devices) + self.assertIn( + "Device is deactivated", + batch.skipped_devices[str(device.pk)][0], + ) + detail_url = reverse( + "connection_api:batch_command_detail", + args=[batch.pk], + ) + detail_response = self.client.get(detail_url) + self.assertEqual(detail_response.status_code, 200) + self.assertIn(str(device.pk), detail_response.data["skipped_devices"]) + + def test_batch_command_execute_org_mismatched_data_provided(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_org2 = self._create_device( + name="api-mm-dev", + mac_address="00:11:22:33:44:88", + organization=org2, + ) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("device org mismatch"): + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device_org2.pk)], + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + {"devices": ["All devices must belong to the same organization."]}, + ) + + with self.subTest("group org mismatch"): + group_org2 = DeviceGroup.objects.create( + name="api-mm-group", + organization=org2, + ) + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "group": str(group_org2.pk), + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + { + "group": [ + ( + "The organization of the group doesn't match " + "the organization of the batch command operation" + ) + ] + }, + ) + + with self.subTest("location org mismatch"): + location_org2 = Location.objects.create( + name="api-mm-loc", + type="indoor", + organization=org2, + ) + response = self.client.post( + url, + data=json.dumps( + { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "location": str(location_org2.pk), + } + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + { + "location": [ + ( + "The organization of the location doesn't match " + "the organization of the batch command operation" + ) + ] + }, + ) + + def test_batch_command_dry_run_org_mismatched_data_provided(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_org2 = self._create_device( + name="dry-mm-dev", + mac_address="00:11:22:33:44:78", + organization=org2, + ) + base_url = reverse("connection_api:batch_command_execute") + + with self.subTest("device org mismatch"): + url = "{0}?organization={1}&devices={2}".format( + base_url, + str(org.pk), + str(device_org2.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + {"devices": ["All devices must belong to the same organization."]}, + ) + + with self.subTest("group org mismatch"): + group_org2 = DeviceGroup.objects.create( + name="dry-mm-group", + organization=org2, + ) + url = "{0}?organization={1}&group={2}".format( + base_url, + str(org.pk), + str(group_org2.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + { + "group": [ + ( + "The organization of the group doesn't match " + "the organization of the batch command operation" + ) + ] + }, + ) + + with self.subTest("location org mismatch"): + location_org2 = Location.objects.create( + name="dry-mm-loc", + type="indoor", + organization=org2, + ) + url = "{0}?organization={1}&location={2}".format( + base_url, + str(org.pk), + str(location_org2.pk), + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.data, + { + "location": [ + ( + "The organization of the location doesn't match " + "the organization of the batch command operation" + ) + ] + }, + ) diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index e3ed6bb97..5e6e29e6f 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -32,6 +32,10 @@ Group = load_model("openwisp_users", "Group") Organization = load_model("openwisp_users", "Organization") Command = load_model("connection", "Command") +BatchCommand = load_model("connection", "BatchCommand") +DeviceGroup = load_model("config", "DeviceGroup") +Location = load_model("geo", "Location") +DeviceLocation = load_model("geo", "DeviceLocation") _connect_path = "paramiko.SSHClient.connect" _exec_command_path = "paramiko.SSHClient.exec_command" @@ -460,14 +464,14 @@ def test_operator_group_permissions(self): permissions = group.permissions.filter( content_type__app_label=f"{self.app_label}" ) - self.assertEqual(permissions.count(), 6) + self.assertEqual(permissions.count(), 8) def test_administrator_group_permissions(self): group = Group.objects.get(name="Administrator") permissions = group.permissions.filter( content_type__app_label=f"{self.app_label}" ) - self.assertEqual(permissions.count(), 12) + self.assertEqual(permissions.count(), 16) def test_device_connection_set_connector(self): dc = self._create_device_connection() @@ -968,6 +972,692 @@ def test_command_multiple_connections(self, connect_mocked): command.refresh_from_db() self.assertIn(command.connection, [dc1, dc2]) + def _create_batch_command(self, organization, **kwargs): + opts = dict( + organization=organization, + type="custom", + input={"command": "echo test"}, + ) + devices = kwargs.pop("devices", None) + opts.update(kwargs) + batch = BatchCommand(**opts) + batch.full_clean() + batch.save() + if devices is not None: + if not isinstance(devices, (list, tuple)): + devices = [devices] + batch.devices.set(devices) + return batch + + def test_batch_command_str(self): + org = self._get_org() + batch = self._create_batch_command(organization=org) + expected = "{0} ({1})".format( + batch.type, + timezone.localtime(batch.created).strftime("%Y-%m-%d %H:%M:%S"), + ) + self.assertEqual(str(batch), expected) + + def test_batch_command_total_devices_successful_failed(self): + org = self._get_org() + device1 = self._create_device(organization=org) + self._create_config(device=device1) + dc1 = self._create_device_connection(device=device1) + device2 = self._create_device( + name="device2", + mac_address="00:11:22:33:44:02", + organization=org, + ) + self._create_config(device=device2) + dc2 = self._create_device_connection( + device=device2, + credentials=self._create_credentials( + name="Test credentials 2", + organization=org, + ), + ) + batch = self._create_batch_command(organization=org) + Command.objects.create( + batch_command=batch, + device=device1, + connection=dc1, + type=batch.type, + input={"command": "echo test"}, + status="success", + ) + Command.objects.create( + batch_command=batch, + device=device2, + connection=dc2, + type=batch.type, + input={"command": "echo test"}, + status="failed", + ) + self.assertEqual(batch.total_devices, 2) + self.assertEqual(batch.successful, 1) + self.assertEqual(batch.failed, 1) + self.assertEqual( + batch.batch_commands.filter(status="success").first().device, device1 + ) + self.assertEqual( + batch.batch_commands.filter(status="failed").first().device, device2 + ) + self.assertFalse( + batch.batch_commands.filter(status="success", device=device2).exists() + ) + self.assertFalse( + batch.batch_commands.filter(status="failed", device=device1).exists() + ) + + def test_batch_command_clean_validation(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_org2 = self._create_device(organization=org2) + + with self.subTest("devices from different org"): + batch = self._create_batch_command(organization=org) + batch.devices.add(device_org2) + with self.assertRaises(ValidationError) as ctx: + batch.clean() + self.assertIn("devices", ctx.exception.message_dict) + self.assertIn( + "must belong to the same organization", + ctx.exception.message_dict["devices"][0], + ) + + with self.subTest("invalid command type for org"): + with mock.patch.dict( + ORGANIZATION_ENABLED_COMMANDS, + {str(org.pk): ("reboot",)}, + ): + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + with self.assertRaises(ValidationError) as ctx: + batch.clean() + self.assertIn("type", ctx.exception.message_dict) + self.assertIn( + "not available for this organization", + ctx.exception.message_dict["type"][0], + ) + + with self.subTest("invalid JSON schema"): + batch = BatchCommand( + organization=org, + type="change_password", + input="not_an_object", + ) + with self.assertRaises(ValidationError) as ctx: + batch.clean() + self.assertIn("input", ctx.exception.message_dict) + + with self.subTest("group org mismatch"): + group = DeviceGroup.objects.create(name="test-group", organization=org2) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group, + ) + with self.assertRaises(ValidationError) as ctx: + batch.clean() + self.assertIn("group", ctx.exception.message_dict) + self.assertIn( + "The organization of the group doesn't match", + ctx.exception.message_dict["group"][0], + ) + + with self.subTest("location org mismatch"): + location = Location.objects.create( + name="test-location", + type="indoor", + organization=org2, + ) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + location=location, + ) + with self.assertRaises(ValidationError) as ctx: + batch.clean() + self.assertIn("location", ctx.exception.message_dict) + self.assertIn( + "The organization of the location doesn't match", + ctx.exception.message_dict["location"][0], + ) + + def test_batch_command_create_commands_deactivated_device(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + self._create_device_connection(device=device) + + with self.subTest("deactivating device does not block create_commands"): + device._is_deactivated = True + device.save(update_fields=["_is_deactivated"]) + device.config.set_status_deactivating() + device = Device.objects.get(pk=device.pk) + batch = self._create_batch_command(organization=org) + batch.devices.add(device) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(batch.skipped_devices, {}) + + with self.subTest("fully deactivated device skipped in create_commands"): + device.config.set_status_deactivated() + device = Device.objects.get(pk=device.pk) + batch = self._create_batch_command(organization=org) + batch.devices.add(device) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 0) + self.assertIn(str(device.pk), batch.skipped_devices) + self.assertIn( + "Device is deactivated", + batch.skipped_devices[str(device.pk)][0], + ) + + def test_batch_command_create_commands_no_credentials(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + batch = self._create_batch_command( + organization=org, + devices=[device], + ) + batch.create_commands() + batch.refresh_from_db() + self.assertIn(str(device.pk), batch.skipped_devices) + self.assertIn( + "Device has no credentials assigned", + batch.skipped_devices[str(device.pk)][0], + ) + + def test_batch_command_resolve_devices(self): + org = self._get_org() + device1 = self._create_device( + name="device1", + mac_address="00:11:22:33:44:01", + organization=org, + ) + device2 = self._create_device( + name="device2", + mac_address="00:11:22:33:44:02", + organization=org, + ) + + with self.subTest("explicit devices via M2M"): + batch = self._create_batch_command( + organization=org, + devices=[device1], + ) + resolved = list(batch.resolve_devices()) + self.assertEqual(resolved, [device1]) + + with self.subTest("organization-scoped (no explicit devices)"): + batch = self._create_batch_command(organization=org) + resolved = list(batch.resolve_devices()) + self.assertIn(device1, resolved) + self.assertIn(device2, resolved) + + with self.subTest("group filtering"): + group = DeviceGroup.objects.create(name="test-group", organization=org) + device1.group = group + device1.save() + batch = self._create_batch_command(organization=org, group=group) + resolved = list(batch.resolve_devices()) + self.assertIn(device1, resolved) + self.assertNotIn(device2, resolved) + + with self.subTest("location filtering"): + location = Location.objects.create( + name="test-location", + type="indoor", + organization=org, + ) + DeviceLocation.objects.create(content_object=device2, location=location) + batch = self._create_batch_command(organization=org, location=location) + resolved = list(batch.resolve_devices()) + self.assertIn(device2, resolved) + self.assertNotIn(device1, resolved) + + with self.subTest("group and location combined"): + DeviceLocation.objects.create(content_object=device1, location=location) + batch = self._create_batch_command( + organization=org, + group=device1.group, + location=location, + ) + resolved = list(batch.resolve_devices()) + self.assertIn(device1, resolved) + self.assertNotIn(device2, resolved) + + with self.subTest("no devices match"): + empty_group = DeviceGroup.objects.create( + name="empty-group", + organization=org, + ) + batch = self._create_batch_command( + organization=org, + group=empty_group, + ) + resolved = list(batch.resolve_devices()) + self.assertEqual(resolved, []) + + def test_batch_command_dry_run_method(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + + with self.subTest("dry_run with type"): + result = BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + + with self.subTest("dry_run without type"): + result = BatchCommand.dry_run(organization=org) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + + with self.subTest("dry_run with explicit devices"): + result = BatchCommand.dry_run( + organization=org, + devices=[device], + ) + self.assertEqual(result["devices"], [device]) + + with self.subTest("dry_run with group"): + group = DeviceGroup.objects.create(name="dry-run-group", organization=org) + device.group = group + device.save() + result = BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group, + ) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + + with self.subTest("dry_run with location"): + location = Location.objects.create( + name="dry-run-loc", + type="indoor", + organization=org, + ) + DeviceLocation.objects.create(content_object=device, location=location) + result = BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + location=location, + ) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + + with self.subTest("dry_run with group and location"): + result = BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group, + location=location, + ) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + + with self.subTest("dry run org-wide"): + device2 = self._create_device( + name="dry-org-dev2", + mac_address="00:11:22:33:44:77", + organization=org, + ) + result = BatchCommand.dry_run(organization=org) + self.assertIn("devices", result) + self.assertIn(device, result["devices"]) + self.assertIn(device2, result["devices"]) + + def test_batch_command_execute_method(self): + org = self._get_org() + empty_org = self._create_org(name="empty-org", slug="empty-org") + + with self.subTest("execute with no devices"): + with self.assertRaises(ValidationError) as ctx: + BatchCommand.execute( + organization=empty_org, + type="custom", + input={"command": "echo test"}, + ) + self.assertIn( + "No devices match", + str(ctx.exception), + ) + + cred = self._create_credentials( + name="exec-cred", + organization=org, + ) + device1 = self._create_device( + name="exec-dev1", + mac_address="00:11:22:33:44:e1", + organization=org, + ) + self._create_config(device=device1) + self._create_device_connection(device=device1, credentials=cred) + device2 = self._create_device( + name="exec-dev2", + mac_address="00:11:22:33:44:e2", + organization=org, + ) + self._create_config(device=device2) + self._create_device_connection(device=device2, credentials=cred) + group = DeviceGroup.objects.create(name="exec-group", organization=org) + device1.group = group + device1.save() + location = Location.objects.create( + name="exec-loc", + type="indoor", + organization=org, + ) + DeviceLocation.objects.create(content_object=device2, location=location) + + with self.subTest("execute with explicit devices"): + batch = BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + devices=[device1], + ) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(batch.batch_commands.first().device, device1) + + with self.subTest("execute with group"): + batch = BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group, + ) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(batch.batch_commands.first().device, device1) + + with self.subTest("execute with location"): + batch = BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + location=location, + ) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(batch.batch_commands.first().device, device2) + + with self.subTest("execute with group and location"): + DeviceLocation.objects.create(content_object=device1, location=location) + batch = BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group, + location=location, + ) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(batch.batch_commands.first().device, device1) + + with self.subTest("execute org-wide"): + batch = BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.create_commands() + self.assertEqual(batch.batch_commands.count(), 2) + cmd_devices = [c.device for c in batch.batch_commands.all()] + self.assertIn(device1, cmd_devices) + self.assertIn(device2, cmd_devices) + + def test_batch_command_execute_org_mismatch(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_org2 = self._create_device( + name="exec-mm-dev", + mac_address="00:11:22:33:44:99", + organization=org2, + ) + self._create_config(device=device_org2) + self._create_device_connection(device=device_org2) + + with self.subTest("device org mismatch"): + with self.assertRaises(ValidationError) as ctx: + BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + devices=[device_org2], + ) + self.assertIn("devices", ctx.exception.message_dict) + self.assertIn( + "must belong to the same organization", + ctx.exception.message_dict["devices"][0], + ) + + with self.subTest("group org mismatch"): + group_org2 = DeviceGroup.objects.create( + name="exec-mm-group", + organization=org2, + ) + with self.assertRaises(ValidationError) as ctx: + BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group_org2, + ) + self.assertIn("group", ctx.exception.message_dict) + self.assertIn( + "The organization of the group doesn't match", + ctx.exception.message_dict["group"][0], + ) + + with self.subTest("location org mismatch"): + location_org2 = Location.objects.create( + name="exec-mm-loc", + type="indoor", + organization=org2, + ) + with self.assertRaises(ValidationError) as ctx: + BatchCommand.execute( + organization=org, + type="custom", + input={"command": "echo test"}, + location=location_org2, + ) + self.assertIn("location", ctx.exception.message_dict) + self.assertIn( + "The organization of the location doesn't match", + ctx.exception.message_dict["location"][0], + ) + + def test_batch_command_dry_run_org_mismatch(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + device_org2 = self._create_device( + name="dry-mm-dev", + mac_address="00:11:22:33:44:98", + organization=org2, + ) + + with self.subTest("device org mismatch"): + with self.assertRaises(ValidationError) as ctx: + BatchCommand.dry_run( + organization=org, + devices=[device_org2], + ) + self.assertIn("devices", ctx.exception.message_dict) + self.assertIn( + "must belong to the same organization", + ctx.exception.message_dict["devices"][0], + ) + + with self.subTest("group org mismatch"): + group_org2 = DeviceGroup.objects.create( + name="dry-mm-group", + organization=org2, + ) + with self.assertRaises(ValidationError) as ctx: + BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + group=group_org2, + ) + self.assertIn("group", ctx.exception.message_dict) + self.assertIn( + "The organization of the group doesn't match", + ctx.exception.message_dict["group"][0], + ) + + with self.subTest("location org mismatch"): + location_org2 = Location.objects.create( + name="dry-mm-loc", + type="indoor", + organization=org2, + ) + with self.assertRaises(ValidationError) as ctx: + BatchCommand.dry_run( + organization=org, + type="custom", + input={"command": "echo test"}, + location=location_org2, + ) + self.assertIn("location", ctx.exception.message_dict) + self.assertIn( + "The organization of the location doesn't match", + ctx.exception.message_dict["location"][0], + ) + + def test_batch_command_create_commands_idempotent(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + self._create_device_connection(device=device) + batch = self._create_batch_command( + organization=org, + devices=[device], + ) + batch.create_commands() + self.assertEqual(Command.objects.filter(batch_command=batch).count(), 1) + batch.create_commands() + self.assertEqual( + Command.objects.filter(batch_command=batch).count(), + 1, + "create_commands must be idempotent", + ) + + def test_batch_command_calculate_and_update_status(self): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + dc = self._create_device_connection(device=device) + batch = self._create_batch_command(organization=org) + + with self.subTest("no commands shows idle"): + batch.status = "in-progress" + batch.save(update_fields=["status"]) + batch.calculate_and_update_status() + batch.refresh_from_db() + self.assertEqual(batch.status, "idle") + + with self.subTest("all in-progress shows in-progress"): + Command.objects.create( + batch_command=batch, + device=device, + connection=dc, + type=batch.type, + input={"command": "echo test"}, + status="in-progress", + ) + batch.calculate_and_update_status() + batch.refresh_from_db() + self.assertEqual(batch.status, "in-progress") + + with self.subTest("some failed shows failed"): + Command.objects.filter(batch_command=batch).update(status="success") + device2 = self._create_device( + name="device2", + mac_address="00:11:22:33:44:02", + organization=org, + ) + self._create_config(device=device2) + dc2 = self._create_device_connection( + device=device2, + credentials=self._create_credentials( + name="Test credentials 2", + organization=org, + ), + ) + Command.objects.create( + batch_command=batch, + device=device2, + connection=dc2, + type=batch.type, + input={"command": "echo test"}, + status="failed", + ) + batch.calculate_and_update_status() + batch.refresh_from_db() + self.assertEqual(batch.status, "failed") + + with self.subTest("all success shows success"): + batch2 = self._create_batch_command(organization=org) + Command.objects.create( + batch_command=batch2, + device=device, + connection=dc, + type=batch2.type, + input={"command": "echo test"}, + status="success", + ) + batch2.calculate_and_update_status() + batch2.refresh_from_db() + self.assertEqual(batch2.status, "success") + + with self.subTest("no change shows no extra save"): + initial_modified = batch2.modified + batch2.calculate_and_update_status() + batch2.refresh_from_db() + self.assertEqual(batch2.modified, initial_modified) + + def test_batch_command_permissions(self): + ct = ContentType.objects.get_by_natural_key( + app_label=self.app_label, model="batchcommand" + ) + operator_group = Group.objects.get(name="Operator") + admin_group = Group.objects.get(name="Administrator") + operator_permissions = operator_group.permissions.filter(content_type=ct) + admin_permissions = admin_group.permissions.filter(content_type=ct) + + with self.subTest("operator permissions"): + self.assertEqual(operator_permissions.count(), 2) + self.assertTrue( + operator_permissions.filter(codename="add_batchcommand").exists() + ) + self.assertTrue( + operator_permissions.filter(codename="view_batchcommand").exists() + ) + + with self.subTest("administrator permissions"): + self.assertEqual(admin_permissions.count(), 4) + class TestModelsTransaction(BaseTestModels, TransactionTestCase): def _prepare_conf_object(self, organization=None): diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index 587c08209..2176500c9 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -16,6 +16,7 @@ Command = load_model("connection", "Command") DeviceConnection = load_model("connection", "DeviceConnection") OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") +BatchCommand = load_model("connection", "BatchCommand") class TestTasks(CreateConnectionsMixin, TestCase): @@ -315,3 +316,67 @@ def _delete_then_raise(self): with mock.patch.object(Command, "execute", _delete_then_raise): tasks.launch_command(command.pk) self.assertFalse(Command.objects.filter(pk=command.pk).exists()) + + @mock.patch("logging.Logger.warning") + def test_launch_batch_command_skips_deleted_batch(self, mocked_warning): + batch_id = uuid.uuid4() + tasks.launch_batch_command(batch_id=batch_id) + mocked_warning.assert_called_with( + f"The BatchCommand object with id {batch_id} has been deleted" + ) + + @mock.patch("openwisp_controller.connection.tasks.launch_command.delay") + def test_launch_batch_command_creates_commands(self, mocked_delay): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + self._create_device_connection(device=device) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo 'test'"}, + ) + batch.full_clean() + batch.save() + batch.devices.set([device]) + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.batch_commands.count(), 1) + command = batch.batch_commands.first() + self.assertEqual(command.device, device) + self.assertEqual(command.type, batch.type) + self.assertEqual(command.status, "in-progress") + mocked_delay.assert_called_once_with(command.pk) + + @mock.patch("openwisp_controller.connection.tasks.launch_command.delay") + def test_launch_batch_command_creates_commands_for_multiple_devices( + self, mocked_delay + ): + org = self._get_org() + cred = self._create_credentials(organization=org, name="Multi device cred") + devices = [] + for i in range(2): + d = self._create_device( + name=f"task-dev-{i}", + mac_address=f"00:11:22:33:44:{i+0x50:02x}", + organization=org, + ) + self._create_config(device=d) + self._create_device_connection(device=d, credentials=cred) + devices.append(d) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.full_clean() + batch.save() + batch.devices.set(devices) + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.batch_commands.count(), 2) + cmd_devices = [c.device for c in batch.batch_commands.all()] + for d in devices: + self.assertIn(d, cmd_devices) + self.assertEqual(batch.status, "in-progress") + self.assertEqual(mocked_delay.call_count, 2) 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 de9613fc0..c9d65a677 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 @@ -4,12 +4,16 @@ import django import django.core.serializers.json +import django.db.migrations.operations.special import django.db.models.deletion import django.utils.timezone import model_utils.fields from django.db import migrations, models from openwisp_controller import connection as connection_config +from openwisp_controller.connection.migrations import ( + assign_batchcommand_permissions_to_groups, +) class Migration(migrations.Migration): @@ -88,6 +92,19 @@ class Migration(migrations.Migration): blank=True, to="sample_config.device", verbose_name="devices" ), ), + ( + "skipped_devices", + models.JSONField( + blank=True, + null=True, + default=dict, + verbose_name="Skipped devices", + help_text=( + "Maps device UUIDs to validation error messages for " + "devices that were skipped during command creation." + ), + ), + ), ( "group", models.ForeignKey( @@ -135,4 +152,8 @@ class Migration(migrations.Migration): to="sample_connection.batchcommand", ), ), + migrations.RunPython( + code=assign_batchcommand_permissions_to_groups, + reverse_code=django.db.migrations.operations.special.RunPython.noop, + ), ] From 02e28de792abf80c7f10521a9ef4da2064b522ec Mon Sep 17 00:00:00 2001 From: dee077 Date: Wed, 24 Jun 2026 04:58:30 +0530 Subject: [PATCH 11/20] [fix] Migration fix and add skipped devices in BatchCommandSerializer --- .../connection/api/serializers.py | 8 ++-- ...0011_batchcommand_command_batch_command.py | 48 +++++++++---------- 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 869868180..d96963e58 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -189,6 +189,7 @@ def validate(self, data): class BatchCommandSerializer(BaseSerializer): device_count = serializers.IntegerField(source="devices.count", read_only=True) + skipped_devices = serializers.JSONField(read_only=True) class Meta: model = BatchCommand @@ -201,6 +202,7 @@ class Meta: "group", "location", "device_count", + "skipped_devices", "created", "modified", ) @@ -216,10 +218,6 @@ class BatchCommandDetailSerializer(BatchCommandSerializer): read_only=True, pk_field=serializers.UUIDField(format="hex_verbose"), ) - skipped_devices = serializers.JSONField(read_only=True) class Meta(BatchCommandSerializer.Meta): - fields = BatchCommandSerializer.Meta.fields + ( - "devices", - "skipped_devices", - ) + fields = BatchCommandSerializer.Meta.fields + ("devices",) 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 41120aad2..6417b98bb 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -1,17 +1,16 @@ -# Generated by Django 5.2.15 on 2026-06-14 18:00 +# Generated by Django 5.2.15 on 2026-06-23 13:40 import uuid -import django import django.core.serializers.json import django.db.migrations.operations.special import django.db.models.deletion import django.utils.timezone import model_utils.fields -import swapper +from django.conf import settings from django.db import migrations, models -from openwisp_controller import connection as connection_config +import openwisp_controller.connection.commands from . import assign_batchcommand_permissions_to_groups @@ -20,9 +19,10 @@ class Migration(migrations.Migration): dependencies = [ ("connection", "0010_replace_jsonfield_with_django_builtin"), - ("openwisp_users", "0022_user_expiration_date"), - ("config", "0063_replace_jsonfield_with_django_builtin"), - ("geo", "0006_create_geo_settings_for_existing_orgs"), + ("openwisp_users", "0024_apikey"), + migrations.swappable_dependency(settings.CONFIG_DEVICEGROUP_MODEL), + migrations.swappable_dependency(settings.CONFIG_DEVICE_MODEL), + migrations.swappable_dependency(settings.GEO_LOCATION_MODEL), ] operations = [ @@ -70,12 +70,8 @@ class Migration(migrations.Migration): ( "type", models.CharField( + choices=openwisp_controller.connection.commands.get_command_choices, # noqa E501 max_length=16, - choices=( - connection_config.commands.COMMAND_CHOICES - if django.VERSION < (5, 0) - else connection_config.commands.get_command_choices - ), ), ), ( @@ -86,25 +82,25 @@ class Migration(migrations.Migration): null=True, ), ), - ( - "devices", - models.ManyToManyField( - blank=True, - to=swapper.get_model_name("config", "Device"), - verbose_name="devices", - ), - ), ( "skipped_devices", models.JSONField( blank=True, - null=True, default=dict, - verbose_name="Skipped devices", help_text=( "Maps device UUIDs to validation error messages for " "devices that were skipped during command creation." ), + null=True, + verbose_name="Skipped devices", + ), + ), + ( + "devices", + models.ManyToManyField( + blank=True, + to=settings.CONFIG_DEVICE_MODEL, + verbose_name="devices", ), ), ( @@ -113,7 +109,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to=swapper.get_model_name("config", "DeviceGroup"), + to=settings.CONFIG_DEVICEGROUP_MODEL, verbose_name="device group", ), ), @@ -123,7 +119,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to=swapper.get_model_name("geo", "Location"), + to=settings.GEO_LOCATION_MODEL, verbose_name="location", ), ), @@ -133,7 +129,7 @@ class Migration(migrations.Migration): blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, - to=swapper.get_model_name("openwisp_users", "Organization"), + to="openwisp_users.organization", ), ), ], @@ -152,7 +148,7 @@ class Migration(migrations.Migration): null=True, on_delete=django.db.models.deletion.SET_NULL, related_name="batch_commands", - to=swapper.get_model_name("connection", "BatchCommand"), + to=settings.CONNECTION_BATCHCOMMAND_MODEL, ), ), migrations.RunPython( From 030a6f92cfd0af7172fe7627cbc7db7ced1849ee Mon Sep 17 00:00:00 2001 From: dee077 Date: Thu, 25 Jun 2026 05:30:55 +0530 Subject: [PATCH 12/20] [tests] Add more tests --- openwisp_controller/connection/api/views.py | 6 +- openwisp_controller/connection/tasks.py | 16 +- .../connection/tests/test_api.py | 164 +++++++++++++++++- .../connection/tests/test_models.py | 89 ++++++++++ .../connection/tests/test_tasks.py | 91 ++++++++++ 5 files changed, 361 insertions(+), 5 deletions(-) diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index 0a4912d76..4559693ac 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -202,6 +202,9 @@ def get_object(self): return obj +batch_command_execute_view = BatchCommandExecuteView.as_view() +batch_command_list_view = BatchCommandListView.as_view() +batch_command_detail_view = BatchCommandDetailView.as_view() command_list_create_view = CommandListCreateView.as_view() command_details_view = CommandDetailsView.as_view() credential_list_create_view = CredentialListCreateView.as_view() @@ -211,6 +214,3 @@ def get_object(self): # TODO: remove in version 1.4 deviceconnection_details_view = deviceconnection_detail_view -batch_command_execute_view = BatchCommandExecuteView.as_view() -batch_command_list_view = BatchCommandListView.as_view() -batch_command_detail_view = BatchCommandDetailView.as_view() diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 2ad4b153a..9e64e5ab5 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -106,7 +106,21 @@ def launch_batch_command(self, batch_id): except BatchCommand.DoesNotExist: logger.warning(f"The BatchCommand object with id {batch_id} has been deleted") return - batch.create_commands() + try: + batch.create_commands() + except SoftTimeLimitExceeded: + batch.status = "failed" + batch.save(update_fields=["status"]) + logger.warning( + f"SoftTimeLimitExceeded raised in launch_batch_command " + f"for batch {batch_id}" + ) + except Exception as e: + batch.status = "failed" + batch.save(update_fields=["status"]) + logger.exception( + f"An exception was raised while executing batch command {batch_id}" + ) @shared_task(soft_time_limit=3600) diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 4df565d53..6c68a00d2 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -15,7 +15,7 @@ from openwisp_users.tests.test_api import AuthenticationMixin from .. import settings as app_settings -from ..api.views import CommandListCreateView +from ..api.views import BatchCommandListView, CommandListCreateView from ..commands import ORGANIZATION_ENABLED_COMMANDS from .utils import CreateCommandMixin, CreateConnectionsMixin @@ -913,6 +913,24 @@ def test_batch_command_list(self): self.assertIn("created", result) self.assertEqual(result["device_count"], 0) + with patch.object(BatchCommandListView, "pagination_page_size", 2, create=True): + with self.subTest("pagination page 1"): + for _ in range(2): + self._create_batch_command(organization=org) + response = self.client.get(url + "?page=1") + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data["results"]), 2) + + with self.subTest("pagination page 2"): + response = self.client.get(url + "?page=2") + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data["results"]), 2) + + with self.subTest("pagination page 3 (partial)"): + response = self.client.get(url + "?page=3") + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data["results"]), 1) + def test_batch_command_detail(self): org = self._get_org() device = self._create_device(organization=org) @@ -1051,8 +1069,10 @@ def test_batch_command_execute(self): self.assertEqual(response.status_code, 201) batch = BatchCommand.objects.get(pk=response.data["batch"]) batch.create_commands() + batch.refresh_from_db() command = Command.objects.get(batch_command=batch, device=device1) self.assertEqual(command.device.pk, device1.pk) + self.assertEqual(batch.skipped_devices, {}) with self.subTest("execute with group"): response = self.client.post( @@ -1070,8 +1090,10 @@ def test_batch_command_execute(self): self.assertEqual(response.status_code, 201) batch = BatchCommand.objects.get(pk=response.data["batch"]) batch.create_commands() + batch.refresh_from_db() command = Command.objects.get(batch_command=batch, device=device1) self.assertEqual(command.device.pk, device1.pk) + self.assertEqual(batch.skipped_devices, {}) with self.subTest("execute with location"): response = self.client.post( @@ -1089,8 +1111,10 @@ def test_batch_command_execute(self): self.assertEqual(response.status_code, 201) batch = BatchCommand.objects.get(pk=response.data["batch"]) batch.create_commands() + batch.refresh_from_db() command = Command.objects.get(batch_command=batch, device=device2) self.assertEqual(command.device.pk, device2.pk) + self.assertEqual(batch.skipped_devices, {}) with self.subTest("execute with group and location"): DeviceLocation.objects.create(content_object=device1, location=location) @@ -1110,8 +1134,10 @@ def test_batch_command_execute(self): self.assertEqual(response.status_code, 201) batch = BatchCommand.objects.get(pk=response.data["batch"]) batch.create_commands() + batch.refresh_from_db() command = Command.objects.get(batch_command=batch, device=device1) self.assertEqual(command.device.pk, device1.pk) + self.assertEqual(batch.skipped_devices, {}) with self.subTest("execute org-wide"): response = self.client.post( @@ -1129,10 +1155,12 @@ def test_batch_command_execute(self): self.assertEqual(response.status_code, 201) batch = BatchCommand.objects.get(pk=response.data["batch"]) batch.create_commands() + batch.refresh_from_db() self.assertEqual( Command.objects.filter(batch_command=batch).count(), 2, ) + self.assertEqual(batch.skipped_devices, {}) def test_batch_command_endpoints_no_of_queries(self): with self.subTest("list queries"): @@ -1215,6 +1243,30 @@ def test_batch_command_execute_org_has_no_devices(self): ["No devices match the specified criteria."], ) + def test_batch_command_execute_disallowed_type(self): + org = self._get_org() + url = reverse("connection_api:batch_command_execute") + with patch.dict( + ORGANIZATION_ENABLED_COMMANDS, + {str(org.pk): ("reboot",)}, + ): + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertIn( + '"custom" command is not available for this organization', + response.data["type"][0], + ) + def test_batch_command_no_org_only_allowed_to_superuser(self): org = self._get_org() self.client.logout() @@ -1310,6 +1362,116 @@ def test_batch_command_endpoints_unauthorized(self): ) self.assertEqual(response.status_code, 401) + def test_batch_command_bearer_authentication(self): + self.client.logout() + org = self._get_org() + batch = self._create_batch_command(organization=org) + device = self._create_device( + name="bearer-dev", + mac_address="00:11:22:33:44:be", + organization=org, + ) + self._create_config(device=device) + self._create_device_connection(device=device) + token = self._obtain_auth_token(username="admin", password="tester") + auth = {"HTTP_AUTHORIZATION": f"Bearer {token}"} + + with self.subTest("list"): + url = reverse("connection_api:batch_command_list") + response = self.client.get(url, **auth) + self.assertEqual(response.status_code, 200) + self.assertIn("results", response.data) + + with self.subTest("detail"): + url = reverse( + "connection_api:batch_command_detail", + args=[batch.pk], + ) + response = self.client.get(url, **auth) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["id"], str(batch.pk)) + + with self.subTest("dry-run"): + url = reverse("connection_api:batch_command_execute") + response = self.client.get( + url, + data={"organization": str(org.pk)}, + **auth, + ) + self.assertEqual(response.status_code, 200) + + with self.subTest("execute"): + url = reverse("connection_api:batch_command_execute") + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + **auth, + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + + def test_batch_command_detail_404(self): + org = self._get_org() + self._create_batch_command(organization=org) + url = reverse( + "connection_api:batch_command_detail", + args=[uuid.uuid4()], + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + def test_batch_command_endpoints_operator_different_org(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + batch_org1 = self._create_batch_command(organization=org) + self.client.logout() + operator = self._create_operator(organizations=[org2]) + self.client.force_login(operator) + + with self.subTest("list"): + url = reverse("connection_api:batch_command_list") + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["count"], 0) + + with self.subTest("detail"): + url = reverse( + "connection_api:batch_command_detail", + args=[batch_org1.pk], + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 404) + + with self.subTest("dry-run"): + url = reverse("connection_api:batch_command_execute") + response = self.client.get( + url, + data={"organization": str(org.pk)}, + ) + self.assertEqual(response.status_code, 400) + + with self.subTest("execute"): + url = reverse("connection_api:batch_command_execute") + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + def test_batch_command_execute_skipped_devices(self): org = self._get_org() org2 = self._create_org(name="org2", slug="org2") diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 5e6e29e6f..1e96aff79 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -1175,6 +1175,95 @@ def test_batch_command_create_commands_no_credentials(self): batch.skipped_devices[str(device.pk)][0], ) + def test_batch_command_create_commands_skip_scenarios(self): + org = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + + with self.subTest("org command not allowed"): + device_a = self._create_device( + name="device-a", + mac_address="00:11:22:33:44:aa", + organization=org, + ) + self._create_config(device=device_a) + self._create_device_connection(device=device_a) + device_b = self._create_device( + name="device-b", + mac_address="00:11:22:33:44:bb", + organization=org2, + ) + self._create_config(device=device_b) + self._create_device_connection(device=device_b) + with mock.patch.dict( + ORGANIZATION_ENABLED_COMMANDS, + {str(org2.pk): ("reboot",)}, + ): + batch = self._create_batch_command( + organization=org, + devices=[device_a, device_b], + ) + batch.create_commands() + batch.refresh_from_db() + 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], + ) + db_batch = BatchCommand.objects.get(pk=batch.pk) + self.assertEqual(batch.skipped_devices, db_batch.skipped_devices) + command_qs = Command.objects.filter(batch_command=batch) + self.assertTrue(command_qs.filter(device=device_a).exists()) + self.assertFalse(command_qs.filter(device=device_b).exists()) + + with self.subTest("mixed skip scenario"): + device_ok = self._create_device( + name="device-ok", + mac_address="00:11:22:33:44:01", + organization=org, + ) + self._create_config(device=device_ok) + ok_cred = self._create_credentials(name="device-ok-cred", organization=org) + self._create_device_connection(device=device_ok, credentials=ok_cred) + device_no_creds = self._create_device( + name="device-no-creds", + mac_address="00:11:22:33:44:02", + organization=org, + ) + self._create_config(device=device_no_creds) + device_deactivated = self._create_device( + name="device-deactivated", + mac_address="00:11:22:33:44:03", + organization=org, + ) + self._create_config(device=device_deactivated) + dd_cred = self._create_credentials(name="device-dd-cred", organization=org) + self._create_device_connection( + device=device_deactivated, credentials=dd_cred + ) + device_deactivated.deactivate() + batch = self._create_batch_command( + organization=org, + devices=[device_ok, device_no_creds, device_deactivated], + ) + batch.create_commands() + batch.refresh_from_db() + command_qs = Command.objects.filter(batch_command=batch) + self.assertEqual(command_qs.count(), 1) + self.assertTrue(command_qs.filter(device=device_ok).exists()) + 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], + ) + self.assertIn(str(device_deactivated.pk), batch.skipped_devices) + self.assertIn( + "Device is deactivated", + batch.skipped_devices[str(device_deactivated.pk)][0], + ) + self.assertNotIn(str(device_ok.pk), batch.skipped_devices) + db_batch = BatchCommand.objects.get(pk=batch.pk) + self.assertEqual(batch.skipped_devices, db_batch.skipped_devices) + def test_batch_command_resolve_devices(self): org = self._get_org() device1 = self._create_device( diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index 2176500c9..8fc356e08 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -380,3 +380,94 @@ def test_launch_batch_command_creates_commands_for_multiple_devices( self.assertIn(d, cmd_devices) self.assertEqual(batch.status, "in-progress") self.assertEqual(mocked_delay.call_count, 2) + + @mock.patch( + "openwisp_controller.connection.base.models.AbstractBatchCommand" + ".create_commands", + side_effect=SoftTimeLimitExceeded(), + ) + def test_launch_batch_command_timeout(self, mocked_create_commands): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.full_clean() + batch.save() + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.status, "failed") + + @mock.patch( + "openwisp_controller.connection.base.models.AbstractBatchCommand" + ".create_commands", + side_effect=RuntimeError("test error"), + ) + def test_launch_batch_command_exception(self, mocked_create_commands): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.full_clean() + batch.save() + with redirect_stderr(StringIO()) as stderr: + tasks.launch_batch_command(batch_id=batch.pk) + self.assertIn( + f"An exception was raised while executing batch " f"command {batch.pk}", + stderr.getvalue(), + ) + batch.refresh_from_db() + self.assertEqual(batch.status, "failed") + + @mock.patch("openwisp_controller.connection.tasks.launch_command.delay") + def test_launch_batch_command_all_devices_skipped(self, mocked_delay): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + device.deactivate() + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.full_clean() + batch.save() + batch.devices.set([device]) + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.batch_commands.count(), 0) + self.assertIn(str(device.pk), batch.skipped_devices) + self.assertEqual(batch.status, "idle") + mocked_delay.assert_not_called() + + @mock.patch("openwisp_controller.connection.tasks.launch_command.delay") + def test_launch_batch_command_already_processed(self, mocked_delay): + org = self._get_org() + device = self._create_device(organization=org) + self._create_config(device=device) + self._create_device_connection(device=device) + batch = BatchCommand( + organization=org, + type="custom", + input={"command": "echo test"}, + ) + batch.full_clean() + batch.save() + batch.devices.set([device]) + # First call — creates commands + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.batch_commands.count(), 1) + first_call_count = mocked_delay.call_count + # Second call — should be a no-op (idempotency guard) + tasks.launch_batch_command(batch_id=batch.pk) + batch.refresh_from_db() + self.assertEqual(batch.batch_commands.count(), 1) + self.assertEqual(mocked_delay.call_count, first_call_count) From 22dd0fda318623e46d5494a373216deffefc72a2 Mon Sep 17 00:00:00 2001 From: dee077 Date: Thu, 25 Jun 2026 20:31:59 +0530 Subject: [PATCH 13/20] [fix] Address coderabbit comments and fix flaky tests and ci --- .../connection/api/serializers.py | 29 ++++++++++++------- openwisp_controller/connection/api/views.py | 12 ++++++-- openwisp_controller/connection/base/models.py | 6 +++- ...0011_batchcommand_command_batch_command.py | 8 ++++- openwisp_controller/connection/tasks.py | 2 +- .../connection/tests/test_api.py | 6 ++-- .../connection/tests/test_selenium.py | 6 +++- .../connection/tests/test_tasks.py | 2 +- 8 files changed, 50 insertions(+), 21 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index d96963e58..c023055a9 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -136,11 +136,13 @@ class BatchCommandExecuteSerializer( ) execute_all = serializers.BooleanField(required=False, default=True) - def __init__(self, *args, **kwargs): + def __init__(self, *args, dry_run=False, **kwargs): super().__init__(*args, **kwargs) - request = self.context.get("request") - if request and request.method == "GET": + if dry_run: + self._skip_target_validation = True self.fields["type"].required = False + else: + self._skip_target_validation = False class Meta: model = BatchCommand @@ -167,13 +169,20 @@ def validate(self, data): raise serializers.ValidationError( _("Only superusers can execute batch commands without an organization.") ) - if not execute_all and not org and not devices and not group and not location: - raise serializers.ValidationError( - _( - "Specify at least one targeting option " - "or set execute_all to true." + if not self._skip_target_validation: + if ( + not execute_all + and not org + and not devices + and not group + and not location + ): + raise serializers.ValidationError( + _( + "Specify at least one targeting option " + "or set execute_all to true." + ) ) - ) if devices: for device in devices: if org and device.organization_id != org.id: @@ -188,7 +197,7 @@ def validate(self, data): class BatchCommandSerializer(BaseSerializer): - device_count = serializers.IntegerField(source="devices.count", read_only=True) + device_count = serializers.IntegerField(read_only=True) skipped_devices = serializers.JSONField(read_only=True) class Meta: diff --git a/openwisp_controller/connection/api/views.py b/openwisp_controller/connection/api/views.py index 4559693ac..41625ad68 100644 --- a/openwisp_controller/connection/api/views.py +++ b/openwisp_controller/connection/api/views.py @@ -1,4 +1,5 @@ from django.core.exceptions import ValidationError +from django.db.models import Count from django.utils.translation import gettext_lazy as _ from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema @@ -166,7 +167,10 @@ def post(self, request): return Response({"batch": str(batch.pk)}, status=201) def get(self, request): - serializer = self.get_serializer(data=request.query_params) + serializer = self.get_serializer( + data=request.query_params, + dry_run=True, + ) serializer.is_valid(raise_exception=True) serializer.validated_data.pop("execute_all", None) try: @@ -181,13 +185,15 @@ def get(self, request): class BatchCommandListView(ProtectedAPIMixin, ListAPIView): - queryset = BatchCommand.objects.all().order_by("-created") + queryset = BatchCommand.objects.annotate(device_count=Count("devices")).order_by( + "-created" + ) serializer_class = BatchCommandSerializer pagination_class = OpenWispPagination class BatchCommandDetailView(ProtectedAPIMixin, RetrieveAPIView): - queryset = BatchCommand.objects.all() + queryset = BatchCommand.objects.annotate(device_count=Count("devices")) serializer_class = BatchCommandDetailSerializer diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 50feba84d..b3376eefe 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -895,7 +895,11 @@ def execute(cls, **kwargs): batch.save() if devices_list: batch.devices.set(devices_list) - batch._validate_org_relations() + try: + batch._validate_org_relations() + except ValidationError: + batch.delete() + raise if not batch.devices.exists(): batch.delete() raise ValidationError( 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 6417b98bb..d437b494a 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -2,6 +2,7 @@ import uuid +import django import django.core.serializers.json import django.db.migrations.operations.special import django.db.models.deletion @@ -23,6 +24,7 @@ class Migration(migrations.Migration): migrations.swappable_dependency(settings.CONFIG_DEVICEGROUP_MODEL), migrations.swappable_dependency(settings.CONFIG_DEVICE_MODEL), migrations.swappable_dependency(settings.GEO_LOCATION_MODEL), + ("config", "0036_device_group"), ] operations = [ @@ -70,7 +72,11 @@ class Migration(migrations.Migration): ( "type", models.CharField( - choices=openwisp_controller.connection.commands.get_command_choices, # noqa E501 + choices=( + openwisp_controller.connection.commands.COMMAND_CHOICES + if django.VERSION < (5, 0) + else openwisp_controller.connection.commands.get_command_choices # noqa: E501 + ), max_length=16, ), ), diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 9e64e5ab5..3ecb76916 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -119,7 +119,7 @@ def launch_batch_command(self, batch_id): batch.status = "failed" batch.save(update_fields=["status"]) logger.exception( - f"An exception was raised while executing batch command {batch_id}" + f"An exception was raised while executing batch " f"command {batch_id}: {e}" ) diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 6c68a00d2..7568ec721 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -1176,7 +1176,7 @@ def test_batch_command_endpoints_no_of_queries(self): devices.append(d) self._create_batch_command(organization=org, devices=devices) url = reverse("connection_api:batch_command_list") - with self.assertNumQueries(4): + with self.assertNumQueries(3): response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertEqual(response.data["count"], 1) @@ -1186,7 +1186,7 @@ def test_batch_command_endpoints_no_of_queries(self): "connection_api:batch_command_detail", args=[response.data["results"][0]["id"]], ) - with self.assertNumQueries(4): + with self.assertNumQueries(3): response = self.client.get(url) self.assertEqual(response.status_code, 200) @@ -1196,7 +1196,7 @@ def test_batch_command_endpoints_no_of_queries(self): for i in range(3): d = self._create_device( name=f"q-exec-{i}", - mac_address=f"00:11:22:33:44:{i+0x10:02x}", + mac_address=f"00:11:22:33:44:{i + 0x10:02x}", organization=org, ) self._create_config(device=d) diff --git a/openwisp_controller/connection/tests/test_selenium.py b/openwisp_controller/connection/tests/test_selenium.py index e75af19d3..c7fde9dfb 100644 --- a/openwisp_controller/connection/tests/test_selenium.py +++ b/openwisp_controller/connection/tests/test_selenium.py @@ -1,3 +1,5 @@ +from time import sleep + from django.contrib.staticfiles.testing import StaticLiveServerTestCase from django.test import tag from django.urls import reverse @@ -53,7 +55,9 @@ def test_command_widget_on_device(self): by=By.CSS_SELECTOR, value='button.ow-command-btn[data-command="reboot"]' ).click() self.find_element(by=By.CSS_SELECTOR, value="#ow-command-confirm-yes").click() - + # Wait for the redirect triggered by command submission to complete. + # Navigating away immediately can race with the redirect + sleep(0.5) self.assertEqual(Command.objects.count(), 1) # TODO: Selenium tests do not support websocket connections. # Thus, we need to refresh the page. Remove this when support for diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index 8fc356e08..cb901c29d 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -358,7 +358,7 @@ def test_launch_batch_command_creates_commands_for_multiple_devices( for i in range(2): d = self._create_device( name=f"task-dev-{i}", - mac_address=f"00:11:22:33:44:{i+0x50:02x}", + mac_address=f"00:11:22:33:44:{i + 0x50:02x}", organization=org, ) self._create_config(device=d) From 4475e7fe10c0c3ec5f28df4444d1ab4d99746da2 Mon Sep 17 00:00:00 2001 From: dee077 Date: Sat, 27 Jun 2026 04:43:41 +0530 Subject: [PATCH 14/20] [fix] Address comments --- .../connection/api/serializers.py | 1 - openwisp_controller/connection/base/models.py | 84 ++++++++----------- openwisp_controller/connection/tasks.py | 9 +- .../connection/tests/test_api.py | 18 ++-- .../connection/tests/test_models.py | 18 ++-- .../connection/tests/test_tasks.py | 1 + 6 files changed, 59 insertions(+), 72 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index c023055a9..665d71660 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -125,7 +125,6 @@ class Meta: class BatchCommandExecuteSerializer( FilterSerializerByOrgManaged, serializers.ModelSerializer ): - type = serializers.CharField() input = serializers.JSONField(allow_null=True, required=False) devices = serializers.PrimaryKeyRelatedField( many=True, diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index b3376eefe..9f63b60b5 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -15,6 +15,7 @@ from swapper import get_model_name, load_model from openwisp_controller.config.base.base import BaseModel +from openwisp_users.mixins import ValidateOrgMixin from openwisp_utils.base import TimeStampedEditableModel from ...base import ShareableOrgMixinUniqueName @@ -734,7 +735,7 @@ def _enforce_not_custom(self): ) -class AbstractBatchCommand(TimeStampedEditableModel): +class AbstractBatchCommand(ValidateOrgMixin, TimeStampedEditableModel): STATUS_CHOICES = ( ("idle", _("idle")), ("in-progress", _("in progress")), @@ -809,27 +810,23 @@ def successful(self): def failed(self): return self.batch_commands.filter(status="failed").count() - def _validate_org_relations(self): - if not self.organization_id: - return - if self.group and self.group.organization != self.organization: - raise ValidationError( - { - "group": _( - "The organization of the group doesn't match " - "the organization of the batch command operation" - ) - } - ) - if self.location and self.location.organization != self.organization: + @staticmethod + def _validate_device_org(device, organization_id): + if organization_id and device.organization_id != organization_id: raise ValidationError( { - "location": _( - "The organization of the location doesn't match " - "the organization of the batch command operation" + "devices": _( + "All devices must belong to the same " + "organization as the batch command." ) } ) + + def _validate_org_relations(self): + if not self.organization_id: + return + self._validate_org_relation("group", field_error="group") + self._validate_org_relation("location", field_error="location") if self.pk and self.devices.exists(): org_mismatch = self.devices.exclude(organization=self.organization).exists() if org_mismatch: @@ -891,27 +888,22 @@ def execute(cls, **kwargs): """ devices_list = kwargs.pop("devices", None) batch = cls(**kwargs) - batch.full_clean() - batch.save() - if devices_list: - batch.devices.set(devices_list) - try: + with transaction.atomic(): + batch.full_clean() + batch.save() + if devices_list: + batch.devices.set(devices_list) batch._validate_org_relations() - except ValidationError: - batch.delete() - raise - if not batch.devices.exists(): - batch.delete() + if not batch.devices.exists(): + raise ValidationError( + _("No devices match the specified criteria."), + ) + elif not any(batch.resolve_devices()): raise ValidationError( _("No devices match the specified criteria."), ) - elif not any(batch.resolve_devices()): - batch.delete() - raise ValidationError( - _("No devices match the specified criteria."), - ) - batch.status = "in-progress" - batch.save(update_fields=["status"]) + batch.status = "in-progress" + batch.save(update_fields=["status"]) transaction.on_commit(lambda: launch_batch_command.delay(batch.pk)) return batch @@ -932,17 +924,7 @@ def dry_run(cls, **kwargs): batch._validate_org_relations() if devices_list: for device in devices_list: - if ( - batch.organization_id - and device.organization_id != batch.organization_id - ): - raise ValidationError( - { - "devices": _( - "All devices must belong to the same organization" - ) - } - ) + cls._validate_device_org(device, batch.organization_id) return {"devices": list(devices_list)} return {"devices": list(batch.resolve_devices())} @@ -967,9 +949,15 @@ def create_commands(self): batch_command=self, ) try: - command.save() - except ValidationError as e: - self.skipped_devices[str(device.pk)] = e.messages + # Validate before the atomic block so errors like + # ValidationError don't create/rollback + command.full_clean() + with transaction.atomic(): + command.save() + except Exception as e: + self.skipped_devices[str(device.pk)] = ( + 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/tasks.py b/openwisp_controller/connection/tasks.py index 3ecb76916..22574e6ed 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -98,7 +98,7 @@ def launch_command(command_id): command._save_without_resurrecting() -@shared_task(bind=True, soft_time_limit=app_settings.SSH_COMMAND_TIMEOUT * 1.2) +@shared_task(bind=True) def launch_batch_command(self, batch_id): BatchCommand = load_model("connection", "BatchCommand") try: @@ -108,13 +108,6 @@ def launch_batch_command(self, batch_id): return try: batch.create_commands() - except SoftTimeLimitExceeded: - batch.status = "failed" - batch.save(update_fields=["status"]) - logger.warning( - f"SoftTimeLimitExceeded raised in launch_batch_command " - f"for batch {batch_id}" - ) except Exception as e: batch.status = "failed" batch.save(update_fields=["status"]) diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 7568ec721..edac7f6e0 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -1208,7 +1208,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(15): + with self.assertNumQueries(17): response = self.client.post( url, data=json.dumps(payload), @@ -1659,8 +1659,8 @@ def test_batch_command_execute_org_mismatched_data_provided(self): { "group": [ ( - "The organization of the group doesn't match " - "the organization of the batch command operation" + "Please ensure that the organization of this Batch command " + "and the organization of the related Device Group match." ) ] }, @@ -1690,8 +1690,8 @@ def test_batch_command_execute_org_mismatched_data_provided(self): { "location": [ ( - "The organization of the location doesn't match " - "the organization of the batch command operation" + "Please ensure that the organization of this Batch command " + "and the organization of the related location match." ) ] }, @@ -1737,8 +1737,8 @@ def test_batch_command_dry_run_org_mismatched_data_provided(self): { "group": [ ( - "The organization of the group doesn't match " - "the organization of the batch command operation" + "Please ensure that the organization of this Batch command " + "and the organization of the related Device Group match." ) ] }, @@ -1762,8 +1762,8 @@ def test_batch_command_dry_run_org_mismatched_data_provided(self): { "location": [ ( - "The organization of the location doesn't match " - "the organization of the batch command operation" + "Please ensure that the organization of this Batch command " + "and the organization of the related location match." ) ] }, diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 1e96aff79..e1f9780f0 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -1105,7 +1105,8 @@ def test_batch_command_clean_validation(self): batch.clean() self.assertIn("group", ctx.exception.message_dict) self.assertIn( - "The organization of the group doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related Device Group match", ctx.exception.message_dict["group"][0], ) @@ -1125,7 +1126,8 @@ def test_batch_command_clean_validation(self): batch.clean() self.assertIn("location", ctx.exception.message_dict) self.assertIn( - "The organization of the location doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related location match", ctx.exception.message_dict["location"][0], ) @@ -1553,7 +1555,8 @@ def test_batch_command_execute_org_mismatch(self): ) self.assertIn("group", ctx.exception.message_dict) self.assertIn( - "The organization of the group doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related Device Group match", ctx.exception.message_dict["group"][0], ) @@ -1572,7 +1575,8 @@ def test_batch_command_execute_org_mismatch(self): ) self.assertIn("location", ctx.exception.message_dict) self.assertIn( - "The organization of the location doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related location match", ctx.exception.message_dict["location"][0], ) @@ -1611,7 +1615,8 @@ def test_batch_command_dry_run_org_mismatch(self): ) self.assertIn("group", ctx.exception.message_dict) self.assertIn( - "The organization of the group doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related Device Group match", ctx.exception.message_dict["group"][0], ) @@ -1630,7 +1635,8 @@ def test_batch_command_dry_run_org_mismatch(self): ) self.assertIn("location", ctx.exception.message_dict) self.assertIn( - "The organization of the location doesn't match", + "Please ensure that the organization of this Batch command " + "and the organization of the related location match", ctx.exception.message_dict["location"][0], ) diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index cb901c29d..aaa3a6c42 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -432,6 +432,7 @@ def test_launch_batch_command_all_devices_skipped(self, mocked_delay): device = self._create_device(organization=org) self._create_config(device=device) device.deactivate() + device.config.set_status_deactivated() batch = BatchCommand( organization=org, type="custom", From 98043a089998df05c1df3761e7b798e415b12c29 Mon Sep 17 00:00:00 2001 From: dee077 Date: Sun, 28 Jun 2026 23:34:59 +0530 Subject: [PATCH 15/20] [fix] Additional tests --- .../connection/tests/test_api.py | 216 ++++++++++++++++++ .../connection/tests/test_selenium.py | 6 +- 2 files changed, 217 insertions(+), 5 deletions(-) diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index edac7f6e0..577748425 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -1300,6 +1300,222 @@ def test_batch_command_no_org_only_allowed_to_superuser(self): str(response.data), ) + def test_batch_command_operator_endpoints_on_managed_org(self): + org = self._get_org() + self._create_credentials(name="op-cred", organization=org) + device = self._create_device( + name="op-dev", + mac_address="00:11:22:33:44:01", + organization=org, + ) + self._create_config(device=device) + self._create_device_connection(device=device) + self.client.logout() + operator = self._create_operator(organizations=[org]) + operator.user_permissions.add( + Permission.objects.get(codename="add_batchcommand"), + Permission.objects.get(codename="view_batchcommand"), + ) + self.client.force_login(operator) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("execute"): + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + + with self.subTest("dry-run"): + response = self.client.get( + url, + data={"organization": str(org.pk)}, + ) + self.assertEqual(response.status_code, 200) + self.assertIn("devices", response.data) + + with self.subTest("execute_all"): + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + + def test_batch_command_administrator_endpoints_on_managed_org(self): + org = self._get_org() + self._create_credentials(name="admin-cred", organization=org) + device = self._create_device( + name="admin-dev", + mac_address="00:11:22:33:44:02", + organization=org, + ) + self._create_config(device=device) + self._create_device_connection(device=device) + self.client.logout() + administrator = self._create_administrator(organizations=[org]) + self.client.force_login(administrator) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("execute"): + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + + with self.subTest("dry-run"): + response = self.client.get( + url, + data={"organization": str(org.pk)}, + ) + self.assertEqual(response.status_code, 200) + self.assertIn("devices", response.data) + + with self.subTest("execute_all"): + payload = { + "organization": str(org.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 201) + self.assertIn("batch", response.data) + + def test_batch_command_operator_endpoints_on_non_managed_org(self): + org1 = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + self._create_credentials(name="op-nm-cred", organization=org1) + device = self._create_device( + name="op-nm-dev", + mac_address="00:11:22:33:44:03", + organization=org1, + ) + self._create_config(device=device) + self._create_device_connection(device=device) + self.client.logout() + operator = self._create_operator(organizations=[org1]) + operator.user_permissions.add( + Permission.objects.get(codename="add_batchcommand"), + Permission.objects.get(codename="view_batchcommand"), + ) + self.client.force_login(operator) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("execute"): + payload = { + "organization": str(org2.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + + with self.subTest("dry-run"): + response = self.client.get( + url, + data={"organization": str(org2.pk)}, + ) + self.assertEqual(response.status_code, 400) + + with self.subTest("execute_all"): + payload = { + "organization": str(org2.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + + def test_batch_command_administrator_endpoints_on_non_managed_org(self): + org1 = self._get_org() + org2 = self._create_org(name="org2", slug="org2") + self._create_credentials(name="admin-nm-cred", organization=org1) + device = self._create_device( + name="admin-nm-dev", + mac_address="00:11:22:33:44:04", + organization=org1, + ) + self._create_config(device=device) + self._create_device_connection(device=device) + self.client.logout() + administrator = self._create_administrator(organizations=[org1]) + self.client.force_login(administrator) + url = reverse("connection_api:batch_command_execute") + + with self.subTest("execute"): + payload = { + "organization": str(org2.pk), + "type": "custom", + "input": {"command": "echo test"}, + "devices": [str(device.pk)], + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + + with self.subTest("dry-run"): + response = self.client.get( + url, + data={"organization": str(org2.pk)}, + ) + self.assertEqual(response.status_code, 400) + + with self.subTest("execute_all"): + payload = { + "organization": str(org2.pk), + "type": "custom", + "input": {"command": "echo test"}, + "execute_all": True, + } + response = self.client.post( + url, + data=json.dumps(payload), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + def test_batch_command_endpoints_organization_scoped(self): org = self._get_org() org2 = self._create_org(name="org2", slug="org2") diff --git a/openwisp_controller/connection/tests/test_selenium.py b/openwisp_controller/connection/tests/test_selenium.py index c7fde9dfb..644b0154f 100644 --- a/openwisp_controller/connection/tests/test_selenium.py +++ b/openwisp_controller/connection/tests/test_selenium.py @@ -57,11 +57,7 @@ def test_command_widget_on_device(self): self.find_element(by=By.CSS_SELECTOR, value="#ow-command-confirm-yes").click() # Wait for the redirect triggered by command submission to complete. # Navigating away immediately can race with the redirect - sleep(0.5) - self.assertEqual(Command.objects.count(), 1) - # TODO: Selenium tests do not support websocket connections. - # Thus, we need to refresh the page. Remove this when support for - # websockets is added. + sleep(0.3) self.open(path) self.wait_for_visibility( By.CSS_SELECTOR, From 9fe526bee60d404aa668db6dcdd79e92859ab2e6 Mon Sep 17 00:00:00 2001 From: dee077 Date: Wed, 1 Jul 2026 15:00:10 +0530 Subject: [PATCH 16/20] [feature] Add label and notes field in model --- .../connection/api/serializers.py | 5 ++++ openwisp_controller/connection/base/models.py | 20 +++++++++++--- ...0011_batchcommand_command_batch_command.py | 17 ++++++++++++ .../connection/tests/test_api.py | 26 +++++++++++++++++++ .../connection/tests/test_models.py | 24 ++++++++++++----- .../connection/tests/test_tasks.py | 6 +++++ ...0005_batchcommand_command_batch_command.py | 17 ++++++++++++ 7 files changed, 104 insertions(+), 11 deletions(-) diff --git a/openwisp_controller/connection/api/serializers.py b/openwisp_controller/connection/api/serializers.py index 665d71660..82b67ffc3 100644 --- a/openwisp_controller/connection/api/serializers.py +++ b/openwisp_controller/connection/api/serializers.py @@ -140,6 +140,7 @@ def __init__(self, *args, dry_run=False, **kwargs): if dry_run: self._skip_target_validation = True self.fields["type"].required = False + self.fields["label"].required = False else: self._skip_target_validation = False @@ -149,6 +150,8 @@ class Meta: "organization", "type", "input", + "label", + "notes", "devices", "group", "location", @@ -207,6 +210,8 @@ class Meta: "status", "type", "input", + "label", + "notes", "group", "location", "device_count", diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 9f63b60b5..2f936a5f9 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -771,6 +771,17 @@ class AbstractBatchCommand(ValidateOrgMixin, TimeStampedEditableModel): null=True, verbose_name=_("location"), ) + label = models.CharField( + max_length=64, + verbose_name=_("label"), + help_text=_("A short label to identify this batch command."), + ) + notes = models.TextField( + blank=True, + default="", + verbose_name=_("notes"), + help_text=_("Optional notes about this batch command."), + ) devices = models.ManyToManyField( get_model_name("config", "Device"), blank=True, @@ -793,10 +804,7 @@ class Meta: verbose_name_plural = _("Batch commands") def __str__(self): - return "{0} ({1})".format( - self.type, - timezone.localtime(self.created).strftime("%Y-%m-%d %H:%M:%S"), - ) + return self.label @cached_property def total_devices(self): @@ -916,9 +924,13 @@ def dry_run(cls, **kwargs): """ devices_list = kwargs.pop("devices", None) cmd_type = kwargs.pop("type", None) + kwargs.pop("label", None) + kwargs.pop("notes", None) batch = cls(**kwargs) if cmd_type: batch.type = cmd_type + if not batch.label: + batch.label = "dry-run" batch.full_clean() else: batch._validate_org_relations() 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 d437b494a..f45dbf74a 100644 --- a/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py +++ b/openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py @@ -129,6 +129,23 @@ class Migration(migrations.Migration): verbose_name="location", ), ), + ( + "label", + models.CharField( + help_text=("A short label to identify this batch command."), + max_length=64, + verbose_name="label", + ), + ), + ( + "notes", + models.TextField( + blank=True, + default="", + verbose_name="notes", + help_text="Optional notes about this batch command.", + ), + ), ( "organization", models.ForeignKey( diff --git a/openwisp_controller/connection/tests/test_api.py b/openwisp_controller/connection/tests/test_api.py index 577748425..f52001af7 100644 --- a/openwisp_controller/connection/tests/test_api.py +++ b/openwisp_controller/connection/tests/test_api.py @@ -880,6 +880,7 @@ def _create_batch_command(self, organization, **kwargs): organization=organization, type="custom", input={"command": "echo test"}, + label="test-label", ) devices = kwargs.pop("devices", None) opts.update(kwargs) @@ -1061,6 +1062,7 @@ def test_batch_command_execute(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device1.pk)], } ), @@ -1082,6 +1084,7 @@ def test_batch_command_execute(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "group": str(group.pk), } ), @@ -1103,6 +1106,7 @@ def test_batch_command_execute(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "location": str(location.pk), } ), @@ -1125,6 +1129,7 @@ def test_batch_command_execute(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "group": str(group.pk), "location": str(location.pk), } @@ -1147,6 +1152,7 @@ def test_batch_command_execute(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } ), @@ -1205,6 +1211,7 @@ def test_batch_command_endpoints_no_of_queries(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(d.pk) for d in devices], } url = reverse("connection_api:batch_command_execute") @@ -1229,6 +1236,7 @@ def test_batch_command_execute_org_has_no_devices(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } url = reverse("connection_api:batch_command_execute") @@ -1254,6 +1262,7 @@ def test_batch_command_execute_disallowed_type(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1277,6 +1286,7 @@ def test_batch_command_no_org_only_allowed_to_superuser(self): payload = { "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } url = reverse("connection_api:batch_command_execute") @@ -1324,6 +1334,7 @@ def test_batch_command_operator_endpoints_on_managed_org(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } response = self.client.post( @@ -1347,6 +1358,7 @@ def test_batch_command_operator_endpoints_on_managed_org(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1377,6 +1389,7 @@ def test_batch_command_administrator_endpoints_on_managed_org(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } response = self.client.post( @@ -1400,6 +1413,7 @@ def test_batch_command_administrator_endpoints_on_managed_org(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1435,6 +1449,7 @@ def test_batch_command_operator_endpoints_on_non_managed_org(self): "organization": str(org2.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } response = self.client.post( @@ -1456,6 +1471,7 @@ def test_batch_command_operator_endpoints_on_non_managed_org(self): "organization": str(org2.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1486,6 +1502,7 @@ def test_batch_command_administrator_endpoints_on_non_managed_org(self): "organization": str(org2.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } response = self.client.post( @@ -1507,6 +1524,7 @@ def test_batch_command_administrator_endpoints_on_non_managed_org(self): "organization": str(org2.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1622,6 +1640,7 @@ def test_batch_command_bearer_authentication(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } response = self.client.post( @@ -1679,6 +1698,7 @@ def test_batch_command_endpoints_operator_different_org(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "execute_all": True, } response = self.client.post( @@ -1714,6 +1734,7 @@ def test_batch_command_execute_skipped_devices(self): payload = { "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device_a.pk), str(device_b.pk)], } response = self.client.post( @@ -1759,6 +1780,7 @@ def test_batch_command_execute_skipped_devices(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } ), @@ -1801,6 +1823,7 @@ def test_batch_command_execute_skipped_devices(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device.pk)], } ), @@ -1841,6 +1864,7 @@ def test_batch_command_execute_org_mismatched_data_provided(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "devices": [str(device_org2.pk)], } ), @@ -1864,6 +1888,7 @@ def test_batch_command_execute_org_mismatched_data_provided(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "group": str(group_org2.pk), } ), @@ -1895,6 +1920,7 @@ def test_batch_command_execute_org_mismatched_data_provided(self): "organization": str(org.pk), "type": "custom", "input": {"command": "echo test"}, + "label": "test-label", "location": str(location_org2.pk), } ), diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index e1f9780f0..bd176f582 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -977,6 +977,7 @@ def _create_batch_command(self, organization, **kwargs): organization=organization, type="custom", input={"command": "echo test"}, + label="test-label", ) devices = kwargs.pop("devices", None) opts.update(kwargs) @@ -992,11 +993,7 @@ def _create_batch_command(self, organization, **kwargs): def test_batch_command_str(self): org = self._get_org() batch = self._create_batch_command(organization=org) - expected = "{0} ({1})".format( - batch.type, - timezone.localtime(batch.created).strftime("%Y-%m-%d %H:%M:%S"), - ) - self.assertEqual(str(batch), expected) + self.assertEqual(str(batch), "test-label") def test_batch_command_total_devices_successful_failed(self): org = self._get_org() @@ -1074,6 +1071,7 @@ def test_batch_command_clean_validation(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) with self.assertRaises(ValidationError) as ctx: batch.clean() @@ -1088,6 +1086,7 @@ def test_batch_command_clean_validation(self): organization=org, type="change_password", input="not_an_object", + label="test-label", ) with self.assertRaises(ValidationError) as ctx: batch.clean() @@ -1099,11 +1098,12 @@ def test_batch_command_clean_validation(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", group=group, ) with self.assertRaises(ValidationError) as ctx: batch.clean() - self.assertIn("group", ctx.exception.message_dict) + self.assertIn("group", ctx.exception.message_dict) self.assertIn( "Please ensure that the organization of this Batch command " "and the organization of the related Device Group match", @@ -1120,11 +1120,12 @@ def test_batch_command_clean_validation(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", location=location, ) with self.assertRaises(ValidationError) as ctx: batch.clean() - self.assertIn("location", ctx.exception.message_dict) + self.assertIn("location", ctx.exception.message_dict) self.assertIn( "Please ensure that the organization of this Batch command " "and the organization of the related location match", @@ -1424,6 +1425,7 @@ def test_batch_command_execute_method(self): organization=empty_org, type="custom", input={"command": "echo test"}, + label="test-label", ) self.assertIn( "No devices match", @@ -1463,6 +1465,7 @@ def test_batch_command_execute_method(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", devices=[device1], ) batch.create_commands() @@ -1474,6 +1477,7 @@ def test_batch_command_execute_method(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", group=group, ) batch.create_commands() @@ -1485,6 +1489,7 @@ def test_batch_command_execute_method(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", location=location, ) batch.create_commands() @@ -1497,6 +1502,7 @@ def test_batch_command_execute_method(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", group=group, location=location, ) @@ -1509,6 +1515,7 @@ def test_batch_command_execute_method(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.create_commands() self.assertEqual(batch.batch_commands.count(), 2) @@ -1533,6 +1540,7 @@ def test_batch_command_execute_org_mismatch(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", devices=[device_org2], ) self.assertIn("devices", ctx.exception.message_dict) @@ -1551,6 +1559,7 @@ def test_batch_command_execute_org_mismatch(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", group=group_org2, ) self.assertIn("group", ctx.exception.message_dict) @@ -1571,6 +1580,7 @@ def test_batch_command_execute_org_mismatch(self): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", location=location_org2, ) self.assertIn("location", ctx.exception.message_dict) diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index aaa3a6c42..68acf8b62 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -335,6 +335,7 @@ def test_launch_batch_command_creates_commands(self, mocked_delay): organization=org, type="custom", input={"command": "echo 'test'"}, + label="test-label", ) batch.full_clean() batch.save() @@ -368,6 +369,7 @@ def test_launch_batch_command_creates_commands_for_multiple_devices( organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.full_clean() batch.save() @@ -394,6 +396,7 @@ def test_launch_batch_command_timeout(self, mocked_create_commands): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.full_clean() batch.save() @@ -414,6 +417,7 @@ def test_launch_batch_command_exception(self, mocked_create_commands): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.full_clean() batch.save() @@ -437,6 +441,7 @@ def test_launch_batch_command_all_devices_skipped(self, mocked_delay): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.full_clean() batch.save() @@ -458,6 +463,7 @@ def test_launch_batch_command_already_processed(self, mocked_delay): organization=org, type="custom", input={"command": "echo test"}, + label="test-label", ) batch.full_clean() batch.save() 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 c9d65a677..dd03617cc 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 @@ -125,6 +125,23 @@ class Migration(migrations.Migration): verbose_name="location", ), ), + ( + "label", + models.CharField( + max_length=64, + verbose_name="label", + help_text="A short label to identify this batch command.", + ), + ), + ( + "notes", + models.TextField( + blank=True, + default="", + verbose_name="notes", + help_text="Optional notes about this batch command.", + ), + ), ( "organization", models.ForeignKey( From 7c807972dd60670d93b5fd04f553c4456ac6c2de Mon Sep 17 00:00:00 2001 From: dee077 Date: Fri, 3 Jul 2026 06:44:00 +0530 Subject: [PATCH 17/20] [docs] Add docs for batch command endpoints --- docs/user/rest-api.rst | 107 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/docs/user/rest-api.rst b/docs/user/rest-api.rst index c9702920d..a023c957b 100644 --- a/docs/user/rest-api.rst +++ b/docs/user/rest-api.rst @@ -481,6 +481,113 @@ Get Command Details GET /api/v1/controller/device/{device_id}/command/{command_id}/ +.. _controller_batch_command_api: + +Dry-Run Batch Command +~~~~~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + GET /api/v1/controller/batch-command/execute/ + +Returns the list of devices that would be targeted without executing +anything. Useful for previewing which devices are affected. + +**Query Parameters:** + +================ ================================================= +Parameter Description +================ ================================================= +``organization`` Organization UUID (optional) +``type`` Command type (optional for dry-run) +``input`` Input data for the command (optional for dry-run) +``devices`` Comma-separated device UUIDs (optional) +``group`` Device group UUID (optional) +``location`` Location UUID (optional) +``execute_all`` Set to ``true`` to target all devices (optional) +================ ================================================= + +Execute a Batch Command +~~~~~~~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + POST /api/v1/controller/batch-command/execute/ + +Creates and executes a batch command on the targeted devices. + +**Request Parameters:** + +================ ================================================ +Parameter Description +================ ================================================ +``organization`` Organization UUID (optional for superusers) +``type`` Type of command to execute (**required**) +``input`` Input data for the command (**required**) +``label`` A short label to identify this batch command + (**required**) +``notes`` Optional notes (optional) +``devices`` List of device UUIDs (optional) +``group`` Device group UUID (optional) +``location`` Location UUID (optional) +``execute_all`` Set to ``true`` to target all devices (optional) +================ ================================================ + +**Available Command Types:** + +See :ref:`controller_execute_command_api` for available command types and +input formats. + +**Example payload:** + +.. code-block:: json + + { + "organization": "org-uuid", + "type": "custom", + "input": {"command": "uptime"}, + "label": "Check uptime", + "execute_all": true + } + +**Example request:** + +.. code-block:: shell + + curl -X POST \ + http://127.0.0.1:8000/api/v1/controller/batch-command/execute/ \ + -H 'authorization: Bearer yoursecretauthtoken' \ + -H 'content-type: application/json' \ + -d '{ + "organization": "org-uuid", + "type": "custom", + "input": {"command": "uptime"}, + "label": "Check uptime", + "execute_all": true + }' + +**Response:** ``201 Created`` with the batch command UUID. + +List Batch Commands +~~~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + GET /api/v1/controller/batch-command/ + +Returns a paginated list of batch commands with device count and skipped +device information. + +Get Batch Command Detail +~~~~~~~~~~~~~~~~~~~~~~~~ + +.. code-block:: text + + GET /api/v1/controller/batch-command/{id}/ + +Returns detailed information about a batch command, including the list of +targeted devices. + List Device Groups ~~~~~~~~~~~~~~~~~~ From 894c9f3bc8ea4c82bf8977509ac3bd4e1e467859 Mon Sep 17 00:00:00 2001 From: dee077 Date: Wed, 1 Jul 2026 03:47:25 +0530 Subject: [PATCH 18/20] [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 2f936a5f9..d80503ac0 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -808,7 +808,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 432952ee54a1750943219bc2bd9805651ac28824 Mon Sep 17 00:00:00 2001 From: dee077 Date: Thu, 2 Jul 2026 23:49:48 +0530 Subject: [PATCH 19/20] [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 d80503ac0..aa680c76f 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -810,6 +810,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 b4dbb98b39aebe096b953349256722fbdfab1ab8 Mon Sep 17 00:00:00 2001 From: dee077 Date: Tue, 7 Jul 2026 02:50:32 +0530 Subject: [PATCH 20/20] [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 %}