[MERGE/PQM] Add XMLRPC support

[Daniel Watkins]

On Wed, 6 Aug 2008 10:39:27 +1200
"Jonathan Lange" <jml at mumak.net> wrote:
> I've decided to leap in and review your XMLRPC patch. I'm in a bit of
> a rush so I've occasionally been terse -- please forgive me.
Thanks again for the review, it's much appreciated (and you're
forgiven :p).

> > === modified file 'bin/pqm'
> > +            (sender, subject, msg, sig) = \
> > +                read_email_from_file(logger, open(script.filename))
> 
> I'd personally prefer breaking the line at the first paren after
> read_email_from_file.
Yup, not sure what I was thinking.  Fixed in attached.

> >  @@ -203,19 +204,10 @@
> >  def do_read_mode(logger, options):
> >      sender = None
> >      try:
> > -        (sender, subject, msg, sig) = read_email(logger)
> > -        if options.verify_sigs:
> > -            sigid, siguid = verify_sig(
> > -                sender, msg, sig, 1, logger, options.keyring)
> > -            open(transaction_file, 'a').write(sigid + '\n')
> > -        fname = 'patch.%d' % (time.time())
> > -        logger.info('new patch ' + fname)
> > -        f = open('tmp.' + fname, 'w')
> > -        f.write('From: ' + sender + '\n')
> > -        f.write('Subject: ' + subject + '\n')
> > -        f.write(string.join(re.split('\r?\n', msg), '\n')) # canonicalize line endings
> > -        f.close()
> > -        os.rename('tmp.' + fname, fname)
> > +        email = read_email_from_file(logger, sys.stdin)
> > +        email_string = get_email_string(logger, options, email)
> > +        queuedir = os.path.abspath('.')
> 
> This smells a bit to me.
> 
> Unfortunately, I don't really understand PQM well enough, so it may
> be the right thing to do.
If you mean the use of "abspath('.')", then yes, it does smell a
little.  It's correct, because the queue directory is changed into at
~line 326.  I'd prefer to leave it as-is for the time being, until
a more thorough refactoring of that part of the code can take place.

If you mean something else, I can't smell it so could you clarify which
part you did mean?

> > === modified file 'pqm/script.py'
> > --- pqm/script.py	2008-07-17 02:03:01 +0000
> > +++ pqm/script.py	2008-07-31 06:17:16 +0000
> > @@ -72,6 +69,36 @@
> >          return (sender, subject, msg.get_payload(), None)
> >
> >
> > +def read_email_from_file(logger, file=None):
> 
> Docstring please.
Done.

> > +    if not file:
> 
> I'd rather 'if file is None'.
Done.

> > +        file = sys.stdin
> > +    msg = email.message_from_file(file)
> > +    return get_email_structure(logger, msg)
> > +
> > +
> > +def read_email_from_string(logger, string):
> 
> Docstring please.
Done.

> > +    msg = email.message_from_string(string)
> > +    return get_email_structure(logger, msg)
> > +
> > +
> > +def get_email_string(logger, options, email):
> 
> Docstring please!
Done.

> > +    (sender, subject, msg, sig) = email
> > +    if options.verify_sigs:
> > +        # TODO: Daniel Watkins, 2008-07-31
> > +        # Get rid of the imports from pqm module itself.
> > +        from pqm import verify_sig
> > +        sigid, siguid = verify_sig(sender, msg, sig, 1, logger,
> > +                                   options.keyring)
> > +        open(transaction_file, 'a').write(sigid + '\n')
> > +    # canonicalize line endings
> > +    body = '\n'.join(re.split('\r?\n', msg))
> > +    return ("From: %(sender)s\n"
> > +            "Subject: %(subject)s\n"
> > +            "%(body)s" % {'sender': sender,
> > +                          'subject': subject,
> > +                          'body': body,})
> > +
> > +
> 
> I'm not super clear what this last function is for, but in general I
> think this is an improvement to the structure of the mail handling
> code.
This gets the standard representation of an email used by PQM.

> > === added file 'pqm/ui/tests/test_xmlrpc.py'
> > --- pqm/ui/tests/test_xmlrpc.py	1970-01-01 00:00:00 +0000
> > +++ pqm/ui/tests/test_xmlrpc.py	2008-08-01 01:01:52 +0000

