Skip to content

Commit 41c1b40

Browse files
pandafypandafy
committed
Merge branch 'issues/204-sortedm2m-queries' of github.com:DragnEmperor/openwisp-controller into DragnEmperor-issues/204-sortedm2m-queries
2 parents 4836913 + 268f1fa commit 41c1b40

6 files changed

Lines changed: 250 additions & 150 deletions

File tree

‎openwisp_controller/config/admin.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,13 @@ def get_fields(self, request, obj):
463463
fields = super().get_fields(request, obj)
464464
return self._error_reason_field_conditional(obj, fields)
465465

466+
def formfield_for_manytomany(self, db_field, request, **kwargs):
467+
# setting queryset none for all requests except POST as queryset
468+
# is required for the form to be valid
469+
if db_field.name == 'templates' and request.method != 'POST':
470+
kwargs['queryset'] = Template.objects.none()
471+
return super().formfield_for_manytomany(db_field, request, **kwargs)
472+
466473

467474
class ChangeDeviceGroupForm(forms.Form):
468475
device_group = forms.ModelChoiceField(
@@ -1319,6 +1326,13 @@ def get_extra_context(self, pk=None):
13191326
}
13201327
return ctx
13211328

1329+
def formfield_for_manytomany(self, db_field, request, **kwargs):
1330+
# setting queryset none for all requests except POST as queryset
1331+
# is required for the form to be valid
1332+
if db_field.name == 'templates' and request.method != 'POST':
1333+
kwargs['queryset'] = Template.objects.none()
1334+
return super().formfield_for_manytomany(db_field, request, **kwargs)
1335+
13221336

13231337
admin.site.register(Device, DeviceAdminExportable)
13241338
admin.site.register(Template, TemplateAdmin)

