[Patchew-devel] [PATCH] rest: POST for message endpoint

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/20180505070956.6665-1-shubhamjain7495@gmail.com
There is a newer version of this series
api/models.py | 39 +++++++++++++++++++++++++++++++--------
api/rest.py   | 20 +++++++++++---------
2 files changed, 42 insertions(+), 17 deletions(-)
[Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Shubham Jain 5 years, 11 months ago
This commit allows to create/POST message from browser
- Add "create" method to MessageManager so that it calls save_mbox()
- Add getter and setter for mbox
- Rename the existing "mbox" field to e.g. "mbox_blob"
- Fix nested writable serializer issue
---
 api/models.py | 39 +++++++++++++++++++++++++++++++--------
 api/rest.py   | 20 +++++++++++---------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/api/models.py b/api/models.py
index 504f2c7..43b1037 100644
--- a/api/models.py
+++ b/api/models.py
@@ -247,6 +247,24 @@ class MessageManager(models.Manager):
             self.delete_subthread(r)
         msg.delete()
 
+    def create(self,project, **validated_data):
+        mbox = validated_data.pop('mbox')
+        m = MboxMessage(mbox)
+        msg = Message(**validated_data)
+        msg.in_reply_to = m.get_in_reply_to() or ""
+        msg.stripped_subject = m.get_subject(strip_tags=True)
+        msg.version = m.get_version()
+        msg.prefixes = m.get_prefixes()
+        msg.is_series_head = m.is_series_head()
+        msg.is_patch = m.is_patch()
+        msg.patch_num = m.get_num()[0]
+        msg.project = project
+        msg.mbox = mbox
+        msg.save_mbox(mbox)
+        msg.save()
+        emit_event("MessageAdded", message=msg)
+        return msg
+
     def add_message_from_mbox(self, mbox, user, project_name=None):
 
         def find_message_projects(m):
@@ -321,20 +339,25 @@ class Message(models.Model):
 
     objects = MessageManager()
 
-    def save_mbox(self, mbox):
-        save_blob(mbox, self.message_id)
+    def save_mbox(self, mbox_blob):
+        save_blob(mbox_blob, self.message_id)
 
     def get_mbox_obj(self):
         self.get_mbox()
         return self._mbox_obj
 
     def get_mbox(self):
-        if hasattr(self, "mbox"):
-            return self.mbox
-        self.mbox = load_blob(self.message_id)
-        self._mbox_obj = MboxMessage(self.mbox)
-        return self.mbox
-
+        if hasattr(self, "mbox_blob"):
+            return self.mbox_blob
+        self.mbox_blob = load_blob(self.message_id)
+        self._mbox_obj = MboxMessage(self.mbox_blob)
+        return self.mbox_blob
+    
+    mbox = property(get_mbox)
+    @mbox.setter
+    def mbox(self,value):
+        self.mbox_blob = value
+    
     def get_num(self):
         assert self.is_patch or self.is_series_head
         cur, total = 1, 1
diff --git a/api/rest.py b/api/rest.py
index fc10b46..7c38d39 100644
--- a/api/rest.py
+++ b/api/rest.py
@@ -140,9 +140,13 @@ class BaseMessageSerializer(serializers.ModelSerializer):
         fields = ('resource_uri', 'message_id', 'subject', 'date', 'sender', 'recipients')
 
     resource_uri = HyperlinkedMessageField(view_name='messages-detail')
-
     recipients = AddressSerializer(many=True)
-    sender = AddressSerializer()
+    sender = AddressSerializer()    
+   
+    def create(self, validated_data):
+        validated_data['recipients'] = self.fields['recipients'].create(validated_data['recipients'])
+        validated_data['sender'] = self.fields['sender'].create(validated_data['sender'])
+        return Message.objects.create(project=self.context['project'], **validated_data)
 
 # a message_id is *not* unique, so we can only list
 class BaseMessageViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
@@ -156,7 +160,9 @@ class BaseMessageViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
 class ProjectMessagesViewSetMixin(mixins.RetrieveModelMixin):
     def get_queryset(self):
         return self.queryset.filter(project=self.kwargs['projects_pk'])
-
+    def get_serializer_context(self):
+        return {'project': Project.objects.get(id=self.kwargs['projects_pk']), 'request': self.request}
+    
 # Series
 
 class ReplySerializer(BaseMessageSerializer):
@@ -287,10 +293,7 @@ class MessageSerializer(BaseMessageSerializer):
     class Meta:
         model = Message
         fields = BaseMessageSerializer.Meta.fields + ('mbox', )
-
-    def get_mbox(self, obj):
-        return obj.get_mbox()
-    mbox = SerializerMethodField()
+    mbox = JSONField()
 
     def get_fields(self):
         fields = super(MessageSerializer, self).get_fields()
@@ -312,9 +315,8 @@ class StaticTextRenderer(renderers.BaseRenderer):
             return data
 
 class MessagesViewSet(ProjectMessagesViewSetMixin,
-                      BaseMessageViewSet):
+                      BaseMessageViewSet, mixins.CreateModelMixin):
     serializer_class = MessageSerializer
