[569285] Implement question tagging based on django-taggit, which has…

… a nicer API and less-intrusive schema requirements than django-tagging.

* Introduce an autocomplete widget with decent user feedback on both adding and removal. Should be reusable elsewhere without much work. Also has non-JS fallback.
* Tag storage for the autocompleter is client-side, which lets us take reaction time from 300ms (or greater) down to around 0. Also provides lighter server load, and, as a side effect, containment search for tags rather than just prefix (the former being tricky to index with MySQL). Uses linear search atm, but performance on an 814-word vocab is acceptable on a 2GHz i7.

apps/questions/fixtures/taggit.json

@@ -0,0 +1,60 @@
+[
+    {
+        "pk": 1,
+        "model": "taggit.tag",
+        "fields": {
+            "name": "red",
+            "slug": "red"
+        }
+    },
+    {
+        "pk": 2,
+        "model": "taggit.tag",
+        "fields": {
+            "name": "green",
+            "slug": "green"
+        }
+    },
+    {
+        "pk": 3,
+        "model": "taggit.tag",
+        "fields": {
+            "name": "lemon",
+            "slug": "lemon"
+        }
+    },
+    {
+        "pk": 5,
+        "model": "taggit.tag",
+        "fields": {
+            "name": "purplepurplepurple",
+            "slug": "purplepurplepurple"
+        }
+    },
+    {
+        "pk": 7,
+        "model": "taggit.tag",
+        "fields": {
+            "name": "colorless",
+            "slug": "colorless"
+        }
+    },
+    {
+        "pk": 217,
+        "model": "taggit.taggeditem",
+        "fields": {
+            "tag": 2,
+            "content_type": ["questions", "question"],
+            "object_id": 2
+        }
+    },
+    {
+        "pk": 226,
+        "model": "taggit.taggeditem",
+        "fields": {
+            "tag": 7,
+            "content_type": ["questions", "question"],
+            "object_id": 2
+        }
+    }
+]

apps/questions/models.py

@@ -6,15 +6,15 @@
 
 import jinja2
 
-from sumo.models import ModelBase
+from sumo.models import ModelBase, TaggableMixin
 from sumo.utils import WikiParser
 from sumo.urlresolvers import reverse
 from sumo.helpers import urlparams
 import questions as constants
 from .tasks import update_question_votes
 
 
-class Question(ModelBase):
+class Question(ModelBase, TaggableMixin):
     """A support question."""
     title = models.CharField(max_length=255)
     creator = models.ForeignKey(User, related_name='questions')
@@ -34,6 +34,9 @@ class Question(ModelBase):
 
     class Meta:
         ordering = ['-updated']
+        permissions = (
+                ('can_tag', 'Can add tags to and remove tags from questions'),
+            )
 
     def __unicode__(self):
         return self.title

apps/questions/tags.py

@@ -0,0 +1,19 @@
+"""Fairly generic tagging utilities headed toward a dedicated app"""
+
+from taggit.models import Tag
+
+
+def add_existing_tag(tag_name, tag_manager):
+    """Add a tag that already exists to an object. Return the normalized name.
+
+    Given a tag name and a TaggableManager, have the manager add the tag of
+    that name. The tag is matched case-insensitively. If there is no such tag,
+    raise Tag.DoesNotExist.
+
+    Return the canonically cased name of the tag.
+
+    """
+    # TODO: Think about adding a new method to _TaggableManager upstream.
+    tag = Tag.objects.get(name__iexact=tag_name)
+    tag_manager.add(tag)
+    return tag.name

apps/questions/templates/questions/answers.html

@@ -103,10 +103,51 @@ <h4>{{ _('System Details') }}</h4>
       </ul>
       <p><a href="#">{{ _('More system details...') }}</a></p>
 
