launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/fix-linkify-email-multiple-icons into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #939644 in Launchpad itself: "Too many icons beside email address"
  https://bugs.launchpad.net/launchpad/+bug/939644

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/fix-linkify-email-multiple-icons/+merge/189212

FormattersAPI.linkify_email() finds every e-mail address mentioned in a given string and linkifies it. Unfortunately, it globally replaces the address, so we end up linking the e-mail the number of times the address appears in the string, leading to hilarity. Stop this nonsense by tracking which e-mail addresses have been seen.
-- 
https://code.launchpad.net/~stevenk/launchpad/fix-linkify-email-multiple-icons/+merge/189212
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/fix-linkify-email-multiple-icons into lp:launchpad.

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2012-12-11 05:41:38 +0000
+++ lib/lp/app/browser/stringformatter.py	2013-10-04 05:55:42 +0000
@@ -820,10 +820,14 @@
         may be a concern but is noted here for posterity anyway.
         """
         text = self._stringtoformat
-
+        seen_addresses = []
         matches = re.finditer(re_email_address, text)
         for match in matches:
             address = match.group()
+            # Since we globally replace the e-mail in the text, if we have seen
+            # the address before, skip it.
+            if address in seen_addresses:
+                continue
             person = None
             # First try to find the person required in the preloaded person
             # data dictionary.
@@ -837,9 +841,9 @@
             if person is not None and not person.hide_email_addresses:
                 # Circular dependancies now. Should be resolved by moving the
                 # object image display api.
-                from lp.app.browser.tales import (
-                    ObjectImageDisplayAPI)
+                from lp.app.browser.tales import ObjectImageDisplayAPI
                 css_sprite = ObjectImageDisplayAPI(person).sprite_css()
+                seen_addresses.append(address)
                 text = text.replace(
                     address, '<a href="%s" class="%s">%s</a>' % (
                         canonical_url(person), css_sprite, address))

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2012-11-29 06:21:28 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2013-10-04 05:55:42 +0000
@@ -22,7 +22,10 @@
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.interfaces import ILaunchBag
-from lp.testing import TestCase
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.pages import find_tags_by_class
 
@@ -160,7 +163,9 @@
             linkify_bug_numbers(text))
 
 
-class TestLinkifyingProtocols(TestCase):
+class TestLinkifyingProtocols(TestCaseWithFactory):
+    
+    layer = DatabaseFunctionalLayer
 
     def test_normal_set(self):
         test_strings = [
@@ -271,6 +276,17 @@
             'http://example.com/&lt;/a&gt;</p>')
         self.assertEqual(expected_html, html)
 
+    def test_double_email_in_linkify_email(self):
+        self.factory.makePerson(email='foo@xxxxxxxxxxx')
+        test_string = ' * Foo. <foo@xxxxxxxxxxx>\n * Bar <foo@xxxxxxxxxxx>'
+        html = FormattersAPI(test_string).linkify_email()
+        expected_html = (
+            ' * Foo. <<a href="http://launchpad.dev/~person-name-100000"; '
+            'class="sprite person">foo@xxxxxxxxxxx</a>>\n * Bar '
+            '<<a href="http://launchpad.dev/~person-name-100000"; '
+            'class="sprite person">foo@xxxxxxxxxxx</a>>')
+        self.assertEqual(expected_html, html)
+
 
 class TestLastParagraphClass(TestCase):