-
     @detail_route(renderer_classes=[StaticTextRenderer])
     def mbox(self, request, *args, **kwargs):
         message = self.get_object()
-- 
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] rest: POST for message endpoint
Posted by Paolo Bonzini 5 years, 11 months ago
On 05/05/2018 09:09, Shubham Jain wrote:
> This commit allows to create/POST message from browser
> - Add "create" method to MessageManager so that it calls save_mbox()
> - Add getter and setter for mbox
> - Rename the existing "mbox" field to e.g. "mbox_blob"
> - Fix nested writable serializer issue

This already looks pretty good, but:

- there are no test cases

- we should use it as an exercise in posting a series composed of
multiple changes

- I have a few small comments below on the code.

If you feel more confident sending everything as a single patch first,
and still without tests (that is with only the code comments addressed),
that's fine for me.

Now, on to the code.

> ---
>  api/models.py | 39 +++++++++++++++++++++++++++++++--------
>  api/rest.py   | 20 +++++++++++---------
>  2 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/api/models.py b/api/models.py
> index 504f2c7..43b1037 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -247,6 +247,24 @@ class MessageManager(models.Manager):
>              self.delete_subthread(r)
>          msg.delete()
>  
> +    def create(self,project, **validated_data):
> +        mbox = validated_data.pop('mbox')
> +        m = MboxMessage(mbox)
> +        msg = Message(**validated_data)
> +        msg.in_reply_to = m.get_in_reply_to() or ""

Setting in_reply_to here is the right thing to do for how the code
behaves right now.  However, the real problem is that it's missing in
MessageSerializer!  Can you add it, and here do:

    if 'in_reply_to' not in validated_data:
        msg.in_reply_to = m.get_in_reply_to() or ""

?

