[feature] Added API to get context of device #118 #138

strang1ato · 2019-12-28T12:35:55Z

Fixes #118

coveralls · 2019-12-28T12:40:33Z

Coverage increased (+0.001%) to 99.907% when pulling 4abc78f on oltarzewskik:issues/118 into a738f2b on openwisp:master.

nemesifier

Good! See my comments

django_netjsonconfig/base/admin.py

nemesifier · 2019-12-28T16:42:34Z

django_netjsonconfig/tests/test_admin.py

+        self._create_config(device=device)
+        url = reverse('admin:django_netjsonconfig_device_context', args=[device.pk])
+        response = self.client.get(url)
+        self.assertEqual(str(response.content.decode()), '{"error": "login required"}')


once you do these changes, the error you get here may be different

nemesifier

Progressing well, see my comment

django_netjsonconfig/base/admin.py

nemesifier · 2019-12-30T20:45:35Z

django_netjsonconfig/base/config.py

-        if app_settings.HARDWARE_ID_ENABLED and self._has_device():
-            c.update({'hardware_id': self.device.hardware_id})
+        if self.context:
+            c.update(self.context)


ups, I didn't notice this, I forgot we had this feature, sorry for my bad advice, we need to rollback to how it was in the previous round of review, and we need to make sure the current config instance is being used, unless we're dealing with a new instance, in which case we should create a new one on the fly.

@nemesisdesign so I have to revert changes made in /base/device.py and /base/config.py?

nemesifier · 2019-12-31T09:37:25Z

django_netjsonconfig/base/admin.py

+            context = json.dumps(device.config.get_context())
+        else:
+            device_config = Config(device=device)
+            context = json.dumps(device_config.get_context())


please do not duplicate code.
Use device_config to store the right config instance and then write only one json.dumps(device_config.get_context()) call.

nemesifier · 2019-12-31T09:38:48Z

django_netjsonconfig/tests/test_admin.py

+        device = self._create_device()
+        url = reverse('admin:django_netjsonconfig_device_context', args=[device.pk])
+        response = self.client.get(url)
+        expected_url = reverse('admin:login') + '?next=' + url


write this as:

'{}?next={}'.format(reverse('admin:login') , url)

nemesifier · 2019-12-31T09:40:14Z

django_netjsonconfig/tests/test_admin.py

+        expected_url = reverse('admin:login') + '?next=' + url
+        self.assertRedirects(response, expected_url)
+        # log in again for next tests
+        self.client.login(username='admin', password='tester')


this is not needed, the setUp method of this test class handles that.

nemesifier · 2019-12-31T09:47:40Z

django_netjsonconfig/tests/test_admin.py

+        device = self._create_device(name='sample-device')
+        url = reverse('admin:django_netjsonconfig_device_context', args=[device.pk])
+        response = self.client.get(url)
+        self.assertContains(response, '"name": "sample-device"')


this way of testing your code is not good enough.

Use response.json() and compare it to the result returned by get_context(), they must be equal.

Eg:

self.assertEqual(response.json(), Config(device=device).get_context())

nemesifier · 2019-12-31T09:48:25Z

django_netjsonconfig/tests/test_admin.py

+        self._create_config(device=device)
+        url = reverse('admin:django_netjsonconfig_device_context', args=[device.pk])
+        response = self.client.get(url)
+        self.assertContains(response, '"name": "sample-device"')


same here, the code will be slightly different.

nemesifier

Further review and testing is showing problems.

I left my findings below, let me know if all is clear.

nemesifier · 2019-12-31T11:35:54Z

django_netjsonconfig/base/admin.py

@@ -11,6 +12,7 @@
 from django.template.response import TemplateResponse
 from django.urls import reverse
 from django.utils.translation import ugettext_lazy as _
+from django_netjsonconfig.models import Config


I just realized you cannot do this. You would break extensibility because you are importing a concrete model into base classes. See the next comment on how to fix this, remove this import please.

nemesifier · 2019-12-31T11:50:02Z

django_netjsonconfig/base/admin.py

