From 65f6b078553e9dca93e9c177ea7ca9a9e44b4068 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 31 May 2024 01:20:01 +0530 Subject: [PATCH 1/4] [change] Notify users of background subnet division rule errors #837 Closes #837 --- .github/workflows/ci.yml | 2 + .../subnet_division/rule_types/base.py | 18 ++++++++- .../subnet_division/tests/test_models.py | 38 ++++++++++++++++++- .../openwisp2/sample_subnet_division/tests.py | 2 +- 4 files changed, 57 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a21f69729..79adadf62 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,6 +72,8 @@ jobs: - name: Install openwisp-controller run: | pip install -U -e . + # TODO: Remove before merging + pip install -IU --no-deps "openwisp-notifications @ https://github.com/Dhanus3133/openwisp-notifications/tarball/feat/display-description-notification-dialog" pip install ${{ matrix.django-version }} - name: QA checks diff --git a/openwisp_controller/subnet_division/rule_types/base.py b/openwisp_controller/subnet_division/rule_types/base.py index 78b3905cf..bbb61ba34 100644 --- a/openwisp_controller/subnet_division/rule_types/base.py +++ b/openwisp_controller/subnet_division/rule_types/base.py @@ -7,6 +7,7 @@ from django.dispatch import Signal from django.utils.translation import gettext_lazy as _ from netaddr import IPNetwork +from openwisp_notifications.signals import notify from swapper import load_model from ..signals import subnet_provisioned @@ -204,7 +205,22 @@ def create_subnets(config, division_rule, max_subnet, generated_indexes): for subnet_id in range(1, division_rule.number_of_subnets + 1): if not ip_network(str(required_subnet)).subnet_of(master_subnet.subnet): - logger.error(f'Cannot create more subnets of {master_subnet}') + notify.send( + sender=config, + type='generic_message', + target=config.device, + action_object=master_subnet, + level='error', + message=( + 'Failed to provision subnets for' + ' [{notification.target}]({notification.target_link})' + ), + description=( + 'The [{notification.action_object}]({notification.action_link})' + ' subnet has run out of space.' + ), + ) + logger.info(f'Cannot create more subnets of {master_subnet}') break subnet_obj = Subnet( name=f'{division_rule.label}_subnet{subnet_id}', diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py index b678935f7..5f5832d6b 100644 --- a/openwisp_controller/subnet_division/tests/test_models.py +++ b/openwisp_controller/subnet_division/tests/test_models.py @@ -26,9 +26,13 @@ VpnClient = load_model('config', 'VpnClient') Device = load_model('config', 'Device') OrganizationConfigSettings = load_model('config', 'OrganizationConfigSettings') +Notification = load_model('openwisp_notifications', 'Notification') class BaseSubnetDivisionRule: + config_label = 'config' + ipam_label = 'openwisp_ipam' + @property def ip_query(self): return IpAddress.objects.exclude(id=self.vpn_server.ip_id) @@ -486,7 +490,7 @@ def test_multiple_vpnclient_delete(self): ip_query.count(), (rule.number_of_subnets * rule.number_of_ips) ) - @patch('logging.Logger.error') + @patch('logging.Logger.info') def test_subnets_exhausted(self, mocked_logger): subnet = self._get_master_subnet( '10.0.0.0/29', master_subnet=self.master_subnet @@ -504,6 +508,8 @@ def test_subnets_exhausted(self, mocked_logger): number_of_ips=2, number_of_subnets=2, ) + # A user is required to verify notification is created + self._get_admin() self.vpn_server.subnet = subnet self.vpn_server.save() self.config.templates.add(self.template) @@ -513,6 +519,36 @@ def test_subnets_exhausted(self, mocked_logger): ) config2.templates.add(self.template) mocked_logger.assert_called_with(f'Cannot create more subnets of {subnet}') + notification = Notification.objects.first() + self.assertEqual(notification.level, 'error') + self.assertEqual(notification.type, 'generic_message') + self.assertEqual(notification.target, config2.device) + self.assertEqual(notification.action_object, subnet) + self.assertEqual( + notification.message, + ( + '

