launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/link-bug-tags-correctly into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #911189 in Launchpad itself: "Bug page tag links are not properly escaped, thus made useless"
  https://bugs.launchpad.net/launchpad/+bug/911189

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/link-bug-tags-correctly/+merge/89187

Fiddle with BugTaskView.{,un}official_tags so they return a dict of tag: url, and then use the url in the TAL. Also add a test to make sure that unofficial_tags returns what we expect, and that the TAL is using the data in the right way.

Fixed up two imports which were in a very odd place.
-- 
https://code.launchpad.net/~stevenk/launchpad/link-bug-tags-correctly/+merge/89187
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/link-bug-tags-correctly into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-01-17 14:14:49 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-01-19 05:24:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IBugTask-related browser views."""
@@ -1038,15 +1038,25 @@
     def official_tags(self):
         """The list of official tags for this bug."""
         target_official_tags = set(self.context.bug.official_tags)
-        return [tag for tag in self.context.bug.tags
-                if tag in target_official_tags]
+        links = {}
+        for tag in self.context.bug.tags:
+            if tag in target_official_tags:
+                links[tag] = '%s/+bugs?field.tag=%s' % (
+                    canonical_url(self.context.target,
+                        force_local_path=True), urllib.quote(tag))
+        return links
 
     @property
     def unofficial_tags(self):
         """The list of unofficial tags for this bug."""
         target_official_tags = set(self.context.bug.official_tags)
-        return [tag for tag in self.context.bug.tags
-                if tag not in target_official_tags]
+        links = {}
+        for tag in self.context.bug.tags:
+            if tag not in target_official_tags:
+                links[tag] = '%s/+bugs?field.tag=%s' % (
+                    canonical_url(self.context.target,
+                        force_local_path=True), urllib.quote(tag))
+        return links
 
     @property
     def available_official_tags_js(self):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-01-18 12:04:16 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-01-19 05:24:25 +0000
@@ -1,9 +1,5 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
-from BeautifulSoup import BeautifulSoup
-
-from lp.registry.interfaces.person import PersonVisibility
-
 
 __metaclass__ = type
 
@@ -16,6 +12,7 @@
 import simplejson
 import urllib
 
+from BeautifulSoup import BeautifulSoup
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.restful.interfaces import IJSONRequestCache
 from lazr.lifecycle.snapshot import Snapshot
@@ -35,6 +32,7 @@
 from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
 
+from lp.registry.interfaces.person import PersonVisibility
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.testing import (
@@ -254,6 +252,19 @@
             self.assertEqual(1, len(view.errors))
             self.assertEqual(product, bug.default_bugtask.target)
 
+    def test_bugtag_urls_are_encoded(self):
+        # The link to bug tags are encoded to protect against special chars.
+        bug = self.factory.makeBug(tags=['depends-on+987'])
+        getUtility(ILaunchBag).add(bug.default_bugtask)
+        view = create_initialized_view(bug.default_bugtask, name=u'+index')
+        expected = {u'depends-on+987':
+            u'/product-name-100003/+bugs?field.tag=depends-on%2B987'}
+        self.assertEqual(expected, view.unofficial_tags)
+        browser = self.getUserBrowser(canonical_url(bug), bug.owner)
+        self.assertIn(
+            'href="/product-name-100003/+bugs?field.tag=depends-on%2B987"',
+            browser.contents)
+
 
 class TestBugTasksAndNominationsView(TestCaseWithFactory):
 

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2011-12-08 20:31:53 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2012-01-19 05:24:25 +0000
@@ -148,11 +148,11 @@
               <a tal:repeat="tag view/official_tags"
                  tal:content="tag"
                  class="official-tag"
-                 tal:attributes="href string:${context/target/fmt:url}/+bugs?field.tag=${tag}">tag</a>
+                 tal:attributes="href string:${view/official_tags/?tag}">tag</a>
               <a tal:repeat="tag view/unofficial_tags"
                  tal:content="tag"
                  class="unofficial-tag"
-                 tal:attributes="href string:${context/target/fmt:url}/+bugs?field.tag=${tag}">tag</a>
+                 tal:attributes="href string:${view/unofficial_tags/?tag}">tag</a>
             </span>
             <form id="tags-form" style="display: inline">
                 <input type="text" id="tag-input" class="hidden" style="width: 35em;" />