diff mbox series

[v4,2/9] api: change <pk> parameter to <cover_id> for cover comments endpoint

Message ID 20210820045030.3364156-3-raxel@google.com
State Accepted
Headers show
Series patch-detail: add unaddressed/addressed status to patch comments | expand

Commit Message

Raxel Gutierrez Aug. 20, 2021, 4:50 a.m. UTC
Rename cover lookup parameter `pk` to `cover_id` for the cover comments
list endpoints to disambiguate from the lookup parameter `comment_id` in
upcoming patches which introduces the cover comments detail endpoint.
This doesn't affect the user-facing API.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 patchwork/api/comment.py            | 6 +++---
 patchwork/api/cover.py              | 2 +-
 patchwork/tests/api/test_comment.py | 4 ++--
 patchwork/urls.py                   | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

Comments

Stephen Finucane Aug. 20, 2021, 10:15 p.m. UTC | #1
On Fri, 2021-08-20 at 04:50 +0000, Raxel Gutierrez wrote:
> Rename cover lookup parameter `pk` to `cover_id` for the cover comments
> list endpoints to disambiguate from the lookup parameter `comment_id` in
> upcoming patches which introduces the cover comments detail endpoint.
> This doesn't affect the user-facing API.
> 
> Signed-off-by: Raxel Gutierrez <raxel@google.com>

LGTM with one comment.

> ---
>  patchwork/api/comment.py            | 6 +++---
>  patchwork/api/cover.py              | 2 +-
>  patchwork/tests/api/test_comment.py | 4 ++--
>  patchwork/urls.py                   | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> index 0c578b44..5d7a77a1 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -83,14 +83,14 @@ class CoverCommentList(ListAPIView):
>      search_fields = ('subject',)
>      ordering_fields = ('id', 'subject', 'date', 'submitter')
>      ordering = 'id'
> -    lookup_url_kwarg = 'pk'
> +    lookup_url_kwarg = 'cover_id'
>  
>      def get_queryset(self):
> -        if not Cover.objects.filter(pk=self.kwargs['pk']).exists():
> +        if not Cover.objects.filter(id=self.kwargs['cover_id']).exists():
>              raise Http404

We should change these two lines to:

   get_object_or_404(Cover, id=self.kwargs['pk'])

and the following import added at the top of the file:

  from rest_framework.generics import get_object_or_404

See [1] and [2] for the reason why.

Other than this, LGTM. If you don't need to respin, we'll fix while merging.

Reviewed-by: Stephen Finucane <stephen@that.guru>

Stephen

[1] https://lists.ozlabs.org/pipermail/patchwork/2021-August/007140.html
[2] https://lists.ozlabs.org/pipermail/patchwork/2021-August/007142.html

>  
>          return CoverComment.objects.filter(
> -            cover=self.kwargs['pk']
> +            cover=self.kwargs['cover_id']
>          ).select_related('submitter')
>  
>  
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index b4a3a8f7..c4355f6b 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -37,7 +37,7 @@ class CoverListSerializer(BaseHyperlinkedModelSerializer):
>  
>      def get_comments(self, cover):
>          return self.context.get('request').build_absolute_uri(
> -            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
> +            reverse('api-cover-comment-list', kwargs={'cover_id': cover.id}))
>  
>      def to_representation(self, instance):
>          # NOTE(stephenfin): This is here to ensure our API looks the same even
> diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
> index 59450d8a..53abf8f0 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -27,7 +27,7 @@ class TestCoverComments(utils.APITestCase):
>          kwargs = {}
>          if version:
>              kwargs['version'] = version
> -        kwargs['pk'] = cover.id
> +        kwargs['cover_id'] = cover.id
>  
>          return reverse('api-cover-comment-list', kwargs=kwargs)
>  
> @@ -79,7 +79,7 @@ class TestCoverComments(utils.APITestCase):
>      def test_list_invalid_cover(self):
>          """Ensure we get a 404 for a non-existent cover letter."""
>          resp = self.client.get(
> -            reverse('api-cover-comment-list', kwargs={'pk': '99999'}))
> +            reverse('api-cover-comment-list', kwargs={'cover_id': '99999'}))
>          self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
>  
>  
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 1e6c12ab..0180e76d 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -337,7 +337,7 @@ if settings.ENABLE_REST_API:
>              name='api-patch-comment-list',
>          ),
>          path(
> -            'covers/<pk>/comments/',
> +            'covers/<cover_id>/comments/',
>              api_comment_views.CoverCommentList.as_view(),
>              name='api-cover-comment-list',
>          ),
Daniel Axtens Aug. 23, 2021, 8:51 a.m. UTC | #2
I made some changes to handle the bugfixes that we made over the weekend
and then applied this.

Raxel Gutierrez <raxel@google.com> writes:

> Rename cover lookup parameter `pk` to `cover_id` for the cover comments
> list endpoints to disambiguate from the lookup parameter `comment_id` in
> upcoming patches which introduces the cover comments detail endpoint.
> This doesn't affect the user-facing API.
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  patchwork/api/comment.py            | 6 +++---
>  patchwork/api/cover.py              | 2 +-
>  patchwork/tests/api/test_comment.py | 4 ++--
>  patchwork/urls.py                   | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> index 0c578b44..5d7a77a1 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -83,14 +83,14 @@ class CoverCommentList(ListAPIView):
>      search_fields = ('subject',)
>      ordering_fields = ('id', 'subject', 'date', 'submitter')
>      ordering = 'id'
> -    lookup_url_kwarg = 'pk'
> +    lookup_url_kwarg = 'cover_id'
>  
>      def get_queryset(self):
> -        if not Cover.objects.filter(pk=self.kwargs['pk']).exists():
> +        if not Cover.objects.filter(id=self.kwargs['cover_id']).exists():
>              raise Http404
>  
>          return CoverComment.objects.filter(
> -            cover=self.kwargs['pk']
> +            cover=self.kwargs['cover_id']
>          ).select_related('submitter')
>  
>  
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index b4a3a8f7..c4355f6b 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -37,7 +37,7 @@ class CoverListSerializer(BaseHyperlinkedModelSerializer):
>  
>      def get_comments(self, cover):
>          return self.context.get('request').build_absolute_uri(
> -            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
> +            reverse('api-cover-comment-list', kwargs={'cover_id': cover.id}))
>  
>      def to_representation(self, instance):
>          # NOTE(stephenfin): This is here to ensure our API looks the same even
> diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
> index 59450d8a..53abf8f0 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -27,7 +27,7 @@ class TestCoverComments(utils.APITestCase):
>          kwargs = {}
>          if version:
>              kwargs['version'] = version
> -        kwargs['pk'] = cover.id
> +        kwargs['cover_id'] = cover.id
>  
>          return reverse('api-cover-comment-list', kwargs=kwargs)
>  
> @@ -79,7 +79,7 @@ class TestCoverComments(utils.APITestCase):
>      def test_list_invalid_cover(self):
>          """Ensure we get a 404 for a non-existent cover letter."""
>          resp = self.client.get(
> -            reverse('api-cover-comment-list', kwargs={'pk': '99999'}))
> +            reverse('api-cover-comment-list', kwargs={'cover_id': '99999'}))
>          self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
>  
>  
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 1e6c12ab..0180e76d 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -337,7 +337,7 @@ if settings.ENABLE_REST_API:
>              name='api-patch-comment-list',
>          ),
>          path(
> -            'covers/<pk>/comments/',
> +            'covers/<cover_id>/comments/',
>              api_comment_views.CoverCommentList.as_view(),
>              name='api-cover-comment-list',
>          ),
> -- 
> 2.33.0.rc2.250.ged5fa647cd-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
diff mbox series

Patch

diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
index 0c578b44..5d7a77a1 100644
--- a/patchwork/api/comment.py
+++ b/patchwork/api/comment.py
@@ -83,14 +83,14 @@  class CoverCommentList(ListAPIView):
     search_fields = ('subject',)
     ordering_fields = ('id', 'subject', 'date', 'submitter')
     ordering = 'id'
-    lookup_url_kwarg = 'pk'
+    lookup_url_kwarg = 'cover_id'
 
     def get_queryset(self):
-        if not Cover.objects.filter(pk=self.kwargs['pk']).exists():
+        if not Cover.objects.filter(id=self.kwargs['cover_id']).exists():
             raise Http404
 
         return CoverComment.objects.filter(
-            cover=self.kwargs['pk']
+            cover=self.kwargs['cover_id']
         ).select_related('submitter')
 
 
diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
index b4a3a8f7..c4355f6b 100644
--- a/patchwork/api/cover.py
+++ b/patchwork/api/cover.py
@@ -37,7 +37,7 @@  class CoverListSerializer(BaseHyperlinkedModelSerializer):
 
     def get_comments(self, cover):
         return self.context.get('request').build_absolute_uri(
-            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
+            reverse('api-cover-comment-list', kwargs={'cover_id': cover.id}))
 
     def to_representation(self, instance):
         # NOTE(stephenfin): This is here to ensure our API looks the same even
diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
index 59450d8a..53abf8f0 100644
--- a/patchwork/tests/api/test_comment.py
+++ b/patchwork/tests/api/test_comment.py
@@ -27,7 +27,7 @@  class TestCoverComments(utils.APITestCase):
         kwargs = {}
         if version:
             kwargs['version'] = version
-        kwargs['pk'] = cover.id
+        kwargs['cover_id'] = cover.id
 
         return reverse('api-cover-comment-list', kwargs=kwargs)
 
@@ -79,7 +79,7 @@  class TestCoverComments(utils.APITestCase):
     def test_list_invalid_cover(self):
         """Ensure we get a 404 for a non-existent cover letter."""
         resp = self.client.get(
-            reverse('api-cover-comment-list', kwargs={'pk': '99999'}))
+            reverse('api-cover-comment-list', kwargs={'cover_id': '99999'}))
         self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
 
 
diff --git a/patchwork/urls.py b/patchwork/urls.py
index 1e6c12ab..0180e76d 100644
--- a/patchwork/urls.py
+++ b/patchwork/urls.py
@@ -337,7 +337,7 @@  if settings.ENABLE_REST_API:
             name='api-patch-comment-list',
         ),
         path(
-            'covers/<pk>/comments/',
+            'covers/<cover_id>/comments/',
             api_comment_views.CoverCommentList.as_view(),
             name='api-cover-comment-list',
         ),