Failed to provision subnets for ' + '' + '{device_name}

' + ).format( + device_path=reverse( + f'admin:{self.config_label}_device_change', args=[config2.device_id] + ), + device_name=config2.device.name, + ), + ) + self.assertEqual( + notification.rendered_description, + ( + '

The ' + '{subnet_name} subnet has run out of space.

' + ).format( + subnet_path=reverse( + f'admin:{self.ipam_label}_subnet_change', args=[subnet.id] + ), + subnet_name=subnet, + ), + ) def test_vpn_subnet_none(self): self.vpn_server.subnet = None diff --git a/tests/openwisp2/sample_subnet_division/tests.py b/tests/openwisp2/sample_subnet_division/tests.py index 6171161a6..148034f2c 100644 --- a/tests/openwisp2/sample_subnet_division/tests.py +++ b/tests/openwisp2/sample_subnet_division/tests.py @@ -21,7 +21,7 @@ class TestSubnetAdmin(BaseTestSubnetAdmin): class TestSubnetDivsionRule(BaseTestSubnetDivisionRule): - pass + config_label = 'sample_config' class TestIPAdmin(BaseTestIPAdmin): From 787e2e508a0571bcf49ca666baaf19d3675d159f Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 3 Jun 2024 21:38:21 +0530 Subject: [PATCH 2/4] [req-changes] Flagged strings as translatable --- openwisp_controller/subnet_division/rule_types/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/subnet_division/rule_types/base.py b/openwisp_controller/subnet_division/rule_types/base.py index bbb61ba34..47144bfad 100644 --- a/openwisp_controller/subnet_division/rule_types/base.py +++ b/openwisp_controller/subnet_division/rule_types/base.py @@ -211,11 +211,11 @@ def create_subnets(config, division_rule, max_subnet, generated_indexes): target=config.device, action_object=master_subnet, level='error', - message=( + message=_( 'Failed to provision subnets for' ' [{notification.target}]({notification.target_link})' ), - description=( + description=_( 'The [{notification.action_object}]({notification.action_link})' ' subnet has run out of space.' ), From 188e393bffff93115e1e6ce82e73ca608d2bd260 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Tue, 18 Jun 2024 12:05:52 +0530 Subject: [PATCH 3/4] [chores] Updated openwisp-notifications dependency --- .github/workflows/ci.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 79adadf62..a21f69729 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,8 +72,6 @@ jobs: - name: Install openwisp-controller run: | pip install -U -e . - # TODO: Remove before merging - pip install -IU --no-deps "openwisp-notifications @ https://github.com/Dhanus3133/openwisp-notifications/tarball/feat/display-description-notification-dialog" pip install ${{ matrix.django-version }} - name: QA checks From a1cc8db40c8fb172f828ddd5c967a01bc248d53b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 24 Jun 2024 23:05:38 +0530 Subject: [PATCH 4/4] [fix] Fixed failing test --- openwisp_controller/subnet_division/tests/test_models.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/subnet_division/tests/test_models.py b/openwisp_controller/subnet_division/tests/test_models.py index 5f5832d6b..cf1a498bc 100644 --- a/openwisp_controller/subnet_division/tests/test_models.py +++ b/openwisp_controller/subnet_division/tests/test_models.py @@ -491,7 +491,7 @@ def test_multiple_vpnclient_delete(self): ) @patch('logging.Logger.info') - def test_subnets_exhausted(self, mocked_logger): + def test_subnets_exhausted(self, mocked_logger, *args): subnet = self._get_master_subnet( '10.0.0.0/29', master_subnet=self.master_subnet ) @@ -518,7 +518,10 @@ def test_subnets_exhausted(self, mocked_logger): device=self._create_device(mac_address='00:11:22:33:44:66') ) config2.templates.add(self.template) - mocked_logger.assert_called_with(f'Cannot create more subnets of {subnet}') + self.assertEqual( + mocked_logger.call_args_list[4][0][0], + f'Cannot create more subnets of {subnet}', + ) notification = Notification.objects.first() self.assertEqual(notification.level, 'error') self.assertEqual(notification.type, 'generic_message')