| Message ID | 20210820045030.3364156-3-raxel@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | patch-detail: add unaddressed/addressed status to patch comments | expand |
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', > ),
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 --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', ),
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(-)