[change] Change relevant templates logic to facilitate changing organization +optimizations by DragnEmperor · Pull Request #1010 · openwisp/openwisp-controller

DragnEmperor · 2025-04-23T19:27:45Z

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
N/A I have updated the documentation.

Reference to Existing Issue

Closes #204
Closes #1050

Description of Changes

To remove queries generated by sortedm2m, 'formfield_for_manytomany' is overridden such that on page load, we can get templates using 'get_relevant_templates'.
Added device/group id in 'get_relevant_templates' to identify selected templates. Changes are made in 'relevantTemplates.js' to ensure templates are viewed and changes are saved as per initial behavior.

As now we are fetching templates via js only, I have done some modifications for the failing test cases.

for test case test_device_templates_m2m_queryset there is a similar selenium test case test_multiple_organization_templates
for test cases test_configuration_templates_removed , test_vpn_clients_deleted, have added new selenium test cases test_add_remove_templates, test_vpn_clients_deleted.

DragnEmperor · 2025-04-23T19:29:55Z

Known Issues:

Ordering is not maintained (done)

devkapilbansal · 2025-05-05T18:19:16Z

Hi @DragnEmperor
I see that the tests are failing. You can keep a PR in draft state unless it is completed

coveralls · 2025-05-11T03:20:05Z

coverage: 98.746% (-0.07%) from 98.816%
when pulling bbd2709 on DragnEmperor:issues/204-sortedm2m-queries
into d449e9b on openwisp:master.

DragnEmperor · 2025-05-11T13:27:58Z

As now we are fetching templates via js only, I have done some modifications for the failing test cases.

for test case test_device_templates_m2m_queryset there is a similar selenium test case test_multiple_organization_templates
for test cases test_configuration_templates_removed , test_vpn_clients_deleted, have added new selenium test cases test_add_remove_templates, test_vpn_clients_deleted.

devkapilbansal · 2025-05-16T17:25:59Z

            hidden=[data['org2'].name, data['inactive'].name],
        )

-    def test_device_templates_m2m_queryset(self):


Please mention in description why these tests are removed from here

Hi Kapil,
I have added in the commit description and in the comment above the reason for removing these test cases.

nemesifier

Are you asserting the number of queries to verify the optimizations? Or what are you doing to ensure that the optimization is effective within automated tests?

nemesifier · 2025-05-17T19:08:28Z

            response, 'value="openwisp_controller.vpn_backends.OpenVpn" selected'
        )

-    def test_vpn_clients_deleted(self):


why are these tests being moved?

As we are adding the templates via js so i migrated these tests to selenium tests. I have added this in conversation above. I will update the description of PR itself for more clarity.

DragnEmperor · 2025-05-17T19:20:27Z

Are you asserting the number of queries to verify the optimizations? Or what are you doing to ensure that the optimization is effective within automated tests?