‎openwisp_controller/config/static/config/js/relevant_templates.js‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ django.jQuery(function ($) {
3333
if (templateConfig.required) {
3434
inputField.prop("disabled", true);
3535
}
36-
if (isSelected || templateConfig.required) {
36+
// mark the template as selected if it is required or if it is enabled for the current device or group
37+
if (isSelected || templateConfig.required || templateConfig.selected) {
3738
inputField.prop("checked", true);
3839
}
3940
return element;
@@ -94,7 +95,10 @@ django.jQuery(function ($) {
9495
selectedTemplates;
9596

9697
// Hide templates if no organization or backend is selected
97-
if (orgID.length === 0 || (!isDeviceGroup() && backend.length === 0)) {
98+
if (
99+
(orgID && orgID.length === 0) ||
100+
(!isDeviceGroup() && backend.length === 0)
101+
) {
98102
resetTemplateOptions();
99103
updateTemplateHelpText();
100104
return;
@@ -115,6 +119,14 @@ django.jQuery(function ($) {
115119
var url = window._relevantTemplateUrl.replace("org_id", orgID);
116120
// Get relevant templates of selected org and backend
117121
url = url + "?backend=" + backend;
122+
if (!isDeviceGroup()) {
123+
var deviceID = $('input[name="config-0-device"]').val();
124+
url = url + "&device=" + deviceID;
125+
} else {
126+
// Get the group id from the URL
127+
var pathParts = window.location.pathname.split("/");
128+
url = url + "&group=" + pathParts[pathParts.length - 3];
129+
}
118130
$.get(url).done(function (data) {
119131
resetTemplateOptions();
120132
var enabledTemplates = [],
@@ -156,7 +168,8 @@ django.jQuery(function ($) {
156168
// Adds "li" elements for templates that are not selected
157169
// in the database.
158170
var counter =
159-
selectedTemplates !== undefined ? selectedTemplates.length : 0;
171+
selectedTemplates !== undefined ? selectedTemplates.length : 0,
172+
deviceTemplates = [];
160173
Object.keys(data).forEach(function (templateId, index) {
161174
// corner case in which backend of template does not match
162175
if (!data[templateId]) {
@@ -186,6 +199,9 @@ django.jQuery(function ($) {
186199
if (isSelected === true) {
187200
enabledTemplates.push(templateId);
188201
}
202+
if (element.children().children("input").prop("checked") === true) {
203+
deviceTemplates.push(templateId);
204+
}
189205
sortedm2mUl.append(element);
190206
if (!isDeviceGroup()) {
191207
sortedm2mPrefixUl.append(prefixElement);
@@ -194,6 +210,25 @@ django.jQuery(function ($) {
194210
if (firstRun === true && selectedTemplates !== undefined) {
195211
updateTemplateSelection(selectedTemplates);
196212
}
213+
// this runs on first load and sets the name of hidden input field which tracks
214+
// the selected templates.
215+
if (selectedTemplates === undefined) {
216+
if (!isDeviceGroup()) {
217+
$(
218+
`#config-0 .field-templates .sortedm2m-container input[type="hidden"]`,
219+
)
220+
.first()
221+
.attr("name", templatesFieldName());
222+
// set the initial value of the hidden input field
223+
// to the selected templates
224+
django._owcInitialValues[templatesFieldName()] =
225+
deviceTemplates.join(",");
226+
} else {
227+
$(
228+
`.field-templates .sortedm2m-container input[type="hidden"]`,
229+
).attr("name", templatesFieldName());
230+
}
231+
}
197232
updateTemplateHelpText();
198233
updateConfigTemplateField(enabledTemplates);
199234
});

‎openwisp_controller/config/tests/test_admin.py‎

Lines changed: 36 additions & 142 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from unittest.mock import patch
55
from uuid import uuid4
66

7+
import django
78
from django.contrib.admin.models import LogEntry
89
from django.contrib.auth import get_user_model
910
from django.core.exceptions import ValidationError
@@ -479,15 +480,6 @@ def test_device_organization_fk_autocomplete_view(self):
479480
hidden=[data['org2'].name, data['inactive'].name],
480481
)
481482

482-
def test_device_templates_m2m_queryset(self):
483-
data = self._create_multitenancy_test_env()
484-
t_shared = self._create_template(name='t-shared', organization=None)
485-
self._test_multitenant_admin(
486-
url=reverse(f'admin:{self.app_label}_device_add'),
487-
visible=[str(data['t1']), str(t_shared)],
488-
hidden=[str(data['t2']), str(data['t3_inactive'])],
489-
)
490-
491483
def test_template_queryset(self):
492484
data = self._create_multitenancy_test_env()
493485
self._test_multitenant_admin(
@@ -796,56 +788,6 @@ def test_template_not_contains_default_templates_js(self):
796788
response = self.client.get(path)
797789
self.assertNotContains(response, '// enable default templates')
798790

799-
def test_configuration_templates_removed(self):
800-
def _update_template(templates):
801-
params.update(
802-
{
803-
'config-0-templates': ','.join(
804-
[str(template.pk) for template in templates]
805-
)
806-
}
807-
)
808-
response = self.client.post(path, data=params, follow=True)
809-
self.assertEqual(response.status_code, 200)
810-
for template in templates:
811-
self.assertContains(
812-
response, f'class="sortedm2m" checked> {template.name}'
813-
)
814-
return response
815-
816-
template = self._create_template()
817-
818-
# Add a new device
819-
path = reverse(f'admin:{self.app_label}_device_add')
820-
params = self._get_device_params(org=self._get_org())
821-
response = self.client.post(path, data=params, follow=True)
822-
self.assertEqual(response.status_code, 200)
823-
824-
config = Device.objects.get(name=params['name']).config
825-
path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id])
826-
params.update(
827-
{
828-
'config-0-id': str(config.pk),
829-
'config-0-device': str(config.device_id),
830-
'config-INITIAL_FORMS': 1,
831-
'_continue': True,
832-
}
833-
)
834-
835-
# Add template to the device
836-
_update_template(templates=[template])
837-
config.refresh_from_db()
838-
self.assertEqual(config.templates.count(), 1)
839-
self.assertEqual(config.status, 'modified')
840-
config.set_status_applied()
841-
self.assertEqual(config.status, 'applied')
842-
843-
# Remove template from the device
844-
_update_template(templates=[])
845-
config.refresh_from_db()
846-
self.assertEqual(config.templates.count(), 0)
847-
self.assertEqual(config.status, 'modified')
848-
849791
def test_vpn_not_contains_default_templates_js(self):
850792
vpn = self._create_vpn()
851793
path = reverse(f'admin:{self.app_label}_vpn_change', args=[vpn.pk])
@@ -1626,89 +1568,6 @@ def test_add_vpn(self):
16261568
response, 'value="openwisp_controller.vpn_backends.OpenVpn" selected'
16271569
)
16281570

1629-
def test_vpn_clients_deleted(self):
1630-
def _update_template(templates):
1631-
params.update(
1632-
{
1633-
'config-0-templates': ','.join(
1634-
[str(template.pk) for template in templates]
1635-
)
1636-
}
1637-
)
1638-
response = self.client.post(path, data=params, follow=True)
1639-
self.assertEqual(response.status_code, 200)
1640-
for template in templates:
1641-
self.assertContains(
1642-
response, f'class="sortedm2m" checked> {template.name}'
1643-
)
1644-
return response
1645-
1646-
vpn = self._create_vpn()
1647-
template = self._create_template()
1648-
vpn_template = self._create_template(
1649-
name='vpn-test',
1650-
type='vpn',
1651-
vpn=vpn,
1652-
auto_cert=True,
1653-
)
1654-
cert_query = Cert.objects.exclude(pk=vpn.cert_id)
1655-
valid_cert_query = cert_query.filter(revoked=False)
1656-
revoked_cert_query = cert_query.filter(revoked=True)
1657-
1658-
# Add a new device
1659-
path = reverse(f'admin:{self.app_label}_device_add')
1660-
params = self._get_device_params(org=self._get_org())
1661-
response = self.client.post(path, data=params, follow=True)
1662-
self.assertEqual(response.status_code, 200)
1663-
1664-
config = Device.objects.get(name=params['name']).config
1665-
self.assertEqual(config.vpnclient_set.count(), 0)
1666-
self.assertEqual(config.templates.count(), 0)
1667-
1668-
path = reverse(f'admin:{self.app_label}_device_change', args=[config.device_id])
1669-
params.update(
1670-
{
1671-
'config-0-id': str(config.pk),
1672-
'config-0-device': str(config.device_id),
1673-
'config-INITIAL_FORMS': 1,
1674-
'_continue': True,
1675-
}
1676-
)
1677-
1678-
with self.subTest('Adding only VpnClient template'):
1679-
# Adding VpnClient template to the device
1680-
_update_template(templates=[vpn_template])
1681-
1682-
self.assertEqual(config.templates.count(), 1)
1683-
self.assertEqual(config.vpnclient_set.count(), 1)
1684-
self.assertEqual(cert_query.count(), 1)
1685-
self.assertEqual(valid_cert_query.count(), 1)
1686-
1687-
# Remove VpnClient template from the device
1688-
_update_template(templates=[])
1689-
1690-
self.assertEqual(config.templates.count(), 0)
1691-
self.assertEqual(config.vpnclient_set.count(), 0)
1692-
# Removing VPN template marks the related certificate as revoked
1693-
self.assertEqual(revoked_cert_query.count(), 1)
1694-
self.assertEqual(valid_cert_query.count(), 0)
1695-
1696-
with self.subTest('Add VpnClient template along with another template'):
1697-
# Adding templates to the device
1698-
_update_template(templates=[template, vpn_template])
1699-
1700-
self.assertEqual(config.templates.count(), 2)
1701-
self.assertEqual(config.vpnclient_set.count(), 1)
1702-
self.assertEqual(valid_cert_query.count(), 1)
1703-
1704-
# Remove VpnClient template from the device
1705-
_update_template(templates=[template])
1706-
1707-
self.assertEqual(config.templates.count(), 1)
1708-
self.assertEqual(config.vpnclient_set.count(), 0)
1709-
self.assertEqual(valid_cert_query.count(), 0)
1710-
self.assertEqual(revoked_cert_query.count(), 2)
1711-
17121571
def test_ip_not_in_add_device(self):
17131572
path = reverse(f'admin:{self.app_label}_device_add')
17141573
response = self.client.get(path)
@@ -2160,6 +2019,41 @@ def test_vpn_template_switch(self):
21602019
)
21612020
self.assertEqual(config.vpnclient_set.count(), 1)
21622021

2022+
# helper for asserting queries executed during template fetch for a device
2023+
def _verify_template_queries(self, config, count):
2024+
path = reverse(f'admin:{self.app_label}_device_change', args=[config.device.pk])
2025+
for i in range(count):
2026+
self._create_template(name=f'template-{i}')
2027+
expected_count = 24
2028+
if django.VERSION < (5, 2):
2029+
# In django version < 5.2, there is an extra SAVEPOINT query
2030+
# leading to extra RELEASE SAVEPOINT query, thus 2 extra queries
2031+
expected_count += 2
2032+
with self.assertNumQueries(expected_count):
2033+
# contains 22 queries for fetching normal device data
2034+
response = self.client.get(path)
2035+
# contains 2 queries, 1 for fetching organization
2036+
# and 1 for fetching templates
2037+
response = self.client.get(
2038+
reverse(
2039+
'admin:get_relevant_templates', args=[config.device.organization.pk]
2040+
)
2041+
)
2042+
self.assertEqual(response.status_code, 200)
2043+
2044+
# ensuring queries are consistent for different number of templates
2045+
def test_templates_fetch_queries_1(self):
2046+
config = self._create_config(organization=self._get_org())
2047+
self._verify_template_queries(config, 1)
2048+
2049+
def test_templates_fetch_queries_5(self):
2050+
config = self._create_config(organization=self._get_org())
2051+
self._verify_template_queries(config, 1)
2052+
2053+
def test_templates_fetch_queries_10(self):
2054+
config = self._create_config(organization=self._get_org())
2055+
self._verify_template_queries(config, 1)
2056+
21632057

21642058
class TestTransactionAdmin(
21652059
CreateConfigTemplateMixin,

0 commit comments

Comments
 (0)