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

[Paolo Bonzini]

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()
>