Code review comment for lp:~barry/bzr/claws

Barry Warsaw (barry) wrote :

Aaron,

Thanks for setting me on the path to a much smaller and better fix. Here's an
incremental against the original patch, which just adds From: support for
Claws along the lines we discussed. This also adds two new tests for Claws,
and 'bzr selftest Claws' appears to pass for me.

=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py 2009-06-03 19:40:52 +0000
+++ bzrlib/mail_client.py 2009-06-04 19:20:42 +0000
@@ -155,7 +155,7 @@
                       extension, **kwargs)

     def _compose(self, prompt, to, subject, attach_path, mime_subtype,
- extension, body=None):
+ extension, body=None, from_=None):
         """Invoke a mail client as a commandline process.

         Overridden by MAPIClient.
@@ -166,6 +166,8 @@
             "text", but the precise subtype can be specified here
         :param extension: A file extension (including period) associated with
             the attachment type.
+ :param body: Optional body text.
+ :param from_: Optional From: header.
         """
         for name in self._get_client_commands():
             cmdline = [self._encode_path(name, 'executable')]
@@ -173,6 +175,8 @@
                 kwargs = {'body': body}
             else:
                 kwargs = {}
+ if from_ is not None:
+ kwargs['from_'] = from_
             cmdline.extend(self._get_compose_commandline(to, subject,
                                                          attach_path,
                                                          **kwargs))
@@ -335,12 +339,12 @@

     _client_commands = ['claws-mail']

- def _get_compose_commandline(self, to, subject, attach_path, body=None):
+ def _get_compose_commandline(self, to, subject, attach_path, body=None,
+ from_=None):
         """See ExternalMailClient._get_compose_commandline"""
         compose_url = []
- email = self.config.get_user_option('email')
- if email is not None:
- compose_url.append('from=' + urllib.quote(email))
+ if from_ is not None:
+ compose_url.append('from=' + urllib.quote(from_))
         if subject is not None:
             # Don't use urllib.quote_plus because Claws doesn't seem
             # to recognise spaces encoded as "+".
@@ -360,6 +364,16 @@
             message_options.extend(
                 ['--attach', self._encode_path(attach_path, 'attachment')])
         return message_options
+
+ def _compose(self, prompt, to, subject, attach_path, mime_subtype,
+ extension, body=None, from_=None):
+ """See ExternalMailClient._compose"""
+ if from_ is None:
+ from_ = self.config.get_user_option('email')
+ super(Claws, self)._compose(prompt, to, subject, attach_path,
+ mime_subtype, extension, body, from_)
+
+
 mail_client_registry.register('claws', Claws,
                               help=Claws.__doc__)

=== modified file 'bzrlib/tests/test_mail_client.py'
--- bzrlib/tests/test_mail_client.py 2009-05-11 18:35:20 +0000
+++ bzrlib/tests/test_mail_client.py 2009-06-04 19:24:46 +0000
@@ -191,9 +191,10 @@
     def test_commandline(self):
         claws = mail_client.Claws(None)
         commandline = claws._get_compose_commandline(
- None, None, 'file%')
+ '<email address hidden>', None, 'file%')
         self.assertEqual(
- ['--compose', 'mailto:?', '--attach', 'file%'], commandline)
+ ['--compose', 'mailto:<email address hidden>?', '--attach', 'file%'],
+ commandline)
         commandline = claws._get_compose_commandline(
             '<email address hidden>', 'Hi there!', None)
         self.assertEqual(
@@ -217,6 +218,21 @@
             self.assertFalse(isinstance(item, unicode),
                 'Command-line item %r is unicode!' % item)

+ def test_with_from(self):
+ claws = mail_client.Claws(None)
+ cmdline = claws._get_compose_commandline(
+ <email address hidden>', None, None, None, <email address hidden>')
+ self.assertEqual(
+ ['--compose',
+ 'mailto:<email address hidden>?from=qrandom%40example.com'],
+ cmdline)
+
+ def test_to_required(self):
+ claws = mail_client.Claws(None)
+ self.assertRaises(errors.NoMailAddressSpecified,
+ claws._get_compose_commandline,
+ None, None, 'file%')
+

 class TestEditor(tests.TestCase):

« Back to merge proposal