> > +class TestPQM_XMLRPC(TestCaseWithQueue):
> > +
> > +    def test_empty_submission(self):
> > +        xmlrpc = self.get_xmlrpc()
> > +        out = xmlrpc.xmlrpc_submit('')
> > +        self.assertEqual("Error: 'No From specified'", out)
> > +
> > +    def test_valid_submission(self):
> 
> It would be good if you could add a comment explaining the behaviour
> you want. This is especially important since the casual reader
> (that's me) doesn't know what sample_message is.
> 
> Something like:
>     # Submitting a message like ______ makes PQM do _____
Done.

> > +        xmlrpc = self.get_xmlrpc()
> > +        out = xmlrpc.xmlrpc_submit(sample_message)
> > +        self.assertEqual('Success!', out)
> > +        queuedir = xmlrpc.pqminfo.queuedir
> > +        patches = [f for f in os.listdir(queuedir) if
> > f.startswith('patch.')]
> > +        self.assertEqual(1, len(patches)) # We should have only
> > one patch
> > +        self.assertFileEqual(sample_message,
> > +                             os.path.join(queuedir, patches[0]))
> >
> 
> I think it would also be good to add a test for a malformed
> submission.
Done.

> > === modified file 'pqm/ui/twistd.py'
> > --- pqm/ui/twistd.py	2008-04-16 10:23:52 +0000
> > +++ pqm/ui/twistd.py	2008-08-01 00:33:29 +0000
> > @@ -156,14 +157,16 @@
> >      def __init__(self, filenames):
> >          self.filenames = filenames
> >          self.messages = []
> > +        self.configp = ConfigParser()
> > +        self.configp.read(self.filenames)
> > +        self.queuedir = pqm.get_queuedir(self.configp, logging, [])
> >
> 
> Two things here.
> 
> configp should probably be config_parser. Characters are cheap.
configp is used throughout the PQM code to refer to a ConfigParser, so
I'm loath to change that.

> The other thing is that if you have a lot of big files that take a
> while to parse, this operation will block, preventing the process
> from handling any other incoming requests. I'm not sure if it's an
> issue now, but it's something to be aware of.
This is just modifying the existing code, so it's presumably not an
issue currently.  It will be borne in mind, however.

> > === added file 'pqm/ui/xmlrpc.py'
> > --- pqm/ui/xmlrpc.py	1970-01-01 00:00:00 +0000
> > +++ pqm/ui/xmlrpc.py	2008-08-01 00:43:22 +0000
> > +
> > +class PQM_XMLRPC(XMLRPC):
> > +
> 
> Docstring please.
Done.

> > +    def __init__(self, pqminfo):
> > +        XMLRPC.__init__(self)
> > +        self.pqminfo = pqminfo
> > +        self.logger = logging.getLogger('pqm')
> > +
> > +    def xmlrpc_submit(self, text):
> 
> And here too!
Done.

> > +        try:
> > +            queuedir = self.pqminfo.queuedir
> > +            queuedir = os.path.abspath(queuedir)
> > +            # Set up options for processing
> > +            options = FakeOptions()
> > +            options.verify_sigs =
> > self.pqminfo.configp.get_boolean_option(
> > +                'DEFAULT', 'verify_sigs', True)
> > +            if options.verify_sigs:
> > +                options.keyring =
> > self.pqminfo.configp.get('DEFAULT',
> > +
> > 'keyring')
> > +            # TODO: Eww.
> 
> Please say *why* it's eww and what can be done about it.
Done.

> > +            pqm.gpgv_path =
> > self.pqminfo.configp.get_option('DEFAULT',
> > +                'gpgv_path', 'gpgv')
> > +            pqm_subdir = os.path.join(queuedir, 'pqm')
> > +            pqm.pqm_subdir = pqm_subdir
> > +            # Process input
> 
> This isn't a particularly enlightening comment.
Removed.

> > +            email = read_email_from_string(self.logger, text)
> > +            email_string = get_email_string(self.logger, options,
> > email)
> > +            # Write script
> 
> Neither is this :)
Removed also.

> > +            pqm.write_script(queuedir, email_string)
> > +            return "Success!"
> 
> I think returning a human-readable string sends mixed messages.
> Perhaps just return True instead.
> 
> > +        except PQMException, e:
> > +            return "Error: %s" % (e,)
> >
> 
> Could you please raise a Fault instead?
Done.

> Thanks for the patch!
Thanks again for the review.  The adjusted patch is attached.


Dan

-- 
Daniel Watkins (Odd_Bloke)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xmlrpc.2.patch
Type: text/x-patch
Size: 39466 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080807/e64a619d/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080807/e64a619d/attachment-0001.pgp