> +        msg.stripped_subject = m.get_subject(strip_tags=True)
> +        msg.version = m.get_version()
> +        msg.prefixes = m.get_prefixes()
> +        msg.is_series_head = m.is_series_head()
> +        msg.is_patch = m.is_patch()
> +        msg.patch_num = m.get_num()[0]
> +        msg.project = project
> +        msg.mbox = mbox
> +        msg.save_mbox(mbox)
> +        msg.save()
> +        emit_event("MessageAdded", message=msg)
> +        return msg
> +
>      def add_message_from_mbox(self, mbox, user, project_name=None):
>  
>          def find_message_projects(m):
> @@ -321,20 +339,25 @@ class Message(models.Model):
>  
>      objects = MessageManager()
>  
> -    def save_mbox(self, mbox):
> -        save_blob(mbox, self.message_id)
> +    def save_mbox(self, mbox_blob):
> +        save_blob(mbox_blob, self.message_id)
>  
>      def get_mbox_obj(self):
>          self.get_mbox()
>          return self._mbox_obj
>  
>      def get_mbox(self):
> -        if hasattr(self, "mbox"):
> -            return self.mbox
> -        self.mbox = load_blob(self.message_id)
> -        self._mbox_obj = MboxMessage(self.mbox)
> -        return self.mbox
> -
> +        if hasattr(self, "mbox_blob"):
> +            return self.mbox_blob
> +        self.mbox_blob = load_blob(self.message_id)
> +        self._mbox_obj = MboxMessage(self.mbox_blob)
> +        return self.mbox_blob
> +    
> +    mbox = property(get_mbox)
> +    @mbox.setter
> +    def mbox(self,value):
> +        self.mbox_blob = value
> +    
>      def get_num(self):
>          assert self.is_patch or self.is_series_head
>          cur, total = 1, 1
> diff --git a/api/rest.py b/api/rest.py
> index fc10b46..7c38d39 100644
> --- a/api/rest.py
> +++ b/api/rest.py
> @@ -140,9 +140,13 @@ class BaseMessageSerializer(serializers.ModelSerializer):
>          fields = ('resource_uri', 'message_id', 'subject', 'date', 'sender', 'recipients')
>  
>      resource_uri = HyperlinkedMessageField(view_name='messages-detail')
> -
>      recipients = AddressSerializer(many=True)
> -    sender = AddressSerializer()
> +    sender = AddressSerializer()    

You're adding whitespace at the end of the line here.

> +   
> +    def create(self, validated_data):
> +        validated_data['recipients'] = self.fields['recipients'].create(validated_data['recipients'])
> +        validated_data['sender'] = self.fields['sender'].create(validated_data['sender'])
> +        return Message.objects.create(project=self.context['project'], **validated_data)
>  
>  # a message_id is *not* unique, so we can only list
>  class BaseMessageViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
> @@ -156,7 +160,9 @@ class BaseMessageViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
>  class ProjectMessagesViewSetMixin(mixins.RetrieveModelMixin):
>      def get_queryset(self):
>          return self.queryset.filter(project=self.kwargs['projects_pk'])
> -
> +    def get_serializer_context(self):
> +        return {'project': Project.objects.get(id=self.kwargs['projects_pk']), 'request': self.request}

Good!  Out of curiosity what do you actually need 'request' for?

>  # Series
>  
>  class ReplySerializer(BaseMessageSerializer):
> @@ -287,10 +293,7 @@ class MessageSerializer(BaseMessageSerializer):
>      class Meta:
>          model = Message
>          fields = BaseMessageSerializer.Meta.fields + ('mbox', )
> -
> -    def get_mbox(self, obj):
> -        return obj.get_mbox()
> -    mbox = SerializerMethodField()
> +    mbox = JSONField()

Can this be a CharField() instead?
>  
>      def get_fields(self):
>          fields = super(MessageSerializer, self).get_fields()
> @@ -312,9 +315,8 @@ class StaticTextRenderer(renderers.BaseRenderer):
>              return data
>  
>  class MessagesViewSet(ProjectMessagesViewSetMixin,
> -                      BaseMessageViewSet):
> +                      BaseMessageViewSet, mixins.CreateModelMixin):
>      serializer_class = MessageSerializer
> -
>      @detail_route(renderer_classes=[StaticTextRenderer])
>      def mbox(self, request, *args, **kwargs):
>          message = self.get_object()
> 

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Shubham Jain 5 years, 11 months ago
On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/05/2018 09:09, Shubham Jain wrote:
> > This commit allows to create/POST message from browser
> > - Add "create" method to MessageManager so that it calls save_mbox()
> > - Add getter and setter for mbox
> > - Rename the existing "mbox" field to e.g. "mbox_blob"
> > - Fix nested writable serializer issue
>
> This already looks pretty good, but:
>
> - there are no test cases
>

