rest: Imporoved series DELETE in the REST API
- Fixed the char limit in line to make code more readable
- overrode perform_destroy function so that that endpoint accepts any message id and not just the series head
- wrote tests for the same
Message-Id: <20180331122557.44970-1-shubhamjain7495@gmail.com>
rest: Added permission class
- Added permission to deletion and restricted it to avoid public deletion.
- Added a corresponding unit test to it.
- Also overrode perform_destroy instead of destory to perform series deletion.
---
api/rest.py | 7 ++++++-
tests/test_rest.py | 25 +++++++++++++++++++++++++
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/api/rest.py b/api/rest.py
index 7c131a4..6aa744d 100644
--- a/api/rest.py
+++ b/api/rest.py
@@ -229,9 +229,11 @@ class SeriesViewSet(BaseMessageViewSet):
queryset = Message.objects.filter(is_series_head=True).order_by('-last_reply_date')
filter_backends = (PatchewSearchFilter,)
search_fields = (SEARCH_PARAM,)
+ permission_classes = (IsAdminUserOrReadOnly,)
+
class ProjectSeriesViewSet(ProjectMessagesViewSetMixin,
- SeriesViewSet):
+ SeriesViewSet, mixins.DestroyModelMixin):
def collect_patches(self, series):
if series.is_patch:
patches = [series]
@@ -265,6 +267,9 @@ class ProjectSeriesViewSet(ProjectMessagesViewSetMixin,
self.collect_replies(i, series.replies)
return series
+ def perform_destroy(self, instance):
+ Message.objects.delete_subthread(instance)
+
# Messages
# TODO: add POST endpoint connected to email plugin?
diff --git a/tests/test_rest.py b/tests/test_rest.py
index 28ca10b..5b22067 100755
--- a/tests/test_rest.py
+++ b/tests/test_rest.py
@@ -218,6 +218,31 @@ class RestTest(PatchewTestCase):
resp = self.api_client.get(self.REST_BASE + 'projects/12345/series/?q=project:QEMU')
self.assertEqual(resp.data['count'], 0)
+ def test_series_delete(self):
+ test_message_id = '1469192015-16487-1-git-send-email-berrange@redhat.com'
+ series = self.apply_and_retrieve('0004-multiple-patch-reviewed.mbox.gz',self.p.id,
+ test_message_id)
+ message = series.data['message']
+ resp_before = self.api_client.get(self.REST_BASE + 'projects/' + str(self.p.id)
+ + '/series/' + test_message_id + '/')
+ resp_reply_before = self.api_client.get(message + 'replies/')
+ resp_without_login = self.api_client.delete(self.REST_BASE + 'projects/' + str(self.p.id)
+ + '/series/' + test_message_id + '/')
+ self.api_client.login(username=self.user, password=self.password)
+ resp = self.api_client.delete(self.REST_BASE + 'projects/' + str(self.p.id)
+ + '/series/' + test_message_id + '/')
+ self.api_client.logout()
+ resp_after = self.api_client.get(self.REST_BASE + 'projects/' + str(self.p.id)
+ + '/series/' + test_message_id + '/')
+ resp_reply_after = self.api_client.get(message + 'replies/')
+
+ self.assertEqual(resp_before.status_code, 200)
+ self.assertEqual(resp_reply_before.status_code, 200)
+ self.assertEqual(resp_without_login.status_code, 403)
+ self.assertEqual(resp.status_code, 204)
+ self.assertEqual(resp_after.status_code, 404)
+ self.assertEqual(resp_reply_after.status_code, 404)
+
def test_message(self):
series = self.apply_and_retrieve('0001-simple-patch.mbox.gz',
self.p.id, '20160628014747.20971-1-famz@redhat.com')
--
2.14.3 (Apple Git-98)
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On 04/04/2018 15:13, Shubham Jain wrote:
> rest: Imporoved series DELETE in the REST API
>
> - Fixed the char limit in line to make code more readable
> - overrode perform_destroy function so that that endpoint accepts any message id and not just the series head
> - wrote tests for the same
> Message-Id: <20180331122557.44970-1-shubhamjain7495@gmail.com>
>
> rest: Added permission class
>
> - Added permission to deletion and restricted it to avoid public deletion.
> - Added a corresponding unit test to it.
> - Also overrode perform_destroy instead of destory to perform series deletion.
> ---
> api/rest.py | 7 ++++++-
> tests/test_rest.py | 25 +++++++++++++++++++++++++
> 2 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/api/rest.py b/api/rest.py
> index 7c131a4..6aa744d 100644
> --- a/api/rest.py
> +++ b/api/rest.py
> @@ -229,9 +229,11 @@ class SeriesViewSet(BaseMessageViewSet):
> queryset = Message.objects.filter(is_series_head=True).order_by('-last_reply_date')
> filter_backends = (PatchewSearchFilter,)
> search_fields = (SEARCH_PARAM,)
> + permission_classes = (IsAdminUserOrReadOnly,)
Thanks!
I changed this to IsMaintainerOrReadOnly and pushed the resulting patch.
As a next step, perhaps you can implement POST for /api/v1/messages?
That should accept a text/plain object; can you find the corresponding
legacy API endpoint?
Thanks,
Paolo
> +
>
> class ProjectSeriesViewSet(ProjectMessagesViewSetMixin,
> - SeriesViewSet):
> + SeriesViewSet, mixins.DestroyModelMixin):
> def collect_patches(self, series):
> if series.is_patch:
> patches = [series]
> @@ -265,6 +267,9 @@ class ProjectSeriesViewSet(ProjectMessagesViewSetMixin,
> self.collect_replies(i, series.replies)
> return series
>
> + def perform_destroy(self, instance):
> + Message.objects.delete_subthread(instance)
> +
> # Messages
>
> # TODO: add POST endpoint connected to email plugin?
> diff --git a/tests/test_rest.py b/tests/test_rest.py
> index 28ca10b..5b22067 100755
> --- a/tests/test_rest.py
> +++ b/tests/test_rest.py
> @@ -218,6 +218,31 @@ class RestTest(PatchewTestCase):
> resp = self.api_client.get(self.REST_BASE + 'projects/12345/series/?q=project:QEMU')
> self.assertEqual(resp.data['count'], 0)
>
> + def test_series_delete(self):
> + test_message_id = '1469192015-16487-1-git-send-email-berrange@redhat.com'
> + series = self.apply_and_retrieve('0004-multiple-patch-reviewed.mbox.gz',self.p.id,
> + test_message_id)
> + message = series.data['message']
> + resp_before = self.api_client.get(self.REST_BASE + 'projects/' + str(self.p.id)
> + + '/series/' + test_message_id + '/')
> + resp_reply_before = self.api_client.get(message + 'replies/')
> + resp_without_login = self.api_client.delete(self.REST_BASE + 'projects/' + str(self.p.id)
> + + '/series/' + test_message_id + '/')
> + self.api_client.login(username=self.user, password=self.password)
> + resp = self.api_client.delete(self.REST_BASE + 'projects/' + str(self.p.id)
> + + '/series/' + test_message_id + '/')
> + self.api_client.logout()
> + resp_after = self.api_client.get(self.REST_BASE + 'projects/' + str(self.p.id)
> + + '/series/' + test_message_id + '/')
> + resp_reply_after = self.api_client.get(message + 'replies/')
> +
> + self.assertEqual(resp_before.status_code, 200)
> + self.assertEqual(resp_reply_before.status_code, 200)
> + self.assertEqual(resp_without_login.status_code, 403)
> + self.assertEqual(resp.status_code, 204)
> + self.assertEqual(resp_after.status_code, 404)
> + self.assertEqual(resp_reply_after.status_code, 404)
> +
> def test_message(self):
> series = self.apply_and_retrieve('0001-simple-patch.mbox.gz',
> self.p.id, '20160628014747.20971-1-famz@redhat.com')
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
> I changed this to IsMaintainerOrReadOnly and pushed the resulting patch. I tried this as well. But it gave me 'Message' object has no attribute 'maintained_by'. > > As a next step, perhaps you can implement POST for /api/v1/messages? > That should accept a text/plain object; can you find the corresponding > legacy API endpoint? > Sure. You mean api/v1/projects/../messages? The legacy endpoint is import right? The import view is adding messages to the project. Thanks, Shubham _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On 04/04/2018 19:15, Shubham Jain wrote: > > I changed this to IsMaintainerOrReadOnly and pushed the resulting patch. > > I tried this as well. But it gave me 'Message' object has no attribute > 'maintained_by'. You are right, I wrote this just before pushing but... I lied. Another patch is needed, "rest: allow IsMaintainerOrReadOnly that applies to Message". Use the commit message to write information about your decisions regarding the patch. This includes decisions that you think are wrong, but you had to make anyway because you were stuck! :) > As a next step, perhaps you can implement POST for /api/v1/messages? > That should accept a text/plain object; can you find the corresponding > legacy API endpoint? > > Sure. You mean api/v1/projects/../messages? /api/v1/messages is the important one. This is because the "import" view looks at an email, and uses the recipients to decide which projects to use. The code that does the import is add_message_from_mbox, in api/models.py. We could add /api/v1/projects/.../messages (corresponding to a non-None value of project_name). It would be a good idea, but less important because it has no equivalent in the old API. > The legacy endpoint is > import right? The import view is adding messages to the project. Right! Thanks, Paolo _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
> Use the commit message to write information about your decisions > regarding the patch. This includes decisions that you think are wrong, > but you had to make anyway because you were stuck! Noted. Will keep in mind in the future. > > > As a next step, perhaps you can implement POST for /api/v1/messages? > > That should accept a text/plain object; can you find the > corresponding > > legacy API endpoint? > It would be a JSON object right? The elements we are extracting from the mbox in add_message_from_mbox() would be provided as JSON? For example, instead of msgid = m.get_message_id() it should be something like this msgid = request.data['message_id']? _______________________________________________ Patchew-devel mailing list Patchew-devel@redhat.com https://www.redhat.com/mailman/listinfo/patchew-devel
On 11/04/2018 11:50, Shubham Jain wrote:
>
> Use the commit message to write information about your decisions
> regarding the patch. This includes decisions that you think are wrong,
> but you had to make anyway because you were stuck!
>
> Noted. Will keep in mind in the future.
>
> > As a next step, perhaps you can implement POST for /api/v1/messages?
> > That should accept a text/plain object; can you find the corresponding
> > legacy API endpoint?
>
> It would be a JSON object right? The elements we are extracting from the
> mbox in add_message_from_mbox() would be provided as JSON?
> For example, instead of msgid = m.get_message_id() it should be
> something like this msgid = request.data['message_id']?
It can be both.
We could accept both application/json and text/plain in the REST API.
In fact, all four combinations make sense:
- /api/v1/messages + application/json: import message, looking at
recipients to find all applicable projects
- /api/v1/projects/{id}/messages + application/json: import message to
specific project
- /api/v1/messages + text/plain: import message from mbox, looking at
recipients to find all applicable projects
- /api/v1/projects/{id}/messages + text/plain: import message from mbox
to specific project
The third is simply what matches the legacy import endpoint. text/plain
also the simplest to implement, because the Python code to create
Messages is more suited to text/plain import.
However, you can attack the problem both ways:
- first write the endpoint to only accept application/json; add code to
MboxMessage to return a JSON representation just like what the REST API
would use; use the new MboxMessage to handle text/plain in the endpoint.
- first write the endpoint to only accept text/plain; then add code to
MboxMessage to return a JSON representation just like what the REST API
would use; finally refactor the endpoint to support application/json too.
I was suggesting the second mostly because the DRF side of it is a bit
simpler. In particular some fields are read-only and should not be
processed by write requests (POST/PUT/PATCH). If you want to write the
API to first accept application/json, that's certainly okay! Keep us
updated on your progress.
(A small hint that just came to mind: the MboxMessage->JSON conversion
logically comes second, but it can come in handy to write tests: if you
have that converter, your tests for the application/json REST API can
reuse the existing .gz files. So perhaps it's better to write that
converter first, together with the associated unit tests).
Thanks,
Paolo
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
© 2016 - 2025 Red Hat, Inc.