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

niteshsinha17 · 2021-03-04T13:30:01Z

@nemesisdesign I have implemented it. Please check it once. After that I will include test in it.
closes #345

coveralls · 2021-03-04T13:38:20Z

Coverage increased (+0.008%) to 99.093% when pulling 72f0ac4 on niteshsinha17:issues/345-show-relevant-templates into 460c86c on openwisp:master.

niteshsinha17 · 2021-03-04T13:41:53Z

openwisp_controller/config/admin.py

+            url(
+                r'^config/get-relevant-templates/(?P<organization_id>[^/]+)/$',
+                get_relevant_templates,
+                name='get_relavant_templates',


I will correct this spelling mistake

nemesifier

looks good, can you:

add missing tests for the functions added
make sure that if a template is not relevant but is flagged as required, hence its checkbox is checked by default and disabled, we enable again the chekbox and remove the check?

atb00ker

Just one question. 😄

atb00ker · 2021-03-09T17:23:59Z

openwisp_controller/config/static/config/js/default_templates.js

+            if (orgID.length === 0 || backend.length === 0) {
+                return;
+            }
+            var url = '/admin/config/device/config/get-relevant-templates/';


Can we try to avoid hardcoding the url here? 🤔
Perhaps a variable entered from the template.
@nemesisdesign What do you think?

Yes, we can use a variable as we are using for get_default_templates. Should I add it?

Yes, it's better because the URL will be automatically updated if we make changes in the python code. Thanks @atb00ker.

nemesifier

Only this remaining: #398 (review)

niteshsinha17 · 2021-03-12T10:09:36Z

looks good, can you:

add missing tests for the functions added

make sure that if a template is not relevant but is flagged as required, hence its checkbox is checked by default and disabled, we enable again the chekbox and remove the check?

@nemesisdesign I have a doubt. I think we need to again disable it and need to make checkbox checked when the template become relevant on changing org or config backend. I have not added it yet but I have added it today in the code and waiting for your input. I will push the code as you agree.

nemesifier · 2021-03-12T15:12:58Z

looks good, can you:

add missing tests for the functions added

make sure that if a template is not relevant but is flagged as required, hence its checkbox is checked by default and disabled, we enable again the chekbox and remove the check?

@nemesisdesign I have a doubt. I think we need to again disable it and need to make checkbox checked when the template become relevant on changing org or config backend. I have not added it yet but I have added it today in the code and waiting for your input. I will push the code as you agree.

@niteshsinha17 yes, I think you're right, go ahead, thanks

niteshsinha17 · 2021-03-13T05:29:03Z

@nemesisdesign Its ready now

closes openwisp#345

pandafy

Good work @niteshsinha17! I tested the PR manually and it works as expected.

I have suggested some minor code improvements and asked few queries. It will be great if you can answer them.

pandafy · 2021-04-06T10:44:41Z

openwisp_controller/config/views.py

+def get_relevant_templates(request, organization_id):
+    """
+    returns relevant templates of specified organization
+    and config backend
+    """
+    backend = request.GET.get("backend", None)
+    user = request.user
+    authenticated = user.is_authenticated
+    if not authenticated and not user.is_staff:
+        return HttpResponse(status=403)
+    org = get_object_or_404(Organization, pk=organization_id, is_active=True)
+    templates = get_relevant_templates_queryset(org.pk, backend, model=Template).only(
+        'id'
+    )
+    uuids = [str(t.pk) for t in templates]
+    return JsonResponse({'templates': uuids})


Wouldn't it be better to do a check like if organization_id in user.organization_managed or user.is_superuser? It will make sure that templates can be only be used by users of the same organization.

Please also add a test for this.

I see this is a problem with get_default_templates view also. I will open a separate issue for this.

Opened #420

pandafy · 2021-04-06T10:49:55Z

openwisp_controller/config/templates/admin/config/change_form.html

+                    {{ default_template_urls| safe }},
+                    {{relevant_template_urls| safe}}


Suggested change

{{ default_template_urls| safe }},

{{relevant_template_urls| safe}}

{{ default_template_urls | safe }},

{{ relevant_template_urls | safe }}

pandafy · 2021-04-06T11:04:15Z

openwisp_controller/config/static/config/js/default_templates.js

+                    localStorage.getItem('config-disabled-templates', null);
+                disabledTemplates = JSON.parse(disabledTemplates) || {};
+                $('input.sortedm2m').each(function () {
+                    if (($(this).val() in relevantTemplates) === false) {


Why not use do relevantTemplates[$(this).val()] and put everything in try catch instead? The "in" operator will take more time if number of entries is more in relevantTemplates.

pandafy · 2021-04-06T11:06:54Z

openwisp_controller/config/static/config/js/default_templates.js

+                    }
+                });
+                disabledTemplates = JSON.stringify(disabledTemplates);
+                localStorage.setItem('config-disabled-templates', disabledTemplates);


Can't we just keep it in the memory instead of using localStorage? How making disabledTemplates persistent helpful?

pandafy · 2021-04-08T17:02:44Z

Hey @niteshsinha17!

I am working on a similar issue #332 and I believe that I can base it on your work.

Thank you very much for creating this PR, I will continue it from here. 🙌🏼

niteshsinha17 · 2021-04-08T17:33:00Z

Hey @niteshsinha17!

I am working on a similar issue #332 and I believe that I can base it on your work.

Thank you very much for creating this PR, I will continue it from here. 🙌🏼

@pandafy What should I do with this PR? Should I close it? 🤔

pandafy · 2021-04-08T17:35:26Z

@pandafy What should I do with this PR? Should I continue it?

No, please leave it. I will continue the development on this. If you made any changes locally you can push them. Don't spend time on refining them since I will modify the implementation.

pandafy · 2021-04-08T20:48:45Z

Closing in favour of #428.

niteshsinha17 requested a review from nemesifier March 4, 2021 13:30

niteshsinha17 self-assigned this Mar 4, 2021

niteshsinha17 commented Mar 4, 2021

View reviewed changes

nemesifier requested changes Mar 6, 2021

View reviewed changes

nemesifier changed the title ~~[enhancement] Show only relevant templates in device admin #345~~ [change] Show only relevant templates in device admin #345 Mar 6, 2021

niteshsinha17 requested a review from nemesifier March 7, 2021 18:10

atb00ker reviewed Mar 9, 2021

View reviewed changes

nemesifier requested changes Mar 11, 2021

View reviewed changes

niteshsinha17 added 4 commits March 13, 2021 12:03

[enhancement] Show only relevant templates in device admin openwisp#345

4c892a2

closes openwisp#345

[chores] Added test

c550f39

[fix] Fixed disabled template issue

a5d6099

[chores] Minor changes

72f0ac4

niteshsinha17 force-pushed the issues/345-show-relevant-templates branch from cae9306 to 72f0ac4 Compare March 13, 2021 06:34

nemesifier added the enhancement label Apr 5, 2021

pandafy reviewed Apr 6, 2021

View reviewed changes

pandafy mentioned this pull request Apr 6, 2021

[bug] Able to query default templates of other organizations #420

Closed

pandafy closed this Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

niteshsinha17 commented Mar 4, 2021

coveralls commented Mar 4, 2021 •

edited

Loading

niteshsinha17 Mar 4, 2021 •

edited

Loading

nemesifier left a comment

atb00ker left a comment

atb00ker Mar 9, 2021

niteshsinha17 Mar 10, 2021

nemesifier Mar 10, 2021

nemesifier left a comment

niteshsinha17 commented Mar 12, 2021 •

edited

Loading

nemesifier commented Mar 12, 2021

niteshsinha17 commented Mar 13, 2021

pandafy left a comment

pandafy Apr 6, 2021

pandafy Apr 6, 2021 •

edited

Loading

pandafy Apr 6, 2021

nemesifier Apr 6, 2021

pandafy Apr 6, 2021

pandafy Apr 6, 2021

pandafy Apr 6, 2021

pandafy commented Apr 8, 2021

niteshsinha17 commented Apr 8, 2021 •

edited

Loading

pandafy commented Apr 8, 2021

pandafy commented Apr 8, 2021 •

edited

Loading

		{{ default_template_urls\| safe }},
		{{relevant_template_urls\| safe}}

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

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

Conversation

niteshsinha17 commented Mar 4, 2021

coveralls commented Mar 4, 2021 • edited Loading

niteshsinha17 Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

atb00ker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

niteshsinha17 commented Mar 12, 2021 • edited Loading

nemesifier commented Mar 12, 2021

niteshsinha17 commented Mar 13, 2021

pandafy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy commented Apr 8, 2021

niteshsinha17 commented Apr 8, 2021 • edited Loading

pandafy commented Apr 8, 2021

pandafy commented Apr 8, 2021 • edited Loading

coveralls commented Mar 4, 2021 •

edited

Loading

niteshsinha17 Mar 4, 2021 •

edited

Loading

niteshsinha17 commented Mar 12, 2021 •

edited

Loading

pandafy Apr 6, 2021 •

edited

Loading

niteshsinha17 commented Apr 8, 2021 •

edited

Loading

pandafy commented Apr 8, 2021 •

edited

Loading