[change] Show only relevant templates in device admin #345 by pandafy · Pull Request #428 · openwisp/openwisp-controller

pandafy · 2021-04-08T20:47:17Z

Improved implementation for fetching relevant templates. Instead of
having separate views to get default and required templats, their
values are added in response of relevant templates views.

Closes #342
Closes #332
Closes #345

Co-authored-by: Nitesh Sinha niteshsinha1707@gmail.com

pandafy · 2021-04-08T20:47:47Z

The build is failing for some reason I don't understand right away. I will try to debug it tomorrow.

The feature work as intended in the UI

nemesifier

Great work @niteshsinha17 and @pandafy, this is going to be a great improvement! 👍

Tested it and works good!

I found a minor code issue, see below.

I have an additional minor request: can we hide all the templates when no organization is selected? We can do so with CSS if needed, so initially the templates are hidden and our JS shows them, or in any other way you feel it would work.

I regarding the commit prefix, I would probably use change since it makes sense here, we're changing the behavior from auto-selecting default templates, to showing only relevant templates, auto selecting default templates and auto-checking required templates. What do you think?

nemesifier · 2021-04-10T01:01:22Z


    @classmethod
    def clean_templates_org(cls, action, instance, pk_set, **kwargs):
+        templates = cls._get_templates_from_pk_set(pk_set)


I don't understand, this looks repeated (after 3 lines we call this again), why is that?
Looks like a mistake to me.

now I see, but why are you doing this before the pre_add check?
I don't see a good reason for this change. If I remember correctly, calling this could generate DB queries so we should avoid doing it unless it's really necessary (that is, we should do it after the if below).

Why do you think it should be done earlier?

coveralls · 2021-04-10T12:00:03Z

Coverage decreased (-0.02%) to 98.958% when pulling 2a05dce on issues/332/345-relevant-templates into da88633 on master.

Improved implementation for fetching relevant templates. Instead of having separate views to get default and required templats, their values are added in response of relevant templates views. Removed sortedm2m code duplication for required templates Closes #332 Closes #345 Closes #342 Co-authored-by: Nitesh Sinha <niteshsinha1707@gmail.com>

nemesifier

I have found an issue during testing.
If you use the search feature of the sorted m2m widget, when you clear the search field, all the options for all the backends and all the orgs appear again. Maybe we should remove/add elements instead of hiding/showing? Or what other solution do you think we can find?

nemesifier · 2021-04-13T01:49:27Z


    @classmethod
    def clean_templates_org(cls, action, instance, pk_set, **kwargs):
+        templates = cls._get_templates_from_pk_set(pk_set)


now I see, but why are you doing this before the pre_add check?
I don't see a good reason for this change. If I remember correctly, calling this could generate DB queries so we should avoid doing it unless it's really necessary (that is, we should do it after the if below).

Why do you think it should be done earlier?

nemesifier · 2021-04-13T01:50:51Z

        response = self.client.get(
-            reverse('admin:get_default_templates', args=[org1.pk])
+            reverse('admin:get_relevant_templates', args=[org1.pk])
        )


can you please add an assertNumQueries here to ensure the amount of queries is under control?

nemesifier · 2021-04-13T01:51:22Z


