Merge lp:~barry/launchpad/394133-urlwrap into lp:launchpad

[Barry Warsaw]

[Barry Warsaw (barry)]

= Summary =

Bug 394133 describes an annoying problem when messages are held for moderator
approval, with a team name that is fairly long and contains hyphens. The
message that gets sent to the team administrators has the url wrapped so that
it's generally not clickable in most mail readers.

== Proposed fix ==

Add a hack to MailWrapper.format() so that you can pass in a callable. This
callable gets the paragraph to wrap as an argument and may return False if the
paragraph should not get wrapped. Otherwise (return True), normal processing
will continue.

== Pre-implementation notes ==

The bug report describes a few alternative implementations. Once we're on
Python 2.6 we can just use textwrap's new argument to suppress breaking on
hyphens. We decided not to try to pull in Python 2.6's textwrap just for
this.

== Implementation details ==

None in particular.

== Tests ==

% bin/test -vv -t textformatting

== Demo and Q/A ==

 * make run_all
 * Create a team with a long name
 * Put the following message text in a file:

{{{
From: <email address hidden>
To: <email address hidden>
Subject: Test
Message-ID: <first>

Testing
}}}

 * Tweak the team name to match the long one you created above.
 * lib/mailman/bin/inject -l frankly-my-dear-i-dont-give-a-damn message.txt
 * Watch your email for the non-wrapped message

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/services/mail/mailwrapper.py
  lib/canonical/launchpad/mailnotification.py
  lib/canonical/launchpad/doc/textformatting.txt

[Abel Deuring (adeuring)]

Hi Barry,

Two remarks about this branch:

- Your changes avoid a broken URL in "moderation approval messages".
  But I am wondering, if this should perhaps be applied to other
  messages as well. After all, wrapped URLs for example in bug
  comments are bad too. (OK, in this case you can visit the related
  web page to see the "good" URL)
- Lines are only wrapped iff the paragraph starts with "http:" or
  "https:", so it won't work when the paragraph contains a few
  introductory words before the URL.

OTOH, you write that the wrapper in Python 2.6 will no longer need
this workaround, so I think this highly specialized fix is fine.

Abel

review: Approve (code)

[Barry Warsaw (barry)]

> Two remarks about this branch:
>
> - Your changes avoid a broken URL in "moderation approval messages".
> But I am wondering, if this should perhaps be applied to other
> messages as well. After all, wrapped URLs for example in bug
> comments are bad too. (OK, in this case you can visit the related
> web page to see the "good" URL)

I think we can handle this on a case-by-case basis. If we get additional bug reports we can implement a no-wrap filter function at that time.

> - Lines are only wrapped iff the paragraph starts with "http:" or
> "https:", so it won't work when the paragraph contains a few
> introductory words before the URL.

This is true, however if we run across such a situation, we can change the no-wrap filter function to suite the particular situation. The one implemented in this fix is only meant to cover the new-held-message email message.

>
> OTOH, you write that the wrapper in Python 2.6 will no longer need
> this workaround, so I think this highly specialized fix is fine.
>

Thanks! Yes, it will be better to use Python 2.6's facilities when we land on that version. Thanks for the review!

=== modified file 'lib/canonical/launchpad/doc/textformatting.txt'
--- lib/canonical/launchpad/doc/textformatting.txt	2009-03-24 12:43:49 +0000
+++ lib/canonical/launchpad/doc/textformatting.txt	2009-11-05 21:40:42 +0000
@@ -237,3 +237,79 @@
      'separated with dos-style line endings.',
      '',
      "Here's the second paragraph."]
