[api] /receive/{id}/ > /topology/{id}/receive/ #62 #79

NoumbissiValere · 2020-07-31T06:36:06Z

Closed issue by renaming /receive/{id}/ to /topology/{id}/receive/
while ensuring backward compatibility with warnings.

Closes #62

coveralls · 2020-07-31T06:40:44Z

Coverage increased (+0.005%) to 98.967% when pulling 4434b33 on issue-62-change-api into bcd2203 on master.

coveralls · 2020-07-31T06:40:45Z

Coverage increased (+0.01%) to 98.973% when pulling c874632 on issue-62-change-api into dff2357 on master.

nemesifier · 2020-07-31T21:56:06Z

openwisp_network_topology/api/views.py

@@ -75,7 +75,15 @@ def post(self, request, pk, format=None):
                'with message "%s"'
            ) % (topology.get_parser_display(), e.__class__.__name__, e)
            return Response({'detail': error}, status=400)
-        return Response({'detail': _('data received successfully')})
+        success_message = 'data received successfully'


should be success_message = _('data received successfully').

The marking for translation must be done this way, I think otherwise the i18n system would not recognize which string to be made translatable.

nemesifier · 2020-07-31T21:56:22Z

openwisp_network_topology/api/views.py

+            warning = (
+                ". But this url is deprecated and will soon be removed."
+                " We now use /topology/{id}/receive/"
+            )


please also log the warning:

logger = logging.getLogger(__name__) logger.warning( ....... )

nemesifier · 2020-07-31T21:57:55Z

openwisp_network_topology/api/views.py

+                " We now use /topology/{id}/receive/"
+            )
+            return Response({'detail': (success_message + warning)})
+        return Response({'detail': _(success_message)})


again... do not repeat yourself.

if request.path == deprecated_url: success_message = success_message + warning return Response({'detail': success_message})

nemesifier · 2020-07-31T22:01:37Z

openwisp_network_topology/api/views.py

+        if request.path == deprecated_url:
+            warning = (
+                ". But this url is deprecated and will soon be removed."
+                " We now use /topology/{id}/receive/"


Simplify: "This URL is depercated and will be removed in future versions, use /topology/{id}/receive/".

Make translatable at the moment of defining it here as well.

warning = _('This URL is depercated and will be removed in future versions, use /topology/{id}/receive/') success_message = '{success_message}. {warning}'

nemesifier · 2020-07-31T23:51:50Z

openwisp_network_topology/api/views.py

+            )
+            logger.warning(warning)
+            success_message = f'{success_message}. {warning}'
+        return Response({'detail': _(success_message)})


should be success_message instead of _(success_message) because it was already marked as translatable before

nemesifier · 2020-07-31T23:52:32Z

openwisp_network_topology/api/views.py

