[PATCH v3 01/10] api: change <pk> parameter to <patch_id> for comments endpoint

[Stephen Finucane]

On Fri, 2021-08-13 at 05:31 +0000, Raxel Gutierrez wrote:
> Refactor patch lookup parameter `pk` to `patch_id` for the comments
> list
> endpoint to disambiguate from the lookup parameter `comment_id` in an
> upcoming patch which introduces the comments detail endpoint. This
> doesn't affect the user-facing API.
> 
> Signed-off-by: Raxel Gutierrez <raxel at google.com>

This looks sensible to me. I'd like it if we could also update the
routes for cover letter comments, but that's a nice-to-have based on
the current design of this feature and could be done in a separate
change if you had time.

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

I'll hold off applying until Daniel has had a chance to look also.

Stephen

PS: Feel free to include the 'Reviewed-by' tag in future revisions if
this isn't applied before then and assuming the patch itself is
unchanged.

> ---
>  patchwork/api/comment.py            | 6 +++---
>  patchwork/api/patch.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 43b26c6..0c578b4 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -102,12 +102,12 @@ class PatchCommentList(ListAPIView):
>      search_fields = ('subject',)
>      ordering_fields = ('id', 'subject', 'date', 'submitter')
>      ordering = 'id'
> -    lookup_url_kwarg = 'pk'
> +    lookup_url_kwarg = 'patch_id'
>  
>      def get_queryset(self):
> -        if not Patch.objects.filter(pk=self.kwargs['pk']).exists():
> +        if not
> Patch.objects.filter(id=self.kwargs['patch_id']).exists():
>              raise Http404
>  
>          return PatchComment.objects.filter(
> -            patch=self.kwargs['pk']
> +            patch=self.kwargs['patch_id']
>          ).select_related('submitter')
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 9d22275..a97a882 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -97,7 +97,7 @@ class
> PatchListSerializer(BaseHyperlinkedModelSerializer):
>  
>      def get_comments(self, patch):
>          return self.context.get('request').build_absolute_uri(
> -            reverse('api-patch-comment-list', kwargs={'pk':
> patch.id}))
> +            reverse('api-patch-comment-list', kwargs={'patch_id':
> patch.id}))
>  
>      def get_check(self, instance):
>          return instance.combined_check_state
> diff --git a/patchwork/tests/api/test_comment.py
> b/patchwork/tests/api/test_comment.py
> index 5bbebf2..59450d8 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -90,7 +90,7 @@ class TestPatchComments(utils.APITestCase):
>          kwargs = {}
>          if version:
>              kwargs['version'] = version
> -        kwargs['pk'] = patch.id
> +        kwargs['patch_id'] = patch.id
>  
>          return reverse('api-patch-comment-list', kwargs=kwargs)
>  
> @@ -142,5 +142,5 @@ class TestPatchComments(utils.APITestCase):
>      def test_list_invalid_patch(self):
>          """Ensure we get a 404 for a non-existent patch."""
>          resp = self.client.get(
> -            reverse('api-patch-comment-list', kwargs={'pk':
> '99999'}))
> +            reverse('api-patch-comment-list', kwargs={'patch_id':
> '99999'}))
>          self.assertEqual(status.HTTP_404_NOT_FOUND,
> resp.status_code)
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 6ac9b81..1e6c12a 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -332,7 +332,7 @@ if settings.ENABLE_REST_API:
>  
>      api_1_1_patterns = [
>          path(
> -            'patches/<pk>/comments/',
> +            'patches/<patch_id>/comments/',
>              api_comment_views.PatchCommentList.as_view(),
>              name='api-patch-comment-list',
>          ),