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