<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Mon, May 7, 2018 at 4:25 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 </blockquote><div> </div><div>I was thinking of doing this when we have POST for both json and text/plain format. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - we should use it as an exercise in posting a series composed of multiple changes </blockquote><div> </div><div>I still don't know how to send the multiple patches in a single patch. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - 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 "" ? </blockquote><div>Yeah, this makes more sense. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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? </blockquote><div>Request is used by dispatch_module_hook is BaseMessageSerializer </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > # 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? </blockquote><div>Okay. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > 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() > </blockquote></div></div>