I was thinking of doing this when we have POST for both json and text/plain
format.

>
> - we should use it as an exercise in posting a series composed of
> multiple changes
>

I still don't know how to send the multiple patches in a single patch.

> - I have a few small comments below on the code.
>
> If you feel more confident sending everything as a single patch first,
> and still without tests (that is with only the code comments addressed),
> that's fine for me.
>
> Now, on to the code.
>
> > ---
> >  api/models.py | 39 +++++++++++++++++++++++++++++++--------
> >  api/rest.py   | 20 +++++++++++---------
> >  2 files changed, 42 insertions(+), 17 deletions(-)
> >
> > diff --git a/api/models.py b/api/models.py
> > index 504f2c7..43b1037 100644
> > --- a/api/models.py
> > +++ b/api/models.py
> > @@ -247,6 +247,24 @@ class MessageManager(models.Manager):
> >              self.delete_subthread(r)
> >          msg.delete()
> >
> > +    def create(self,project, **validated_data):
> > +        mbox = validated_data.pop('mbox')
> > +        m = MboxMessage(mbox)
> > +        msg = Message(**validated_data)
> > +        msg.in_reply_to = m.get_in_reply_to() or ""
>
> Setting in_reply_to here is the right thing to do for how the code
> behaves right now.  However, the real problem is that it's missing in
> MessageSerializer!  Can you add it, and here do:
>
>     if 'in_reply_to' not in validated_data:
>         msg.in_reply_to = m.get_in_reply_to() or ""
>
> ?
>
Yeah, this makes more sense.

> > +        msg.stripped_subject = m.get_subject(strip_tags=True)
> > +        msg.version = m.get_version()
> > +        msg.prefixes = m.get_prefixes()
> > +        msg.is_series_head = m.is_series_head()
> > +        msg.is_patch = m.is_patch()
> > +        msg.patch_num = m.get_num()[0]
> > +        msg.project = project
> > +        msg.mbox = mbox
> > +        msg.save_mbox(mbox)
> > +        msg.save()
> > +        emit_event("MessageAdded", message=msg)
> > +        return msg
> > +
> >      def add_message_from_mbox(self, mbox, user, project_name=None):
> >
> >          def find_message_projects(m):
> > @@ -321,20 +339,25 @@ class Message(models.Model):
> >
> >      objects = MessageManager()
> >
> > -    def save_mbox(self, mbox):
> > -        save_blob(mbox, self.message_id)
> > +    def save_mbox(self, mbox_blob):
> > +        save_blob(mbox_blob, self.message_id)
> >
> >      def get_mbox_obj(self):
> >          self.get_mbox()
> >          return self._mbox_obj
> >
> >      def get_mbox(self):
> > -        if hasattr(self, "mbox"):
> > -            return self.mbox
> > -        self.mbox = load_blob(self.message_id)
> > -        self._mbox_obj = MboxMessage(self.mbox)
> > -        return self.mbox
> > -
> > +        if hasattr(self, "mbox_blob"):
> > +            return self.mbox_blob
> > +        self.mbox_blob = load_blob(self.message_id)
> > +        self._mbox_obj = MboxMessage(self.mbox_blob)
> > +        return self.mbox_blob
> > +
> > +    mbox = property(get_mbox)
> > +    @mbox.setter
> > +    def mbox(self,value):
> > +        self.mbox_blob = value
> > +
> >      def get_num(self):
> >          assert self.is_patch or self.is_series_head
> >          cur, total = 1, 1
> > diff --git a/api/rest.py b/api/rest.py
> > index fc10b46..7c38d39 100644
> > --- a/api/rest.py
> > +++ b/api/rest.py
> > @@ -140,9 +140,13 @@ class
> BaseMessageSerializer(serializers.ModelSerializer):
> >          fields = ('resource_uri', 'message_id', 'subject', 'date',
> 'sender', 'recipients')
> >
> >      resource_uri = HyperlinkedMessageField(view_name='messages-detail')
> > -
> >      recipients = AddressSerializer(many=True)
> > -    sender = AddressSerializer()
> > +    sender = AddressSerializer()
>
> You're adding whitespace at the end of the line here.
>
> > +
> > +    def create(self, validated_data):
> > +        validated_data['recipients'] =
> self.fields['recipients'].create(validated_data['recipients'])
> > +        validated_data['sender'] =
> self.fields['sender'].create(validated_data['sender'])
> > +        return Message.objects.create(project=self.context['project'],
> **validated_data)
> >
> >  # a message_id is *not* unique, so we can only list
> >  class BaseMessageViewSet(mixins.ListModelMixin,
> viewsets.GenericViewSet):
> > @@ -156,7 +160,9 @@ class BaseMessageViewSet(mixins.ListModelMixin,
> viewsets.GenericViewSet):
> >  class ProjectMessagesViewSetMixin(mixins.RetrieveModelMixin):
> >      def get_queryset(self):
> >          return self.queryset.filter(project=self.kwargs['projects_pk'])
> > -
> > +    def get_serializer_context(self):
> > +        return {'project':
> Project.objects.get(id=self.kwargs['projects_pk']), 'request': self.request}
>
> Good!  Out of curiosity what do you actually need 'request' for?
>
> Request is used by dispatch_module_hook is BaseMessageSerializer

