Skip to content
6 changes: 5 additions & 1 deletion openwisp_controller/config/base/device_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
from .. import settings as app_settings
from ..signals import group_templates_changed
from ..sortedm2m.fields import SortedManyToManyField
from ..tasks import bulk_invalidate_config_get_cached_checksum
from ..tasks import (
bulk_invalidate_config_get_cached_checksum,
invalidate_controller_views_for_group,
)
from .config import TemplatesThrough


Expand Down Expand Up @@ -88,6 +91,7 @@ def save(
bulk_invalidate_config_get_cached_checksum.delay(
{"device__group_id": str(self.id)}
)
invalidate_controller_views_for_group.delay(str(self.id))

def get_context(self):
return deepcopy(self.context)
Expand Down
6 changes: 5 additions & 1 deletion openwisp_controller/config/base/multitenancy.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@

from .. import settings as app_settings
from ..exceptions import OrganizationDeviceLimitExceeded
from ..tasks import bulk_invalidate_config_get_cached_checksum
from ..tasks import (
bulk_invalidate_config_get_cached_checksum,
invalidate_controller_views_cache,
)


class AbstractOrganizationConfigSettings(UUIDModel):
Expand Down Expand Up @@ -100,6 +103,7 @@ def save(
bulk_invalidate_config_get_cached_checksum.delay(
{"device__organization_id": str(self.organization_id)}
)
invalidate_controller_views_cache.delay(str(self.organization_id))


class AbstractOrganizationLimits(models.Model):
Expand Down
16 changes: 16 additions & 0 deletions openwisp_controller/config/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,19 @@ def invalidate_controller_views_cache(organization_id):
Vpn.objects.filter(organization_id=organization_id).only("id").iterator()
):
GetVpnView.invalidate_get_vpn_cache(vpn)


@shared_task(base=OpenwispCeleryTask)
def invalidate_controller_views_for_group(group_id):
"""
Invalidates the DeviceChecksumView cache only for devices in the given group.

Unlike invalidate_controller_views_cache, this is scoped to a single device
group and does not invalidate VPN caches.
"""
from .controller.views import DeviceChecksumView

Device = load_model("config", "Device")

for device in Device.objects.filter(group_id=group_id).only("id").iterator():
DeviceChecksumView.invalidate_get_device_cache(device)
Comment on lines +222 to +235
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding soft_time_limit for consistency with similar tasks.

The new task lacks the soft_time_limit parameter that other iteration-based tasks in this file use (e.g., invalidate_controller_views_cache implicitly inherits defaults, but tasks like update_template_related_config_status explicitly set soft_time_limit=7200). For large device groups, this task could run longer than expected without a safeguard.

💡 Suggested fix
-@shared_task(base=OpenwispCeleryTask)
+@shared_task(base=OpenwispCeleryTask, soft_time_limit=7200)
 def invalidate_controller_views_for_group(group_id):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/config/tasks.py` around lines 222 - 237, The task
decorator for invalidate_controller_views_for_group should include a
soft_time_limit to match other long-running iteration tasks; update the
`@shared_task`(...) call on the invalidate_controller_views_for_group function to
pass soft_time_limit=7200 (preserving base=OpenwispCeleryTask) so the task will
have a soft timeout safeguard for large device groups.

61 changes: 61 additions & 0 deletions openwisp_controller/config/tests/test_device.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,67 @@ def test_changing_group_variable_invalidates_cache(self):
new_checksum = config.get_cached_checksum()
self.assertNotEqual(old_checksum, new_checksum)

def test_status_update_on_org_variable_change(self):
org = self._get_org()
cs = OrganizationConfigSettings.objects.create(organization=org, context={})
c1 = self._create_config(organization=org)
c1.templates.add(
self._create_template(
name="t-with-var",
config={"interfaces": [{"name": "{{ ssid }}", "type": "ethernet"}]},
default_values={"ssid": "eth0"},
)
)
c1.set_status_applied()
d2 = self._create_device(
organization=org, name="d2", mac_address="00:11:22:33:44:56"
)
c2 = self._create_config(device=d2)
c2.set_status_applied()
cs.context = {"ssid": "OrgA"}
cs.full_clean()
cs.save()
c1.refresh_from_db()
c2.refresh_from_db()
with self.subTest("affected config changes to modified"):
self.assertEqual(c1.status, "modified")
with self.subTest("unaffected config remains applied"):
self.assertEqual(c2.status, "applied")
Comment on lines +436 to +461
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding explicit mock assertion for org-level cache invalidation.

The organization variable change test validates the status transition correctly, but unlike test_status_update_on_group_variable_change, it doesn't explicitly verify that invalidate_controller_views_cache.delay is called when OrganizationConfigSettings.context changes. Adding a mock assertion would strengthen the regression test for the PR's core fix.

💡 Suggested addition
         cs.context = {"ssid": "OrgA"}
         cs.full_clean()
-        cs.save()
+        patch_path = (
+            "openwisp_controller.config.base.multitenancy"
+            ".invalidate_controller_views_cache"
+        )
+        with mock.patch(patch_path) as mocked_task:
+            cs.save()
+            mocked_task.delay.assert_called_once_with(str(org.id))
         c1.refresh_from_db()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openwisp_controller/config/tests/test_device.py` around lines 436 - 461, The
test test_status_update_on_org_variable_change verifies config status changes
but omits asserting that the org-level cache invalidation task was enqueued;
update the test to patch/mocking invalidate_controller_views_cache.delay (same
approach used in test_status_update_on_group_variable_change), set
OrganizationConfigSettings.context, save, then assert
invalidate_controller_views_cache.delay was called (and with appropriate args if
required) after saving OrganizationConfigSettings; reference
OrganizationConfigSettings and invalidate_controller_views_cache.delay to locate
where to add the mock and assertion.


def test_status_update_on_group_variable_change(self):
org = self._get_org()
dg = self._create_device_group(organization=org, context={})
d1 = self._create_device(organization=org, group=dg)
c1 = self._create_config(device=d1)
c1.templates.add(
self._create_template(
name="t-with-var",
config={"interfaces": [{"name": "{{ ssid }}", "type": "ethernet"}]},
default_values={"ssid": "eth0"},
)
)
c1.set_status_applied()
d2 = self._create_device(
organization=org, group=dg, name="d2", mac_address="00:11:22:33:44:56"
)
c2 = self._create_config(device=d2)
c2.set_status_applied()
dg.context = {"ssid": "OrgA"}
dg.full_clean()
patch_path = (
"openwisp_controller.config.base.device_group"
".invalidate_controller_views_for_group"
)
with mock.patch(patch_path) as mocked_task:
dg.save()
mocked_task.delay.assert_called_once_with(str(dg.id))
c1.refresh_from_db()
c2.refresh_from_db()
with self.subTest("affected config changes to modified"):
self.assertEqual(c1.status, "modified")
with self.subTest("unaffected config remains applied"):
self.assertEqual(c2.status, "applied")

def test_management_ip_changed_not_emitted_on_creation(self):
with catch_signal(management_ip_changed) as handler:
self._create_device(organization=self._get_org())
Expand Down
Loading