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

[Shubham Jain]

On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <pbonzini at 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()
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/patchew-devel/attachments/20180507/aae24c03/attachment.htm>