From ea50f6050dc80d505470d69382432fb8f0364490 Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Thu, 24 Apr 2025 00:54:18 +0530 Subject: [PATCH 01/15] [change] Reduce sortedm2m queries for templates #204 To remove queries generated by sortedm2m, 'formfield_for_manytomany' is overriden such that on page loads, we can get templates using 'get_relevant_templates'. Added device/group id to identify selected templates. Changes are made in JS to ensure templates are viewed and changes are saved as per initial behaviour. Ordering is not maintained. Closes #204 Signed-off-by: DragnEmperor --- openwisp_controller/config/admin.py | 14 ++++++++ .../static/config/js/relevant_templates.js | 34 +++++++++++++++++-- openwisp_controller/config/views.py | 14 ++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 544b76188..9016791d6 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -463,6 +463,13 @@ def get_fields(self, request, obj): fields = super().get_fields(request, obj) return self._error_reason_field_conditional(obj, fields) + def formfield_for_manytomany(self, db_field, request, **kwargs): + # setting queryset none for all requests except POST as queryset + # is required for the form to be valid + if db_field.name == 'templates' and request.method != 'POST': + kwargs['queryset'] = Template.objects.none() + return super().formfield_for_manytomany(db_field, request, **kwargs) + class ChangeDeviceGroupForm(forms.Form): device_group = forms.ModelChoiceField( @@ -1319,6 +1326,13 @@ def get_extra_context(self, pk=None): } return ctx + def formfield_for_manytomany(self, db_field, request, **kwargs): + # setting queryset none for all requests except POST as queryset + # is required for the form to be valid + if db_field.name == 'templates' and request.method != 'POST': + kwargs['queryset'] = Template.objects.none() + return super().formfield_for_manytomany(db_field, request, **kwargs) + admin.site.register(Device, DeviceAdminExportable) admin.site.register(Template, TemplateAdmin) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index 9684d409c..7335a2f60 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -33,7 +33,8 @@ django.jQuery(function ($) { if (templateConfig.required) { inputField.prop("disabled", true); } - if (isSelected || templateConfig.required) { + // mark the template as selected if it is required or if it is enabled for the current device or group + if (isSelected || templateConfig.required || templateConfig.selected) { inputField.prop("checked", true); } return element; @@ -115,6 +116,14 @@ django.jQuery(function ($) { var url = window._relevantTemplateUrl.replace("org_id", orgID); // Get relevant templates of selected org and backend url = url + "?backend=" + backend; + if (!isDeviceGroup()) { + var deviceID = $('input[name="config-0-device"]').val(); + url = url + "&device=" + deviceID; + } else { + // Get the group id from the URL + var pathParts = window.location.pathname.split("/"); + url = url + "&group=" + pathParts[pathParts.length - 3]; + } $.get(url).done(function (data) { resetTemplateOptions(); var enabledTemplates = [], @@ -156,7 +165,8 @@ django.jQuery(function ($) { // Adds "li" elements for templates that are not selected // in the database. var counter = - selectedTemplates !== undefined ? selectedTemplates.length : 0; + selectedTemplates !== undefined ? selectedTemplates.length : 0, + deviceTemplates = []; Object.keys(data).forEach(function (templateId, index) { // corner case in which backend of template does not match if (!data[templateId]) { @@ -186,6 +196,9 @@ django.jQuery(function ($) { if (isSelected === true) { enabledTemplates.push(templateId); } + if (element.children().children("input").prop("checked") === true) { + deviceTemplates.push(templateId); + } sortedm2mUl.append(element); if (!isDeviceGroup()) { sortedm2mPrefixUl.append(prefixElement); @@ -194,6 +207,23 @@ django.jQuery(function ($) { if (firstRun === true && selectedTemplates !== undefined) { updateTemplateSelection(selectedTemplates); } + // this runs on first load and sets the name of hidden input field which tracks + // the selected templates. + if (selectedTemplates === undefined) { + if (!isDeviceGroup()) { + $( + `.has_original .field-templates .sortedm2m-container input[type="hidden"]`, + ).attr("name", templatesFieldName()); + // set the initial value of the hidden input field + // to the selected templates + django._owcInitialValues[templatesFieldName()] = + deviceTemplates.join(","); + } else { + $( + `.field-templates .sortedm2m-container input[type="hidden"]`, + ).attr("name", templatesFieldName()); + } + } updateTemplateHelpText(); updateConfigTemplateField(enabledTemplates); }); diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index 2cc185718..e360487f9 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -15,6 +15,7 @@ Organization = load_model("openwisp_users", "Organization") Template = load_model("config", "Template") +Config = load_model("config", "Config") DeviceGroup = load_model("config", "DeviceGroup") OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") @@ -24,6 +25,8 @@ def get_relevant_templates(request, organization_id): returns default templates of specified organization """ backend = request.GET.get("backend", None) + device_id = request.GET.get("device", None) + group_id = request.GET.get("group", None) user = request.user if not user.is_superuser and not user.is_manager(organization_id): return HttpResponse(status=403) @@ -38,6 +41,16 @@ def get_relevant_templates(request, organization_id): .filter(Q(organization_id=org.pk) | Q(organization_id=None)) .only("id", "name", "backend", "default", "required") ) + # for checking which templates are enabled for given device or group + selected_templates = [] + if device_id: + selected_templates = Config.objects.filter(device_id=device_id).values_list( + 'templates__id', flat=True + ) + if group_id: + selected_templates = DeviceGroup.objects.filter( + pk=group_id, organization_id=organization_id + ).values_list('templates__id', flat=True) relevant_templates = {} for template in queryset: relevant_templates[str(template.pk)] = dict( @@ -45,6 +58,7 @@ def get_relevant_templates(request, organization_id): backend=template.get_backend_display(), default=template.default, required=template.required, + selected=template.pk in selected_templates, ) return JsonResponse(relevant_templates) From 019499fd8cc26ec7cb41bc6b9306bdf4e3c4f307 Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Tue, 29 Apr 2025 23:09:26 +0530 Subject: [PATCH 02/15] [fix] Fixed sorting issue in templates Using the 'sort_value' of the 'through_model', created a subquery which annotates a sort_value and selected field to each template. The sort_value is used for ordering the templates and also for checking if template is selected or not using a large default value which happens if template is not selected for a device config or group. Signed-off-by: DragnEmperor --- openwisp_controller/config/views.py | 50 ++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index e360487f9..9b3c6935c 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -2,7 +2,9 @@ from copy import deepcopy from uuid import UUID +from django.db import models as django_models from django.db.models import Q +from django.db.models.functions import Coalesce from django.http import HttpResponse, JsonResponse from django.utils import timezone from django.utils.module_loading import import_string @@ -31,26 +33,50 @@ def get_relevant_templates(request, organization_id): if not user.is_superuser and not user.is_manager(organization_id): return HttpResponse(status=403) org = get_object_or_404(Organization, pk=organization_id, is_active=True) + org_filters = Q(organization_id=org.pk) | Q(organization_id=None) filter_options = {} if backend: filter_options.update(backend=backend) else: filter_options.update(required=False, default=False) + sort_value_subquery = None + # fetch the selected templates for the device or group by creating a subquery. + # through_model and lookup_field are set based on the presence of device_id or + # group_id. we need through_model as `sort_value` is a field of the through model. + # the subquery will be used to annotate the queryset with the sort_value + # of the selected templates. + if device_id and (lookup := Config.objects.filter(device_id=device_id).first()): + through_model = Config.templates.through + lookup_field = "config_id" + if group_id and ( + lookup := DeviceGroup.objects.filter(Q(pk=group_id) & (org_filters)).first() + ): + through_model = DeviceGroup.templates.through + lookup_field = "devicegroup_id" + if device_id or group_id: + sort_value_subquery = django_models.Subquery( + through_model.objects.filter( + **{lookup_field: lookup.id}, template_id=django_models.OuterRef('pk') + ).values('sort_value')[:1], + output_field=django_models.IntegerField(), + ) + # annotated a selected field which is True based on sort_value + # if sort_value is 9999 then selected is False else True queryset = ( Template.objects.filter(**filter_options) - .filter(Q(organization_id=org.pk) | Q(organization_id=None)) + .filter(org_filters) + .annotate( + sort_value=Coalesce(sort_value_subquery, django_models.Value(9999)), + selected=django_models.Case( + django_models.When(sort_value=9999, then=django_models.Value(False)), + default=django_models.Value(True), + output_field=django_models.BooleanField(), + ), + ) + .order_by("sort_value") .only("id", "name", "backend", "default", "required") ) - # for checking which templates are enabled for given device or group - selected_templates = [] - if device_id: - selected_templates = Config.objects.filter(device_id=device_id).values_list( - 'templates__id', flat=True - ) - if group_id: - selected_templates = DeviceGroup.objects.filter( - pk=group_id, organization_id=organization_id - ).values_list('templates__id', flat=True) + relevant_templates = {} for template in queryset: relevant_templates[str(template.pk)] = dict( @@ -58,7 +84,7 @@ def get_relevant_templates(request, organization_id): backend=template.get_backend_display(), default=template.default, required=template.required, - selected=template.pk in selected_templates, + selected=template.selected, ) return JsonResponse(relevant_templates) From 22fc12f9e985af2602c19d36169cd2d982e02609 Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Sat, 10 May 2025 18:37:24 +0530 Subject: [PATCH 03/15] [tests] Replace failing tests with selenium tests As now templates are being loaded via js, normal tests were failing. For these failing tests, added relevant selenium tests. Modified `get-relevant_templates` to allow fetching templates during device creation by superuser and org users. Signed-off-by: DragnEmperor --- .../static/config/js/relevant_templates.js | 11 +- .../config/tests/test_admin.py | 142 ------------------ .../config/tests/test_selenium.py | 112 +++++++++++++- .../config/tests/test_views.py | 3 + openwisp_controller/config/views.py | 30 +++- 5 files changed, 146 insertions(+), 152 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index 7335a2f60..21e8079ee 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -95,7 +95,10 @@ django.jQuery(function ($) { selectedTemplates; // Hide templates if no organization or backend is selected - if (orgID.length === 0 || (!isDeviceGroup() && backend.length === 0)) { + if ( + (orgID && orgID.length === 0) || + (!isDeviceGroup() && backend.length === 0) + ) { resetTemplateOptions(); updateTemplateHelpText(); return; @@ -212,8 +215,10 @@ django.jQuery(function ($) { if (selectedTemplates === undefined) { if (!isDeviceGroup()) { $( - `.has_original .field-templates .sortedm2m-container input[type="hidden"]`, - ).attr("name", templatesFieldName()); + `#config-0 .field-templates .sortedm2m-container input[type="hidden"]`, + ) + .first() + .attr("name", templatesFieldName()); // set the initial value of the hidden input field // to the selected templates django._owcInitialValues[templatesFieldName()] = diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 9ef195cdc..5ebc3ac41 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -479,15 +479,6 @@ def test_device_organization_fk_autocomplete_view(self): hidden=[data["org2"].name, data["inactive"].name], ) - def test_device_templates_m2m_queryset(self): - data = self._create_multitenancy_test_env() - t_shared = self._create_template(name="t-shared", organization=None) - self._test_multitenant_admin( - url=reverse(f"admin:{self.app_label}_device_add"), - visible=[str(data["t1"]), str(t_shared)], - hidden=[str(data["t2"]), str(data["t3_inactive"])], - ) - def test_template_queryset(self): data = self._create_multitenancy_test_env() self._test_multitenant_admin( @@ -796,56 +787,6 @@ def test_template_not_contains_default_templates_js(self): response = self.client.get(path) self.assertNotContains(response, "// enable default templates") - def test_configuration_templates_removed(self): - def _update_template(templates): - params.update( - { - "config-0-templates": ",".join( - [str(template.pk) for template in templates] - ) - } - ) - response = self.client.post(path, data=params, follow=True) - self.assertEqual(response.status_code, 200) - for template in templates: - self.assertContains( - response, f'class="sortedm2m" checked> {template.name}' - ) - return response - - template = self._create_template() - - # Add a new device - path = reverse(f"admin:{self.app_label}_device_add") - params = self._get_device_params(org=self._get_org()) - response = self.client.post(path, data=params, follow=True) - self.assertEqual(response.status_code, 200) - - config = Device.objects.get(name=params["name"]).config - path = reverse(f"admin:{self.app_label}_device_change", args=[config.device_id]) - params.update( - { - "config-0-id": str(config.pk), - "config-0-device": str(config.device_id), - "config-INITIAL_FORMS": 1, - "_continue": True, - } - ) - - # Add template to the device - _update_template(templates=[template]) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.status, "modified") - config.set_status_applied() - self.assertEqual(config.status, "applied") - - # Remove template from the device - _update_template(templates=[]) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 0) - self.assertEqual(config.status, "modified") - def test_vpn_not_contains_default_templates_js(self): vpn = self._create_vpn() path = reverse(f"admin:{self.app_label}_vpn_change", args=[vpn.pk]) @@ -1626,89 +1567,6 @@ def test_add_vpn(self): response, 'value="openwisp_controller.vpn_backends.OpenVpn" selected' ) - def test_vpn_clients_deleted(self): - def _update_template(templates): - params.update( - { - "config-0-templates": ",".join( - [str(template.pk) for template in templates] - ) - } - ) - response = self.client.post(path, data=params, follow=True) - self.assertEqual(response.status_code, 200) - for template in templates: - self.assertContains( - response, f'class="sortedm2m" checked> {template.name}' - ) - return response - - vpn = self._create_vpn() - template = self._create_template() - vpn_template = self._create_template( - name="vpn-test", - type="vpn", - vpn=vpn, - auto_cert=True, - ) - cert_query = Cert.objects.exclude(pk=vpn.cert_id) - valid_cert_query = cert_query.filter(revoked=False) - revoked_cert_query = cert_query.filter(revoked=True) - - # Add a new device - path = reverse(f"admin:{self.app_label}_device_add") - params = self._get_device_params(org=self._get_org()) - response = self.client.post(path, data=params, follow=True) - self.assertEqual(response.status_code, 200) - - config = Device.objects.get(name=params["name"]).config - self.assertEqual(config.vpnclient_set.count(), 0) - self.assertEqual(config.templates.count(), 0) - - path = reverse(f"admin:{self.app_label}_device_change", args=[config.device_id]) - params.update( - { - "config-0-id": str(config.pk), - "config-0-device": str(config.device_id), - "config-INITIAL_FORMS": 1, - "_continue": True, - } - ) - - with self.subTest("Adding only VpnClient template"): - # Adding VpnClient template to the device - _update_template(templates=[vpn_template]) - - self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.vpnclient_set.count(), 1) - self.assertEqual(cert_query.count(), 1) - self.assertEqual(valid_cert_query.count(), 1) - - # Remove VpnClient template from the device - _update_template(templates=[]) - - self.assertEqual(config.templates.count(), 0) - self.assertEqual(config.vpnclient_set.count(), 0) - # Removing VPN template marks the related certificate as revoked - self.assertEqual(revoked_cert_query.count(), 1) - self.assertEqual(valid_cert_query.count(), 0) - - with self.subTest("Add VpnClient template along with another template"): - # Adding templates to the device - _update_template(templates=[template, vpn_template]) - - self.assertEqual(config.templates.count(), 2) - self.assertEqual(config.vpnclient_set.count(), 1) - self.assertEqual(valid_cert_query.count(), 1) - - # Remove VpnClient template from the device - _update_template(templates=[template]) - - self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.vpnclient_set.count(), 0) - self.assertEqual(valid_cert_query.count(), 0) - self.assertEqual(revoked_cert_query.count(), 2) - def test_ip_not_in_add_device(self): path = reverse(f"admin:{self.app_label}_device_add") response = self.client.get(path) diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index f6d489472..86f42e75e 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -14,17 +14,46 @@ from openwisp_utils.tests import SeleniumTestMixin -from .utils import CreateConfigTemplateMixin, TestWireguardVpnMixin +from .utils import CreateConfigTemplateMixin, TestVpnX509Mixin, TestWireguardVpnMixin Device = load_model("config", "Device") +Cert = load_model("django_x509", "Cert") @tag("selenium_tests") class TestDeviceAdmin( SeleniumTestMixin, CreateConfigTemplateMixin, + TestVpnX509Mixin, StaticLiveServerTestCase, ): + # helper function for adding/removing templates + def _update_template(self, device_id, templates, is_enabled=False): + self.open( + reverse('admin:config_device_change', args=[device_id]) + '#config-group' + ) + self.wait_for_presence(By.CSS_SELECTOR, 'input[name="config-0-templates"]') + + # if not is_enabled: + self.hide_loading_overlay() + for template in templates: + template_element = self.find_element( + By.XPATH, f'//*[@value="{template.id}"][@type="checkbox"]' + ) + # if enabled by default, assert that the checkbox is selected and enabled + if is_enabled: + self.assertEqual(template_element.is_enabled(), True) + self.assertEqual(template_element.is_selected(), True) + # enable/disable the checkbox + template_element.click() + + # Hide user tools because it covers the save button + self.web_driver.execute_script( + 'document.querySelector("#ow-user-tools").style.display="none"' + ) + self.find_element(by=By.NAME, value='_save').click() + self.wait_for_presence(By.CSS_SELECTOR, '.messagelist .success', timeout=5) + def test_create_new_device(self): required_template = self._create_template(name="Required", required=True) default_template = self._create_template(name="Default", default=True) @@ -86,7 +115,7 @@ def test_create_new_device(self): 'document.querySelector("#ow-user-tools").style.display="none"' ) self.find_element(by=By.NAME, value="_save").click() - self.wait_for_presence(By.CSS_SELECTOR, ".messagelist .success") + self.wait_for_presence(By.CSS_SELECTOR, ".messagelist .success", timeout=5) self.assertEqual( self.find_elements(by=By.CLASS_NAME, value="success")[0].text, "The Device “11:22:33:44:55:66” was added successfully.", @@ -296,6 +325,85 @@ def test_force_delete_multiple_devices_with_deactivating_config(self): delete_confirm.click() self.assertEqual(Device.objects.count(), 0) + def test_add_remove_templates(self): + template = self._create_template(organization=self._get_org()) + config = self._create_config(organization=self._get_org()) + device = config.device + self.login() + # some times the url fetching in js gives unauthorized error + # so we add a wait to allow login to complete + time.sleep(2) + + with self.subTest('Template should be added'): + self._update_template(device.id, templates=[template]) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 1) + self.assertEqual(config.status, 'modified') + config.set_status_applied() + self.assertEqual(config.status, 'applied') + + with self.subTest('Template should be removed'): + self._update_template(device.id, templates=[template], is_enabled=True) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 0) + self.assertEqual(config.status, 'modified') + + def test_vpn_clients_deleted(self): + vpn = self._create_vpn() + template = self._create_template() + vpn_template = self._create_template( + name='vpn-test', + type='vpn', + vpn=vpn, + auto_cert=True, + ) + cert_query = Cert.objects.exclude(pk=vpn.cert_id) + valid_cert_query = cert_query.filter(revoked=False) + revoked_cert_query = cert_query.filter(revoked=True) + + config = self._create_config(organization=self._get_org()) + device = config.device + self.assertEqual(config.vpnclient_set.count(), 0) + self.assertEqual(config.templates.count(), 0) + self.login() + # some times the url fetching in js gives unauthorized error + # so we add a wait to allow login to complete + time.sleep(2) + + with self.subTest('Adding only VpnClient template'): + # Adding VpnClient template to the device + self._update_template(device.id, [vpn_template]) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 1) + self.assertEqual(config.vpnclient_set.count(), 1) + self.assertEqual(cert_query.count(), 1) + self.assertEqual(valid_cert_query.count(), 1) + + # Remove VpnClient template from the device + self._update_template(device.id, [vpn_template], is_enabled=True) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 0) + self.assertEqual(config.vpnclient_set.count(), 0) + # Removing VPN template marks the related certificate as revoked + self.assertEqual(revoked_cert_query.count(), 1) + self.assertEqual(valid_cert_query.count(), 0) + + with self.subTest('Add VpnClient template along with another template'): + # Adding templates to the device + self._update_template(device.id, [template, vpn_template]) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 2) + self.assertEqual(config.vpnclient_set.count(), 1) + self.assertEqual(valid_cert_query.count(), 1) + + # Remove VpnClient template from the device + self._update_template(device.id, [vpn_template], is_enabled=True) + config.refresh_from_db() + self.assertEqual(config.templates.count(), 1) + self.assertEqual(config.vpnclient_set.count(), 0) + self.assertEqual(valid_cert_query.count(), 0) + self.assertEqual(revoked_cert_query.count(), 2) + @tag("selenium_tests") class TestDeviceAdminUnsavedChanges( diff --git a/openwisp_controller/config/tests/test_views.py b/openwisp_controller/config/tests/test_views.py index ba6fa0696..95d9e3fef 100644 --- a/openwisp_controller/config/tests/test_views.py +++ b/openwisp_controller/config/tests/test_views.py @@ -107,6 +107,7 @@ def test_get_relevant_templates_without_backend_filter(self): "required": t4.required, "default": t4.default, "name": t4.name, + "selected": False, } }, ) @@ -144,6 +145,7 @@ def test_get_relevant_templates_with_backend_filtering(self): "required": t1.required, "default": t1.default, "name": t1.name, + "selected": False, } }, ) @@ -163,6 +165,7 @@ def test_get_relevant_templates_with_backend_filtering(self): "required": t2.required, "default": t2.default, "name": t2.name, + "selected": False, } }, ) diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index 9b3c6935c..c0af2197a 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -30,16 +30,35 @@ def get_relevant_templates(request, organization_id): device_id = request.GET.get("device", None) group_id = request.GET.get("group", None) user = request.user - if not user.is_superuser and not user.is_manager(organization_id): + # organization_id is passed as 'null' for add device + organization_id = None if organization_id == 'null' else organization_id + if ( + not user.is_superuser + and organization_id + and not user.is_manager(organization_id) + ): return HttpResponse(status=403) - org = get_object_or_404(Organization, pk=organization_id, is_active=True) - org_filters = Q(organization_id=org.pk) | Q(organization_id=None) + + # TODO: do we skip all these checks if user is superuser? + if organization_id: + org = get_object_or_404(Organization, pk=organization_id, is_active=True) + org_filters = Q(organization_id=org.pk) + # if the user is superuser then we need to fetch all the templates + elif user.is_superuser: + org_filters = Q(organization_id__isnull=False) + # else fetch templates of organizations managed by the user + else: + org_filters = Q(organization_id__in=user.organizations_managed) + + # this filter is for shared templates + org_filters |= Q(organization_id=None) + filter_options = {} if backend: filter_options.update(backend=backend) else: filter_options.update(required=False, default=False) - sort_value_subquery = None + sort_value_subquery = through_model = None # fetch the selected templates for the device or group by creating a subquery. # through_model and lookup_field are set based on the presence of device_id or # group_id. we need through_model as `sort_value` is a field of the through model. @@ -53,7 +72,8 @@ def get_relevant_templates(request, organization_id): ): through_model = DeviceGroup.templates.through lookup_field = "devicegroup_id" - if device_id or group_id: + # fetch selected templates only if device or group exists + if device_id or group_id and through_model: sort_value_subquery = django_models.Subquery( through_model.objects.filter( **{lookup_field: lookup.id}, template_id=django_models.OuterRef('pk') From 9125aec6ad2dd11de890131879a325dce4aabf1c Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Tue, 20 May 2025 23:26:44 +0530 Subject: [PATCH 04/15] [tests] Added assertion test for device template queries Added a test to assert the queries executed during templates fetch on device change page. Previous count of this fetch combined with `get_relevant_templates` was 26 due to template fetch query being duplicated. With new changes the count is down to 24. Signed-off-by: DragnEmperor --- .../config/tests/test_admin.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 5ebc3ac41..b871286df 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -4,6 +4,7 @@ from unittest.mock import patch from uuid import uuid4 +import django from django.contrib.admin.models import LogEntry from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError @@ -2018,6 +2019,41 @@ def test_vpn_template_switch(self): ) self.assertEqual(config.vpnclient_set.count(), 1) + # helper for asserting queries executed during template fetch for a device + def _verify_template_queries(self, config, count): + path = reverse(f'admin:{self.app_label}_device_change', args=[config.device.pk]) + for i in range(count): + self._create_template(name=f'template-{i}') + expected_count = 24 + if django.VERSION < (5, 2): + # In django version < 5.2, there is an extra SAVEPOINT query + # leading to extra RELEASE SAVEPOINT query, thus 2 extra queries + expected_count += 2 + with self.assertNumQueries(expected_count): + # contains 22 queries for fetching normal device data + response = self.client.get(path) + # contains 2 queries, 1 for fetching organization + # and 1 for fetching templates + response = self.client.get( + reverse( + 'admin:get_relevant_templates', args=[config.device.organization.pk] + ) + ) + self.assertEqual(response.status_code, 200) + + # ensuring queries are consistent for different number of templates + def test_templates_fetch_queries_1(self): + config = self._create_config(organization=self._get_org()) + self._verify_template_queries(config, 1) + + def test_templates_fetch_queries_5(self): + config = self._create_config(organization=self._get_org()) + self._verify_template_queries(config, 1) + + def test_templates_fetch_queries_10(self): + config = self._create_config(organization=self._get_org()) + self._verify_template_queries(config, 1) + class TestTransactionAdmin( CreateConfigTemplateMixin, From 8b66ab18cdd9b4ef36b9c788de32f99205f87dd9 Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Thu, 22 May 2025 23:20:30 +0530 Subject: [PATCH 05/15] [change] Modified existing queries for better readability The queries for fetching the templates with sorted in `get_relevant_templates` via Subquery, while were working, were a bit complex to understand for future. Fetching templates using config/devicegroup maintains sorting, making the fetching query much readable. Signed-off-by: DragnEmperor --- openwisp_controller/config/views.py | 60 +++++++++++------------------ 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index c0af2197a..f77ed0def 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -2,9 +2,7 @@ from copy import deepcopy from uuid import UUID -from django.db import models as django_models from django.db.models import Q -from django.db.models.functions import Coalesce from django.http import HttpResponse, JsonResponse from django.utils import timezone from django.utils.module_loading import import_string @@ -39,7 +37,6 @@ def get_relevant_templates(request, organization_id): ): return HttpResponse(status=403) - # TODO: do we skip all these checks if user is superuser? if organization_id: org = get_object_or_404(Organization, pk=organization_id, is_active=True) org_filters = Q(organization_id=org.pk) @@ -58,45 +55,25 @@ def get_relevant_templates(request, organization_id): filter_options.update(backend=backend) else: filter_options.update(required=False, default=False) - sort_value_subquery = through_model = None - # fetch the selected templates for the device or group by creating a subquery. - # through_model and lookup_field are set based on the presence of device_id or - # group_id. we need through_model as `sort_value` is a field of the through model. - # the subquery will be used to annotate the queryset with the sort_value - # of the selected templates. - if device_id and (lookup := Config.objects.filter(device_id=device_id).first()): - through_model = Config.templates.through - lookup_field = "config_id" - if group_id and ( - lookup := DeviceGroup.objects.filter(Q(pk=group_id) & (org_filters)).first() - ): - through_model = DeviceGroup.templates.through - lookup_field = "devicegroup_id" - # fetch selected templates only if device or group exists - if device_id or group_id and through_model: - sort_value_subquery = django_models.Subquery( - through_model.objects.filter( - **{lookup_field: lookup.id}, template_id=django_models.OuterRef('pk') - ).values('sort_value')[:1], - output_field=django_models.IntegerField(), - ) - # annotated a selected field which is True based on sort_value - # if sort_value is 9999 then selected is False else True + queryset = ( Template.objects.filter(**filter_options) .filter(org_filters) - .annotate( - sort_value=Coalesce(sort_value_subquery, django_models.Value(9999)), - selected=django_models.Case( - django_models.When(sort_value=9999, then=django_models.Value(False)), - default=django_models.Value(True), - output_field=django_models.BooleanField(), - ), - ) - .order_by("sort_value") .only("id", "name", "backend", "default", "required") ) + selected_templates = [] + if device_id: + selected_templates = ( + Config.templates.through.objects.filter(config__device_id=device_id) + .order_by("sort_value") + .values_list("template_id", flat=True) + ) + if group_id: + selected_templates = DeviceGroup.templates.through.objects.filter( + devicegroup_id=group_id, devicegroup__organization_id=organization_id + ).values_list('template_id', flat=True) + selected_templates = [str(template) for template in selected_templates] relevant_templates = {} for template in queryset: relevant_templates[str(template.pk)] = dict( @@ -104,8 +81,17 @@ def get_relevant_templates(request, organization_id): backend=template.get_backend_display(), default=template.default, required=template.required, - selected=template.selected, + selected=str(template.pk) in selected_templates, + ) + # sort based on order of selected_templates + relevant_templates = dict( + sorted( + relevant_templates.items(), + key=lambda item: selected_templates.index(item[0]) + if item[0] in selected_templates + else len(selected_templates), ) + ) return JsonResponse(relevant_templates) From 12cedbe38096bbf77db38d899879b07911acbef7 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 26 May 2025 23:57:09 +0530 Subject: [PATCH 06/15] [change] Improved relevant templates --- .../static/config/js/relevant_templates.js | 131 ++++------ .../config/tests/test_admin.py | 79 ++++++ .../config/tests/test_selenium.py | 238 +++++++++++++----- openwisp_controller/config/views.py | 54 ++-- 4 files changed, 321 insertions(+), 181 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index 21e8079ee..6f1f69a24 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -89,16 +89,35 @@ django.jQuery(function ($) { } } }, + getRelevantTemplateUrl = function (orgID, backend) { + // Returns the URL to fetch relevant templates + var baseUrl = window._relevantTemplateUrl.replace("org_id", orgID); + var url = new URL(baseUrl, window.location.origin); + + // Get relevant templates of selected org and backend + if (backend) { + url.searchParams.set("backend", backend); + } + if (isDeviceGroup() && !$(".add-form").length) { + // Get the group id from the URL + // TODO: This is fragile, consider using a more robust way to get the group id. + var pathParts = window.location.pathname.split("/"); + url.searchParams.set("group_id", pathParts[pathParts.length - 3]); + } else if ($('input[name="config-0-device"]').length) { + url.searchParams.set( + "device_id", + $('input[name="config-0-device"]').val(), + ); + } + return url.toString(); + }, showRelevantTemplates = function () { var orgID = $(orgFieldSelector).val(), backend = isDeviceGroup() ? "" : $(backendFieldSelector).val(), selectedTemplates; // Hide templates if no organization or backend is selected - if ( - (orgID && orgID.length === 0) || - (!isDeviceGroup() && backend.length === 0) - ) { + if (!orgID || (!isDeviceGroup() && backend.length === 0)) { resetTemplateOptions(); updateTemplateHelpText(); return; @@ -109,77 +128,24 @@ django.jQuery(function ($) { // when the user has changed any of organization or backend field. // selectedTemplates will be an empty string if no template is selected // ''.split(',') returns [''] hence, this case requires special handling - selectedTemplates = isDeviceGroup() - ? parseSelectedTemplates($("#id_templates").val()) - : parseSelectedTemplates( - django._owcInitialValues[templatesFieldName()], - ); - } - - var url = window._relevantTemplateUrl.replace("org_id", orgID); - // Get relevant templates of selected org and backend - url = url + "?backend=" + backend; - if (!isDeviceGroup()) { - var deviceID = $('input[name="config-0-device"]').val(); - url = url + "&device=" + deviceID; - } else { - // Get the group id from the URL - var pathParts = window.location.pathname.split("/"); - url = url + "&group=" + pathParts[pathParts.length - 3]; + selectedTemplates = parseSelectedTemplates( + $('input[name="' + templatesFieldName() + '"]').val(), + ); } + var url = getRelevantTemplateUrl(orgID, backend); $.get(url).done(function (data) { resetTemplateOptions(); var enabledTemplates = [], sortedm2mUl = $("ul.sortedm2m-items:first"), sortedm2mPrefixUl = $("ul.sortedm2m-items:last"); - // Adds "li" elements for templates that are already selected - // in the database. Select these templates and remove their key from "data" - // This maintains the order of the templates and keep - // enabled templates on the top - if (selectedTemplates !== undefined) { - selectedTemplates.forEach(function (templateId, index) { - // corner case in which backend of template does not match - if (!data[templateId]) { - return; - } - var element = getTemplateOptionElement( - index, - templateId, - data[templateId], - true, - false, - ), - prefixElement = getTemplateOptionElement( - index, - templateId, - data[templateId], - true, - true, - ); - sortedm2mUl.append(element); - if (!isDeviceGroup()) { - sortedm2mPrefixUl.append(prefixElement); - } - delete data[templateId]; - }); - } - - // Adds "li" elements for templates that are not selected - // in the database. - var counter = - selectedTemplates !== undefined ? selectedTemplates.length : 0, - deviceTemplates = []; + // Adds "li" elements for templates Object.keys(data).forEach(function (templateId, index) { - // corner case in which backend of template does not match - if (!data[templateId]) { - return; - } - index = index + counter; var isSelected = - data[templateId].default && - selectedTemplates === undefined && - !data[templateId].required, + data[templateId].selected || + (data[templateId].default && + selectedTemplates === undefined && + !data[templateId].required), element = getTemplateOptionElement( index, templateId, @@ -199,9 +165,6 @@ django.jQuery(function ($) { if (isSelected === true) { enabledTemplates.push(templateId); } - if (element.children().children("input").prop("checked") === true) { - deviceTemplates.push(templateId); - } sortedm2mUl.append(element); if (!isDeviceGroup()) { sortedm2mPrefixUl.append(prefixElement); @@ -210,30 +173,20 @@ django.jQuery(function ($) { if (firstRun === true && selectedTemplates !== undefined) { updateTemplateSelection(selectedTemplates); } - // this runs on first load and sets the name of hidden input field which tracks - // the selected templates. - if (selectedTemplates === undefined) { - if (!isDeviceGroup()) { - $( - `#config-0 .field-templates .sortedm2m-container input[type="hidden"]`, - ) - .first() - .attr("name", templatesFieldName()); - // set the initial value of the hidden input field - // to the selected templates - django._owcInitialValues[templatesFieldName()] = - deviceTemplates.join(","); - } else { - $( - `.field-templates .sortedm2m-container input[type="hidden"]`, - ).attr("name", templatesFieldName()); - } - } updateTemplateHelpText(); updateConfigTemplateField(enabledTemplates); }); }, + initTemplateField = function () { + // sortedm2m generates a hidden input dynamically using rendered input checkbox elements, + // but because the queryset is set to None in the Django admin, the input is created + // without a name attribute. This workaround assigns the correct name to the hidden input. + $('.sortedm2m-container input[type="hidden"][id="undefined"]') + .first() + .attr("name", templatesFieldName()); + }, bindDefaultTemplateLoading = function () { + initTemplateField(); var backendField = $(backendFieldSelector); $(orgFieldSelector).change(function () { // Only fetch templates when backend field is present @@ -246,11 +199,13 @@ django.jQuery(function ($) { addChangeEventHandlerToBackendField(); } else if (isDeviceGroup()) { // Initially request data to get templates + initTemplateField(); showRelevantTemplates(); } else { // Add view: backendField is added when user adds configuration $("#config-group > fieldset.module").ready(function () { $("div.add-row > a").one("click", function () { + initTemplateField(); addChangeEventHandlerToBackendField(); }); }); diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index b871286df..521141e41 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1568,6 +1568,85 @@ def test_add_vpn(self): response, 'value="openwisp_controller.vpn_backends.OpenVpn" selected' ) + def test_vpn_clients_deleted(self): + def _update_template(templates): + params.update( + { + 'config-0-templates': ','.join( + [str(template.pk) for template in templates] + ) + } + ) + response = self.client.post(path, data=params, follow=True) + self.assertEqual(response.status_code, 200) + return response + + vpn = self._create_vpn() + template = self._create_template() + vpn_template = self._create_template( + name='vpn-test', + type='vpn', + vpn=vpn, + auto_cert=True, + ) + cert_query = Cert.objects.exclude(pk=vpn.cert_id) + valid_cert_query = cert_query.filter(revoked=False) + revoked_cert_query = cert_query.filter(revoked=True) + + # Add a new device + path = reverse(f'admin:{self.app_label}_device_add') + params = self._get_device_params(org=self._get_org()) + response = self.client.post(path, data=params, follow=True) + self.assertEqual(response.status_code, 200) + + config = Device.objects.get(name=params['name']).config + self.assertEqual(config.vpnclient_set.count(), 0) + self.assertEqual(config.templates.count(), 0) + + path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id]) + params.update( + { + 'config-0-id': str(config.pk), + 'config-0-device': str(config.device_id), + 'config-INITIAL_FORMS': 1, + '_continue': True, + } + ) + + with self.subTest('Adding only VpnClient template'): + # Adding VpnClient template to the device + _update_template(templates=[vpn_template]) + + self.assertEqual(config.templates.count(), 1) + self.assertEqual(config.vpnclient_set.count(), 1) + self.assertEqual(cert_query.count(), 1) + self.assertEqual(valid_cert_query.count(), 1) + + # Remove VpnClient template from the device + _update_template(templates=[]) + + self.assertEqual(config.templates.count(), 0) + self.assertEqual(config.vpnclient_set.count(), 0) + # Removing VPN template marks the related certificate as revoked + self.assertEqual(revoked_cert_query.count(), 1) + self.assertEqual(valid_cert_query.count(), 0) + + with self.subTest('Add VpnClient template along with another template'): + # Adding templates to the device + _update_template(templates=[template, vpn_template]) + + self.assertEqual(config.templates.count(), 2) + self.assertEqual(config.vpnclient_set.count(), 1) + self.assertEqual(valid_cert_query.count(), 1) + + # Remove VpnClient template from the device + _update_template(templates=[template]) + + self.assertEqual(config.templates.count(), 1) + self.assertEqual(config.vpnclient_set.count(), 0) + self.assertEqual(valid_cert_query.count(), 0) + self.assertEqual(revoked_cert_query.count(), 2) + def test_ip_not_in_add_device(self): path = reverse(f"admin:{self.app_label}_device_add") response = self.client.get(path) diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index 86f42e75e..be83ead0d 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -12,15 +12,43 @@ from selenium.webdriver.support.ui import Select, WebDriverWait from swapper import load_model -from openwisp_utils.tests import SeleniumTestMixin +from openwisp_controller.config.base import device +from openwisp_users.tests.utils import TestOrganizationMixin +from openwisp_utils.tests import SeleniumTestMixin as BaseSeleniumTestMixin from .utils import CreateConfigTemplateMixin, TestVpnX509Mixin, TestWireguardVpnMixin Device = load_model("config", "Device") +DeviceGroup = load_model("config", "DeviceGroup") Cert = load_model("django_x509", "Cert") -@tag("selenium_tests") +class SeleniumTestMixin(BaseSeleniumTestMixin): + def _select_organization(self, org): + self.find_element( + by=By.CSS_SELECTOR, value='#select2-id_organization-container' + ).click() + self.wait_for_invisibility( + By.CSS_SELECTOR, '.select2-results__option.loading-results' + ) + self.find_element(by=By.CLASS_NAME, value='select2-search__field').send_keys( + org.name + ) + self.wait_for_invisibility( + By.CSS_SELECTOR, '.select2-results__option.loading-results' + ) + self.find_element(by=By.CLASS_NAME, value='select2-results__option').click() + + def _verify_templates_visibility(self, hidden=None, visible=None): + hidden = hidden or [] + visible = visible or [] + for template in hidden: + self.wait_for_invisibility(By.XPATH, f'//*[@value="{template.id}"]') + for template in visible: + self.wait_for_visibility(By.XPATH, f'//*[@value="{template.id}"]') + + +@tag('selenium_tests') class TestDeviceAdmin( SeleniumTestMixin, CreateConfigTemplateMixin, @@ -162,30 +190,55 @@ def test_multiple_organization_templates(self): name="org2 default", organization=org2, default=True ) - org1_device = self._create_config( + device = self._create_config( device=self._create_device(organization=org1) ).device self.login() self.open( - reverse("admin:config_device_change", args=[org1_device.id]) - + "#config-group" + reverse("admin:config_device_change", args=[device.id]) + "#config-group" ) self.hide_loading_overlay() - # org2 templates should not be visible - self.wait_for_invisibility( - By.XPATH, f'//*[@value="{org2_required_template.id}"]' - ) - self.wait_for_invisibility( - By.XPATH, f'//*[@value="{org2_default_template.id}"]' - ) - # org1 and shared templates should be visible - self.wait_for_visibility(By.XPATH, f'//*[@value="{org1_required_template.id}"]') - self.wait_for_visibility(By.XPATH, f'//*[@value="{org1_default_template.id}"]') - self.wait_for_visibility( - By.XPATH, f'//*[@value="{shared_required_template.id}"]' - ) + with self.subTest('only org1 and shared templates should be visible'): + self._verify_templates_visibility( + hidden=[org2_required_template, org2_default_template], + visible=[ + org1_required_template, + org1_default_template, + shared_required_template, + ], + ) + + with self.subTest('changing org should update templates'): + self.find_element( + By.CSS_SELECTOR, value='a[href="#overview-group"]' + ).click() + self._select_organization(org2) + self.find_element( + by=By.CSS_SELECTOR, value='a[href="#config-group"]' + ).click() + self._verify_templates_visibility( + hidden=[ + org1_required_template, + org1_default_template, + ], + visible=[ + org2_required_template, + org2_default_template, + shared_required_template, + ], + ) + self.find_element( + by=By.CSS_SELECTOR, value='input[name="_continue"]' + ).click() + self._wait_until_page_ready() + device.refresh_from_db() + device.config.refresh_from_db() + self.assertEqual(device.organization, org2) + self.assertEqual(device.config.templates.count(), 2) + self.assertIn(org2_required_template, device.config.templates.all()) + self.assertIn(org2_default_template, device.config.templates.all()) def test_change_config_backend(self): device = self._create_config(organization=self._get_org()).device @@ -348,61 +401,108 @@ def test_add_remove_templates(self): self.assertEqual(config.templates.count(), 0) self.assertEqual(config.status, 'modified') - def test_vpn_clients_deleted(self): - vpn = self._create_vpn() - template = self._create_template() - vpn_template = self._create_template( - name='vpn-test', - type='vpn', - vpn=vpn, - auto_cert=True, + +@tag('selenium_tests') +class TestDeviceGroupAdmin( + SeleniumTestMixin, + CreateConfigTemplateMixin, + StaticLiveServerTestCase, +): + def test_show_relevant_templates(self): + org1 = self._create_org(name='org1', slug='org1') + org2 = self._create_org(name='org2', slug='org2') + shared_template = self._create_template(name='shared template') + org1_template = self._create_template(name='org1 template', organization=org1) + org1_required_template = self._create_template( + name='org1 required', organization=org1, required=True + ) + org1_default_template = self._create_template( + name='org1 default', organization=org1, default=True + ) + org2_template = self._create_template(name='org2 template', organization=org2) + org2_required_template = self._create_template( + name='org2 required', organization=org2, required=True + ) + org2_default_template = self._create_template( + name='org2 default', organization=org2, default=True ) - cert_query = Cert.objects.exclude(pk=vpn.cert_id) - valid_cert_query = cert_query.filter(revoked=False) - revoked_cert_query = cert_query.filter(revoked=True) - config = self._create_config(organization=self._get_org()) - device = config.device - self.assertEqual(config.vpnclient_set.count(), 0) - self.assertEqual(config.templates.count(), 0) self.login() - # some times the url fetching in js gives unauthorized error - # so we add a wait to allow login to complete - time.sleep(2) - - with self.subTest('Adding only VpnClient template'): - # Adding VpnClient template to the device - self._update_template(device.id, [vpn_template]) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.vpnclient_set.count(), 1) - self.assertEqual(cert_query.count(), 1) - self.assertEqual(valid_cert_query.count(), 1) - - # Remove VpnClient template from the device - self._update_template(device.id, [vpn_template], is_enabled=True) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 0) - self.assertEqual(config.vpnclient_set.count(), 0) - # Removing VPN template marks the related certificate as revoked - self.assertEqual(revoked_cert_query.count(), 1) - self.assertEqual(valid_cert_query.count(), 0) - - with self.subTest('Add VpnClient template along with another template'): - # Adding templates to the device - self._update_template(device.id, [template, vpn_template]) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 2) - self.assertEqual(config.vpnclient_set.count(), 1) - self.assertEqual(valid_cert_query.count(), 1) + self.open(reverse('admin:config_devicegroup_add')) + self.assertEqual( + self.wait_for_visibility( + By.CSS_SELECTOR, '.sortedm2m-container .help' + ).text, + 'No Template available', + ) + self.find_element(by=By.CSS_SELECTOR, value='input[name="name"]').send_keys( + 'Test Device Group' + ) + self._select_organization(org1) + self._verify_templates_visibility( + hidden=[ + org1_default_template, + org1_required_template, + org2_template, + org2_default_template, + org2_required_template, + ], + visible=[shared_template, org1_template], + ) + # Select org1 template + self.find_element( + by=By.XPATH, value=f'//*[@value="{org1_template.id}"]' + ).click() + self.web_driver.execute_script( + "window.scrollTo(0, document.body.scrollHeight);" + ) + self.find_element(by=By.CSS_SELECTOR, value='input[name="_continue"]').click() + self._wait_until_page_ready() + device_group = DeviceGroup.objects.first() + self.assertEqual(device_group.name, 'Test Device Group') + self.assertIn(org1_template, device_group.templates.all()) + self.assertEqual( + self.find_element( + by=By.CSS_SELECTOR, + value=f'input[type="checkbox"][value="{org1_template.id}"]', + ).is_selected(), + True, + ) - # Remove VpnClient template from the device - self._update_template(device.id, [vpn_template], is_enabled=True) - config.refresh_from_db() - self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.vpnclient_set.count(), 0) - self.assertEqual(valid_cert_query.count(), 0) - self.assertEqual(revoked_cert_query.count(), 2) + with self.subTest('Change organization to org2'): + self._select_organization(org2) + self._verify_templates_visibility( + hidden=[ + org1_template, + org1_default_template, + org1_required_template, + org2_required_template, + org2_default_template, + ], + visible=[ + shared_template, + org2_template, + ], + ) + self.assertEqual( + self.find_element( + by=By.CSS_SELECTOR, + value=f'input[type="checkbox"][value="{org2_template.id}"]', + ).is_selected(), + False, + ) + self.find_element( + by=By.CSS_SELECTOR, value='input[name="_continue"]' + ).click() + self._wait_until_page_ready() + self.assertEqual(device_group.templates.count(), 0) + self.assertEqual( + self.find_element( + by=By.CSS_SELECTOR, + value=f'input[type="checkbox"][value="{org2_template.id}"]', + ).is_selected(), + False, + ) @tag("selenium_tests") diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index f77ed0def..25da3e1fa 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -1,9 +1,11 @@ import json +import re +from collections import OrderedDict from copy import deepcopy from uuid import UUID from django.db.models import Q -from django.http import HttpResponse, JsonResponse +from django.http import Http404, HttpResponse, JsonResponse from django.utils import timezone from django.utils.module_loading import import_string from django.utils.translation import gettext as _ @@ -11,7 +13,6 @@ from swapper import load_model from .settings import BACKENDS, VPN_BACKENDS -from .utils import get_object_or_404 Organization = load_model("openwisp_users", "Organization") Template = load_model("config", "Template") @@ -25,8 +26,8 @@ def get_relevant_templates(request, organization_id): returns default templates of specified organization """ backend = request.GET.get("backend", None) - device_id = request.GET.get("device", None) - group_id = request.GET.get("group", None) + device_id = request.GET.get("device_id", None) + group_id = request.GET.get("group_id", None) user = request.user # organization_id is passed as 'null' for add device organization_id = None if organization_id == 'null' else organization_id @@ -38,8 +39,9 @@ def get_relevant_templates(request, organization_id): return HttpResponse(status=403) if organization_id: - org = get_object_or_404(Organization, pk=organization_id, is_active=True) - org_filters = Q(organization_id=org.pk) + if not Organization.objects.filter(pk=organization_id).exists(): + raise Http404(_("Organization does not exist.")) + org_filters = Q(organization_id=organization_id) # if the user is superuser then we need to fetch all the templates elif user.is_superuser: org_filters = Q(organization_id__isnull=False) @@ -64,34 +66,38 @@ def get_relevant_templates(request, organization_id): selected_templates = [] if device_id: selected_templates = ( - Config.templates.through.objects.filter(config__device_id=device_id) - .order_by("sort_value") - .values_list("template_id", flat=True) + Config.objects.prefetch_related('templates') + .only('templates') + .get(device_id=device_id) + .templates.all() ) if group_id: - selected_templates = DeviceGroup.templates.through.objects.filter( - devicegroup_id=group_id, devicegroup__organization_id=organization_id - ).values_list('template_id', flat=True) + selected_templates = ( + DeviceGroup.objects.prefetch_related('templates') + .only('templates') + .get(pk=group_id) + .templates.filter(organization_id=organization_id) + ) - selected_templates = [str(template) for template in selected_templates] - relevant_templates = {} - for template in queryset: + relevant_templates = OrderedDict() + for template in selected_templates: relevant_templates[str(template.pk)] = dict( name=template.name, backend=template.get_backend_display(), default=template.default, required=template.required, - selected=str(template.pk) in selected_templates, + selected=True, ) - # sort based on order of selected_templates - relevant_templates = dict( - sorted( - relevant_templates.items(), - key=lambda item: selected_templates.index(item[0]) - if item[0] in selected_templates - else len(selected_templates), + + for template in queryset.exclude(pk__in=relevant_templates.keys()): + relevant_templates[str(template.pk)] = dict( + name=template.name, + backend=template.get_backend_display(), + default=template.default, + required=template.required, + selected=False, ) - ) + return JsonResponse(relevant_templates) From ca78b6506346cebb9852f800509ec0e88ef69d4a Mon Sep 17 00:00:00 2001 From: DragnEmperor Date: Tue, 27 May 2025 18:57:35 +0530 Subject: [PATCH 07/15] [changes] Formatting changes as per new QA rules Formatted the changes as per the new QA checks and rules in `openwisp-qa-format`. For the tests inside `test_views` to work properly, made minor modifications in `get_relevant_templates`. Also fixed the type in template count in `test_templates_fetch_queries`. Signed-off-by: DragnEmperor --- openwisp_controller/config/admin.py | 8 +-- .../config/tests/test_admin.py | 34 +++++----- .../config/tests/test_selenium.py | 66 +++++++++---------- .../config/tests/test_views.py | 2 +- openwisp_controller/config/views.py | 20 +++--- 5 files changed, 66 insertions(+), 64 deletions(-) diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py index 9016791d6..3c56d320f 100644 --- a/openwisp_controller/config/admin.py +++ b/openwisp_controller/config/admin.py @@ -466,8 +466,8 @@ def get_fields(self, request, obj): def formfield_for_manytomany(self, db_field, request, **kwargs): # setting queryset none for all requests except POST as queryset # is required for the form to be valid - if db_field.name == 'templates' and request.method != 'POST': - kwargs['queryset'] = Template.objects.none() + if db_field.name == "templates" and request.method != "POST": + kwargs["queryset"] = Template.objects.none() return super().formfield_for_manytomany(db_field, request, **kwargs) @@ -1329,8 +1329,8 @@ def get_extra_context(self, pk=None): def formfield_for_manytomany(self, db_field, request, **kwargs): # setting queryset none for all requests except POST as queryset # is required for the form to be valid - if db_field.name == 'templates' and request.method != 'POST': - kwargs['queryset'] = Template.objects.none() + if db_field.name == "templates" and request.method != "POST": + kwargs["queryset"] = Template.objects.none() return super().formfield_for_manytomany(db_field, request, **kwargs) diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py index 521141e41..49152293c 100644 --- a/openwisp_controller/config/tests/test_admin.py +++ b/openwisp_controller/config/tests/test_admin.py @@ -1572,7 +1572,7 @@ def test_vpn_clients_deleted(self): def _update_template(templates): params.update( { - 'config-0-templates': ','.join( + "config-0-templates": ",".join( [str(template.pk) for template in templates] ) } @@ -1584,8 +1584,8 @@ def _update_template(templates): vpn = self._create_vpn() template = self._create_template() vpn_template = self._create_template( - name='vpn-test', - type='vpn', + name="vpn-test", + type="vpn", vpn=vpn, auto_cert=True, ) @@ -1594,26 +1594,26 @@ def _update_template(templates): revoked_cert_query = cert_query.filter(revoked=True) # Add a new device - path = reverse(f'admin:{self.app_label}_device_add') + path = reverse(f"admin:{self.app_label}_device_add") params = self._get_device_params(org=self._get_org()) response = self.client.post(path, data=params, follow=True) self.assertEqual(response.status_code, 200) - config = Device.objects.get(name=params['name']).config + config = Device.objects.get(name=params["name"]).config self.assertEqual(config.vpnclient_set.count(), 0) self.assertEqual(config.templates.count(), 0) - path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id]) + path = reverse(f"admin:{self.app_label}_device_change", args=[config.device_id]) params.update( { - 'config-0-id': str(config.pk), - 'config-0-device': str(config.device_id), - 'config-INITIAL_FORMS': 1, - '_continue': True, + "config-0-id": str(config.pk), + "config-0-device": str(config.device_id), + "config-INITIAL_FORMS": 1, + "_continue": True, } ) - with self.subTest('Adding only VpnClient template'): + with self.subTest("Adding only VpnClient template"): # Adding VpnClient template to the device _update_template(templates=[vpn_template]) @@ -1631,7 +1631,7 @@ def _update_template(templates): self.assertEqual(revoked_cert_query.count(), 1) self.assertEqual(valid_cert_query.count(), 0) - with self.subTest('Add VpnClient template along with another template'): + with self.subTest("Add VpnClient template along with another template"): # Adding templates to the device _update_template(templates=[template, vpn_template]) @@ -2100,9 +2100,9 @@ def test_vpn_template_switch(self): # helper for asserting queries executed during template fetch for a device def _verify_template_queries(self, config, count): - path = reverse(f'admin:{self.app_label}_device_change', args=[config.device.pk]) + path = reverse(f"admin:{self.app_label}_device_change", args=[config.device.pk]) for i in range(count): - self._create_template(name=f'template-{i}') + self._create_template(name=f"template-{i}") expected_count = 24 if django.VERSION < (5, 2): # In django version < 5.2, there is an extra SAVEPOINT query @@ -2115,7 +2115,7 @@ def _verify_template_queries(self, config, count): # and 1 for fetching templates response = self.client.get( reverse( - 'admin:get_relevant_templates', args=[config.device.organization.pk] + "admin:get_relevant_templates", args=[config.device.organization.pk] ) ) self.assertEqual(response.status_code, 200) @@ -2127,11 +2127,11 @@ def test_templates_fetch_queries_1(self): def test_templates_fetch_queries_5(self): config = self._create_config(organization=self._get_org()) - self._verify_template_queries(config, 1) + self._verify_template_queries(config, 5) def test_templates_fetch_queries_10(self): config = self._create_config(organization=self._get_org()) - self._verify_template_queries(config, 1) + self._verify_template_queries(config, 10) class TestTransactionAdmin( diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index be83ead0d..44a90265b 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -12,8 +12,6 @@ from selenium.webdriver.support.ui import Select, WebDriverWait from swapper import load_model -from openwisp_controller.config.base import device -from openwisp_users.tests.utils import TestOrganizationMixin from openwisp_utils.tests import SeleniumTestMixin as BaseSeleniumTestMixin from .utils import CreateConfigTemplateMixin, TestVpnX509Mixin, TestWireguardVpnMixin @@ -26,18 +24,18 @@ class SeleniumTestMixin(BaseSeleniumTestMixin): def _select_organization(self, org): self.find_element( - by=By.CSS_SELECTOR, value='#select2-id_organization-container' + by=By.CSS_SELECTOR, value="#select2-id_organization-container" ).click() self.wait_for_invisibility( - By.CSS_SELECTOR, '.select2-results__option.loading-results' + By.CSS_SELECTOR, ".select2-results__option.loading-results" ) - self.find_element(by=By.CLASS_NAME, value='select2-search__field').send_keys( + self.find_element(by=By.CLASS_NAME, value="select2-search__field").send_keys( org.name ) self.wait_for_invisibility( - By.CSS_SELECTOR, '.select2-results__option.loading-results' + By.CSS_SELECTOR, ".select2-results__option.loading-results" ) - self.find_element(by=By.CLASS_NAME, value='select2-results__option').click() + self.find_element(by=By.CLASS_NAME, value="select2-results__option").click() def _verify_templates_visibility(self, hidden=None, visible=None): hidden = hidden or [] @@ -48,7 +46,7 @@ def _verify_templates_visibility(self, hidden=None, visible=None): self.wait_for_visibility(By.XPATH, f'//*[@value="{template.id}"]') -@tag('selenium_tests') +@tag("selenium_tests") class TestDeviceAdmin( SeleniumTestMixin, CreateConfigTemplateMixin, @@ -58,7 +56,7 @@ class TestDeviceAdmin( # helper function for adding/removing templates def _update_template(self, device_id, templates, is_enabled=False): self.open( - reverse('admin:config_device_change', args=[device_id]) + '#config-group' + reverse("admin:config_device_change", args=[device_id]) + "#config-group" ) self.wait_for_presence(By.CSS_SELECTOR, 'input[name="config-0-templates"]') @@ -79,8 +77,8 @@ def _update_template(self, device_id, templates, is_enabled=False): self.web_driver.execute_script( 'document.querySelector("#ow-user-tools").style.display="none"' ) - self.find_element(by=By.NAME, value='_save').click() - self.wait_for_presence(By.CSS_SELECTOR, '.messagelist .success', timeout=5) + self.find_element(by=By.NAME, value="_save").click() + self.wait_for_presence(By.CSS_SELECTOR, ".messagelist .success", timeout=5) def test_create_new_device(self): required_template = self._create_template(name="Required", required=True) @@ -200,7 +198,7 @@ def test_multiple_organization_templates(self): ) self.hide_loading_overlay() - with self.subTest('only org1 and shared templates should be visible'): + with self.subTest("only org1 and shared templates should be visible"): self._verify_templates_visibility( hidden=[org2_required_template, org2_default_template], visible=[ @@ -210,7 +208,7 @@ def test_multiple_organization_templates(self): ], ) - with self.subTest('changing org should update templates'): + with self.subTest("changing org should update templates"): self.find_element( By.CSS_SELECTOR, value='a[href="#overview-group"]' ).click() @@ -387,56 +385,56 @@ def test_add_remove_templates(self): # so we add a wait to allow login to complete time.sleep(2) - with self.subTest('Template should be added'): + with self.subTest("Template should be added"): self._update_template(device.id, templates=[template]) config.refresh_from_db() self.assertEqual(config.templates.count(), 1) - self.assertEqual(config.status, 'modified') + self.assertEqual(config.status, "modified") config.set_status_applied() - self.assertEqual(config.status, 'applied') + self.assertEqual(config.status, "applied") - with self.subTest('Template should be removed'): + with self.subTest("Template should be removed"): self._update_template(device.id, templates=[template], is_enabled=True) config.refresh_from_db() self.assertEqual(config.templates.count(), 0) - self.assertEqual(config.status, 'modified') + self.assertEqual(config.status, "modified") -@tag('selenium_tests') +@tag("selenium_tests") class TestDeviceGroupAdmin( SeleniumTestMixin, CreateConfigTemplateMixin, StaticLiveServerTestCase, ): def test_show_relevant_templates(self): - org1 = self._create_org(name='org1', slug='org1') - org2 = self._create_org(name='org2', slug='org2') - shared_template = self._create_template(name='shared template') - org1_template = self._create_template(name='org1 template', organization=org1) + org1 = self._create_org(name="org1", slug="org1") + org2 = self._create_org(name="org2", slug="org2") + shared_template = self._create_template(name="shared template") + org1_template = self._create_template(name="org1 template", organization=org1) org1_required_template = self._create_template( - name='org1 required', organization=org1, required=True + name="org1 required", organization=org1, required=True ) org1_default_template = self._create_template( - name='org1 default', organization=org1, default=True + name="org1 default", organization=org1, default=True ) - org2_template = self._create_template(name='org2 template', organization=org2) + org2_template = self._create_template(name="org2 template", organization=org2) org2_required_template = self._create_template( - name='org2 required', organization=org2, required=True + name="org2 required", organization=org2, required=True ) org2_default_template = self._create_template( - name='org2 default', organization=org2, default=True + name="org2 default", organization=org2, default=True ) self.login() - self.open(reverse('admin:config_devicegroup_add')) + self.open(reverse("admin:config_devicegroup_add")) self.assertEqual( self.wait_for_visibility( - By.CSS_SELECTOR, '.sortedm2m-container .help' + By.CSS_SELECTOR, ".sortedm2m-container .help" ).text, - 'No Template available', + "No Template available", ) self.find_element(by=By.CSS_SELECTOR, value='input[name="name"]').send_keys( - 'Test Device Group' + "Test Device Group" ) self._select_organization(org1) self._verify_templates_visibility( @@ -459,7 +457,7 @@ def test_show_relevant_templates(self): self.find_element(by=By.CSS_SELECTOR, value='input[name="_continue"]').click() self._wait_until_page_ready() device_group = DeviceGroup.objects.first() - self.assertEqual(device_group.name, 'Test Device Group') + self.assertEqual(device_group.name, "Test Device Group") self.assertIn(org1_template, device_group.templates.all()) self.assertEqual( self.find_element( @@ -469,7 +467,7 @@ def test_show_relevant_templates(self): True, ) - with self.subTest('Change organization to org2'): + with self.subTest("Change organization to org2"): self._select_organization(org2) self._verify_templates_visibility( hidden=[ diff --git a/openwisp_controller/config/tests/test_views.py b/openwisp_controller/config/tests/test_views.py index 95d9e3fef..49c525171 100644 --- a/openwisp_controller/config/tests/test_views.py +++ b/openwisp_controller/config/tests/test_views.py @@ -243,7 +243,7 @@ def test_get_relevant_templates_400(self): response = self.client.get( reverse("admin:get_relevant_templates", args=["wrong"]) ) - self.assertEqual(response.status_code, 404) + self.assertEqual(response.status_code, 400) def test_get_default_values_authorization(self): org1 = self._get_org() diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index 25da3e1fa..d6818ccad 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -1,11 +1,10 @@ import json -import re from collections import OrderedDict from copy import deepcopy from uuid import UUID from django.db.models import Q -from django.http import Http404, HttpResponse, JsonResponse +from django.http import Http404, HttpResponse, HttpResponseBadRequest, JsonResponse from django.utils import timezone from django.utils.module_loading import import_string from django.utils.translation import gettext as _ @@ -30,7 +29,7 @@ def get_relevant_templates(request, organization_id): group_id = request.GET.get("group_id", None) user = request.user # organization_id is passed as 'null' for add device - organization_id = None if organization_id == 'null' else organization_id + organization_id = None if organization_id == "null" else organization_id if ( not user.is_superuser and organization_id @@ -39,7 +38,12 @@ def get_relevant_templates(request, organization_id): return HttpResponse(status=403) if organization_id: - if not Organization.objects.filter(pk=organization_id).exists(): + # return 400 if organization_id is not a valid UUID + try: + organization_id = UUID(organization_id, version=4) + except ValueError: + return HttpResponseBadRequest(_(f"{organization_id} is not a valid UUID.")) + if not Organization.objects.filter(pk=organization_id, is_active=True).exists(): raise Http404(_("Organization does not exist.")) org_filters = Q(organization_id=organization_id) # if the user is superuser then we need to fetch all the templates @@ -66,15 +70,15 @@ def get_relevant_templates(request, organization_id): selected_templates = [] if device_id: selected_templates = ( - Config.objects.prefetch_related('templates') - .only('templates') + Config.objects.prefetch_related("templates") + .only("templates") .get(device_id=device_id) .templates.all() ) if group_id: selected_templates = ( - DeviceGroup.objects.prefetch_related('templates') - .only('templates') + DeviceGroup.objects.prefetch_related("templates") + .only("templates") .get(pk=group_id) .templates.filter(organization_id=organization_id) ) From 51fac6bcf3310ce833ea8fe67745eb927fe18e3d Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 28 May 2025 15:33:23 +0530 Subject: [PATCH 08/15] [fix] Fixed relevant templates logic --- .../static/config/js/relevant_templates.js | 73 +++++++++--------- openwisp_controller/config/views.py | 75 +++++++++++-------- 2 files changed, 78 insertions(+), 70 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index 6f1f69a24..1157a442d 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -1,6 +1,6 @@ "use strict"; django.jQuery(function ($) { - var firstRun = true, + var pageLoading = true, backendFieldSelector = "#id_config-0-backend", orgFieldSelector = "#id_organization", isDeviceGroup = function () { @@ -9,6 +9,11 @@ django.jQuery(function ($) { templatesFieldName = function () { return isDeviceGroup() ? "templates" : "config-0-templates"; }, + isAddingNewObject = function () { + return isDeviceGroup() + ? !$(".add-form").length + : $('input[name="config-0-id"]').val().length === 0; + }, getTemplateOptionElement = function ( index, templateId, @@ -42,15 +47,6 @@ django.jQuery(function ($) { resetTemplateOptions = function () { $("ul.sortedm2m-items").empty(); }, - updateTemplateSelection = function (selectedTemplates) { - // Marks currently applied templates from database as selected - // Only executed at page load. - selectedTemplates.forEach(function (templateId) { - $( - `li.sortedm2m-item input[type="checkbox"][value="${templateId}"]:first`, - ).prop("checked", true); - }); - }, updateTemplateHelpText = function () { var helpText = "Choose items and order by drag & drop."; if ($("li.sortedm2m-item:first").length === 0) { @@ -74,12 +70,22 @@ django.jQuery(function ($) { showRelevantTemplates(); }, updateConfigTemplateField = function (templates) { - $(`input[name="${templatesFieldName()}"]`).attr( - "value", - templates.join(","), - ); + var value = templates.join(","), + templateField = templatesFieldName(); + $(`input[name="${templateField}"]`).attr("value", value); + if (pageLoading) { + django._owcInitialValues[templateField] = value; + } $("input.sortedm2m:first").trigger("change"); }, + getSelectedTemplates = function () { + // Returns the selected templates from the sortedm2m input + var selectedTemplates = {}; + $("input.sortedm2m:checked").each(function (index, element) { + selectedTemplates[$(element).val()] = $(element).prop("checked"); + }); + return selectedTemplates; + }, parseSelectedTemplates = function (selectedTemplates) { if (selectedTemplates !== undefined) { if (selectedTemplates === "") { @@ -103,18 +109,15 @@ django.jQuery(function ($) { // TODO: This is fragile, consider using a more robust way to get the group id. var pathParts = window.location.pathname.split("/"); url.searchParams.set("group_id", pathParts[pathParts.length - 3]); - } else if ($('input[name="config-0-device"]').length) { - url.searchParams.set( - "device_id", - $('input[name="config-0-device"]').val(), - ); + } else if ($('input[name="config-0-id"]').length) { + url.searchParams.set("config_id", $('input[name="config-0-id"]').val()); } return url.toString(); }, showRelevantTemplates = function () { var orgID = $(orgFieldSelector).val(), backend = isDeviceGroup() ? "" : $(backendFieldSelector).val(), - selectedTemplates; + currentSelection = getSelectedTemplates(); // Hide templates if no organization or backend is selected if (!orgID || (!isDeviceGroup() && backend.length === 0)) { @@ -123,15 +126,6 @@ django.jQuery(function ($) { return; } - if (firstRun) { - // selectedTemplates will be undefined on device add page or - // when the user has changed any of organization or backend field. - // selectedTemplates will be an empty string if no template is selected - // ''.split(',') returns [''] hence, this case requires special handling - selectedTemplates = parseSelectedTemplates( - $('input[name="' + templatesFieldName() + '"]').val(), - ); - } var url = getRelevantTemplateUrl(orgID, backend); $.get(url).done(function (data) { resetTemplateOptions(); @@ -142,10 +136,19 @@ django.jQuery(function ($) { // Adds "li" elements for templates Object.keys(data).forEach(function (templateId, index) { var isSelected = + // Template is selected in the database data[templateId].selected || + // Shared template which was already selected + (currentSelection[templateId] !== undefined && + currentSelection[templateId]) || + // Default template should be selected when: + // 1. A new object is created. + // 2. Organization or backend field has changed. + // (when the fields are changed, the currentSelection will be non-empty) (data[templateId].default && - selectedTemplates === undefined && - !data[templateId].required), + (pageLoading || + isAddingNewObject() || + Object.keys(currentSelection).length > 0)), element = getTemplateOptionElement( index, templateId, @@ -159,9 +162,6 @@ django.jQuery(function ($) { isSelected, true, ); - // Default templates should only be enabled for new - // device or when user has changed any of organization - // or backend field if (isSelected === true) { enabledTemplates.push(templateId); } @@ -170,9 +170,6 @@ django.jQuery(function ($) { sortedm2mPrefixUl.append(prefixElement); } }); - if (firstRun === true && selectedTemplates !== undefined) { - updateTemplateSelection(selectedTemplates); - } updateTemplateHelpText(); updateConfigTemplateField(enabledTemplates); }); @@ -210,7 +207,7 @@ django.jQuery(function ($) { }); }); } - firstRun = false; + pageLoading = false; $("#content-main form").submit(function () { $( 'ul.sortedm2m-items:first input[type="checkbox"][data-required="true"]', diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index d6818ccad..a471694f5 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -3,6 +3,7 @@ from copy import deepcopy from uuid import UUID +import ipdb from django.db.models import Q from django.http import Http404, HttpResponse, HttpResponseBadRequest, JsonResponse from django.utils import timezone @@ -20,12 +21,25 @@ OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings") +def _get_relevant_templates_dict(queryset, selected=False): + relevant_templates = OrderedDict() + for template in queryset: + relevant_templates[str(template.pk)] = dict( + name=template.name, + backend=template.get_backend_display(), + default=template.default, + required=template.required, + selected=True, + ) + return relevant_templates + + def get_relevant_templates(request, organization_id): """ returns default templates of specified organization """ backend = request.GET.get("backend", None) - device_id = request.GET.get("device_id", None) + config_id = request.GET.get("config_id", None) group_id = request.GET.get("group_id", None) user = request.user # organization_id is passed as 'null' for add device @@ -65,43 +79,40 @@ def get_relevant_templates(request, organization_id): queryset = ( Template.objects.filter(**filter_options) .filter(org_filters) + .order_by("-required", "-default") .only("id", "name", "backend", "default", "required") ) selected_templates = [] - if device_id: - selected_templates = ( - Config.objects.prefetch_related("templates") - .only("templates") - .get(device_id=device_id) - .templates.all() - ) - if group_id: - selected_templates = ( - DeviceGroup.objects.prefetch_related("templates") - .only("templates") - .get(pk=group_id) - .templates.filter(organization_id=organization_id) - ) + if config_id: + try: + selected_templates = ( + Config.objects.prefetch_related("templates") + .only("templates") + .get(pk=config_id) + .templates.filter(org_filters) + .filter(**filter_options) + ) + except (Config.DoesNotExist, ValueError): + pass - relevant_templates = OrderedDict() - for template in selected_templates: - relevant_templates[str(template.pk)] = dict( - name=template.name, - backend=template.get_backend_display(), - default=template.default, - required=template.required, - selected=True, - ) + if group_id: + try: + selected_templates = ( + DeviceGroup.objects.prefetch_related("templates") + .only("templates") + .get(pk=group_id) + .templates.filter(org_filters) + .filter(**filter_options) + ) + except (DeviceGroup.DoesNotExist, ValueError): + pass - for template in queryset.exclude(pk__in=relevant_templates.keys()): - relevant_templates[str(template.pk)] = dict( - name=template.name, - backend=template.get_backend_display(), - default=template.default, - required=template.required, - selected=False, + relevant_templates = _get_relevant_templates_dict(selected_templates, selected=True) + relevant_templates.update( + _get_relevant_templates_dict( + queryset.exclude(pk__in=relevant_templates.keys()), selected=False ) - + ) return JsonResponse(relevant_templates) From 44a5d4b6ba7a9640c3c3f022b7286734c742403b Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 30 May 2025 01:47:01 +0530 Subject: [PATCH 09/15] [tests] Fixed tests --- .../config/tests/test_selenium.py | 24 +++++++++++++++---- openwisp_controller/config/views.py | 3 +-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index 44a90265b..c0ae0d511 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -168,8 +168,8 @@ def test_device_preview_keyboard_shortcuts(self): self.wait_for_invisibility(By.CSS_SELECTOR, ".djnjc-overlay:not(.loading)") def test_multiple_organization_templates(self): - shared_required_template = self._create_template( - name="shared required", organization=None + shared_template = self._create_template( + name="shared", organization=None ) org1 = self._create_org(name="org1", slug="org1") @@ -204,10 +204,15 @@ def test_multiple_organization_templates(self): visible=[ org1_required_template, org1_default_template, - shared_required_template, + shared_template, ], ) + # Select shared template + self.find_element( + by=By.XPATH, value=f'//*[@value="{shared_template.id}"]' + ).click() + with self.subTest("changing org should update templates"): self.find_element( By.CSS_SELECTOR, value='a[href="#overview-group"]' @@ -224,9 +229,17 @@ def test_multiple_organization_templates(self): visible=[ org2_required_template, org2_default_template, - shared_required_template, + shared_template, ], ) + # Verify that shared template is selected + self.assertEqual( + self.find_element( + by=By.CSS_SELECTOR, + value=f'input[type="checkbox"][value="{shared_template.id}"]', + ).is_selected(), + True, + ) self.find_element( by=By.CSS_SELECTOR, value='input[name="_continue"]' ).click() @@ -234,9 +247,10 @@ def test_multiple_organization_templates(self): device.refresh_from_db() device.config.refresh_from_db() self.assertEqual(device.organization, org2) - self.assertEqual(device.config.templates.count(), 2) + self.assertEqual(device.config.templates.count(), 3) self.assertIn(org2_required_template, device.config.templates.all()) self.assertIn(org2_default_template, device.config.templates.all()) + self.assertIn(shared_template, device.config.templates.all()) def test_change_config_backend(self): device = self._create_config(organization=self._get_org()).device diff --git a/openwisp_controller/config/views.py b/openwisp_controller/config/views.py index a471694f5..8f8f3cd61 100644 --- a/openwisp_controller/config/views.py +++ b/openwisp_controller/config/views.py @@ -3,7 +3,6 @@ from copy import deepcopy from uuid import UUID -import ipdb from django.db.models import Q from django.http import Http404, HttpResponse, HttpResponseBadRequest, JsonResponse from django.utils import timezone @@ -29,7 +28,7 @@ def _get_relevant_templates_dict(queryset, selected=False): backend=template.get_backend_display(), default=template.default, required=template.required, - selected=True, + selected=selected, ) return relevant_templates From bd315ff05b599ffa2ec157485bd89ada27547436 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Fri, 30 May 2025 16:41:46 +0530 Subject: [PATCH 10/15] [qa] Fixed formatting --- openwisp_controller/config/tests/test_selenium.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index c0ae0d511..c00e225c4 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -168,9 +168,7 @@ def test_device_preview_keyboard_shortcuts(self): self.wait_for_invisibility(By.CSS_SELECTOR, ".djnjc-overlay:not(.loading)") def test_multiple_organization_templates(self): - shared_template = self._create_template( - name="shared", organization=None - ) + shared_template = self._create_template(name="shared", organization=None) org1 = self._create_org(name="org1", slug="org1") org1_required_template = self._create_template( From 1332f2b3f960b0e617eb2522c08e7904a513223c Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 5 Jun 2025 16:23:58 +0530 Subject: [PATCH 11/15] [req-change] Made retrieving group id robust --- .../config/static/config/js/relevant_templates.js | 5 +---- .../config/templates/admin/device_group/change_form.html | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index 1157a442d..d86271026 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -105,10 +105,7 @@ django.jQuery(function ($) { url.searchParams.set("backend", backend); } if (isDeviceGroup() && !$(".add-form").length) { - // Get the group id from the URL - // TODO: This is fragile, consider using a more robust way to get the group id. - var pathParts = window.location.pathname.split("/"); - url.searchParams.set("group_id", pathParts[pathParts.length - 3]); + url.searchParams.set("group_id", window._deviceGroupId); } else if ($('input[name="config-0-id"]').length) { url.searchParams.set("config_id", $('input[name="config-0-id"]').val()); } diff --git a/openwisp_controller/config/templates/admin/device_group/change_form.html b/openwisp_controller/config/templates/admin/device_group/change_form.html index 71df46ead..2b1ed8ad2 100644 --- a/openwisp_controller/config/templates/admin/device_group/change_form.html +++ b/openwisp_controller/config/templates/admin/device_group/change_form.html @@ -13,6 +13,7 @@ $(document).ready( function () { window._relevantTemplateUrl = "{{ relevant_template_url | safe }}"; window._deviceGroup = true; + window._deviceGroupId = "{{ original.pk }}"; window.bindDefaultTemplateLoading(); }) }) (django.jQuery); From 4e16424bff5791a6f2cad58eb64cba74b1c5d056 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 5 Jun 2025 17:16:59 +0530 Subject: [PATCH 12/15] [req-change] Made retrieving group id robust --- .../config/static/config/js/relevant_templates.js | 2 +- .../config/templates/admin/device_group/change_form.html | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index d86271026..adc3d76c0 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -4,7 +4,7 @@ django.jQuery(function ($) { backendFieldSelector = "#id_config-0-backend", orgFieldSelector = "#id_organization", isDeviceGroup = function () { - return window._deviceGroup; + return window._deviceGroupId !== undefined; }, templatesFieldName = function () { return isDeviceGroup() ? "templates" : "config-0-templates"; diff --git a/openwisp_controller/config/templates/admin/device_group/change_form.html b/openwisp_controller/config/templates/admin/device_group/change_form.html index 2b1ed8ad2..ae51524a1 100644 --- a/openwisp_controller/config/templates/admin/device_group/change_form.html +++ b/openwisp_controller/config/templates/admin/device_group/change_form.html @@ -12,7 +12,6 @@ (function ($) { $(document).ready( function () { window._relevantTemplateUrl = "{{ relevant_template_url | safe }}"; - window._deviceGroup = true; window._deviceGroupId = "{{ original.pk }}"; window.bindDefaultTemplateLoading(); }) From b4d0b93aec5265f6eecfe000837b0c4ef1c6f521 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Mon, 9 Jun 2025 16:33:39 +0530 Subject: [PATCH 13/15] [fix] Fixed unsaved changes alert on device page --- .../static/config/js/relevant_templates.js | 17 ++++++-- .../config/static/config/js/widget.js | 12 ++---- .../config/tests/test_selenium.py | 40 +++++++++++-------- 3 files changed, 41 insertions(+), 28 deletions(-) diff --git a/openwisp_controller/config/static/config/js/relevant_templates.js b/openwisp_controller/config/static/config/js/relevant_templates.js index adc3d76c0..e8aeef746 100644 --- a/openwisp_controller/config/static/config/js/relevant_templates.js +++ b/openwisp_controller/config/static/config/js/relevant_templates.js @@ -71,12 +71,23 @@ django.jQuery(function ($) { }, updateConfigTemplateField = function (templates) { var value = templates.join(","), - templateField = templatesFieldName(); + templateField = templatesFieldName(), + updateInitialValue = false; $(`input[name="${templateField}"]`).attr("value", value); - if (pageLoading) { + if ( + pageLoading || + // Handle cases where the AJAX request finishes after initial page load. + // If we're editing an existing object and the initial value hasn't been set, + // assign it now to avoid false positives in the unsaved changes warning. + (!isAddingNewObject() && + django._owcInitialValues[templateField] === undefined) + ) { django._owcInitialValues[templateField] = value; + updateInitialValue = true; } - $("input.sortedm2m:first").trigger("change"); + $("input.sortedm2m:first").trigger("change", { + updateInitialValue: updateInitialValue, + }); }, getSelectedTemplates = function () { // Returns the selected templates from the sortedm2m input diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index 9bcbe457a..9ff42e1a0 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -326,13 +326,7 @@ } alert(message); } - var contextField = $("#id_config-0-context"); - if (contextField.length) { - var contextValue = JSON.parse(contextField.val()); - contextField.val( - JSON.stringify(removeUnchangedDefaultValues(contextValue)), - ); - } + if ($advancedEl.is(":hidden")) { return; } @@ -494,8 +488,8 @@ getDefaultValues(true); }); } - $(".sortedm2m-items").on("change", function () { - getDefaultValues(); + $(".sortedm2m-items").on("change", function (event) { + getDefaultValues(event.updateInitialValue === true); }); $(".sortedm2m-items").on("sortstop", function () { getDefaultValues(); diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index c00e225c4..fe0e36d5c 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -523,6 +523,21 @@ class TestDeviceAdminUnsavedChanges( ): browser = "chrome" + def _is_unsaved_changes_alert_present(self): + for entry in self.get_browser_logs(): + if ( + entry["level"] == "WARNING" + and "You haven't saved your changes yet!" in entry["message"] + ): + return True + return False + + def _override_unsaved_changes_alert(self): + self.web_driver.execute_script( + 'django.jQuery(window).on("beforeunload", function(e) {' + " console.warn(e.returnValue); });" + ) + def test_unsaved_changes(self): """ Execute this test using Chrome instead of Firefox. @@ -530,17 +545,17 @@ def test_unsaved_changes(self): impossible to test the unsaved changes alert. """ self.login() + self._create_template(default=True, default_values={"ssid": "default"}) device = self._create_config(organization=self._get_org()).device path = reverse("admin:config_device_change", args=[device.id]) with self.subTest("Alert should not be displayed without any change"): self.open(path) self.hide_loading_overlay() - try: - WebDriverWait(self.web_driver, 1).until(EC.alert_is_present()) - except TimeoutException: - pass - else: + self._override_unsaved_changes_alert() + # Simulate navigating away from the page + self.open(reverse("admin:index")) + if self._is_unsaved_changes_alert_present(): self.fail("Unsaved changes alert displayed without any change") with self.subTest("Alert should be displayed after making changes"): @@ -550,10 +565,9 @@ def test_unsaved_changes(self): # # our own JS code sets e.returnValue when triggered # so we just need to ensure it's set as expected - self.web_driver.execute_script( - 'django.jQuery(window).on("beforeunload", function(e) {' - " console.warn(e.returnValue); });" - ) + self.open(path) + self.hide_loading_overlay() + self._override_unsaved_changes_alert() # simulate hand gestures self.find_element(by=By.TAG_NAME, value="body").click() self.find_element(by=By.NAME, value="name").click() @@ -562,13 +576,7 @@ def test_unsaved_changes(self): # simulate hand gestures self.find_element(by=By.TAG_NAME, value="body").click() self.web_driver.refresh() - for entry in self.get_browser_logs(): - if ( - entry["level"] == "WARNING" - and "You haven't saved your changes yet!" in entry["message"] - ): - break - else: + if not self._is_unsaved_changes_alert_present(): self.fail("Unsaved changes code was not executed.") From ccdf7eafb305e9d6fc270750087327044ed1db39 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Thu, 12 Jun 2025 13:53:26 +0530 Subject: [PATCH 14/15] [fix] Fixed default values event handler --- openwisp_controller/config/static/config/js/widget.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index 9ff42e1a0..ebe50a007 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -488,8 +488,8 @@ getDefaultValues(true); }); } - $(".sortedm2m-items").on("change", function (event) { - getDefaultValues(event.updateInitialValue === true); + $(".sortedm2m-items").on("change", function (event, data) { + getDefaultValues(data && data.updateInitialValue === true); }); $(".sortedm2m-items").on("sortstop", function () { getDefaultValues(); From a32cec0addf9fb47fbac925d54ac29656375690f Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 18 Jun 2025 01:28:16 +0530 Subject: [PATCH 15/15] [fix] Don't save template default values in Device.config.context --- .../config/static/config/js/widget.js | 8 +- .../config/tests/test_selenium.py | 84 ++++++++++--------- 2 files changed, 53 insertions(+), 39 deletions(-) diff --git a/openwisp_controller/config/static/config/js/widget.js b/openwisp_controller/config/static/config/js/widget.js index ebe50a007..4a689a168 100644 --- a/openwisp_controller/config/static/config/js/widget.js +++ b/openwisp_controller/config/static/config/js/widget.js @@ -326,7 +326,13 @@ } alert(message); } - + var contextField = $("#id_config-0-context"); + if (contextField.length) { + var contextValue = JSON.parse(contextField.val()); + contextField.val( + JSON.stringify(removeUnchangedDefaultValues(contextValue)), + ); + } if ($advancedEl.is(":hidden")) { return; } diff --git a/openwisp_controller/config/tests/test_selenium.py b/openwisp_controller/config/tests/test_selenium.py index fe0e36d5c..ae9061d39 100644 --- a/openwisp_controller/config/tests/test_selenium.py +++ b/openwisp_controller/config/tests/test_selenium.py @@ -5,7 +5,6 @@ from django.urls.base import reverse from selenium.common.exceptions import TimeoutException from selenium.webdriver.common.action_chains import ActionChains -from selenium.webdriver.common.alert import Alert from selenium.webdriver.common.by import By from selenium.webdriver.common.keys import Keys from selenium.webdriver.support import expected_conditions as EC @@ -267,43 +266,6 @@ def test_change_config_backend(self): config_backend_select.select_by_visible_text("OpenWISP Firmware 1.x") self.wait_for_invisibility(By.XPATH, f'//*[@value="{template.id}"]') - def test_template_context_variables(self): - self._create_template( - name="Template1", default_values={"vni": "1"}, required=True - ) - self._create_template( - name="Template2", default_values={"vni": "2"}, required=True - ) - device = self._create_config(organization=self._get_org()).device - self.login() - self.open( - reverse("admin:config_device_change", args=[device.id]) + "#config-group" - ) - self.hide_loading_overlay() - try: - WebDriverWait(self.web_driver, 2).until( - EC.text_to_be_present_in_element_value( - ( - By.XPATH, - '//*[@id="flat-json-config-0-context"]/div[2]/div/div/input[1]', - ), - "vni", - ) - ) - except TimeoutException: - self.fail("Timed out wating for configuration variabled to get loaded") - self.find_element( - by=By.XPATH, value='//*[@id="main-content"]/div[2]/a[3]' - ).click() - try: - WebDriverWait(self.web_driver, 2).until(EC.alert_is_present()) - except TimeoutException: - pass - else: - alert = Alert(self.web_driver) - alert.accept() - self.fail("Unsaved changes alert displayed without any change") - def test_force_delete_device_with_deactivating_config(self): self._create_template(default=True) config = self._create_config(organization=self._get_org()) @@ -579,6 +541,52 @@ def test_unsaved_changes(self): if not self._is_unsaved_changes_alert_present(): self.fail("Unsaved changes code was not executed.") + def test_template_context_variables(self): + self._create_template( + name="Template1", default_values={"vni": "1"}, required=True + ) + self._create_template( + name="Template2", default_values={"vni": "2"}, required=True + ) + device = self._create_config(organization=self._get_org()).device + self.login() + self.open( + reverse("admin:config_device_change", args=[device.id]) + "#config-group" + ) + self.hide_loading_overlay() + try: + WebDriverWait(self.web_driver, 2).until( + EC.text_to_be_present_in_element_value( + ( + By.XPATH, + '//*[@id="flat-json-config-0-context"]/div[2]/div/div/input[1]', + ), + "vni", + ) + ) + except TimeoutException: + self.fail("Timed out waiting for configuration variables to get loaded") + + with self.subTest("Navigating away from the page should not show alert"): + self._override_unsaved_changes_alert() + # Simulate navigating away from the page + self.find_element( + by=By.XPATH, value='//*[@id="main-content"]/div[2]/a[3]' + ).click() + if self._is_unsaved_changes_alert_present(): + self.fail("Unsaved changes alert displayed without any change") + + with self.subTest("Saving the objects should not save context variables"): + self.open(reverse("admin:config_device_change", args=[device.id])) + self.web_driver.execute_script( + "window.scrollTo(0, document.body.scrollHeight);" + ) + self.find_element( + by=By.CSS_SELECTOR, value='input[name="_continue"]' + ).click() + device.refresh_from_db() + self.assertEqual(device.config.context, {}) + @tag("selenium_tests") class TestVpnAdmin(