[MERGE] Send actually *sends* the merge directive

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
This currently fails to send an email to a Unicode user. At least my 
test
for:
bzr send --mail-to "Jöhn john at arbash-meinel.com" ../jam-integration

(I updated jam-integration to be 1 revision behind bzr.dev)

Failed with a traceback:
Traceback (most recent call last):
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/commands.py", line 781, 
in run_bzr_catch_errors
     return run_bzr(argv)
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/commands.py", line 743, 
in run_bzr
     ret = run(*run_argv)
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/commands.py", line 475, 
in run_argv_aliases
     return self.run(**all_cmd_args)
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/builtins.py", line 3864, 
in run
     kwargs.get('from', '.'), mail_to, message)
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/builtins.py", line 3961, 
in _run
     outfile.getvalue())
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/mail_client.py", line 
227, in compose_merge_request
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/mail_client.py", line 46, 
in compose_merge_request
     'x-patch', '.patch')
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/mail_client.py", line 72, 
in compose

   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/email_message.py", line 
162, in send
     SMTPConnection(config).send_email(msg)
   File "/Users/jameinel/dev/bzr/bzr.dev/bzrlib/smtp_connection.py", line 
138, in send_email
     message.as_string())
   File 
"/opt/local/Library/Frameworks/Python.framework/Versions/2.4/lib/python2.4/smtplib.py", 
line 685, in sendmail
     (code,resp)=self.rcpt(each, rcpt_options)
   File 
"/opt/local/Library/Frameworks/Python.framework/Versions/2.4/lib/python2.4/smtplib.py", 
line 468, in rcpt
     self.putcmd("rcpt","TO:%s%s" % (quoteaddr(recip),optionlist))
   File 
"/opt/local/Library/Frameworks/Python.framework/Versions/2.4/lib/python2.4/smtplib.py", 
line 329, in putcmd
     self.send(str)
   File 
"/opt/local/Library/Frameworks/Python.framework/Versions/2.4/lib/python2.4/smtplib.py", 
line 316, in send
     self.sock.sendall(str)
   File 
"/opt/local/Library/Frameworks/Python.framework/Versions/2.4/lib/python2.4/smtplib.py", 
line 140, in send
     self.sslobj.write(str)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf6' in 
position 10: ordinal not in range(128)



You should really have some doc strings for the different functions.

+class MailClient(object):
+    """A mail client that can send messages with attachements."""
+
+    def __init__(self, config):
+        self.config = config
+
+    def compose(self, prompt, to, subject, attachment, mime_subtype,
+                extension):
+        raise NotImplementedError
+
+    def compose_merge_request(self, to, subject, directive):
+        prompt = self._get_merge_prompt("Please describe these 
changes:", to,
+                                        subject, directive)
+        self.compose(prompt, to, subject, directive,
+            'x-patch', '.patch')
+
+    def _get_merge_prompt(self, prompt, to, subject, attachment):
+        return ''

Having docstrings to explain the parameters, as well as explain when 
they
are used/what child classes should be overriding would be very useful.

Also, I was worried about how you would handle Unicode characters, but 
you
seem to be doing the "right thing". Which means that some of the
parameters aren't what I would expect them to be. At least, when you 
have:

+    def _get_merge_prompt(self, prompt, to, subject, attachment):
+        return "%s\n\nTo: %s\nSubject: %s\n\n%s" % (prompt, to, 
subject,
+                attachment.decode('utf-8', 'replace'))

It *looks* like you are returning a plain string. But actually, you are
*always* returning a Unicode string. (attachment.decode() will always
upgrade itself to a Unicode object, and str % (unicode,) will usually
upgrade itself to a Unicode string. And since it is always upcasting, it
would be better to start with a unicode string.

return u"%s\nnTo: %s\nSubject: %s\n\n%s" % (...)

It might be a bit more legible as:

return (u"%s\n\n"
         u"To: %s\n"
         u"Subject: %s\n\n"
         u"%s") % (prompt, to, subject,
                   attachment.decode('utf-8', 'replace'))

But not everyone would prefer that form. So I'm not very sticky on it.



On Mac OS X, it would be nice to be able to use Thunderbird, but none of
the commands are in my path. I would be okay with explicitly setting the
path to the "thunderbird", but I didn't see any way to do that.
I did find that if I manually run:

/Applications/Mozilla/Thunderbird.app/Contents/MacOS/thunderbird-bin 
-compose to=john at arbash-meinel.com,from=me at testing.com,subject='Who the 
hell are 
you?',attachment='file:///Users/jameinel/dev/bzr/patches/send-attached3.patch'

It starts Thunderbird, and has a compose window with the attachment
listed, and the right subject and to value. However, if I already have
Thunderbird running, it gives me a pop-up saying that you can only have
one instance at a time.

I don't know why this is still a problem, as I'm using Thunderbird
2.0.0.6, but it certainly isn't working.

Now, on Mac, I know that I can do "open mailto:john at arbash-meinel.com"
and it will start a new compose window from my existing Thunderbird
process. I haven't found any way to pass it anything other than the 
email
address (everything else I try just gives me "no such file"). I did find
that I can do:
   open 'mailto:John%20Meinel%20%3cjohn at arbash-meinel.com%3e'
To give a full username + email address. Or if I want to send to 
multiple
addresses, you can do:
'mailto:' + urllib.quote('John Meinel <john at arbash-meinel.com>,Aaron 
Bentley <abentley at utoronto.ca>')
which gives
'mailto:John%20Meinel%20%3Cjohn%40arbash-meinel.com%3E%2CAaron%20Bentley%20%3Cabentley%40utoronto.ca%3E'

And sure enough, it composes to 2 people. But I wasn't able to create a
subject or an attachment. ('subject=foo' just resulted in sending to 
someone
named 'subject=foo')
Anyway, just because you don't work on Mac isn't a reason to block. 
Though
it would be nice if we could figure out how 'mailto:' is doing the right
thing. It might be that Mac uses something similar to MAPI.



I didn't see any code in any of the commands to handle Unicode email
addresses. So Lukáš Lalinský will be a poor address. (I still haven't
figured out how to type š on either Mac or Linux. I can't find that u
combining character.)
Anyway, we need to figure out how we should be sanitizing the 
parameters.
(bzrlib.user_encoding I think.)


I think we've already solved the encoding issues when a user *writes* a
unicode message in their editor. Though we should really be testing that
the rest of the email sending code is handling it properly. (obviously 
it
is failing somewhere for "Jöhn john at arbash-meinel.com").


In some ways, I would be okay with merging it, even though it doesn't
support Unicode. But if we do, then we need to open some bugs on it.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C46C1E17B.70700%40utoronto.ca%3E