> >  # Series
> >
> >  class ReplySerializer(BaseMessageSerializer):
> > @@ -287,10 +293,7 @@ class MessageSerializer(BaseMessageSerializer):
> >      class Meta:
> >          model = Message
> >          fields = BaseMessageSerializer.Meta.fields + ('mbox', )
> > -
> > -    def get_mbox(self, obj):
> > -        return obj.get_mbox()
> > -    mbox = SerializerMethodField()
> > +    mbox = JSONField()
>
> Can this be a CharField() instead?
>
Okay.

> >
> >      def get_fields(self):
> >          fields = super(MessageSerializer, self).get_fields()
> > @@ -312,9 +315,8 @@ class StaticTextRenderer(renderers.BaseRenderer):
> >              return data
> >
> >  class MessagesViewSet(ProjectMessagesViewSetMixin,
> > -                      BaseMessageViewSet):
> > +                      BaseMessageViewSet, mixins.CreateModelMixin):
> >      serializer_class = MessageSerializer
> > -
> >      @detail_route(renderer_classes=[StaticTextRenderer])
> >      def mbox(self, request, *args, **kwargs):
> >          message = self.get_object()
> >
>
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Paolo Bonzini 5 years, 11 months ago
On 07/05/2018 13:03, Shubham Jain wrote:
> 
> 
> On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
> 
>     On 05/05/2018 09:09, Shubham Jain wrote:
>     > This commit allows to create/POST message from browser
>     > - Add "create" method to MessageManager so that it calls save_mbox()
>     > - Add getter and setter for mbox
>     > - Rename the existing "mbox" field to e.g. "mbox_blob"
>     > - Fix nested writable serializer issue
> 
>     This already looks pretty good, but:
> 
>     - there are no test cases
> 
> I was thinking of doing this when we have POST for both json and
> text/plain format. 

No, please do it immediately.  Every patch should have testcases if
applicable.

>     - we should use it as an exercise in posting a series composed of
>     multiple changes
>  
> I still don't know how to send the multiple patches in a single patch.  

Good, that's what I wanted to know! :)

You can do

    git format-patch -opatches-post --cover-letter origin/master..

"-o" gives the output directory.  "--cover-letter" says that you want to
include an introductory message.