-      <h4>{{ _('Tags') }}</h4>
-      <div class="tags">
-        <a href="#">mac</a>, <a href="#">flash</a>, <a href="#">crash</a>
-      </div>
+      {% set tags = question.tags.all() %}
+      {% if tags or can_tag %}
+        <h4>{{ _('Tags') }}</h4>
+        <div class="tags"{% if can_tag %} data-tag-vocab-json="{{ tag_vocab }}"{% endif %}>
+          {% if can_tag %}
+            <form action="{{ url('questions.remove_tag', question.id) }}"
+                  data-action-async="{{ url('questions.remove_tag_async', question.id) }}"
+                  method="POST"
+                  class="remove-tag-form">
+              {{ csrf() }}
+          {% endif %}
+          <ul class="tag-list{% if not can_tag %} immutable{% endif %}">
+            {% for tag in tags %}
+              <li class="tag">{# -#}
+                <a class="tag-name" href="#">{{ tag }}</a>
+                {%- if can_tag -%}
+                  <input type="submit"
+                         name="remove-tag-{{ tag }}"
+                         value="&#x2716;"
+                         class="remover" />
+                {%- endif -%}
+              </li>
+            {% endfor %}
+          </ul>
+          {% if can_tag %}
+            </form>
+          {% endif %}
+
+          {% if can_tag %}
+            {% if tag_adding_error %}
+              <p class="tag-error-message">{{ tag_adding_error }}</p>
+            {% endif %}
+            <form action="{{ url('questions.add_tag', question.id) }}"
+                  data-action-async="{{ url('questions.add_tag_async', question.id) }}"
+                  method="POST"
+                  class="add-tag-form">
+              {{ csrf() }}
+              <input type="text" name="tag-name" size="12"
+                     class="autocomplete-tags {% if tag_adding_error %} invalid{% endif %}"
+                     value="{{ tag_adding_value }}" />
+              <input class="adder btn g-btn" type="submit" value="{{ _('Add') }}" />
+            </form>
+          {% endif %}
+        </div>
+      {% endif %}
 
       <h4>{{ _('Related Questions') }}</h4>
       <ul class="related">
@@ -242,4 +283,4 @@ <h3>{{ _('Post a Reply') }}</h3>
 {% endblock %}
 
 {% block side %}
-{% endblock %}
+{% endblock %}

apps/questions/templates/questions/questions.html

@@ -39,7 +39,7 @@
   {% if questions.object_list %}
     <ol class="questions">
       {% for question in questions.object_list %}
-        <li id="question-{{ question.id }}">
+        <li id="question-{{ question.id }}" class="question">
           {% include 'questions/includes/have_problem.html' %}
           <div class="title">
             <h2><a href="{{ question.get_absolute_url() }}">{{ question.title }}</a></h2>
@@ -59,15 +59,25 @@ <h2><a href="{{ question.get_absolute_url() }}">{{ question.title }}</a></h2>
           </div>
           <p>{{ question.content_parsed|striptags()|truncate(170) }}</p>
           <div class="meta">
+            {% set bullet = joiner("&bull;") %}
             {% if question.metadata.ff_version %}
+              {{ bullet()|safe }}
               <span>Firefox {{ question.metadata.ff_version }}</span>
-              &bull;
             {% endif %}
             {% if question.metadata.os %}
+              {{ bullet()|safe }}
               <span>{{ question.metadata.os }}</span>
-              &bull;
             {% endif %}
-            <span>{{ _('Tagged') }} {# TODO #}lorem, ipsum, dolor, sit amet</span>
+            {% set tags = question.tags.all() %}
+            {% if tags %}
+              {{ bullet()|safe }}
+              {{ _('Tagged') }}
+              <ul class="tag-list immutable">
+                {% for tag in tags %}
+                    <li><a class="tag-name" href="#">{{ tag }}</a></li>
+                {% endfor %}
+              </ul>
+            {% endif %}
           </div>
         </li>
       {% endfor %}

apps/questions/tests/__init__.py

@@ -1,10 +1,7 @@
 from django.test import TestCase, client
-from django.contrib.auth.models import User
-
-from nose.tools import eq_
 
 from sumo.urlresolvers import reverse
-from questions.models import Question, Answer
+from questions.models import Question
 
 
 get = lambda c, v, **kw: c.get(reverse(v, **kw), follow=True)
@@ -13,6 +10,7 @@
 
 class TestCaseBase(TestCase):
     """Base TestCase for the Questions app test cases."""
+
     fixtures = ['users.json', 'questions.json']
 
     def setUp(self):
@@ -21,6 +19,12 @@ def setUp(self):
         q = Question.objects.get(pk=1)
         q.last_answer_id = 1
         q.save()
-        
+
         self.client = client.Client()
         self.client.get('/')
+
+
+class TaggingTestCaseBase(TestCaseBase):
+    """Base testcase with additional setup for testing tagging"""
+
+    fixtures = TestCaseBase.fixtures + ['taggit.json']

apps/questions/tests/test_forms.py

@@ -17,7 +17,7 @@ def test_metadata_keys(self):
         expected = ['useragent']
         actual = form.metadata_field_keys
         eq_(expected, actual)
-        
+
         # Test the form with a product
         product = {'key': 'desktop',
                    'name': 'Firefox on desktop',
@@ -52,15 +52,15 @@ def test_cleaned_metadata(self):
         expected = {}
         actual = form.cleaned_metadata
         eq_(expected, actual)
-        
+
         # Test with metadata
         data['os'] = u'Linux'
         form = NewQuestionForm(product=product, data=data)
         form.is_valid()
         expected = {'os': u'Linux'}
         actual = form.cleaned_metadata
         eq_(expected, actual)
-        
+
         # Add an empty metadata value
         data['ff_version'] = u''
         form = NewQuestionForm(product=product, data=data)

apps/questions/tests/test_models.py

@@ -1,7 +1,11 @@
-from nose.tools import eq_
+from nose.tools import eq_, raises
+
+import sumo.models
+from taggit.models import Tag
 
 from questions.models import Question, QuestionMetaData, Answer
-from questions.tests import TestCaseBase
+from questions.tags import add_existing_tag
+from questions.tests import TestCaseBase, TaggingTestCaseBase
 
 
 class TestAnswer(TestCaseBase):
@@ -65,17 +69,59 @@ def test_metadata_property(self):
         eq_('1234567890', self.question.metadata['crash_id'])
 
 
-class TestQuestionSave(TestCaseBase):
-    """Tests the Question.save() method."""
+class QuestionTests(TestCaseBase):
+    """Tests for Question model"""
 
-    def test_updated(self):
+    def test_save_updated(self):
+        """Make sure saving updates the `updated` field."""
         q = Question.objects.all()[0]
         updated = q.updated
         q.save()
         self.assertNotEqual(updated, q.updated)
 
-    def test_no_update(self):
+    def test_save_no_update(self):
+        """Saving with the `no_update` option shouldn't update `updated`."""
         q = Question.objects.all()[0]
         updated = q.updated
         q.save(no_update=True)
         eq_(updated, q.updated)
+
+    def test_default_manager(self):
+        """Assert Question's default manager is SUMO's ManagerBase.
+
+        This is easy to get wrong with taggability.
+
+        """
+        eq_(Question._default_manager.__class__, sumo.models.ManagerBase)
+
+
+class AddExistingTagTests(TaggingTestCaseBase):
+    """Tests for the add_existing_tag helper function."""
+
+    def setUp(self):
+        super(AddExistingTagTests, self).setUp()
+        self.untagged_question = Question.objects.get(pk=1)
+
+    def test_tags_manager(self):
+        """Make sure the TaggableManager exists.
+
+        Full testing of functionality is a matter for taggit's tests.
+
+        """
+        _tags_eq_(self.untagged_question, [])
+
+    def test_add_existing_case_insensitive(self):
+        """Assert add_existing_tag works case-insensitively."""
+        add_existing_tag('LEMON', self.untagged_question.tags)
+        _tags_eq_(self.untagged_question, [u'lemon'])
+
+    @raises(Tag.DoesNotExist)
+    def test_add_existing_no_such_tag(self):
+        """Assert add_existing_tag doesn't work when the tag doesn't exist."""
+        add_existing_tag('nonexistent tag', self.untagged_question.tags)
+
+
+def _tags_eq_(tagged_object, tag_names):
+    """Assert that the names of the tags on tagged_object are tag_names."""
+    eq_(sorted([t.name for t in tagged_object.tags.all()]),
+        sorted(tag_names))