@@ -200,6 +205,15 @@ def download_view(self, request, pk):
        return send_file(filename='{0}.tar.gz'.format(config.name),
                         contents=config_archive.getvalue())

+    def device_context_view(self, request, pk):


after reviewing the code again, I realized we're editing a base class that is then inherited by the Template admin and VPN admin, this means the context API will generate different views and we're testing only one:

(show_urls is a django-extensions command)

./manage.py show_urls | grep context /admin/django_netjsonconfig/device/<pk>/context\.json django_netjsonconfig.base.admin.device_context_view admin:django_netjsonconfig_device_context /admin/django_netjsonconfig/template/<pk>/context\.json django_netjsonconfig.base.admin.device_context_view admin:django_netjsonconfig_template_context /admin/django_netjsonconfig/vpn/<pk>/context\.json django_netjsonconfig.base.admin.device_context_view admin:django_netjsonconfig_vpn_context

Those views are now raising an exception and are not being tested, we should test the template and vpn views as well.

device_context_view should be renamed to context_view.

nemesifier · 2019-12-31T11:50:22Z

django_netjsonconfig/base/admin.py

@@ -200,6 +205,15 @@ def download_view(self, request, pk):
        return send_file(filename='{0}.tar.gz'.format(config.name),
                         contents=config_archive.getvalue())

+    def device_context_view(self, request, pk):
+        device = get_object_or_404(self.model, pk=pk)


device should be renamed instance as in the beginning, you were right on that one

nemesifier · 2019-12-31T11:52:42Z

django_netjsonconfig/base/admin.py

+            device_config = device.config
+        else:
+            device_config = Config(device=device)
+        context = json.dumps(device_config.get_context())


then you just have to do:

context = json.dumps(instance.get_context())

nemesifier · 2019-12-31T11:54:04Z

django_netjsonconfig/base/admin.py

+        if hasattr(device, 'config'):
+            device_config = device.config
+        else:
+            device_config = Config(device=device)


this logic has to be moved into a get_context method on AbstractDevice, with the difference that in order to get the Config model, you will have to use the get_config_model method.

the get_context method on the device then will have to simply return config.get_context()

please also write a unit test for this new method on the device model in test_models

I just found out that AbstractTemplate class doesn't have get_context function, I should make function there too, right?

strang1ato · 2019-12-31T18:35:38Z

django_netjsonconfig/base/template.py

@@ -100,5 +100,12 @@ def clean(self, *args, **kwargs):
        if self.type == 'vpn' and not self.config:
            self.config = self.vpn.auto_client(auto_cert=self.auto_cert)

+    def get_context(self):


This, function will probably need changes, (I am not sure what this function have to output, can you tell please?)

right, let's take advantage of this situation to do some cleanup.

This module has a setting, NETJSONCONFIG_CONTEXT which contents are added to the Config context (see the line containing c.update(app_settings.CONTEXT)), but not in the result returned by Vpn.get_context(), nor this new method.

So the best thing and most consistent thing to do, is to add a new get_context() method on django_netjsonconfig.base.base.BaseConfig, which just does something like:

def get_context(self): return app_settings.CONTEXT

Then all the other get_context methods on the classes inheriting from BaseConfig must do something like:

def get_context(self): c = super(ModelName, self).get_context() # add more data to c ... return c

nemesifier · 2020-01-02T12:43:58Z

django_netjsonconfig/base/device.py

@@ -79,6 +79,13 @@ def _get_config_attr(self, attr):
        attr = getattr(self.config, attr)
        return attr() if callable(attr) else attr

+    def get_context(self):
+        if hasattr(self, 'config'):


there's a method for this: _has_config

nemesifier · 2020-01-02T12:55:40Z

django_netjsonconfig/base/template.py

@@ -100,5 +100,12 @@ def clean(self, *args, **kwargs):
        if self.type == 'vpn' and not self.config:
            self.config = self.vpn.auto_client(auto_cert=self.auto_cert)

+    def get_context(self):


right, let's take advantage of this situation to do some cleanup.

