On Wed, 2021-08-11 at 09:39 +1000, Daniel Axtens wrote:
> Raxel Gutierrez <ra...@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 <ra...@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.
Yeah, they're all stored in a separate file and we maintain a list of stored
files to prevent duplicates. We don't actually use this information currently
but I plan to start including it in documentation as API samples. The only
reason I haven't done so is because the test data isn't very "complete" so these
examples are currently worse than what our Sphinx extension currently generates
for us.
>
> 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.
Nope, I'm good with nesting. If anything we could do with more of it (I'd like
to nest patches under projects in API v2.0).
Stephen
>
> 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@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> _______________________________________________
> 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