Now edit "patches-post/0000-cover-letter.patch" to include the
introductory message, and then

    git send-mbox *whatever options you need* patches-post/*

Do you know how to split a patch in multiple parts?  The idea is

    git add -p
    ... add changes for the first patch ...
    git stash --keep-index
    ... test ...
    git commit -a
    git stash pop

and repeat until the last part is missing.  Then just:

    ... test ...
    git commit -a

to commit the last part.

But again, it's okay (perhaps better for your comfort) if you first do
the requested changes and then we work on testcases and posting
splitting the patches.  This way, the mailing list post also acts as a
sort of backup. :)

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Shubham Jain 5 years, 11 months ago
Alright. One more question.
Setting in_reply_to here is the right thing to do for how the code
behaves right now.  However, the real problem is that it's missing in
MessageSerializer!  Can you add it, and here do:

    if 'in_reply_to' not in validated_data:
        msg.in_reply_to = m.get_in_reply_to() or ""

Is this same for every field? msg.stripped_subject ,msg.version,
msg.prefixes, msg.is_series_head ......

On Mon, May 7, 2018 at 4:42 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 07/05/2018 13:03, Shubham Jain wrote:
> >
> >
> > On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <pbonzini@redhat.com
> > <mailto:pbonzini@redhat.com>> wrote:
> >
> >     On 05/05/2018 09:09, Shubham Jain wrote:
> >     > This commit allows to create/POST message from browser
> >     > - Add "create" method to MessageManager so that it calls
> save_mbox()
> >     > - Add getter and setter for mbox
> >     > - Rename the existing "mbox" field to e.g. "mbox_blob"
> >     > - Fix nested writable serializer issue
> >
> >     This already looks pretty good, but:
> >
> >     - there are no test cases
> >
> > I was thinking of doing this when we have POST for both json and
> > text/plain format.
>
> No, please do it immediately.  Every patch should have testcases if
> applicable.
>
> >     - we should use it as an exercise in posting a series composed of
> >     multiple changes
> >
> > I still don't know how to send the multiple patches in a single patch.
>
> Good, that's what I wanted to know! :)
>
> You can do
>
>     git format-patch -opatches-post --cover-letter origin/master..
>
> "-o" gives the output directory.  "--cover-letter" says that you want to
> include an introductory message.
>
> Now edit "patches-post/0000-cover-letter.patch" to include the
> introductory message, and then
>
>     git send-mbox *whatever options you need* patches-post/*
>
> Do you know how to split a patch in multiple parts?  The idea is
>
>     git add -p
>     ... add changes for the first patch ...
>     git stash --keep-index
>     ... test ...
>     git commit -a
>     git stash pop
>
> and repeat until the last part is missing.  Then just:
>
>     ... test ...
>     git commit -a
>
> to commit the last part.
>
> But again, it's okay (perhaps better for your comfort) if you first do
> the requested changes and then we work on testcases and posting
> splitting the patches.  This way, the mailing list post also acts as a
> sort of backup. :)
>
> Paolo
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Paolo Bonzini 5 years, 11 months ago
On 07/05/2018 13:19, Shubham Jain wrote:
> Alright. One more question.
> Setting in_reply_to here is the right thing to do for how the code
> behaves right now.  However, the real problem is that it's missing in
> MessageSerializer!  Can you add it, and here do:
> 
>     if 'in_reply_to' not in validated_data:
>         msg.in_reply_to = m.get_in_reply_to() or ""
> 
> Is this same for every field? msg.stripped_subject ,msg.version,
> msg.prefixes, msg.is_series_head ......

I had actually thought about that, and I think no.  Perhaps I should
have been more verbose in my original reply, perhaps it's better that
you thought about it and asked the question.

The reason why I think no, is that unlike in-reply-to those are
basically cached versions of data that is already elsewhere in the
Message object.

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Patchew-devel] [PATCH] rest: POST for message endpoint
Posted by Fam Zheng 5 years, 11 months ago
On Mon, 05/07 11:03, Shubham Jain wrote:
> On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > On 05/05/2018 09:09, Shubham Jain wrote:
> > > This commit allows to create/POST message from browser
> > > - Add "create" method to MessageManager so that it calls save_mbox()
> > > - Add getter and setter for mbox
> > > - Rename the existing "mbox" field to e.g. "mbox_blob"
> > > - Fix nested writable serializer issue
> >
> > This already looks pretty good, but:
> >
> > - there are no test cases
> >
> 
> I was thinking of doing this when we have POST for both json and text/plain
> format.
> 
> >
> > - we should use it as an exercise in posting a series composed of
> > multiple changes
> >
> 
> I still don't know how to send the multiple patches in a single patch.

You can use git-publish

$ sudo dnf install git-publish

$ git checkout master -b foo-feature
$ # add sub-feature A and commit
$ # add sub-feature B and commit
$ # add sub-feature C and commit
$ # continue until the whole feature is done...
$ git publish

Here you'll be brought to an editor to compose the cover letter. The first line
is the subject, followed by a blank line, then the cover letter body.  Similar
to writing commit messages.

Once that is done, select "send" (a) from the interactive menu.

Fam

> 
> > - I have a few small comments below on the code.
> >
> > If you feel more confident sending everything as a single patch first,
> > and still without tests (that is with only the code comments addressed),
> > that's fine for me.
> >
> > Now, on to the code.
> >
> > > ---
> > >  api/models.py | 39 +++++++++++++++++++++++++++++++--------
> > >  api/rest.py   | 20 +++++++++++---------
> > >  2 files changed, 42 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/api/models.py b/api/models.py
> > > index 504f2c7..43b1037 100644
> > > --- a/api/models.py
> > > +++ b/api/models.py
> > > @@ -247,6 +247,24 @@ class MessageManager(models.Manager):
> > >              self.delete_subthread(r)
> > >          msg.delete()
> > >
> > > +    def create(self,project, **validated_data):
> > > +        mbox = validated_data.pop('mbox')
> > > +        m = MboxMessage(mbox)
> > > +        msg = Message(**validated_data)
> > > +        msg.in_reply_to = m.get_in_reply_to() or ""
> >
> > Setting in_reply_to here is the right thing to do for how the code
> > behaves right now.  However, the real problem is that it's missing in
> > MessageSerializer!  Can you add it, and here do:
> >
> >     if 'in_reply_to' not in validated_data:
> >         msg.in_reply_to = m.get_in_reply_to() or ""
> >
> > ?
> >
> Yeah, this makes more sense.
> 
> > > +        msg.stripped_subject = m.get_subject(strip_tags=True)
> > > +        msg.version = m.get_version()
> > > +        msg.prefixes = m.get_prefixes()
> > > +        msg.is_series_head = m.is_series_head()
> > > +        msg.is_patch = m.is_patch()
> > > +        msg.patch_num = m.get_num()[0]
> > > +        msg.project = project
> > > +        msg.mbox = mbox
> > > +        msg.save_mbox(mbox)
> > > +        msg.save()
> > > +        emit_event("MessageAdded", message=msg)
> > > +        return msg
> > > +
> > >      def add_message_from_mbox(self, mbox, user, project_name=None):
> > >
> > >          def find_message_projects(m):
> > > @@ -321,20 +339,25 @@ class Message(models.Model):
> > >
> > >      objects = MessageManager()
> > >
> > > -    def save_mbox(self, mbox):
> > > -        save_blob(mbox, self.message_id)
> > > +    def save_mbox(self, mbox_blob):
> > > +        save_blob(mbox_blob, self.message_id)
> > >
> > >      def get_mbox_obj(self):
> > >          self.get_mbox()
> > >          return self._mbox_obj
> > >
> > >      def get_mbox(self):
> > > -        if hasattr(self, "mbox"):
> > > -            return self.mbox
> > > -        self.mbox = load_blob(self.message_id)
> > > -        self._mbox_obj = MboxMessage(self.mbox)
> > > -        return self.mbox
> > > -
> > > +        if hasattr(self, "mbox_blob"):
> > > +            return self.mbox_blob
> > > +        self.mbox_blob = load_blob(self.message_id)
> > > +        self._mbox_obj = MboxMessage(self.mbox_blob)
> > > +        return self.mbox_blob
> > > +
> > > +    mbox = property(get_mbox)
> > > +    @mbox.setter
> > > +    def mbox(self,value):
> > > +        self.mbox_blob = value
> > > +
> > >      def get_num(self):
> > >          assert self.is_patch or self.is_series_head
> > >          cur, total = 1, 1
> > > diff --git a/api/rest.py b/api/rest.py
> > > index fc10b46..7c38d39 100644
> > > --- a/api/rest.py
> > > +++ b/api/rest.py
> > > @@ -140,9 +140,13 @@ class
> > BaseMessageSerializer(serializers.ModelSerializer):
> > >          fields = ('resource_uri', 'message_id', 'subject', 'date',
> > 'sender', 'recipients')
> > >
> > >      resource_uri = HyperlinkedMessageField(view_name='messages-detail')
> > > -
> > >      recipients = AddressSerializer(many=True)
> > > -    sender = AddressSerializer()
> > > +    sender = AddressSerializer()
> >
> > You're adding whitespace at the end of the line here.
> >
> > > +
> > > +    def create(self, validated_data):
> > > +        validated_data['recipients'] =
> > self.fields['recipients'].create(validated_data['recipients'])
> > > +        validated_data['sender'] =
> > self.fields['sender'].create(validated_data['sender'])
> > > +        return Message.objects.create(project=self.context['project'],
> > **validated_data)
> > >
> > >  # a message_id is *not* unique, so we can only list
> > >  class BaseMessageViewSet(mixins.ListModelMixin,
> > viewsets.GenericViewSet):
> > > @@ -156,7 +160,9 @@ class BaseMessageViewSet(mixins.ListModelMixin,
> > viewsets.GenericViewSet):
> > >  class ProjectMessagesViewSetMixin(mixins.RetrieveModelMixin):
> > >      def get_queryset(self):
> > >          return self.queryset.filter(project=self.kwargs['projects_pk'])
> > > -
> > > +    def get_serializer_context(self):
> > > +        return {'project':
> > Project.objects.get(id=self.kwargs['projects_pk']), 'request': self.request}
> >
> > Good!  Out of curiosity what do you actually need 'request' for?
> >
> > Request is used by dispatch_module_hook is BaseMessageSerializer
> 
> > >  # Series
> > >
> > >  class ReplySerializer(BaseMessageSerializer):
> > > @@ -287,10 +293,7 @@ class MessageSerializer(BaseMessageSerializer):
> > >      class Meta:
> > >          model = Message
> > >          fields = BaseMessageSerializer.Meta.fields + ('mbox', )
> > > -
> > > -    def get_mbox(self, obj):
> > > -        return obj.get_mbox()
> > > -    mbox = SerializerMethodField()
> > > +    mbox = JSONField()
> >
> > Can this be a CharField() instead?
> >
> Okay.
> 
> > >
> > >      def get_fields(self):
> > >          fields = super(MessageSerializer, self).get_fields()
> > > @@ -312,9 +315,8 @@ class StaticTextRenderer(renderers.BaseRenderer):
> > >              return data
> > >
> > >  class MessagesViewSet(ProjectMessagesViewSetMixin,
> > > -                      BaseMessageViewSet):
> > > +                      BaseMessageViewSet, mixins.CreateModelMixin):
> > >      serializer_class = MessageSerializer
> > > -
> > >      @detail_route(renderer_classes=[StaticTextRenderer])
> > >      def mbox(self, request, *args, **kwargs):
> > >          message = self.get_object()
> > >
> >
> >

> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel