Merge pull request #242 from lmorchard/simplify-wiki-urls-754534

bug 754534: Simplify wiki URLs

apps/devmo/helpers.py

@@ -77,16 +77,20 @@ def devmo_url(context, path):
         Look for a wiki page in the current locale first,
         then default to given path
     """
+    if not settings.DEKIWIKI_ENDPOINT:
+        # HACK: If MindTouch is unavailable, skip the rest of this and lean on
+        # locale processing redirects to resolve things. Might be interesting
+        # to resolve some of the redirects first, and come up with the ultimate
+        # real URL. See bug 759356 for followup.
+        path = path.replace('/en', '')
+        return '/%s/docs%s' % (context['request'].locale, path)
+
     # HACK: If DEKIWIKI_MOCK is True, just skip hitting the API. This can speed
     # up a lot of tests without adding decorators, and should never be true in
     # production.
     if getattr(settings, 'DEKIWIKI_MOCK', False):
         return path
 
-    if not settings.DEKIWIKI_ENDPOINT:
-        # If MindTouch is unavailable, skip the rest of this
-        return path
-
     current_locale = context['request'].locale
     m = get_locale_path_match(path)
     if not m:

apps/devmo/tests/test_misc.py

@@ -115,7 +115,7 @@ def test_devmo_url(self):
 
     @attr('current')
     @mock.patch('devmo.helpers.check_devmo_local_page')
-    def test_devmo_url_midtouch_disabled(self, mock_check_devmo_local_page):
+    def test_devmo_url_mindtouch_disabled(self, mock_check_devmo_local_page):
         _old = settings.DEKIWIKI_ENDPOINT
         settings.DEKIWIKI_ENDPOINT = False
 
@@ -132,11 +132,8 @@ def my_check_devmo_local_page(username, password, force=False):
         req = test_utils.RequestFactory().get('/')
         context = {'request': req}
 
-        # NOTE: This is undesirable behavior, but expected. Since devmo_url can
-        # no longer consult MindTouch, it will punt and claim that /en/HTML is
-        # the proper path to the de locale page. See also, bug 759356
         req.locale = 'de'
-        eq_(devmo_url(context, localized_page), '/en/HTML')
+        eq_(devmo_url(context, localized_page), '/de/docs/HTML')
 
         ok_(not trap['was_called'])
         settings.DEKIWIKI_ENDPOINT = _old

apps/wiki/models.py

@@ -176,7 +176,7 @@
     'tag/[^/]+'
 )
 
-DOCUMENT_LAST_MODIFIED_CACHE_KEY_TMPL = 'kuma:document-last-modified:%s'
+DOCUMENT_LAST_MODIFIED_CACHE_KEY_TMPL = u'kuma:document-last-modified:%s'
 
 
 class UniqueCollision(Exception):
@@ -425,6 +425,11 @@ class Document(NotificationsMixin, ModelBase):
     def natural_key(self):
         return (self.locale, self.slug,)
 
+    @property
+    def natural_cache_key(self):
+        nk = u'/'.join(self.natural_key())
+        return hashlib.md5(nk.encode('utf8')).hexdigest()
+
     class Meta(object):
         unique_together = (('parent', 'locale'), ('slug', 'locale'))
         permissions = (
@@ -549,8 +554,8 @@ def save(self, *args, **kwargs):
         super(Document, self).save(*args, **kwargs)
 
         # Delete any cached last-modified timestamp.
-        path_hash = hashlib.md5(self.full_path).hexdigest()
-        cache_key = DOCUMENT_LAST_MODIFIED_CACHE_KEY_TMPL % path_hash
+        cache_key = (DOCUMENT_LAST_MODIFIED_CACHE_KEY_TMPL %
+                     self.natural_cache_key)
         cache.delete(cache_key)
 
         # Make redirects if there's an approved revision and title or slug
@@ -620,8 +625,11 @@ def language(self):
 
     @property
     def full_path(self):
-        """The full path of a document consists of {locale}/{slug}"""
-        return '%s/%s' % (self.locale, self.slug)
+        """The full path of a document consists of {slug}"""
+        # TODO: See about removing this and all references to full_path? Once
+        # upon a time, this was composed of {locale}/{slug}, but bug 754534
+        # reverted that.
+        return self.slug
 
     def get_absolute_url(self, ui_locale=None):
         """Build the absolute URL to this document from its full path"""
@@ -632,36 +640,38 @@ def get_absolute_url(self, ui_locale=None):
 
     @staticmethod
     def locale_and_slug_from_path(path, request=None):
-        """Given a docs URL path, derive locale and slug for model lookup, and
-        a suggestion of whether this path deserves a redirect"""
+        """Given a proposed doc path, try to see if there's a legacy MindTouch
+        locale or even a modern Kuma domain in the path. If so, signal for a
+        redirect to a more canonical path. In any case, produce a locale and
+        slug derived from the given path."""
         locale, slug, needs_redirect = '', path, False
+        mdn_languages_lower = dict((x.lower(), x)
+                                   for x in settings.MDN_LANGUAGES)
 
-        # If there's a slash in the path, then the first segment is likely to
-        # be the locale.
+        # If there's a slash in the path, then the first segment could be a
+        # locale. And, that locale could even be a legacy MindTouch locale.
         if '/' in path:
-            locale, slug = path.split('/', 1)
+            maybe_locale, maybe_slug = path.split('/', 1)
+            l_locale = maybe_locale.lower()
 
-            if locale.lower() in settings.MT_TO_KUMA_LOCALE_MAP:
-                # If this looks like a MindTouch locale, remap it.
-                old_locale = locale
-                locale = settings.MT_TO_KUMA_LOCALE_MAP[locale.lower()]
-                # But, we only need a redirect if the locale actually changed.
-                needs_redirect = (locale != old_locale)
+            if l_locale in settings.MT_TO_KUMA_LOCALE_MAP:
+                # The first segment looks like a MindTouch locale, remap it.
+                needs_redirect = True
+                locale = settings.MT_TO_KUMA_LOCALE_MAP[l_locale]
+                slug = maybe_slug
 
-            if locale not in settings.MDN_LANGUAGES:
-                # Oops, that doesn't look like a supported locale, so back out.
+            elif l_locale in mdn_languages_lower:
+                # The first segment looks like an MDN locale, redirect.
                 needs_redirect = True
-                locale, slug = '', path
+                locale = mdn_languages_lower[l_locale]
+                slug = maybe_slug
 
-        # No locale by this point? Go with the locale detected from user agent
-        # if we have a request.
+        # No locale yet? Try the locale detected by the request.
         if locale == '' and request:
-            needs_redirect = True
             locale = request.locale
 
-        # Still no locale? Go with the site default locale.
+        # Still no locale? Probably no request. Go with the site default.
         if locale == '':
-            needs_redirect = True
             locale = getattr(settings, 'WIKI_DEFAULT_LANGUAGE', 'en-US')
 
         return (locale, slug, needs_redirect)

apps/wiki/templates/wiki/list_documents.html

@@ -33,7 +33,10 @@ <h1>{{ title }}</h1>
         {% if documents.object_list %}
           <ul class="documents cols-3">
             {% for doc in documents.object_list %}
-              <li><a href="{{ doc.get_absolute_url() }}">{{ doc.title }}</a></li>
+              <li>
+                <a href="{{ doc.get_absolute_url() }}">{{ doc.title }}</a> ({{ doc.locale }})<br/>
+                <small>{{ doc.get_absolute_url() }}</small>
+              </li>
             {% endfor %}
           </ul>
           {{ documents|paginator }}

apps/wiki/tests/test_templates.py

@@ -998,21 +998,21 @@ def setUp(self):
     def test_translate_GET_logged_out(self):
         """Try to create a translation while logged out."""
         self.client.logout()
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         eq_(302, response.status_code)
-        expected_url = '%s?next=%s' % (reverse('users.login', locale='en-US'),
+        expected_url = '%s?next=%s' % (reverse('users.login', locale='es'),
                                        translate_uri)
         ok_(expected_url in response['Location'])
 
     def test_translate_GET_with_perm(self):
         """HTTP GET to translate URL renders the form."""
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         eq_(200, response.status_code)
@@ -1025,18 +1025,18 @@ def test_translate_disallow(self):
         """HTTP GET to translate URL returns 400 when not localizable."""
         self.d.is_localizable = False
         self.d.save()
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         eq_(400, response.status_code)
 
     def test_invalid_document_form(self):
         """Make sure we handle invalid document form without a 500."""
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         data = _translation_data()
         data['slug'] = ''  # Invalid slug
@@ -1046,9 +1046,9 @@ def test_invalid_document_form(self):
     def test_invalid_revision_form(self):
         """When creating a new translation, an invalid revision form shouldn't
         result in a new Document being created."""
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         data = _translation_data()
         data['content'] = ''  # Content is required
@@ -1064,9 +1064,9 @@ def test_first_translation_to_locale(self, get_current, edited_fire,
         """Create the first translation of a doc to new locale."""
         get_current.return_value.domain = 'testserver'
 
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         data = _translation_data()
         response = self.client.post(translate_uri, data)
@@ -1110,9 +1110,9 @@ def test_another_translation_to_locale(self, get_current, edited_fire,
         rev_enUS.save()
 
         # Verify the form renders with correct content
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         doc = pq(response.content)
@@ -1124,7 +1124,7 @@ def test_another_translation_to_locale(self, get_current, edited_fire,
         data['content'] = 'loremo ipsumo doloro sito ameto nuevo'
         response = self.client.post(translate_uri, data)
         eq_(302, response.status_code)
-        eq_('http://testserver/en-US/docs/es/un-test-articulo',
+        eq_('http://testserver/es/docs/un-test-articulo',
             response['location'])
         doc = Document.objects.get(slug=data['slug'])
         rev = doc.revisions.filter(content=data['content'])[0]
@@ -1149,17 +1149,17 @@ def test_translate_update_doc_only(self):
         """Submitting the document form should update document. No new
         revisions should be created."""
         rev_es = self._create_and_approve_first_translation()
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         data = _translation_data()
         new_title = 'Un nuevo titulo'
         data['title'] = new_title
         data['form'] = 'doc'
         response = self.client.post(translate_uri, data)
         eq_(302, response.status_code)
-        eq_('http://testserver/en-US/docs/es/un-test-articulo$edit'
+        eq_('http://testserver/es/docs/un-test-articulo$edit'
             '?opendescription=1',
             response['location'])
         revisions = rev_es.document.revisions.all()
@@ -1171,17 +1171,17 @@ def test_translate_update_rev_and_doc(self):
         """Submitting the revision form should create a new revision.
         And since Kuma docs default to approved, should update doc too."""
         rev_es = self._create_and_approve_first_translation()
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         data = _translation_data()
         new_title = 'Un nuevo titulo'
         data['title'] = new_title
         data['form'] = 'rev'
         response = self.client.post(translate_uri, data)
         eq_(302, response.status_code)
-        eq_('http://testserver/en-US/docs/es/un-test-articulo',
+        eq_('http://testserver/es/docs/un-test-articulo',
             response['location'])
         revisions = rev_es.document.revisions.all()
         eq_(2, revisions.count())  # New revision is created
@@ -1192,9 +1192,9 @@ def test_translate_form_content_fallback(self):
         """If there are existing but unapproved translations, prefill
         content with latest."""
         self.test_first_translation_to_locale()
-        translate_path = 'es/' + self.d.slug
+        translate_path = self.d.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         doc = pq(response.content)
@@ -1213,9 +1213,9 @@ def test_translate_based_on(self):
         d = Document.objects.get(pk=base_rev.document.id)
         eq_(r, base_rev.document.current_revision)
 
-        translate_path = 'es/' + d.slug
+        translate_path = d.slug
         uri = urllib.quote(reverse('wiki.new_revision_based_on',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path,
                                                    base_rev.id]))
         response = self.client.get(uri)
@@ -1229,9 +1229,9 @@ def test_translate_rejected_parent(self):
         en_revision = revision(is_approved=False, save=True, reviewer=user,
                                reviewed=datetime.now())
 
-        translate_path = 'es/' + en_revision.document.slug
+        translate_path = en_revision.document.slug
         translate_uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale='es',
                                              args=[translate_path]))
         response = self.client.get(translate_uri)
         doc = pq(response.content)
@@ -1246,9 +1246,9 @@ def _test_form_maintains_based_on_rev(client, doc, view, post_data,
     form was first loaded, even if other revisions have been approved in the
     meantime."""
     if trans_lang:
-        translate_path = trans_lang + '/' + doc.slug
+        translate_path = doc.slug
         uri = urllib.quote(reverse('wiki.translate',
-                                             locale='en-US',
+                                             locale=trans_lang,
                                              args=[translate_path]))
     else:
         uri = reverse(view, locale=locale, args=[doc.full_path])