-        r = self.client.get(
-            reverse('admin:get_default_templates', args=[org1.pk]),
+        response = self.client.get(


same here for assertNumQueries

nemesifier

Another issue I found during testing:

create a default template in the system
assign it to a device and save, this works normally
now unassign it, suppose you don't need that default template to be assigned to that specific device, hit save
when page reloads the default template which was just unassigned gets reassigned again

The automatic default template assignment should be done only when switching org, or backend.

pandafy · 2021-04-13T18:15:43Z

I think we will need to introduce another endpoint to allow fetch currently applied templates from the backend.

The endpoint can be /admin/config/device/<device-id>/get-templates/ and it will return a list(Array) of template ids. This endpoint will only be triggered once, i.e. on the page load.

Or, would you like to add this information in the current endpoint? But here additional database queries will be made each time.

I am in favour of creating a separate endpoint.

nemesifier · 2021-04-13T18:20:59Z

I think we will need to introduce another endpoint to allow fetch currently applied templates from the backend.

The endpoint can be /admin/config/device/<device-id>/get-templates/ and it will return a list(Array) of template ids. This endpoint will only be triggered once, i.e. on the page load.

Or, would you like to add this information in the current endpoint? But here additional database queries will be made each time.

I am in favour of creating a separate endpoint.

I think the information about the currently applied templates should be available in the form and we can get it from there, what do you think?

pandafy · 2021-04-13T18:23:53Z

Yes, it will be available. But I was also thinking about #204 (comment). We can leave it for late, I guess.

nemesifier · 2021-04-14T00:14:15Z

Yes, it will be available. But I was also thinking about #204 (comment). We can leave it for late, I guess.

Ok I understand what you mean, yes let's leave it for later and proceed incrementally.

pandafy · 2021-04-14T01:36:00Z

-        self.assertEqual(len(templates), 2)
-        self.assertIn(str(t2.pk), templates)
-        self.assertIn(str(t3.pk), templates)
+        with self.assertNumQueries(4):


2 queries are made for authentication, 1 for getting organization and 1 for getting templates

pandafy · 2021-04-14T01:37:56Z

@@ -0,0 +1,116 @@
+'use strict';
+django.jQuery(function ($) {
+    var selectedTemplates = django._owcInitialValues["config-0-templates"].split(','),


Getting selectedTemplates using jQuery selector was resulting in "unsaved changes" dialog box appearing even when no changes were made due to order of template IDs. Using this here, resolved that issue.

pandafy · 2021-04-14T01:39:41Z

+                    }
+
+                    $('fieldset ul.sortedm2m-items:visible').append(element);
+                    $('fieldset ul.sortedm2m-items:not(:visible)').append(prefixElement);


Prefix elements __prefix__ are used by Django internally for form validation. It was working fine without adding them here, but I insisted on adding them to avoid any future bugs.

nemesifier

I pulled the latest changes (also tried destroying and recreating the branch), but I get the following JS errors when I open http://localhost:8000/admin/config/device/add/

jquery.js:4055 Uncaught TypeError: Cannot read property 'split' of undefined
    at HTMLDocument.<anonymous> (relevant_templates.js:3)
    at mightThrow (jquery.js:3762)
    at process (jquery.js:3830)
(anonymous) @ relevant_templates.js:3
mightThrow @ jquery.js:3762
process @ jquery.js:3830
setTimeout (async)
jQuery.readyException @ jquery.js:4054
(anonymous) @ jquery.js:4074
mightThrow @ jquery.js:3762
process @ jquery.js:3830
setTimeout (async)
(anonymous) @ jquery.js:3868
fire @ jquery.js:3496
fireWith @ jquery.js:3626
fire @ jquery.js:3634
fire @ jquery.js:3496
fireWith @ jquery.js:3626
process @ jquery.js:3850
setTimeout (async)
(anonymous) @ jquery.js:3868
fire @ jquery.js:3496
fireWith @ jquery.js:3626
fire @ jquery.js:3634
fire @ jquery.js:3496
fireWith @ jquery.js:3626
ready @ jquery.js:4106
completed @ jquery.js:4116
jquery.js:4055 Uncaught TypeError: window.bindDefaultTemplateLoading is not a function
    at HTMLDocument.<anonymous> ((index):1706)
    at mightThrow (jquery.js:3762)
    at process (jquery.js:3830)

pandafy · 2021-04-14T07:44:09Z

Manually tested device add and change view.

Build was failing due to openwisp/django-loci@041a82f

nemesifier

The add page now works well, but in the change page I am experiencing an issue: when the page loads, the right templates are not selected (I mean, they appear initially but then disappear), see below:

nemesifier

We're still not there, the order of tempate (which is importat since templates coming after can override the ones coming before) is not retained correctly when operating on an existing device:

On a new device, the drag and drop doesn't work for me:

nemesifier · 2021-04-14T21:38:24Z

        bindLoadUi();
        // fill device context field with default values of selected templates.
-        getDefaultValues(true);
+        setTimeout(function () {


why the delayed call?

Unsaved changes will fire an event after loading initial values. This event will be used to trigger the initial loading of default values(configuration variables) of templates.

pandafy · 2021-04-15T07:40:22Z

We're still not there, the order of tempate (which is importat since templates coming after can override the ones coming before) is not retained correctly when operating on an existing device:

I broke it unintentionally. I ordered the queryset because after reloading the page the template options was getting jumbled. I have removed the order_by clause from the query

On a new device, the drag and drop doesn't work for me:

I checked it on current master and on all 0.8.x releases. It is not possible to order templates on device add view. Can we create a separate issue for this?

why the delayed call?

I was getting the following error occasionally when no templates were selected for the device(no default or required templates).

I have improved the implementation in 2a05dce. Instead of a delayed call. it will now be executed only when an event is fired.

nemesifier

Thanks!

pandafy requested review from nemesifier and niteshsinha17 April 8, 2021 20:47

pandafy self-assigned this Apr 8, 2021

pandafy mentioned this pull request Apr 8, 2021

[change] Show only relevant templates in device admin #345 #398

Closed

pandafy force-pushed the issues/332/345-relevant-templates branch 2 times, most recently from 77894d7 to 264627d Compare April 9, 2021 11:06

nemesifier changed the title ~~[enhancement] Show only relevant templates in device admin #345~~ [feature] Show only relevant templates in device admin #345 Apr 10, 2021

nemesifier changed the title ~~[feature] Show only relevant templates in device admin #345~~ [change] Show only relevant templates in device admin #345 Apr 10, 2021

nemesifier requested changes Apr 10, 2021

View reviewed changes

pandafy force-pushed the issues/332/345-relevant-templates branch from 264627d to bd6b8f3 Compare April 10, 2021 11:53

pandafy force-pushed the issues/332/345-relevant-templates branch from bd6b8f3 to 522519f Compare April 10, 2021 12:31

pandafy requested a review from nemesifier April 12, 2021 03:34

nemesifier mentioned this pull request Apr 13, 2021

[change] Optimize queries of sortedm2m #204

Closed

nemesifier requested changes Apr 13, 2021

View reviewed changes

pandafy force-pushed the issues/332/345-relevant-templates branch from 8c8eb7b to 64a288e Compare April 14, 2021 01:35

[requested-changes] Select default values only on page load

253123a

pandafy force-pushed the issues/332/345-relevant-templates branch from 64a288e to 253123a Compare April 14, 2021 01:41

pandafy commented Apr 14, 2021

View reviewed changes

[minor] Formatted JS code

eab80ad

nemesifier reviewed Apr 14, 2021

View reviewed changes

[requested-changes] Fixed errors on new device view

2564bed

pandafy added 2 commits April 14, 2021 13:48

[chores/requirements] Use master branch of django-loci

01bb166

Build was failing due to openwisp/django-loci@041a82f

[chores/requirements] Use master branch of django-loci

f02a70a

Build was failing due to openwisp/django-loci@041a82f

nemesifier requested changes Apr 14, 2021

View reviewed changes

[fix] Default selected templatesw were getting disappeared

ab72ea9

pandafy force-pushed the issues/332/345-relevant-templates branch from 5d8d033 to ab72ea9 Compare April 14, 2021 17:25

nemesifier requested changes Apr 14, 2021

View reviewed changes

[fix] Removed ordering of templates in internal APIs

2a05dce

Unsaved changes will fire an event after loading initial values. This event will be used to trigger the initial loading of default values(configuration variables) of templates.

pandafy mentioned this pull request Apr 15, 2021

[bug] Re-ordering templates in device add view #434

Closed

nemesifier approved these changes Apr 16, 2021

View reviewed changes

nemesifier merged commit 79b9826 into master Apr 16, 2021

nemesifier deleted the issues/332/345-relevant-templates branch April 16, 2021 02:21

Uh oh!

Uh oh!

Conversation

pandafy commented Apr 8, 2021 • edited by nemesifier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pandafy commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

coveralls commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

pandafy commented Apr 13, 2021

Uh oh!

nemesifier commented Apr 13, 2021

Uh oh!

pandafy commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemesifier commented Apr 14, 2021

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!

pandafy commented Apr 14, 2021

Uh oh!

nemesifier left a comment • 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!

pandafy commented Apr 15, 2021

Uh oh!

nemesifier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pandafy commented Apr 8, 2021 •

edited by nemesifier

Loading

pandafy commented Apr 8, 2021 •

edited

Loading

coveralls commented Apr 10, 2021 •

edited

Loading

pandafy commented Apr 13, 2021 •

edited

Loading

nemesifier left a comment •

edited

Loading