Yes I have checked the queries and while there is an improvement its not much i think ( around 4-5 queries less now, some queries' results are cached so i am not exactly sure of the count)
I have not added the tests for asserting number of queries. That would be something I need to work on, Thanks!

DragnEmperor · 2025-05-20T18:54:16Z

@nemesifier @pandafy
Upon inspection of the queries executed, the following query was being duplicated twice leading to total count of 26.

SELECT "config_template"."id", "config_template"."created", "config_template"."modified", "config_template"."name", "config_template"."backend", "config_template"."config", "config_template"."organization_id", "config_template"."vpn_id", "config_template"."type", "config_template"."default", "config_template"."required", "config_template"."auto_cert", "config_template"."default_values" FROM "config_template" ORDER BY "config_template"."name" ASC

With the changes, templates are being added via js using get_relevant_templates so this query is not executed, bringing the count to 24.

pandafy · 2025-05-26T19:20:57Z

+    # 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)
+


@DragnEmperor can you help me understand why do we need three tests?

I have added these checks to ensure that queries are not changed and remain the same irrespective of number of templates

The count argument is 1 in all the tests. Is that correct?

Yes that was my mistake, these should be correct in new commit.

nemesifier

Please merge with the latest master and run openwisp-qa-format again.
I see some changes from other PRs that have already been merged which are showing up again here, I hope that updating on master and running openwisp-qa-format fixes that, if not please look into resolving it as it makes reviewing tougher with no added benefits.

nemesifier

There's one failing test: test_multiple_organization_templates.

@DragnEmperor thank you for your effort! You can leave this to us now and focus on the other work you have.

nemesifier · 2025-05-29T22:13:27Z

+                .only("templates")
+                .get(pk=group_id)
+                .templates.filter(org_filters)
+                .filter(**filter_options)


What's the value of filter_options here? Can we add organization_id=organization_id here?

nemesifier · 2025-06-04T22:02:57Z

+      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("/");


We are overriding /templates/admin/device_group/change_form.html, so why don't we add the group ID there as an HTML element which is easy to retrieve here?

pandafy · 2025-06-05T11:49:20Z

            $(document).ready( function () {
                window._relevantTemplateUrl = "{{ relevant_template_url | safe }}";
-                window._deviceGroup = true;
+                window._deviceGroupId = "{{ original.pk }}";


We were already setting the _devieGroup property here to determine whether the relevant_template,js script was loading from device page or device group page. I have updated the logic to pass the device group ID to the JS.

@nemesifier if you prefer an HTML element, I would update the code accordingly.

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 openwisp#204 Signed-off-by: DragnEmperor <dragnemperor@gmail.com>

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 <dragnemperor@gmail.com>

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 <dragnemperor@gmail.com>

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 <dragnemperor@gmail.com>

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 <dragnemperor@gmail.com>

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 <dragnemperor@gmail.com>

nemesifier

I finally took the time to review the entire set of changes, I have one question remaining below.

nemesifier · 2025-06-17T19:08:28Z

-        contextField.val(
-          JSON.stringify(removeUnchangedDefaultValues(contextValue)),
-        );
-      }


Why was this removed?

nemesifier · 2025-06-17T21:54:35Z

Thanks a lot @DragnEmperor @pandafy! 👏 👍

I hit the confirm merge by mistake without polishing the commit message, I am going to fix this on master.

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from b7e4d02 to 46f4a12 Compare April 29, 2025 17:41

DragnEmperor mentioned this pull request Apr 30, 2025

[change] Optimize queries of sortedm2m #204

Closed

DragnEmperor marked this pull request as draft May 6, 2025 02:31

codesankalp self-requested a review May 10, 2025 04:54

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from d29a35b to 9088448 Compare May 10, 2025 13:07

DragnEmperor marked this pull request as ready for review May 11, 2025 12:37

devkapilbansal self-requested a review May 16, 2025 17:22

devkapilbansal reviewed May 16, 2025

View reviewed changes

devkapilbansal self-requested a review May 16, 2025 17:26

nemesifier reviewed May 17, 2025

View reviewed changes

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from c98b148 to 4f85589 Compare May 20, 2025 18:49

pandafy reviewed May 26, 2025

View reviewed changes

nemesifier assigned pandafy and DragnEmperor May 26, 2025

nemesifier reviewed May 26, 2025

View reviewed changes

DragnEmperor force-pushed the issues/204-sortedm2m-queries branch from c8856a4 to cb3aac4 Compare May 27, 2025 09:38

pandafy added this to OpenWISP Contributor's Board May 27, 2025

github-project-automation Bot moved this to To do (general) in OpenWISP Contributor's Board May 27, 2025

nemesifier reviewed May 28, 2025

View reviewed changes

nemesifier previously approved these changes May 29, 2025

View reviewed changes

github-project-automation Bot moved this from To do (general) to In progress in OpenWISP Contributor's Board May 29, 2025

pandafy dismissed nemesifier’s stale review via 96f0c5e May 30, 2025 18:34

nemesifier reviewed Jun 4, 2025

View reviewed changes

pandafy reviewed Jun 5, 2025

View reviewed changes

DragnEmperor and others added 12 commits June 9, 2025 15:37

[change] Improved relevant templates

12cedbe

[fix] Fixed relevant templates logic

51fac6b

[tests] Fixed tests

44a5d4b

[qa] Fixed formatting

bd315ff

[req-change] Made retrieving group id robust

1332f2b

[req-change] Made retrieving group id robust

4e16424

pandafy force-pushed the issues/204-sortedm2m-queries branch 2 times, most recently from c082f0e to b00bcc8 Compare June 9, 2025 12:58

[fix] Fixed unsaved changes alert on device page

b4d0b93

pandafy force-pushed the issues/204-sortedm2m-queries branch from b00bcc8 to b4d0b93 Compare June 9, 2025 13:06

[fix] Fixed default values event handler

ccdf7ea

pandafy force-pushed the issues/204-sortedm2m-queries branch from 43899e1 to ccdf7ea Compare June 12, 2025 08:36

nemesifier reviewed Jun 17, 2025

View reviewed changes

nemesifier added the enhancement label Jun 17, 2025

[fix] Don't save template default values in Device.config.context

a32cec0

pandafy force-pushed the issues/204-sortedm2m-queries branch from 497d3d0 to a32cec0 Compare June 17, 2025 20:08

Merge branch 'master' into issues/204-sortedm2m-queries

bbd2709

nemesifier approved these changes Jun 17, 2025

View reviewed changes

nemesifier changed the title ~~[change] Reduce sortedm2m queries for templates #204~~ [change] Change relevant templates logic to facilitate changing organization +optimizations Jun 17, 2025

nemesifier merged commit 2d7e500 into openwisp:master Jun 17, 2025
15 checks passed

github-project-automation Bot moved this from In progress to Done in OpenWISP Contributor's Board Jun 17, 2025

This was referenced Jul 19, 2025

[change] Moved python dependencies to requirements.txt files for maintainability openwisp/ansible-openwisp2#529

Merged

[tests] Add selenium test test_multiple_organization_templates openwisp/ansible-openwisp2#530

Open

Uh oh!

Conversation

DragnEmperor commented Apr 23, 2025 • edited by nemesifier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Description of Changes

Uh oh!

DragnEmperor commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devkapilbansal commented May 5, 2025

Uh oh!

coveralls commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DragnEmperor commented May 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DragnEmperor commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DragnEmperor commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pandafy May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nemesifier commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DragnEmperor commented Apr 23, 2025 •

edited by nemesifier

Loading

DragnEmperor commented Apr 23, 2025 •

edited

Loading

coveralls commented May 11, 2025 •

edited

Loading

DragnEmperor May 16, 2025 •

edited

Loading

DragnEmperor commented May 17, 2025 •

edited

Loading

DragnEmperor commented May 20, 2025 •

edited

Loading

pandafy May 26, 2025 •

edited

Loading