+        if request.path == deprecated_url:
+            warning = _(
+                "This URL is depercated and will be removed in "
+                "future versions, use /topology/{id}/receive/"


use single quotes for consistency

nemesifier

Good progress, it works, but the URL shown in the UI needs to be updated so the new one is used:

nemesifier · 2020-08-05T02:52:10Z

openwisp_network_topology/api/views.py

+        if request.path == deprecated_url:
+            warning = _(
+                'This URL is depercated and will be removed in '
+                'future versions, use /topology/{id}/receive/'


this here should be /api/v1/topology/{id}/receive/

nemesifier · 2020-08-05T02:52:31Z

openwisp_network_topology/tests/test_api.py

+        self.node_model.objects.all().delete()
+        data = self._load('static/netjson-1-link.json')
+        t = self.topology_model.objects.first()
+        path = f'/api/v1/receive/{t.pk}/?key=test'


please use reverse(), do not hardcode URLs

@nemesisdesign since we have two urls mapping to same view (because we are supporting backward compatibility), reverse will get confused which url to use since both of them have the same view name.

@NoumbissiValere the solution is to change the old URL to have receive_topology_deprecated and use reverse here.

NoumbissiValere · 2020-08-11T23:07:24Z

openwisp_network_topology/integrations/device/tests.py

@@ -269,6 +269,7 @@ def test_monitoring_integration(self, *args):
                        'openwisp_monitoring.device',
                        'openwisp_monitoring.check',
                        'openwisp_controller.connection',
+                        'openwisp_notifications',


this test was failing without this line

please remove this and the other thing related to markdown, if there's an issue with this part it must be resolved in another patch, not in this one

NoumbissiValere · 2020-08-11T23:08:21Z

requirements-test.txt

@@ -2,3 +2,4 @@ coveralls
 responses
 freezegun
 openwisp-utils[qa]~=0.5.1
+markdown


needed by openwisp-notifications during testing

nemesifier · 2020-08-12T02:37:05Z

openwisp_network_topology/integrations/device/tests.py

@@ -269,6 +269,7 @@ def test_monitoring_integration(self, *args):
                        'openwisp_monitoring.device',
                        'openwisp_monitoring.check',
                        'openwisp_controller.connection',
+                        'openwisp_notifications',


please remove this and the other thing related to markdown, if there's an issue with this part it must be resolved in another patch, not in this one

nemesifier · 2020-08-12T02:38:33Z

openwisp_network_topology/tests/test_api.py

+        self.node_model.objects.all().delete()
+        data = self._load('static/netjson-1-link.json')
+        t = self.topology_model.objects.first()
+        path = f'/api/v1/receive/{t.pk}/?key=test'


@NoumbissiValere the solution is to change the old URL to have receive_topology_deprecated and use reverse here.

nemesifier · 2020-08-13T22:52:42Z

openwisp_network_topology/api/views.py

@@ -75,7 +77,16 @@ def post(self, request, pk, format=None):
                'with message "%s"'
            ) % (topology.get_parser_display(), e.__class__.__name__, e)
            return Response({'detail': error}, status=400)
-        return Response({'detail': _('data received successfully')})
+        success_message = _('data received successfully')
+        deprecated_url = f'/api/v1/receive/{pk}/'


do not hardcode URLs, use reverse()

nemesifier · 2020-08-13T22:52:51Z

openwisp_network_topology/api/views.py

+        if request.path == deprecated_url:
+            warning = _(
+                'This URL is depercated and will be removed in '
+                'future versions, use /api/v1/topology/{id}/receive/'


do not hardcode URLs, use reverse()

Closed issue by renaming /receive/{id}/ to /topology/{id}/receive/ while ensuring backward compatibility with warnings. Closes #62

NoumbissiValere requested a review from nemesifier July 31, 2020 06:36

nemesifier requested changes Jul 31, 2020

View reviewed changes

NoumbissiValere force-pushed the issue-62-change-api branch 2 times, most recently from 3359292 to 1940f9c Compare July 31, 2020 22:41

NoumbissiValere requested a review from nemesifier July 31, 2020 22:45

nemesifier requested changes Jul 31, 2020

View reviewed changes

NoumbissiValere force-pushed the issue-62-change-api branch 3 times, most recently from 6a8a2a3 to 8cc436c Compare August 1, 2020 00:27

nemesifier requested changes Aug 5, 2020

View reviewed changes

NoumbissiValere force-pushed the issue-62-change-api branch 4 times, most recently from 5d26e53 to 1e34993 Compare August 11, 2020 23:04

NoumbissiValere commented Aug 11, 2020

View reviewed changes

NoumbissiValere requested a review from nemesifier August 11, 2020 23:09

nemesifier requested changes Aug 12, 2020

View reviewed changes

NoumbissiValere force-pushed the issue-62-change-api branch from 1e34993 to aee4d01 Compare August 13, 2020 18:28

NoumbissiValere self-assigned this Aug 13, 2020

NoumbissiValere requested a review from nemesifier August 13, 2020 18:47

NoumbissiValere force-pushed the issue-62-change-api branch 2 times, most recently from e531b25 to fa23c13 Compare August 13, 2020 21:54

nemesifier requested changes Aug 13, 2020

View reviewed changes

[api] Change /receive/{id}/ to /topology/{id}/receive/ #62

c874632

Closed issue by renaming /receive/{id}/ to /topology/{id}/receive/ while ensuring backward compatibility with warnings. Closes #62

NoumbissiValere force-pushed the issue-62-change-api branch from fa23c13 to c874632 Compare August 14, 2020 06:26

nemesifier approved these changes Aug 14, 2020

View reviewed changes

nemesifier merged commit 8711a9b into master Aug 14, 2020

nemesifier deleted the issue-62-change-api branch August 14, 2020 21:25

nemesifier mentioned this pull request Dec 13, 2020

[change] Remove deprecated receive API URL #101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] /receive/{id}/ > /topology/{id}/receive/ #62 #79

[api] /receive/{id}/ > /topology/{id}/receive/ #62 #79

NoumbissiValere commented Jul 31, 2020

coveralls commented Jul 31, 2020

coveralls commented Jul 31, 2020 •

edited

Loading

nemesifier Jul 31, 2020

nemesifier Jul 31, 2020

nemesifier Jul 31, 2020

nemesifier Jul 31, 2020

nemesifier Jul 31, 2020

nemesifier Jul 31, 2020

nemesifier left a comment

nemesifier Aug 5, 2020

nemesifier Aug 5, 2020

NoumbissiValere Aug 11, 2020

nemesifier Aug 12, 2020

NoumbissiValere Aug 11, 2020

nemesifier Aug 12, 2020

NoumbissiValere Aug 11, 2020

nemesifier Aug 12, 2020

nemesifier Aug 12, 2020

nemesifier Aug 13, 2020

nemesifier Aug 13, 2020

[api] /receive/{id}/ > /topology/{id}/receive/ #62 #79

[api] /receive/{id}/ > /topology/{id}/receive/ #62 #79

Conversation

NoumbissiValere commented Jul 31, 2020

coveralls commented Jul 31, 2020

coveralls commented Jul 31, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 31, 2020 •

edited

Loading