From 9a8b3ba7b15e23855336e51b761f111128a1829f Mon Sep 17 00:00:00 2001
From: Andy McKay <amckay@mozilla.com>
Date: Fri, 7 Jan 2011 10:17:17 -0800
Subject: [PATCH] if the tag can't resolve, don't link to it - stop gap until
 tags are cleaned (bug 623786)

---
 apps/devhub/models.py                  |  7 +++++--
 apps/devhub/tests/test_models.py       | 11 +++++++++++
 apps/tags/models.py                    |  8 ++++++++
 apps/tags/templates/tags/tag_link.html |  6 +++++-
 apps/tags/templates/tags/tag_list.html | 10 +++++++---
 apps/tags/tests/test_helpers.py        |  7 +++----
 apps/tags/tests/test_views.py          |  3 +++
 7 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/apps/devhub/models.py b/apps/devhub/models.py
index 8448edf46b..b60f3a65d8 100644
--- a/apps/devhub/models.py
+++ b/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>',
-                             arg.get_url_path(), arg.tag_text)
+                if arg.can_reverse():
+                    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)
diff --git a/apps/devhub/tests/test_models.py b/apps/devhub/tests/test_models.py
index 88e07e5ca0..4ec77c4e51 100644
--- a/apps/devhub/tests/test_models.py
+++ b/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',
diff --git a/apps/tags/models.py b/apps/tags/models.py
index 520b3eee5c..57140af74b 100644
--- a/apps/tags/models.py
+++ b/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])
 
diff --git a/apps/tags/templates/tags/tag_link.html b/apps/tags/templates/tags/tag_link.html
index c049ef2bdb..241cac044b 100644
--- a/apps/tags/templates/tags/tag_link.html
+++ b/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 %}
diff --git a/apps/tags/templates/tags/tag_list.html b/apps/tags/templates/tags/tag_list.html
index 77a3f5cb35..57dc743c5f 100644
--- a/apps/tags/templates/tags/tag_list.html
+++ b/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">
-    {{ tag.tag_text }}
-  </a>
+  {% if tag.can_reverse() %}
+    <a href="{{ url('tags.detail', tag.tag_text) }}" class="tagitem">
+      {{ tag.tag_text }}
+    </a>
+  {% else %}
+    <span class="tagitem">{{ tag.tag_text }}</span>
+  {% endif %}
 </li>
 {%- endmacro %}
 
diff --git a/apps/tags/tests/test_helpers.py b/apps/tags/tests/test_helpers.py
index 4e97d98f51..5ffab8cb1c 100644
--- a/apps/tags/tests/test_helpers.py
+++ b/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):
diff --git a/apps/tags/tests/test_views.py b/apps/tags/tests/test_views.py
index 725d972f90..8d9f391caf 100644
--- a/apps/tags/tests/test_views.py
+++ b/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']