if the tag can't resolve, don't link to it - stop gap until tags are cleaned (bug 623786)

apps/devhub/models.py

@@ -220,8 +220,11 @@ class ActivityLog(amo.models.ModelBase):
                                     arg.get_url_path(), arg.name)
                 arguments.remove(arg)
             if isinstance(arg, Tag) and not tag:
-                tag = self.f(u'<a href="{0}">{1}</a>',
+                if arg.can_reverse():
-                             arg.get_url_path(), arg.tag_text)
+                    tag = self.f(u'<a href="{0}">{1}</a>',
+                                 arg.get_url_path(), arg.tag_text)
+                else:
+                    tag = self.f('{0}', arg.tag_text)
         try:
             kw = dict(addon=addon, review=review, version=version,
                       collection=collection, tag=tag)

apps/devhub/tests/test_models.py

@@ -2,11 +2,13 @@ import jingo
 import test_utils
 from nose.tools import eq_
 from mock import Mock
+from pyquery import PyQuery as pq
 
 import amo
 from addons.models import Addon, AddonUser
 from bandwagon.models import Collection
 from devhub.models import ActivityLog
+from tags.models import Tag
 from files.models import File
 from reviews.models import Review
 from users.models import UserProfile
@@ -125,6 +127,15 @@ class TestActivityLog(test_utils.TestCase):
             '<p>&lt;script src=&#34;x.js&#34;&gt; role changed to Owner for <a'
             ' href="/en-US/firefox/addon/a3615/">Delicious Bookmarks</a>.</p>')
 
+    def test_tag_no_match(self):
+        addon = Addon.objects.get()
+        tag = Tag.objects.create(tag_text='http://foo.com')
+        amo.log(amo.LOG.ADD_TAG, addon, tag)
+        log = ActivityLog.objects.get()
+        text = jingo.env.from_string('<p>{{ log }}</p>').render(log=log)
+        # There should only be one a, the link to the addon, but no tag link.
+        eq_(len(pq(text)('a')), 1)
+
 
 class TestVersion(test_utils.TestCase):
     fixtures = ['base/apps', 'base/users', 'base/addon_3615',

apps/tags/models.py

@@ -1,4 +1,5 @@
 from django.db import models
+from django.core.urlresolvers import NoReverseMatch
 
 import amo.models
 from amo.urlresolvers import reverse
@@ -30,6 +31,13 @@ class Tag(amo.models.ModelBase):
     def popularity(self):
         return self.tagstat.num_addons
 
+    def can_reverse(self):
+        try:
+            self.get_url_path()
+            return True
+        except NoReverseMatch:
+            return False
+
     def get_url_path(self):
         return reverse('tags.detail', args=[self.tag_text])
 

apps/tags/templates/tags/tag_link.html

@@ -1 +1,5 @@
-<a class="tagLevel{{ factor }} tag" href="{{ url('tags.detail', tag.tag_text.lower()) }}">{{ tag.tag_text }}</a>
+{% if tag.can_reverse() %}
+    <a class="tagLevel{{ factor }} tag" href="{{ url('tags.detail', tag.tag_text.lower()) }}">{{ tag.tag_text }}</a>
+{% else %}
+    <span class="tagLevel{{ factor }} tag">{{ tag.tag_text }}</span>
+{% endif %}

apps/tags/templates/tags/tag_list.html

@@ -1,8 +1,12 @@
 {% macro tag_li(tag, addon=None, css_class='') -%}
 <li id="taglink-{{ tag.id }}" class="{{ css_class }}">
-  <a href="{{ remora_url('/tag/'+tag.tag_text) }}" class="tagitem">
+  {% if tag.can_reverse() %}
-    {{ tag.tag_text }}
+    <a href="{{ url('tags.detail', tag.tag_text) }}" class="tagitem">
-  </a>
+      {{ tag.tag_text }}
+    </a>
+  {% else %}
+    <span class="tagitem">{{ tag.tag_text }}</span>
+  {% endif %}
 </li>
 {%- endmacro %}
 

apps/tags/tests/test_helpers.py

@@ -11,7 +11,6 @@ from addons.models import Addon
 from tags.models import AddonTag, Tag, TagStat
 from tags.helpers import tag_link
 
-from django.core.urlresolvers import NoReverseMatch
 
 xss = "<script>alert('xss')</script>"
 
@@ -34,8 +33,7 @@ class TestHelpers(test.TestCase):
             'LANG': 'en-us',
             'request': request,
             'addon': addon,
-            'tags': tags
+            'tags': tags}
-        }
 
         # initialize jingo
         jingo.load_helpers()
@@ -59,7 +57,8 @@ class TestHelpers(test.TestCase):
         tag.save()
         TagStat.objects.create(tag=tag, num_addons=1)
 
-        self.assertRaises(NoReverseMatch, tag_link, tag, 1, 1)
+        doc = pq(tag_link(tag, 1, 1))
+        assert not doc('a')
 
 
 def create_tags(addon, author, number):

apps/tags/tests/test_views.py

@@ -87,6 +87,9 @@ class TestXSSURLFail(test_utils.TestCase):
         self.assertRaises(NoReverseMatch, reverse,
                           'tags.top_cloud', args=[self.xss])
 
+    def test_no_reverse(self):
+        assert not self.tag.can_reverse()
+
 
 class TestNoTags(test_utils.TestCase):
     fixtures = ['base/addon_3615']