[API] Added REST API for Notifications #48

pandafy · 2020-06-17T00:32:25Z

Bumps openwisp-utils to version >=0.5
Implements API for listing marking notification as read
Closes #3

NoumbissiValere · 2020-06-17T08:08:52Z

openwisp_notifications/api/views.py

+            notification_id = uuid.UUID(notification_id)
+            notification = queryset.get(id=notification_id)
+        except ValueError:
+            return Response(status=status.HTTP_400_BAD_REQUEST)


@pandafy will it not be nice for us to give users some more information as to why their response didn't go through? It this case; it was caused by invalid notification_id

NoumbissiValere · 2020-06-17T08:09:28Z

openwisp_notifications/api/views.py

+        except ValueError:
+            return Response(status=status.HTTP_400_BAD_REQUEST)
+        except Notification.DoesNotExist:
+            return Response(status=status.HTTP_404_NOT_FOUND)


NoumbissiValere

Great work @pandafy 👍
But I think instead of having a mark_read patch api, we could make this a details api for a specific notification. remember from the js widget, we should be able to read a notification. so replacing this with a get API for a specific notification will ease that work as getting a specific notification from the api will mark it as read.
Also, I think we should have an API to list all read notifications as well unread notifications, this could be use as a filter on the js widget.

pandafy · 2020-06-19T10:47:20Z

Also, I think we should have an API to list all read notifications as well unread notifications, this could be use as a filter on the js widget.

@NoumbissiValere Instead of adding them as individual endpoints, I have set them as filters using django-filters.

NoumbissiValere

Great job @pandafy.
see some minor requests below

NoumbissiValere · 2020-06-19T18:02:59Z

openwisp_notifications/api/urls.py

