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

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

+            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

+            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

+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

+                    {{ 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

+                    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

+                    }
+                });
+                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

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

Uh oh!

Uh oh!

Conversation

niteshsinha17 commented Mar 4, 2021

Uh oh!

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

Uh oh!

Uh oh!

niteshsinha17 Mar 4, 2021 • 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!

atb00ker 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!

niteshsinha17 commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemesifier commented Mar 12, 2021

Uh oh!

niteshsinha17 commented Mar 13, 2021

Uh oh!

pandafy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pandafy Apr 6, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pandafy commented Apr 8, 2021

Uh oh!

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

Uh oh!

Uh oh!

pandafy commented Apr 8, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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