This module has a setting, NETJSONCONFIG_CONTEXT which contents are added to the Config context (see the line containing c.update(app_settings.CONTEXT)), but not in the result returned by Vpn.get_context(), nor this new method.

So the best thing and most consistent thing to do, is to add a new get_context() method on django_netjsonconfig.base.base.BaseConfig, which just does something like:

def get_context(self): return app_settings.CONTEXT

Then all the other get_context methods on the classes inheriting from BaseConfig must do something like:

def get_context(self): c = super(ModelName, self).get_context() # add more data to c ... return c

strang1ato · 2020-01-02T17:38:31Z

django_netjsonconfig/base/template.py

+            'id': str(self.id),
+            'name': self.name,
+        }
+        c.update(super(AbstractTemplate, self).get_context())


I added this line in end instead of beginning because if we change this dict to json we want app_settings.CONTEXT on end

django_netjsonconfig/tests/test_vpn.py

strang1ato · 2020-01-02T17:44:17Z

django_netjsonconfig/tests/test_vpn.py

@@ -223,9 +225,9 @@ def test_dh(self):
        self.assertTrue(v.dh.startswith('-----BEGIN DH PARAMETERS-----'))
        self.assertTrue(v.dh.endswith('-----END DH PARAMETERS-----\n'))

-    def test_context_empty(self):
+    def test_get_context_empty_vpn(self):


I changed name of this function, because old name would lead to misunderstanding, it now clearly say that we are testing empty vpn object

Fixes #118

nemesifier

Great work! 👍

nemesifier reviewed Dec 28, 2019

View reviewed changes

nemesifier suggested changes Dec 30, 2019

View reviewed changes

django_netjsonconfig/base/admin.py Outdated Show resolved Hide resolved

django_netjsonconfig/base/admin.py Show resolved Hide resolved

nemesifier suggested changes Dec 30, 2019

View reviewed changes

django_netjsonconfig/base/admin.py Outdated Show resolved Hide resolved

nemesifier reviewed Dec 30, 2019

View reviewed changes

nemesifier suggested changes Dec 31, 2019

View reviewed changes

strang1ato commented Dec 31, 2019

View reviewed changes

nemesifier suggested changes Jan 2, 2020

View reviewed changes

strang1ato commented Jan 2, 2020

View reviewed changes

django_netjsonconfig/tests/test_vpn.py Outdated Show resolved Hide resolved

strang1ato commented Jan 2, 2020

View reviewed changes

[feature] Added API to get context of device #118

4abc78f

Fixes #118

nemesifier approved these changes Jan 3, 2020

View reviewed changes

nemesifier merged commit ce0a694 into openwisp:master Jan 3, 2020

strang1ato deleted the issues/118 branch January 3, 2020 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Added API to get context of device #118 #138

[feature] Added API to get context of device #118 #138

strang1ato commented Dec 28, 2019

coveralls commented Dec 28, 2019 •

edited

Loading

nemesifier left a comment

nemesifier Dec 28, 2019

nemesifier left a comment

nemesifier Dec 30, 2019

strang1ato Dec 30, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier left a comment

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019 •

edited

Loading

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

nemesifier Dec 31, 2019

strang1ato Dec 31, 2019

strang1ato Dec 31, 2019 •

edited

Loading

nemesifier Jan 2, 2020

nemesifier Jan 2, 2020

nemesifier Jan 2, 2020

strang1ato Jan 2, 2020

strang1ato Jan 2, 2020

nemesifier left a comment

[feature] Added API to get context of device #118 #138

[feature] Added API to get context of device #118 #138

Conversation

strang1ato commented Dec 28, 2019

coveralls commented Dec 28, 2019 • edited Loading

nemesifier left a 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

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

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier Dec 31, 2019 • 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

Choose a reason for hiding this comment

strang1ato Dec 31, 2019 • 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

nemesifier left a comment

Choose a reason for hiding this comment

coveralls commented Dec 28, 2019 •

edited

Loading

nemesifier Dec 31, 2019 •

edited

Loading

strang1ato Dec 31, 2019 •

edited

Loading