+    url(
+        r'^openwisp_notifications/notifications/$',
+        views.list_notifications,
+        name='api_list_notifications',


I think it will be good if we have this as list_notifications

NoumbissiValere · 2020-06-19T18:03:47Z

openwisp_notifications/api/urls.py

+    url(
+        r'^openwisp_notifications/notifications/(?P<notification_id>[0-9A-Fa-f-]+)/$',
+        views.read_notification,
+        name='api_read_notifications',


same here read_notification. since we are getting a single notification, keep away the s

NoumbissiValere · 2020-06-19T18:06:17Z

openwisp_notifications/api/views.py

+    queryset = Notification.objects.all()
+    serializer_class = NotificationSerializer
+    filter_backends = [DjangoFilterBackend]
+    filterset_fields = ['unread']


I don't think filter_backends and filterset_fields are needed in NotificationReadView so they should be removed from the BaseNotificationView and send to the NotificationListView`

Great catch! 👍

nemesifier

Good progress! See my comments

nemesifier · 2020-06-21T18:50:25Z

README.rst

+
+.. code-block:: text
+
+    GET /api/v1/openwisp_notifications/notifications/


shorten openwisp_notifications/notifications/ to just notifications/

nemesifier · 2020-06-21T18:50:31Z

README.rst

+
+.. code-block:: text
+
+    PATCH /api/v1/openwisp_notifications/notifications/{id}/


nemesifier · 2020-06-21T18:54:32Z

openwisp_notifications/api/urls.py

+app_name = 'openwisp_notifications'
+
+urlpatterns = [
+    url(


time to start using path instead of url, see the django docs

nemesifier · 2020-06-21T18:54:53Z

openwisp_notifications/api/urls.py

+        name='list_notifications',
+    ),
+    url(
+        r'^openwisp_notifications/notifications/(?P<notification_id>[0-9A-Fa-f-]+)/$',


use path with uuid converter, eg: notifications/<uuid:pk>/.

Use pk instead of id which is more generic.

nemesifier · 2020-06-21T18:57:20Z

tests/openwisp2/sample_notifications/urls.py

+
+urlpatterns = [
+    url(
+        r'^api/v1/sample_notifications/notifications/$',


find a way to avoid repeating the url/path patterns, see how @atb00ker is doing it in the other modules he's working on

tests/openwisp2/urls.py

nemesifier · 2020-06-21T19:02:13Z

openwisp_notifications/api/views.py

+        return self.list(request, *args, **kwargs)
+
+
+class NotificationReadView(BaseNotificationView):


I would call this NotificationDetailView

This view does not provide details about a particular notification. It just mark them as read.

I understand, I misread this.

Marking notifications as read does not need to return anything, because the consumer of the API is supposed to already have the list of notifications and we don't need to send back anything.

I would do the following:

GET /api/v1/notifications/ to get the list of notifications

GET /api/v1/notifications/<pk>/ to get the details of a specific notification, we add this just to make the API more complete

POST /api/v1/notifications/read/ to flag all notifications as read, make it return just 200 OK and empty response body.

nemesifier · 2020-06-21T19:05:30Z

README.rst

+Authentication
+~~~~~~~~~~~~~~
+
+The API authentication is based on session based authentication via  REST framework.


Why isn't this as https://github.com/openwisp/openwisp-firmware-upgrader/#authentication
How to authenticate is explained in openwisp-users to avoid duplicating the information in every module.
When we'll have the comprehensive developer documentation we'll be linking a different section of the same documentation instead of a different repository, that will be the best, but for now, linking a different repo is the best option we have (duplicating the info in all the modules is unmaintainable for us right now).

I have used session based authentication as in openwisp-ipam. I didn't thought it would be necessary to use use token based authentication.

openwisp-ipam is not up to date, see openwisp/openwisp-ipam#33

Base your work by following openwisp-firmware-upgrader, which is up to date.

And yes, please add support for bearer authentication, we want to make sure the API can be used also outside of django easily. Beware of this bug in openwisp-users: openwisp/openwisp-users#126 @atb00ker should fix that soon, coordinate with him if you have any issue.

@pandafy openwisp/openwisp-firmware-upgrader#85 please use this to avoid openwisp/openwisp-users#126

nemesifier · 2020-06-21T19:06:36Z

README.rst

+Browsable web interface
+~~~~~~~~~~~~~~~~~~~~~~~
+
+.. image:: docs/images/api-ui.png


great to see you added screenshots, we'll have to update them when the URLs are changed as requested

nemesifier · 2020-06-21T19:51:01Z

openwisp_notifications/api/views.py

+        return self.list(request, *args, **kwargs)
+
+
+class NotificationReadView(BaseNotificationView):


I understand, I misread this.

Marking notifications as read does not need to return anything, because the consumer of the API is supposed to already have the list of notifications and we don't need to send back anything.

I would do the following:

GET /api/v1/notifications/ to get the list of notifications

GET /api/v1/notifications/<pk>/ to get the details of a specific notification, we add this just to make the API more complete

POST /api/v1/notifications/read/ to flag all notifications as read, make it return just 200 OK and empty response body.

nemesifier

Did a local test and it looks good (haven't been able to create test notifications in the dev env quicky though).

See below for further improvements and we'll be ready to move on.

nemesifier · 2020-06-24T00:33:24Z

README.rst

+Live documentation
+~~~~~~~~~~~~~~~~~~
+
+.. image:: docs/images/api-docs.png


I forgot to mention that relative paths don't work when the README is published elsewhere (eg: pypi).
with @atb00ker we had the same issue before.

Yeah, using the complete links for now https://github.com/......api-docs.png should fix the issue! 😄

Thanks, I was not aware of this. 😄

nemesifier · 2020-06-24T00:36:44Z

openwisp_notifications/api/views.py

+class BaseNotificationView(GenericAPIView):
+    model = Notification
+    authentication_classes = [BearerAuthentication, SessionAuthentication]
+    permission_classes = [DjangoModelPermissions]


spotted, I think we can simplify this and just use IsAuthenticated, because we want all users to be able to access their notifications if this module is enabled, regardless of the permissions (django model permissions are not really useful in this case). Using IsAuthenticated will result in less load on the server when retrieving notifications (permission checks are not free).

tests/openwisp2/urls.py

nemesifier · 2020-06-24T00:45:15Z

openwisp_notifications/tests/test_api.py

+NOT_FOUND_ERROR = ErrorDetail(string='Not found.', code='not_found')
+
+
+class TestNotificationApi(TestCase, TestOrganizationMixin, AuthenticationMixin):


can you add a test which does the following:

given 2 users, Joe and Karen

ensure Joe cannot GET, PATCH or DELETE a notification ID that is assigned to Karen

Given the current code, it looks to me that this should work, but someone in the future may inadvertently change the code and break it.

Better safe than sorry.

nemesifier · 2020-06-24T00:58:10Z

README.rst

+    urlpatterns = [
+        path('admin/', admin.site.urls),
+        path('api/v1/', include(('openwisp_users.api.urls', 'users'), namespace='users')),
+        path('',include('openwisp_notifications.urls', namespace='openwisp_notifications')),


missing space before include

Added API endpoint for notifications: List, retrieve, delete and flag notifications as read.

nemesifier · 2020-06-26T03:47:21Z

openwisp_notifications/tests/test_api.py

+            self.assertEqual(response.status_code, 404)
+            self.assertDictEqual(response.data, {'detail': NOT_FOUND_ERROR})
+            # Check Karen's notification is still unread
+            n = Notification.objects.first()


did you know you can use n.refresh_from_db() ?

pandafy added this to Open Pull-Requests in [GSOC20] Notifications via automation Jun 17, 2020

pandafy force-pushed the issues/3-rest-api branch from 129921a to 32559b9 Compare June 17, 2020 00:34

NoumbissiValere reviewed Jun 17, 2020

View reviewed changes

pandafy changed the title ~~Issues/3 rest api~~ [API] Added REST API for Notifications Jun 17, 2020

pandafy marked this pull request as ready for review June 18, 2020 08:41

pandafy force-pushed the issues/3-rest-api branch from 02cf1f8 to dcd0868 Compare June 18, 2020 08:42

pandafy requested review from NoumbissiValere and nemesifier June 18, 2020 09:34

pandafy moved this from Open Pull-Requests to Ready for Review/Test in [GSOC20] Notifications Jun 18, 2020

NoumbissiValere reviewed Jun 18, 2020

View reviewed changes

pandafy force-pushed the issues/3-rest-api branch from 7597720 to f6a69c2 Compare June 19, 2020 07:08

NoumbissiValere mentioned this pull request Jun 19, 2020

[admin] Add a JS widget to browse notifications #4

Closed

3 tasks

pandafy force-pushed the issues/3-rest-api branch from f6a69c2 to e38a680 Compare June 19, 2020 10:38

pandafy force-pushed the issues/3-rest-api branch from e38a680 to fa6b5fc Compare June 19, 2020 10:50

pandafy requested a review from NoumbissiValere June 19, 2020 11:01

NoumbissiValere suggested changes Jun 19, 2020

View reviewed changes

[GSOC20] Notifications automation moved this from Ready for Review/Test to Open Pull-Requests Jun 19, 2020

pandafy force-pushed the issues/3-rest-api branch 4 times, most recently from 64dab81 to ff37070 Compare June 21, 2020 11:55

nemesifier requested changes Jun 21, 2020

View reviewed changes

pandafy force-pushed the issues/3-rest-api branch from ff37070 to 10e1ff7 Compare June 23, 2020 11:03

[dependencies] Bumped openwisp-utils[qa,rest]>=0.5

419cc03

pandafy force-pushed the issues/3-rest-api branch 3 times, most recently from 6892050 to 78dd814 Compare June 23, 2020 12:57

pandafy force-pushed the issues/3-rest-api branch 4 times, most recently from 6a85043 to 1bbd411 Compare June 23, 2020 14:36

pandafy requested a review from NoumbissiValere June 23, 2020 14:44

pandafy moved this from Open Pull-Requests to Ready for Review/Test in [GSOC20] Notifications Jun 23, 2020

pandafy force-pushed the issues/3-rest-api branch from 1bbd411 to b506c0d Compare June 23, 2020 19:38

pandafy requested a review from nemesifier June 23, 2020 19:43

pandafy force-pushed the issues/3-rest-api branch from b506c0d to 781ffb5 Compare June 23, 2020 20:58

nemesifier mentioned this pull request Jun 24, 2020

Release 0.1.0 #53

Closed

nemesifier requested changes Jun 24, 2020

View reviewed changes

[GSOC20] Notifications automation moved this from Ready for Review/Test to Open Pull-Requests Jun 24, 2020

pandafy force-pushed the issues/3-rest-api branch 2 times, most recently from d65d099 to 0784a8f Compare June 24, 2020 15:33

pandafy requested a review from nemesifier June 24, 2020 15:37

pandafy moved this from Open Pull-Requests to Ready for Review/Test in [GSOC20] Notifications Jun 24, 2020

pandafy force-pushed the issues/3-rest-api branch from 0784a8f to 3e62713 Compare June 25, 2020 06:28

[api] Added API endpoints for notifications #3

47b0f4f

Added API endpoint for notifications: List, retrieve, delete and flag notifications as read.

pandafy force-pushed the issues/3-rest-api branch from 3e62713 to 47b0f4f Compare June 25, 2020 17:42

nemesifier approved these changes Jun 26, 2020

View reviewed changes

nemesifier merged commit f90ae68 into master Jun 26, 2020

[GSOC20] Notifications automation moved this from Ready for Review/Test to Done Jun 26, 2020

pandafy deleted the issues/3-rest-api branch July 3, 2020 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Added REST API for Notifications #48

[API] Added REST API for Notifications #48

pandafy commented Jun 17, 2020

NoumbissiValere Jun 17, 2020 •

edited

Loading

NoumbissiValere Jun 17, 2020

NoumbissiValere left a comment

pandafy commented Jun 19, 2020

NoumbissiValere left a comment

NoumbissiValere Jun 19, 2020

NoumbissiValere Jun 19, 2020

NoumbissiValere Jun 19, 2020 •

edited

Loading

pandafy Jun 19, 2020 •

edited

Loading

nemesifier left a comment

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020

pandafy Jun 21, 2020

nemesifier Jun 21, 2020 •

edited

Loading

nemesifier Jun 21, 2020

pandafy Jun 21, 2020

nemesifier Jun 21, 2020

atb00ker Jun 21, 2020 •

edited

Loading

nemesifier Jun 21, 2020

nemesifier Jun 21, 2020 •

edited

Loading

nemesifier left a comment

nemesifier Jun 24, 2020

atb00ker Jun 24, 2020

pandafy Jun 24, 2020

nemesifier Jun 24, 2020

nemesifier Jun 24, 2020

nemesifier Jun 24, 2020

nemesifier Jun 26, 2020


		.. code-block:: text

		GET /api/v1/openwisp_notifications/notifications/


		.. code-block:: text

		PATCH /api/v1/openwisp_notifications/notifications/{id}/

		return self.list(request, args, *kwargs)


		class NotificationReadView(BaseNotificationView):

		NOT_FOUND_ERROR = ErrorDetail(string='Not found.', code='not_found')


		class TestNotificationApi(TestCase, TestOrganizationMixin, AuthenticationMixin):

[API] Added REST API for Notifications #48

[API] Added REST API for Notifications #48

Conversation

pandafy commented Jun 17, 2020

NoumbissiValere Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere left a comment

Choose a reason for hiding this comment

pandafy commented Jun 19, 2020

NoumbissiValere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoumbissiValere Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

pandafy Jun 19, 2020 • edited Loading

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 Jun 21, 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

atb00ker Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier Jun 21, 2020 • edited Loading

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

NoumbissiValere Jun 17, 2020 •

edited

Loading

NoumbissiValere Jun 19, 2020 •

edited

Loading

pandafy Jun 19, 2020 •

edited

Loading

nemesifier Jun 21, 2020 •

edited

Loading

atb00ker Jun 21, 2020 •

edited

Loading

nemesifier Jun 21, 2020 •

edited

Loading