+
+Sometimes certain paragraphs should not be wrapped, e.g. a line containing a
+long hyphenated URL.  Under normal circumstances, this will get wrapped.
+
+    >>> from canonical.launchpad.helpers import get_email_template
+    >>> template = get_email_template('new-held-message.txt')
+    >>> text = template % dict(
+    ...     user="Scarlett O'Hara",
+    ...     team='frankly-my-dear-i-dont-give-a-damn',
+    ...     subject='Thing',
+    ...     author_name='Rhett Butler',
+    ...     author_url='http://whatever.example.com/rhett',
+    ...     date='today',
+    ...     message_id='<aardvark>',
+    ...     # And this is the one we're really interested in.
+    ...     review_url=('http://launchpad.dev/~frankly-my-dear-i-'
+    ...                 'dont-give-a-damn/+review-moderation-messages'),
+    ...     )
+
+    >>> wrapper = MailWrapper(72)
+    >>> body = wrapper.format(text, force_wrap=True)
+    >>> print body
+    Hello Scarlett O'Hara,
+    <BLANKLINE>
+    frankly-my-dear-i-dont-give-a-damn has a new message requiring your
+    approval.
+    <BLANKLINE>
+        Subject: Thing
+        Author name: Rhett Butler
+        Author url: http://whatever.example.com/rhett
+        Date: today
+        Message-ID: <aardvark>
+    <BLANKLINE>
+    A message has been posted to the mailing list for your team, but this
+    message requires your approval before it will be sent to the list
+    members.  After reviewing the message, you may approve, discard or
+    reject it.
+    <BLANKLINE>
+    To review all messages pending approval, visit:
+    <BLANKLINE>
+        http://launchpad.dev/~frankly-my-dear-i-dont-give-a-damn/+review-
+    moderation-messages
+    <BLANKLINE>
+    Regards,
+    The Launchpad team
+
+But if we don't want the line with the url to be wrapped, we can pass in a
+callable to format().  This callable prevents wrapping when it returns False.
+The callable's argument is the pre-wrapped paragraph.
+
+    >>> def nowrap(paragraph):
+    ...     return paragraph.startswith('http://')
+
+    >>> body = wrapper.format(text, force_wrap=True, wrap_func=nowrap)
+    >>> print body
+    Hello Scarlett O'Hara,
+    <BLANKLINE>
+    frankly-my-dear-i-dont-give-a-damn has a new message requiring your approval.
+    <BLANKLINE>
+        Subject: Thing
+        Author name: Rhett Butler
+        Author url: http://whatever.example.com/rhett
+        Date: today
+        Message-ID: <aardvark>
+    <BLANKLINE>
+    A message has been posted to the mailing list for your team, but this
+    message requires your approval before it will be sent to the list
+    members.  After reviewing the message, you may approve, discard or
+    reject it.
+    <BLANKLINE>
+    To review all messages pending approval, visit:
+    <BLANKLINE>
+        http://launchpad.dev/~frankly-my-dear-i-dont-give-a-damn/+review-moderation-messages
+    <BLANKLINE>
+    Regards,
+    The Launchpad team
 
=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py	2009-10-30 13:08:01 +0000
+++ lib/canonical/launchpad/mailnotification.py	2009-11-05 21:40:42 +0000
@@ -1180,13 +1180,18 @@
         'team': team.displayname,
         }
 
+    # Don't wrap the paragraph with the url.
+    def wrap_function(paragraph):
+        return (paragraph.startswith('http:') or
+                paragraph.startswith('https:'))
+
     # Send one message to every team administrator.
     person_set = getUtility(IPersonSet)
     for address in team.getTeamAdminsEmailAddresses():
         user = person_set.getByEmail(address)
         replacements['user'] = user.displayname
         body = MailWrapper(72).format(
-            template % replacements, force_wrap=True)
+            template % replacements, force_wrap=True, wrap_func=wrap_function)
         simple_sendmail(from_address, address, subject, body)
 
 
 
=== modified file 'lib/lp/services/mail/mailwrapper.py'
--- lib/lp/services/mail/mailwrapper.py	2009-06-25 04:06:00 +0000
+++ lib/lp/services/mail/mailwrapper.py	2009-11-05 21:40:42 +0000
@@ -32,11 +32,17 @@
             width=width, subsequent_indent=indent,
             replace_whitespace=False, break_long_words=False)
 
-    def format(self, text, force_wrap=False):
+    def format(self, text, force_wrap=False, wrap_func=None):
         """Format the text to be included in an email.
 
-        If force_wrap is False, only paragraphs containing a single line
-        will be wrapped.
+        :param force_wrap: When False (the default), only paragraphs
+            containing a single line will be wrapped.  Otherwise paragraphs in
+            text will be re-wrapped.
+        :type force_wrap: bool
+        :param wrap_func: A function to call at the beginning of each
+            paragraph to be wrapped.  If the function returns False, the
+            paragraph is not wrapped.
+        :type wrap_func: callable or None
         """
         wrapped_lines = []
 
@@ -54,7 +60,13 @@
         for paragraph in text.split('\n\n'):
             lines = paragraph.split('\n')
 
-            if len(lines) == 1:
+            if wrap_func is not None and not wrap_func(paragraph):
+                # The user's callback function has indicated that the
+                # paragraph should not be wrapped.
+                wrapped_lines += (
+                    [indentation + lines[0]] +
+                    [self.indent + line for line in lines[1:]])
+            elif len(lines) == 1:
                 # We use TextWrapper only if the paragraph consists of a
                 # single line, like in the case where a person enters a
                 # comment via the web ui, without breaking the lines