Repository: openwisp/openwisp-controller (language: python)

Issue:
[bug] Subnet division rules which do not provision anything
Maybe we want to avoid possibility of creating subnet division rules which do not provision anything.
At the moment this is possible, it doesn't sound like something which could be useful or legitimate, but maybe we want to keep using this feature a little longer before deciding what to do with this case (either forbid it or allow it as a special case if we find any use to it).


Discussion / hints:

Fix patch:
diff --git a/openwisp_controller/subnet_division/base/models.py b/openwisp_controller/subnet_division/base/models.py
index 06eb61a2f..249bfa813 100644
--- a/openwisp_controller/subnet_division/base/models.py
+++ b/openwisp_controller/subnet_division/base/models.py
@@ -2,6 +2,7 @@
 
 import swapper
 from django.core.exceptions import ValidationError
+from django.core.validators import MinValueValidator
 from django.db import models, transaction
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
@@ -32,17 +33,19 @@ class AbstractSubnetDivisionRule(TimeStampedEditableModel, OrgMixin):
         max_length=30,
         help_text=_('Label used to calculate the configuration variables'),
     )
-    number_of_subnets = models.PositiveIntegerField(
+    number_of_subnets = models.PositiveSmallIntegerField(
         verbose_name=_('Number of Subnets'),
         help_text=_('Indicates how many subnets will be created'),
+        validators=[MinValueValidator(1)],
     )
-    size = models.PositiveIntegerField(
+    size = models.PositiveSmallIntegerField(
         verbose_name=_('Size of subnets'),
         help_text=_('Indicates the size of each created subnet'),
     )
-    number_of_ips = models.PositiveIntegerField(
+    number_of_ips = models.PositiveSmallIntegerField(
         verbose_name=_('Number of IPs'),
         help_text=_('Indicates how many IP addresses will be created for each subnet'),
+        validators=[MinValueValidator(1)],
     )
 
     class Meta:
diff --git a/openwisp_controller/subnet_division/migrations/0005_number_of_subnets_and_ips.py b/openwisp_controller/subnet_division/migrations/0005_number_of_subnets_and_ips.py
new file mode 100644
index 000000000..b059acf21
--- /dev/null
+++ b/openwisp_controller/subnet_division/migrations/0005_number_of_subnets_and_ips.py
@@ -0,0 +1,42 @@
+# Generated by Django 3.2.16 on 2023-03-24 16:33
+
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('subnet_division', '0004_index_rule_on_delete'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='subnetdivisionrule',
+            name='number_of_ips',
+            field=models.PositiveSmallIntegerField(
+                help_text=(
+                    'Indicates how many IP addresses will be created for each subnet'
+                ),
+                verbose_name='Number of IPs',
+                validators=[django.core.validators.MinValueValidator(1)],
+            ),
+        ),
+        migrations.AlterField(
+            model_name='subnetdivisionrule',
+            name='number_of_subnets',
+            field=models.PositiveSmallIntegerField(
+                help_text='Indicates how many subnets will be created',
+                validators=[django.core.validators.MinValueValidator(1)],
+                verbose_name='Number of Subnets',
+            ),
+        ),
+        migrations.AlterField(
+            model_name='subnetdivisionrule',
+            name='size',
+            field=models.PositiveSmallIntegerField(
+                help_text='Indicates the size of each created subnet',
+                verbose_name='Size of subnets',
+            ),
+        ),
+    ]


Test patch:
diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py
index 73ffb6f28..87f442e06 100644
--- a/openwisp_controller/subnet_division/tests/test_models.py
+++ b/openwisp_controller/subnet_division/tests/test_models.py
@@ -125,6 +125,34 @@ def test_field_validations(self):
                 context_manager.exception.message_dict, expected_message_dict
             )
 
+        with self.subTest('Test rule does not provision any subnet'):
+            options = default_options.copy()
+            options['number_of_subnets'] = 0
+            rule = SubnetDivisionRule(**options)
+            with self.assertRaises(ValidationError) as context_manager:
+                rule.full_clean()
+            expected_message_dict = {
+                'number_of_subnets': [
+                    'Ensure this value is greater than or equal to 1.'
+                ]
+            }
+            self.assertDictEqual(
+                context_manager.exception.message_dict, expected_message_dict
+            )
+
+        with self.subTest('Test rule does not provision any IP'):
+            options = default_options.copy()
+            options['number_of_ips'] = 0
+            rule = SubnetDivisionRule(**options)
+            with self.assertRaises(ValidationError) as context_manager:
+                rule.full_clean()
+            expected_message_dict = {
+                'number_of_ips': ['Ensure this value is greater than or equal to 1.']
+            }
+            self.assertDictEqual(
+                context_manager.exception.message_dict, expected_message_dict
+            )
+
         with self.subTest('Test rule size is illegal'):
             options = default_options.copy()
             options['size'] = 33
@@ -186,7 +214,7 @@ def test_field_validations(self):
             # Test decreasing number_of_ips
             with self.assertRaises(ValidationError) as error:
                 rule.size = 28
-                rule.number_of_ips = 0
+                rule.number_of_ips = 1
                 rule.full_clean()
             self.assertDictEqual(
                 error.exception.message_dict,
diff --git a/tests/openwisp2/sample_subnet_division/migrations/0001_initial.py b/tests/openwisp2/sample_subnet_division/migrations/0001_initial.py
index 196643164..87d3fbe38 100644
--- a/tests/openwisp2/sample_subnet_division/migrations/0001_initial.py
+++ b/tests/openwisp2/sample_subnet_division/migrations/0001_initial.py
@@ -2,6 +2,7 @@
 
 import uuid
 
+import django.core.validators
 import django.db.models.deletion
 import django.utils.timezone
 import model_utils.fields
@@ -75,26 +76,28 @@ class Migration(migrations.Migration):
                 ),
                 (
                     'number_of_subnets',
-                    models.PositiveIntegerField(
+                    models.PositiveSmallIntegerField(
                         help_text='Indicates how many subnets will be created',
                         verbose_name='Number of Subnets',
+                        validators=[django.core.validators.MinValueValidator(1)],
                     ),
                 ),
                 (
                     'size',
-                    models.PositiveIntegerField(
+                    models.PositiveSmallIntegerField(
                         help_text='Indicates the size of each created subnet',
                         verbose_name='Size of subnets',
                     ),
                 ),
                 (
                     'number_of_ips',
-                    models.PositiveIntegerField(
+                    models.PositiveSmallIntegerField(
                         help_text=(
                             'Indicates how many IP addresses will '
                             'be created for each subnet'
                         ),
                         verbose_name='Number of IPs',
+                        validators=[django.core.validators.MinValueValidator(1)],
                     ),
                 ),
                 (