Launchpad itself

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

  1. 394133-urlwrap
  2. Merge into devel
Proposed by Barry Warsaw
Status: Merged
Approved by: Barry Warsaw
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://qastaging/~barry/launchpad/394133-urlwrap
Merge into: lp://qastaging/launchpad
Diff against target: 147 lines (+98/-5)
3 files modified
lib/canonical/launchpad/doc/textformatting.txt (+76/-0)
lib/canonical/launchpad/mailnotification.py (+6/-1)
lib/lp/services/mail/mailwrapper.py (+16/-4)
To merge this branch: bzr merge lp://qastaging/~barry/launchpad/394133-urlwrap
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+14504@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :

= 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

Revision history for this message
Abel Deuring (adeuring) wrote :

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)
Revision history for this message
Barry Warsaw (barry) wrote :

> 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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/textformatting.txt'
2--- lib/canonical/launchpad/doc/textformatting.txt 2009-03-24 12:43:49 +0000
3+++ lib/canonical/launchpad/doc/textformatting.txt 2009-11-05 21:40:42 +0000
4@@ -237,3 +237,79 @@
5 'separated with dos-style line endings.',
6 '',
7 "Here's the second paragraph."]
8+
9+Sometimes certain paragraphs should not be wrapped, e.g. a line containing a
10+long hyphenated URL. Under normal circumstances, this will get wrapped.
11+
12+ >>> from canonical.launchpad.helpers import get_email_template
13+ >>> template = get_email_template('new-held-message.txt')
14+ >>> text = template % dict(
15+ ... user="Scarlett O'Hara",
16+ ... team='frankly-my-dear-i-dont-give-a-damn',
17+ ... subject='Thing',
18+ ... author_name='Rhett Butler',
19+ ... author_url='http://whatever.example.com/rhett',
20+ ... date='today',
21+ ... message_id='<aardvark>',
22+ ... # And this is the one we're really interested in.
23+ ... review_url=('http://launchpad.dev/~frankly-my-dear-i-'
24+ ... 'dont-give-a-damn/+review-moderation-messages'),
25+ ... )
26+
27+ >>> wrapper = MailWrapper(72)
28+ >>> body = wrapper.format(text, force_wrap=True)
29+ >>> print body
30+ Hello Scarlett O'Hara,
31+ <BLANKLINE>
32+ frankly-my-dear-i-dont-give-a-damn has a new message requiring your
33+ approval.
34+ <BLANKLINE>
35+ Subject: Thing
36+ Author name: Rhett Butler
37+ Author url: http://whatever.example.com/rhett
38+ Date: today
39+ Message-ID: <aardvark>
40+ <BLANKLINE>
41+ A message has been posted to the mailing list for your team, but this
42+ message requires your approval before it will be sent to the list
43+ members. After reviewing the message, you may approve, discard or
44+ reject it.
45+ <BLANKLINE>
46+ To review all messages pending approval, visit:
47+ <BLANKLINE>
48+ http://launchpad.dev/~frankly-my-dear-i-dont-give-a-damn/+review-
49+ moderation-messages
50+ <BLANKLINE>
51+ Regards,
52+ The Launchpad team
53+
54+But if we don't want the line with the url to be wrapped, we can pass in a
55+callable to format(). This callable prevents wrapping when it returns False.
56+The callable's argument is the pre-wrapped paragraph.
57+
58+ >>> def nowrap(paragraph):
59+ ... return paragraph.startswith('http://')
60+
61+ >>> body = wrapper.format(text, force_wrap=True, wrap_func=nowrap)
62+ >>> print body
63+ Hello Scarlett O'Hara,
64+ <BLANKLINE>
65+ frankly-my-dear-i-dont-give-a-damn has a new message requiring your approval.
66+ <BLANKLINE>
67+ Subject: Thing
68+ Author name: Rhett Butler
69+ Author url: http://whatever.example.com/rhett
70+ Date: today
71+ Message-ID: <aardvark>
72+ <BLANKLINE>
73+ A message has been posted to the mailing list for your team, but this
74+ message requires your approval before it will be sent to the list
75+ members. After reviewing the message, you may approve, discard or
76+ reject it.
77+ <BLANKLINE>
78+ To review all messages pending approval, visit:
79+ <BLANKLINE>
80+ http://launchpad.dev/~frankly-my-dear-i-dont-give-a-damn/+review-moderation-messages
81+ <BLANKLINE>
82+ Regards,
83+ The Launchpad team
84
85=== modified file 'lib/canonical/launchpad/mailnotification.py'
86--- lib/canonical/launchpad/mailnotification.py 2009-10-30 13:08:01 +0000
87+++ lib/canonical/launchpad/mailnotification.py 2009-11-05 21:40:42 +0000
88@@ -1180,13 +1180,18 @@
89 'team': team.displayname,
90 }
91
92+ # Don't wrap the paragraph with the url.
93+ def wrap_function(paragraph):
94+ return (paragraph.startswith('http:') or
95+ paragraph.startswith('https:'))
96+
97 # Send one message to every team administrator.
98 person_set = getUtility(IPersonSet)
99 for address in team.getTeamAdminsEmailAddresses():
100 user = person_set.getByEmail(address)
101 replacements['user'] = user.displayname
102 body = MailWrapper(72).format(
103- template % replacements, force_wrap=True)
104+ template % replacements, force_wrap=True, wrap_func=wrap_function)
105 simple_sendmail(from_address, address, subject, body)
106
107
108
109=== modified file 'lib/lp/services/mail/mailwrapper.py'
110--- lib/lp/services/mail/mailwrapper.py 2009-06-25 04:06:00 +0000
111+++ lib/lp/services/mail/mailwrapper.py 2009-11-05 21:40:42 +0000
112@@ -32,11 +32,17 @@
113 width=width, subsequent_indent=indent,
114 replace_whitespace=False, break_long_words=False)
115
116- def format(self, text, force_wrap=False):
117+ def format(self, text, force_wrap=False, wrap_func=None):
118 """Format the text to be included in an email.
119
120- If force_wrap is False, only paragraphs containing a single line
121- will be wrapped.
122+ :param force_wrap: When False (the default), only paragraphs
123+ containing a single line will be wrapped. Otherwise paragraphs in
124+ text will be re-wrapped.
125+ :type force_wrap: bool
126+ :param wrap_func: A function to call at the beginning of each
127+ paragraph to be wrapped. If the function returns False, the
128+ paragraph is not wrapped.
129+ :type wrap_func: callable or None
130 """
131 wrapped_lines = []
132
133@@ -54,7 +60,13 @@
134 for paragraph in text.split('\n\n'):
135 lines = paragraph.split('\n')
136
137- if len(lines) == 1:
138+ if wrap_func is not None and not wrap_func(paragraph):
139+ # The user's callback function has indicated that the
140+ # paragraph should not be wrapped.
141+ wrapped_lines += (
142+ [indentation + lines[0]] +
143+ [self.indent + line for line in lines[1:]])
144+ elif len(lines) == 1:
145 # We use TextWrapper only if the paragraph consists of a
146 # single line, like in the case where a person enters a
147 # comment via the web ui, without breaking the lines