Hi!
sorry for delayed answer. my stomach made strange things these
days. long bed days basically. :)
so, thx for the extensive review, let see what changed:
Raphael Hertzog <hert...@debian.org> writes:
> stretch is coming in a few months and we are not using Django from stable
> either, so I would suggest to use directly DRF 3.4.
Done.
>> > Reported-by: Paul Wise <p...@debian.org>
>>
>> Done.
>
> You are still missing: s/_/-/
Done :)
> What about using the API self-documentation feature?
> http://www.django-rest-framework.org/topics/documenting-your-api/#self-describing-apis
I agree.
> That will also force you to choose good names to your views.
I'm not really good choosing names, i went for minimalist approach this
time :)
> Copyright 2016 only for new code. :) Applies on all files.
I didn't get this bit, so i just dropped it. It can be an additional
commit if necessary.
> Please don't use the same "name" for both URL. Use something like
> "...-action-item-list" and "...-action-item-detail".
>
> Also, after having looked at the DRF doc, we should not hardcode the
> versioning at this level, we should rather use NamespaceVersioning:
> http://www.django-rest-framework.org/api-guide/versioning/#namespaceversioning
Implemented it.
>> +class ActionItemListAPIView(generics.ListAPIView):
>
> Shouldn't we merge both objects with a single ReadOnlyModelViewSet?
> http://www.django-rest-framework.org/api-guide/viewsets/#readonlymodelviewset
I'm not a really great fan of viewsets. i think we cannot split hairs
using viewsets. But i took the idea from them to use the mixins + a
generic view.
>> + """
>> + List all ActionItem instances.
>> + """
>
> Here you must explain the GET query parameter that we support.
Now there's a hopefully more verbose explanation.
>> + def get_queryset(self):
>> + queryset = ActionItem.objects.all()
>> + package_name = self.request.GET.get('package_name', None)
>
> package_name is a required abstraction at the database level but it makes
> no sense for end users. I would just use "package" as the query parameter.
done.
>
> Also, one of the early design decision seems to be whether we use an
> hyperlinked serializer or not. I don't have a strong opinion on that but it
> makes it easier to navigate interactively in the data through the browser. So
> unless we have a reason not to, I believe it would be nice to use it:
> http://www.django-rest-framework.org/tutorial/5-relationships-and-hyperlinked-apis/#hyperlinking-our-api
Well imho, it makes sense if we are serializing also other models with a
correspondant URL. So i'm happy to implement this behaviour as the API
grows. But i guess for this specific issue it is not really a must.
> But even if we use the hyperlinked version of the serializer, I would still
> include the "id" attribute everywhere.
>
> Cheers,
> --
> Raphaël Hertzog ◈ Debian Developer
>
> Support Debian LTS: http://www.freexian.com/services/debian-lts.html
> Learn to master Debian: http://debian-handbook.info/get/
Cheers,
--
efkin.
>From 55c077611db6a91b1cfbae45e2f809c12b1b6eb4 Mon Sep 17 00:00:00 2001
From: efkin <ef...@riseup.net>
Date: Wed, 14 Dec 2016 13:04:52 +0100
Subject: [PATCH 1/3] Include rest_framework dependency
---
distro_tracker/project/settings/defaults.py | 1 +
docs/setup/setup.rst | 2 ++
2 files changed, 3 insertions(+)
diff --git a/distro_tracker/project/settings/defaults.py b/distro_tracker/project/settings/defaults.py
index ad7defd..0f27309 100644
--- a/distro_tracker/project/settings/defaults.py
+++ b/distro_tracker/project/settings/defaults.py
@@ -249,6 +249,7 @@ INSTALLED_APPS = (
'django.contrib.staticfiles',
'django.contrib.admin',
'django_email_accounts',
+ 'rest_framework',
'distro_tracker.html',
'distro_tracker.core',
'distro_tracker.accounts',
diff --git a/docs/setup/setup.rst b/docs/setup/setup.rst
index dad9ac0..1898528 100644
--- a/docs/setup/setup.rst
+++ b/docs/setup/setup.rst
@@ -15,6 +15,7 @@ Distro Tracker currently depends on the following Debian packages:
- python-django-jsonfield (>= 1.0.0)
- python-django-debug-toolbar (in development mode only)
- python-django-captcha (optional)
+- python-djangorestframework (= 3.4.X from Stretch)
- python-debian
- python-apt
- python-gpgme
@@ -37,6 +38,7 @@ Here is the list of required packages for development on Debian Jessie::
$ sudo apt install python-django python-requests python-django-jsonfield python-django-debug-toolbar python-debian python-apt python-gpgme python-yaml python-bs4 python-soappy python-ldap python-pyinotify python-tox python-mock python-lzma python-selenium python3-django python3-requests python3-django-jsonfield python3-django-debug-toolbar python3-debian python3-apt python3-gpgme python3-yaml python3-bs4 python3-pyinotify python3-selenium chromium chromedriver
+
.. _database_setup:
Database
--
2.1.4
>From 64e781ade6ef6226f313c03944c91936c8de79f6 Mon Sep 17 00:00:00 2001
From: efkin <ef...@riseup.net>
Date: Wed, 14 Dec 2016 13:14:35 +0100
Subject: [PATCH 2/3] Create distro_tracker submodule for API development
---
distro_tracker/api/__init__.py | 0
distro_tracker/api/tests.py | 3 +++
distro_tracker/api/views.py | 3 +++
distro_tracker/project/settings/defaults.py | 1 +
4 files changed, 7 insertions(+)
create mode 100644 distro_tracker/api/__init__.py
create mode 100644 distro_tracker/api/tests.py
create mode 100644 distro_tracker/api/views.py
diff --git a/distro_tracker/api/__init__.py b/distro_tracker/api/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/distro_tracker/api/tests.py b/distro_tracker/api/tests.py
new file mode 100644
index 0000000..7ce503c
--- /dev/null
+++ b/distro_tracker/api/tests.py
@@ -0,0 +1,3 @@
+from django.test import TestCase
+
+# Create your tests here.
diff --git a/distro_tracker/api/views.py b/distro_tracker/api/views.py
new file mode 100644
index 0000000..91ea44a
--- /dev/null
+++ b/distro_tracker/api/views.py
@@ -0,0 +1,3 @@
+from django.shortcuts import render
+
+# Create your views here.
diff --git a/distro_tracker/project/settings/defaults.py b/distro_tracker/project/settings/defaults.py
index 0f27309..0d42a37 100644
--- a/distro_tracker/project/settings/defaults.py
+++ b/distro_tracker/project/settings/defaults.py
@@ -250,6 +250,7 @@ INSTALLED_APPS = (
'django.contrib.admin',
'django_email_accounts',
'rest_framework',
+ 'distro_tracker.api',
'distro_tracker.html',
'distro_tracker.core',
'distro_tracker.accounts',
--
2.1.4
>From 532cacd4cb085b39544fccdc43fca764d804763b Mon Sep 17 00:00:00 2001
From: efkin <ef...@riseup.net>
Date: Wed, 14 Dec 2016 18:45:09 +0100
Subject: [PATCH] Create basic API list/detail endpoint for ActionItem model
instances
Reported-by: Paul Wise <p...@debian.org>
---
distro_tracker/api/paginators.py | 7 +++
distro_tracker/api/serializers.py | 27 +++++++++++
distro_tracker/api/tests.py | 95 +++++++++++++++++++++++++++++++++++++-
distro_tracker/api/tracker_urls.py | 18 ++++++++
distro_tracker/api/views.py | 47 ++++++++++++++++++-
5 files changed, 190 insertions(+), 4 deletions(-)
create mode 100644 distro_tracker/api/paginators.py
create mode 100644 distro_tracker/api/serializers.py
create mode 100644 distro_tracker/api/tracker_urls.py
diff --git a/distro_tracker/api/paginators.py b/distro_tracker/api/paginators.py
new file mode 100644
index 0000000..7b4f52f
--- /dev/null
+++ b/distro_tracker/api/paginators.py
@@ -0,0 +1,7 @@
+from rest_framework.pagination import LimitOffsetPagination
+
+
+class StandardResultsSetPaignation(LimitOffsetPagination):
+ default_limit = 100
+ max_limit = 500
+
diff --git a/distro_tracker/api/serializers.py b/distro_tracker/api/serializers.py
new file mode 100644
index 0000000..1f1d098
--- /dev/null
+++ b/distro_tracker/api/serializers.py
@@ -0,0 +1,27 @@
+from rest_framework import serializers
+
+from distro_tracker.core.models import ActionItem
+from distro_tracker.core.models import PackageName
+
+
+class ActionItemSerializer(serializers.ModelSerializer):
+
+ package_name = serializers.ReadOnlyField(source="package.name")
+ item_type_name = serializers.ReadOnlyField(source="item_type.type_name")
+
+ class Meta:
+ model = ActionItem
+ fields = (
+ 'id',
+ 'package_name',
+ 'item_type_name',
+ 'short_description',
+ 'severity',
+ 'created_timestamp',
+ 'last_updated_timestamp',
+ 'extra_data',
+ )
+
+
+
+
diff --git a/distro_tracker/api/tests.py b/distro_tracker/api/tests.py
index 7ce503c..7c53110 100644
--- a/distro_tracker/api/tests.py
+++ b/distro_tracker/api/tests.py
@@ -1,3 +1,94 @@
-from django.test import TestCase
+from django.core.urlresolvers import reverse
-# Create your tests here.
+from rest_framework import status
+from rest_framework.test import APITestCase
+
+from distro_tracker.core.models import ActionItem
+from distro_tracker.core.models import ActionItemType
+from distro_tracker.core.models import SourcePackageName
+
+
+class ActionItemListAPIViewTest(APITestCase):
+ """
+ Test for the :class:`distro_tracker.api.views.ActionItemListAPIView`.
+ """
+
+ def setUp(self):
+ self.url = reverse('dtracker-api-v1-action-items')
+
+ def test_empty_list(self):
+ """
+ Test when the queryset is empty.
+ """
+ response = self.client.get(self.url)
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
+ self.assertEqual(
+ 0,
+ response.data['count'],
+ )
+
+ def test_account_item(self):
+ """
+ Test with actual content.
+ """
+ package = SourcePackageName.objects.create(name='dummy-package')
+ action_type = ActionItemType.objects.create(
+ type_name='test',
+ full_description_template='action-item-test.html',
+ )
+ action_item = ActionItem.objects.create(
+ package=package,
+ item_type=action_type,
+ short_description="Short description of item",
+ )
+ response = self.client.get(self.url)
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
+ self.assertEqual(
+ 1,
+ response.data['count'],
+ )
+ result = response.data['results'][0]
+ self.assertEqual(
+ 'dummy-package',
+ result['package_name'],
+ )
+
+
+class ActionItemDetailAPIViewTest(APITestCase):
+ """
+ Test for the :class:`distro_tracker.api.views.ActionItemDetailAPIView`.
+ """
+
+ def setUp(self):
+ self.url = reverse('dtracker-api-v1-action-items', kwargs={'pk':1})
+
+ def test_404_on_non_existing_pk(self):
+ """
+ Test when the pk does not return any instance.
+ """
+ response = self.client.get(self.url)
+
+ self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+
+ def test_existing_instance(self):
+ """
+ Test when the pk does return an actual instance.
+ """
+ package = SourcePackageName.objects.create(name='dummy-package')
+ action_type = ActionItemType.objects.create(
+ type_name='test',
+ full_description_template='action-item-test.html',
+ )
+ action_item = ActionItem.objects.create(
+ package=package,
+ item_type=action_type,
+ short_description="Short description of item",
+ )
+ response = self.client.get(self.url)
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
+ self.assertEqual(
+ 'dummy-package',
+ response.data['package_name'],
+ )
+
+
diff --git a/distro_tracker/api/tracker_urls.py b/distro_tracker/api/tracker_urls.py
new file mode 100644
index 0000000..3febcac
--- /dev/null
+++ b/distro_tracker/api/tracker_urls.py
@@ -0,0 +1,18 @@
+from django.conf.urls import url, include
+
+from .views import ActionItemView
+
+
+v1_patterns = [
+ url(r'^action-items/?$',
+ ActionItemView.as_view(),
+ name='dtracker-api-v1-action-item-list'),
+ url(r'^action-items/(?P<pk>[0-9]+)/?$',
+ ActionItemView.as_view(),
+ name='dtracker-api-v1-action-item-detail'),
+]
+
+
+urlpatterns = [
+ url(r'^api/v1/', include(v1_patterns, namespace='v1')),
+]
diff --git a/distro_tracker/api/views.py b/distro_tracker/api/views.py
index 91ea44a..7c791b6 100644
--- a/distro_tracker/api/views.py
+++ b/distro_tracker/api/views.py
@@ -1,3 +1,46 @@
-from django.shortcuts import render
+from django.http import Http404
-# Create your views here.
+from rest_framework import status
+from rest_framework import generics
+from rest_framework.mixins import ListModelMixin
+from rest_framework.mixins import RetrieveModelMixin
+from rest_framework.response import Response
+
+from distro_tracker.api.serializers import ActionItemSerializer
+from distro_tracker.api.paginators import StandardResultsSetPaignation
+from distro_tracker.core.models import ActionItem
+
+
+class ActionItemView(RetrieveModelMixin,
+ ListModelMixin,
+ generics.GenericAPIView):
+ """
+ This is a readonly endpoint. It retrieves the list of the
+ `ActionItem` instances or a single instance if the `pk` is
+ provided in the URL in the form `/api/v1/action-items/<pk>/`.
+
+ Eventually it accepts also the following query parameters:
+
+ * `package`: a string containing the name of the package
+ * `limit`: an integer indicating the max number of items per query
+ * `offset`: an integer indicating the starting position of the query
+ in relation to the complete set of unpaginated items.
+
+ """
+
+ serializer_class = ActionItemSerializer
+ pagination_class = StandardResultsSetPaignation
+
+ def get_queryset(self):
+ queryset = ActionItem.objects.all()
+ package_name = self.request.query_params.get('package', None)
+ if package_name is not None:
+ queryset = queryset.filter(package__name=package_name)
+ return queryset
+
+ def get(self, request, *args, **kwargs):
+ if 'pk' in kwargs:
+ return self.retrieve(request, *args, **kwargs)
+ else:
+ return self.list(request, *args, **kwargs)
+
--
2.1.4
