[PATCH v2 2/5] tests: add tests for patch comments detail endpoint
Daniel Axtens
dja at axtens.net
Wed Aug 11 09:39:21 AEST 2021
Raxel Gutierrez <raxel at google.com> writes:
> Add tests for the new api/.../comments/<comment_id> endpoint that takes
> GET, PATCH, and PUT requests. The tests cover retrieval and update
> requests and handle calls from the various API versions. Also, they
> handle permissions for update requests on the new `addressed` field and
> invalid update values for the `addressed` field.
>
> Add `addressed` field to create_patch_comment helper in api tests
> utils.py.
>
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
> patchwork/tests/api/test_comment.py | 199 +++++++++++++++++++++++++---
> patchwork/tests/utils.py | 1 +
> 2 files changed, 183 insertions(+), 17 deletions(-)
>
> diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
> index 59450d8..f43d1c7 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -9,11 +9,16 @@ from django.conf import settings
> from django.urls import NoReverseMatch
> from django.urls import reverse
>
> +from patchwork.models import PatchComment
> from patchwork.tests.api import utils
> from patchwork.tests.utils import create_cover
> from patchwork.tests.utils import create_cover_comment
> from patchwork.tests.utils import create_patch
> from patchwork.tests.utils import create_patch_comment
> +from patchwork.tests.utils import create_maintainer
> +from patchwork.tests.utils import create_project
> +from patchwork.tests.utils import create_person
> +from patchwork.tests.utils import create_user
> from patchwork.tests.utils import SAMPLE_CONTENT
>
> if settings.ENABLE_REST_API:
> @@ -86,34 +91,40 @@ class TestCoverComments(utils.APITestCase):
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> class TestPatchComments(utils.APITestCase):
> @staticmethod
> - def api_url(patch, version=None):
> - kwargs = {}
> + def api_url(patch, version=None, item=None):
> + kwargs = {'patch_id': patch.id}
> if version:
> kwargs['version'] = version
> - kwargs['patch_id'] = patch.id
> + if item is None:
> + return reverse('api-patch-comment-list', kwargs=kwargs)
> + kwargs['comment_id'] = item.id
> + return reverse('api-patch-comment-detail', kwargs=kwargs)
>
> - return reverse('api-patch-comment-list', kwargs=kwargs)
> + def setUp(self):
> + super(TestPatchComments, self).setUp()
> + self.project = create_project()
> + self.user = create_maintainer(self.project)
> + self.patch = create_patch(project=self.project)
>
> def assertSerialized(self, comment_obj, comment_json):
> self.assertEqual(comment_obj.id, comment_json['id'])
> self.assertEqual(comment_obj.submitter.id,
> comment_json['submitter']['id'])
> + self.assertEqual(comment_obj.addressed, comment_json['addressed'])
> self.assertIn(SAMPLE_CONTENT, comment_json['content'])
>
> def test_list_empty(self):
> """List patch comments when none are present."""
> - patch = create_patch()
> - resp = self.client.get(self.api_url(patch))
> + resp = self.client.get(self.api_url(self.patch))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(0, len(resp.data))
>
> @utils.store_samples('patch-comment-list')
> def test_list(self):
> """List patch comments."""
> - patch = create_patch()
> - comment = create_patch_comment(patch=patch)
> + comment = create_patch_comment(patch=self.patch)
>
> - resp = self.client.get(self.api_url(patch))
> + resp = self.client.get(self.api_url(self.patch))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> self.assertSerialized(comment, resp.data[0])
> @@ -121,26 +132,180 @@ class TestPatchComments(utils.APITestCase):
>
> def test_list_version_1_1(self):
> """List patch comments using API v1.1."""
> - patch = create_patch()
> - comment = create_patch_comment(patch=patch)
> + comment = create_patch_comment(patch=self.patch)
>
> - resp = self.client.get(self.api_url(patch, version='1.1'))
> + resp = self.client.get(self.api_url(self.patch, version='1.1'))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> self.assertSerialized(comment, resp.data[0])
> self.assertNotIn('list_archive_url', resp.data[0])
>
> def test_list_version_1_0(self):
> - """List patch comments using API v1.0."""
> - patch = create_patch()
> - create_patch_comment(patch=patch)
> + """List patch comments using API v1.0.
>
> - # check we can't access comments using the old version of the API
> + Ensure we can't access comments using the old version of the API.
> + """
> with self.assertRaises(NoReverseMatch):
> - self.client.get(self.api_url(patch, version='1.0'))
> + self.client.get(self.api_url(self.patch, version='1.0'))
>
> 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={'patch_id': '99999'}))
> self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + @utils.store_samples('patch-comment-detail')
> + def test_detail(self):
> + """Show a patch comment."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + resp = self.client.get(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_3(self):
> + """Show a patch comment using API v1.3."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + resp = self.client.get(
> + self.api_url(self.patch, version='1.3', item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_2(self):
> + """Show a patch comment using API v1.2."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.2', item=comment))
> +
> + def test_detail_version_1_1(self):
> + """Show a patch comment using API v1.1."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.1', item=comment))
> +
> + def test_detail_version_1_0(self):
> + """Show a patch comment using API v1.0."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.0', item=comment))
> +
> + @utils.store_samples('patch-comment-detail-error-not-found')
> + def test_detail_invalid_patch(self):
> + """Ensure we handle non-existent patches."""
> + comment = create_patch_comment()
> + resp = self.client.get(
> + reverse('api-patch-comment-detail', kwargs={
> + 'patch_id': '99999',
> + 'comment_id': comment.id}
> + ),
> + )
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def _test_update(self, person, **kwargs):
> + submitter = kwargs.get('submitter', person)
> + patch = kwargs.get('patch', self.patch)
> + comment = create_patch_comment(submitter=submitter, patch=patch)
> +
> + if kwargs.get('authenticate', True):
> + self.client.force_authenticate(user=person.user)
> + return self.client.patch(
> + self.api_url(patch, item=comment),
> + {'addressed': kwargs.get('addressed', True)},
> + validate_request=kwargs.get('validate_request', True)
> + )
> +
> + @utils.store_samples('patch-comment-detail-update-authorized')
> + def test_update_authorized(self):
> + """Update an existing patch comment as an authorized user.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, patch submitter, patch delegate, or
> + patch comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + # Update as maintainer
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person)
> + self.assertEqual(1, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch submitter
> + person = create_person(name='patch-submitter', user=create_user())
> + patch = create_patch(submitter=person)
> + resp = self._test_update(person=person, patch=patch)
> + self.assertEqual(2, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch delegate
> + person = create_person(name='patch-delegate', user=create_user())
> + patch = create_patch(delegate=person.user)
> + resp = self._test_update(person=person, patch=patch)
> + self.assertEqual(3, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch comment submitter
> + person = create_person(name='comment-submitter', user=create_user())
> + patch = create_patch()
> + resp = self._test_update(person=person, patch=patch, submitter=person)
> + self.assertEqual(4, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + @utils.store_samples('patch-comment-detail-update-not-authorized')
Stephen would be better able to speak to which methods should get
store_samples and which shouldn't. I was initially worried that because
you're doing multiple requests in the test case you'd get a really
complex sample file but it seems that only one request is stored.
Anyway you don't need to do anything here, just flagging that sfin might
know more about this area.
> + def test_update_not_authorized(self):
> + """Update an existing patch comment when not signed in and not authorized.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, patch submitter, patch delegate, or
> + patch comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person, authenticate=False)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + person = create_person() # normal user without edit permissions
> + resp = self._test_update(person=person) # signed-in
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + @utils.store_samples('patch-comment-detail-update-error-bad-request')
> + def test_update_invalid_addressed(self):
> + """Update an existing patch comment using invalid values.
> +
> + Ensure we handle invalid patch comment addressed values.
> + """
> + person = create_person(name='patch-submitter', user=create_user())
> + patch = create_patch(submitter=person)
> + resp = self._test_update(person=person,
> + patch=patch,
> + addressed='not-valid',
> + validate_request=False)
> + self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
> + self.assertFalse(
> + getattr(PatchComment.objects.all().first(), 'addressed')
> + )
> +
> + def test_create_delete(self):
> + """Ensure creates and deletes aren't allowed"""
> + comment = create_patch_comment(patch=self.patch)
> + self.user.is_superuser = True
> + self.user.save()
> + self.client.force_authenticate(user=self.user)
> +
> + resp = self.client.post(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> +
> + resp = self.client.delete(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> index cc09e84..2d5bd4d 100644
> --- a/patchwork/tests/utils.py
> +++ b/patchwork/tests/utils.py
> @@ -260,6 +260,7 @@ def create_patch_comment(**kwargs):
> 'patch': create_patch() if 'patch' not in kwargs else None,
> 'msgid': make_msgid(),
> 'content': SAMPLE_CONTENT,
> + 'addressed': False,
Addressed is false by default so you don't need to set it explictly.
Apart from that I think this looks pretty good. I will be interested to
see if Stephen wants to flatten the comments endpoint (that is, go from
/patch/NN/comment/MM/ to /comment/XX). I think I prefer it as-is, fwiw.
I test db query load on the next revision.
Kind regards,
Daniel
> }
> values.update(kwargs)
>
> --
> 2.32.0.554.ge1b32706d8-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list