launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-654585-linkify-bug-acknowledgement into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #654585 Line break not shown in custom bug reported acknowledgement
  https://bugs.launchpad.net/bugs/654585
  #710291 urls in 'message to show after filing a bug' are not linkified
  https://bugs.launchpad.net/bugs/710291

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-654585-linkify-bug-acknowledgement/+merge/48254

This branch adjust the bug acknowledgement notification code to run the text through text_to_html. This allows paragraphs and links to be handled more sensibly.

The change itself was pretty trivial, but styling was not. The bottom padding of the notification <div> combined with the bottom margin of the final <p> to create a large gap, requiring the elimination of the final margin. While this is trivially done using :last-child, it is still not supported by IE. So I added an option to text_to_html to set a class on the last paragraph, and removed the margin based on the presence of that class.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-654585-linkify-bug-acknowledgement/+merge/48254
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-654585-linkify-bug-acknowledgement into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2011-01-12 00:00:56 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2011-02-01 23:54:19 +0000
@@ -995,6 +995,9 @@
 .informational.message::before {
     content: url(/@@/info-large);
     }
+.informational p.last {
+    margin-bottom: 0;
+    }
 .debugging {
     /* Debugging messages are white on grey, alerts have an info icon. */
     background: #666;

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-12-23 18:41:43 +0000
+++ lib/lp/app/browser/stringformatter.py	2011-02-01 23:54:19 +0000
@@ -497,7 +497,8 @@
     # re-attaches parens if we do want them to be part of the url.
     _re_url_trailers = re.compile(r'([,.?:);>]+)$')
 
-    def text_to_html(self, linkify_text=True, linkify_substitution=None):
+    def text_to_html(self, linkify_text=True, linkify_substitution=None,
+                     last_paragraph_class=None):
         """Quote text according to DisplayingParagraphsOfText."""
         # This is based on the algorithm in the
         # DisplayingParagraphsOfText spec, but is a little more
@@ -509,12 +510,14 @@
         #    second does not begin with white space.
         # 3. Use <br /> to split logical lines within a paragraph.
         output = []
-        first_para = True
-        for para in split_paragraphs(self._stringtoformat):
-            if not first_para:
+        paras = list(split_paragraphs(self._stringtoformat))
+        for index, para in zip(range(len(paras)), paras):
+            if index > 0:
                 output.append('\n')
-            first_para = False
-            output.append('<p>')
+            cls = (
+                ' class="%s"' % last_paragraph_class
+                if last_paragraph_class and index == len(paras) - 1 else "")
+            output.append('<p%s>' % cls)
             first_line = True
             for line in para:
                 if not first_line:

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2010-12-21 15:09:21 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2011-02-01 23:54:19 +0000
@@ -264,6 +264,15 @@
         self.assertEqual(expected_html, html)
 
 
+class TestLastParagraphClass(TestCase):
+
+    def test_last_paragraph_class(self):
+        self.assertEqual(
+            '<p>Foo</p>\n<p class="last">Bar</p>',
+            FormattersAPI("Foo\n\nBar").text_to_html(
+                last_paragraph_class="last"))
+
+
 class TestDiffFormatter(TestCase):
     """Test the string formatter fmt:diff."""
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2011-01-31 16:30:10 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2011-02-01 23:54:19 +0000
@@ -84,6 +84,7 @@
     LaunchpadFormView,
     safe_action,
     )
+from lp.app.browser.stringformatter import FormattersAPI
 from lp.app.browser.tales import BugTrackerFormatterAPI
 from lp.app.enums import ServiceUsage
 from lp.app.errors import (
@@ -503,7 +504,10 @@
         else:
             private = False
 
-        notifications = [self.getAcknowledgementMessage(self.context)]
+        linkified_ack = structured(FormattersAPI(
+            self.getAcknowledgementMessage(self.context)).text_to_html(
+                last_paragraph_class="last"))
+        notifications = [linkified_ack]
         params = CreateBugParams(
             title=title, comment=comment, owner=self.user,
             security_related=security_related, private=private,

=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-guidelines.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-guidelines.txt	2010-12-17 13:23:41 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-guidelines.txt	2011-02-01 23:54:19 +0000
@@ -22,7 +22,8 @@
     ...         "See http://example.com for more details." % (context_name,))
     ...     admin_browser.getControl(
     ...         name='field.bug_reported_acknowledgement').value = (
-    ...         "Thank you for filing a bug for %s" % context_name)
+    ...         "Thank you for filing a bug for https://launchpad.dev/%s"; %
+    ...         context_path)
     ...     if context_path == 'ubuntu':
     ...         admin_browser.getControl('Change', index=3).click()
     ...     else:
@@ -70,21 +71,21 @@
     Ubuntu bug reporting guidelines:
     The version of Ubuntu you're using.
     See http://example.com for more details.
-    Thank you for filing a bug for Ubuntu
+    Thank you for filing a bug for https://launchpad.dev/ubuntu
     *
     Mozilla
       <http://launchpad.dev/mozilla/+filebug>
     The Mozilla Project bug reporting guidelines:
     The version of Mozilla you're using.
     See http://example.com for more details.
-    Thank you for filing a bug for Mozilla
+    Thank you for filing a bug for https://launchpad.dev/mozilla
     *
     Firefox
       <http://launchpad.dev/firefox/+filebug>
     Mozilla Firefox bug reporting guidelines:
     The version of Firefox you're using.
     See http://example.com for more details.
-    Thank you for filing a bug for Firefox
+    Thank you for filing a bug for https://launchpad.dev/firefox
     *
     alsa-utils in Ubuntu
       <http://launchpad.dev/ubuntu/+source/alsa-utils/+filebug>
@@ -94,7 +95,13 @@
     Ubuntu bug reporting guidelines:
     The version of Ubuntu you're using.
     See http://example.com for more details.
-    Thank you for filing a bug for alsa-utils in Ubuntu
+    Thank you for filing a bug for https://launchpad.dev/ubuntu/+source/alsa-utils
+
+URLs are linkified.
+
+    >>> print find_tags_by_class(
+    ...     user_browser.contents, 'informational message')[0]
+    <div ...><p class="last">Thank you for filing a bug for <a...https://launchpad.dev/ubuntu/+source/alsa-utils.../a></p></div>
 
 Note how the alsa-utils in Ubuntu specific guidelines were displayed
 followed by the general Ubuntu bug reporting guidelines.