[Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test

Shubham Jain posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/patchew-ci tags/patchew/20180404131358.15398-1-shubhamjain7495@gmail.com
api/rest.py        |  7 ++++++-
tests/test_rest.py | 25 +++++++++++++++++++++++++
2 files changed, 31 insertions(+), 1 deletion(-)
[Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Shubham Jain 5 years, 11 months ago
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
Re: [Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Paolo Bonzini 5 years, 11 months ago
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
Re: [Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Shubham Jain 5 years, 11 months ago
> 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
Re: [Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Paolo Bonzini 5 years, 11 months ago
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
Re: [Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Shubham Jain 5 years, 11 months ago
> 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
Re: [Patchew-devel] [PATCH v3] rest: add support for series DELETE in the REST API, and a corresponding unit test
Posted by Paolo Bonzini 5 years, 11 months ago
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