On Fri, 2021-08-13 at 09:56 +0100, Stephen Finucane wrote:
> 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 <ra...@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 <step...@that.guru>
>
> I'll hold off applying until Daniel has had a chance to look also.
Tell a lie. I've gone and applied this since the next few patches look sane and
it seems Daniel was pretty happy with this in v2 once it was split out.
@Daniel: If you've any outstanding concerns, feel free to revert or ask for a
follow-up (I'd still like to see the cover comments routes updated too)
Stephen
> 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',
> > ),
>
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
