Merge lp:~fabricematrat/charmworld/redirect-charm into lp:charmworld

[Fabrice Matrat]

Add a redirect for charms
R=bac, jcsackett

Add a redirect for charms

[Brad Crittenden (bac)]

Looks good Fabrice, thanks for the branch. A few suggestions.

review: Approve (code)

[j.c.sackett (jcsackett)]

Fabrice--

This looks alright. I have some concerns about the redirects, but I don't think there's a better solution at this time--possibly something worth filing issues against jujucharms.com for?

[j.c.sackett (jcsackett)]

review: Approve

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

[Brad Crittenden (bac)]

Even better

review: Approve (code)

=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2014-04-02 14:12:22 +0000
+++ charmworld/views/charms.py	2015-02-09 18:46:28 +0000
@@ -9,6 +9,7 @@
 import json
 import os
 import pymongo
+from pyramid.httpexceptions import HTTPMovedPermanently
 from pyramid.view import view_config
 from webob import Response
 
@@ -36,7 +37,6 @@
     found,
     interfaces,
     name_filter,
-    sub_filter,
 )
 
 
@@ -246,42 +246,42 @@
     route_name="charm-collection",
     renderer="charmworld:templates/charm-collection.pt")
 def charm_collection(request):
-    query, sub_only = sub_filter({}, request)
-    return found_charm_collection(
-        request.db, query, sub_only, only_promulgated=True)
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    location = ('{url}/solutions'.format(url=redirect_url))
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
     route_name="personal-collection",
     renderer="charmworld:templates/charm-collection.pt")
 def personal_collection(request):
-    query, sub_only = sub_filter({
-        "owner": request.matchdict["owner"]
-    }, request)
-    return found_charm_collection(request.db, query, sub_only)
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    location = ('{url}/q/{owner}'.format(url=redirect_url,
+                                         owner=request.matchdict["owner"]))
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
     route_name="series-collection",
     renderer="charmworld:templates/charm-collection.pt")
 def series_collection(request):
-    query, sub_only = sub_filter(
-        {"series": request.matchdict["series"]}, request)
-    return found_charm_collection(
-        request.db, query, sub_only, only_promulgated=True)
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    location = ('{url}/q/{series}'.format(url=redirect_url,
+                                          series=request.matchdict["series"]))
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
     route_name="personal-series",
     renderer="charmworld:templates/charm-collection.pt")
 def personal_series_collection(request):
-    query, sub_only = sub_filter(
-        {
-            "owner": request.matchdict["owner"],
-            "series": request.matchdict["series"]
-        },
-        request)
-    return found_charm_collection(request.db, query, sub_only)
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    location = ('{url}/q/{owner}/{series}'.format(
+        url=redirect_url,
+        owner=request.matchdict["owner"],
+        series=request.matchdict["series"])
+    )
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
@@ -292,7 +292,18 @@
     renderer="charmworld:templates/charm.pt")
 def personal_charm(request):
     _reconcile_revision(request)
-    return _charm_view(request, find_charm(request))
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    match = request.matchdict
+    location = '{url}/u/{owner}/{charm}/{series}'.format(
+        url=redirect_url,
+        owner=match['owner'],
+        charm=match['charm'],
+        series=match['series']
+    )
+    if 'revision' in match:
+        location = '{location}/{revision}'.format(location=location,
+                                                  revision=match['revision'])
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
@@ -316,7 +327,17 @@
     renderer="charmworld:templates/charm.pt")
 def distro_charm(request):
     _reconcile_revision(request)
-    return _charm_view(request, find_charm(request, promulgated=True))
+    redirect_url = request.registry.settings.get('redirect_jujucharms')
+    match = request.matchdict
+    location = '{url}/{charm}/{series}'.format(
+        url=redirect_url,
+        charm=match['charm'],
+        series=match['series']
+    )
+    if 'revision' in match:
+        location = '{location}/{revision}'.format(location=location,
+                                                  revision=match['revision'])
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(

=== modified file 'charmworld/views/tests/test_charms.py'
--- charmworld/views/tests/test_charms.py	2014-04-02 14:12:22 +0000
+++ charmworld/views/tests/test_charms.py	2015-02-09 18:46:28 +0000
@@ -4,6 +4,8 @@
 from datetime import date
 from logging import getLogger
 
+from pyramid.httpexceptions import HTTPMovedPermanently
+
 from charmworld.jobs.ingest import (
     add_files,
 )
@@ -131,189 +133,30 @@
 
     def test_charm_collection_basic(self):
         """Simple sanity check."""
-        one_id, one = factory.makeCharm(self.db, promulgated=True)
-        two_id, two = factory.makeCharm(self.db, promulgated=True, owner='foo')
-        three_id, three = factory.makeCharm(self.db, promulgated=False)
-
-        response = charm_collection(self.getRequest())
-        charm_names = set(charm.name for charm in response['charms'])
-        expected = set((one['name'], two['name']))
-        self.assertEqual(expected, charm_names)
-
-    def test_charm_collection_sub_filter(self):
-        """With the subordinate filter we limit out the non-sub charms."""
-        one_id, one = factory.makeCharm(
-            self.db, subordinate=True, promulgated=True)
-        two_id, two = factory.makeCharm(self.db, promulgated=True)
-
-        request = self.getRequest()
-        request.GET['sub'] = 'true'
-        response = charm_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
-
-    def test_charm_collection_charms_with_errors(self):
-        """Charms with errors are not returned by charm_collection()."""
-        one_id, one = factory.makeCharm(self.db, promulgated=True)
-        two_id, two = factory.makeCharm(
-            self.db, charm_error=True, promulgated=True)
-        response = charm_collection(self.getRequest())
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
+        with self.assertRaises(HTTPMovedPermanently) as e:
+            charm_collection(self.getRequest())
+        self.assertIn('/solutions', e.exception.location)
 
     def test_personal_collection(self):
-        # personal_collection() returns only charms owned by the given
-        # person.
-        one_id, one = factory.makeCharm(self.db, owner='foo')
-        two_id, two = factory.makeCharm(self.db, owner='bar')
-        request = self.getRequest()
-        request.matchdict = {'owner': 'foo'}
-        response = personal_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual('foo', charms[0].owner)
-
-    def test_personal_collection_sub_filter(self):
-        # If the request parameter 'sub' is set, only subordinate charms
-        # are returned.
-        one_id, one = factory.makeCharm(self.db, owner='foo')
-        two_id, two = factory.makeCharm(self.db, owner='bar')
-        three_id, three = factory.makeCharm(
-            self.db, owner='foo', subordinate=True)
-        request = self.getRequest()
-        request.GET['sub'] = 'true'
-        request.matchdict = {'owner': 'foo'}
-        response = personal_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(three['name'], charms[0].name)
-        # If the filter is no given in the request, all charms are returned.
-        request = self.getRequest()
-        request.matchdict = {'owner': 'foo'}
-        response = personal_collection(request)
-        charms = response['charms']
-        self.assertEqual(2, len(charms))
-        self.assertEqual('foo', charms[0].owner)
-        self.assertEqual('foo', charms[1].owner)
-
-    def test_personal_collection_charms_with_errors(self):
-        # Charms with errors are not returned by personal_collection().
-        one_id, one = factory.makeCharm(self.db, owner='foo')
-        two_id, two = factory.makeCharm(self.db, owner='bar')
-        three_id, three = factory.makeCharm(
-            self.db, owner='foo', charm_error=True)
-        request = self.getRequest()
-        request.matchdict = {'owner': 'foo'}
-        response = personal_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
+        request = self.getRequest()
+        request.matchdict = {'owner': 'foo'}
+        with self.assertRaises(HTTPMovedPermanently) as e:
+            personal_collection(request)
+        self.assertIn('/q/foo', e.exception.location)
 
     def test_series_collection(self):
-        # series_collection() returns only promulgated charms for a
-        # given series.
-        one_id, one = factory.makeCharm(
-            self.db, series='one', promulgated=True, name='charm-a')
-        two_id, two = factory.makeCharm(
-            self.db, series='two', promulgated=True)
-        three_id, three = factory.makeCharm(
-            self.db, series='one')
-        four_id, four = factory.makeCharm(
-            self.db, series='one', promulgated=True, owner='foo',
-            name='charm-b')
-        request = self.getRequest()
-        request.matchdict = {'series': 'one'}
-        response = series_collection(request)
-        charms = response['charms']
-        self.assertEqual(
-            ['charm-a', 'charm-b'], [charm.name for charm in charms])
-        self.assertEqual('one', charms[0].series)
-
-    def test_series_collection_sub_filter(self):
-        # If the request parameter 'sub' is set, only subordinate charms
-        # are returned.
-        one_id, one = factory.makeCharm(
-            self.db, series='one', promulgated=True)
-        two_id, two = factory.makeCharm(
-            self.db, series='two', promulgated=True)
-        three_is, three = factory.makeCharm(
-            self.db, series='one', subordinate=True, promulgated=True)
-        request = self.getRequest()
-        request.matchdict = {'series': 'one'}
-        request.GET['sub'] = 'true'
-        response = series_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(three['name'], charms[0].name)
-
-    def test_series_collection_charms_with_errors(self):
-        # Charms with errors are not returned by series_collection().
-        one_id, one = factory.makeCharm(
-            self.db, series='one', promulgated=True)
-        two_id, two = factory.makeCharm(
-            self.db, series='two', promulgated=True)
-        three_is, three = factory.makeCharm(
-            self.db, series='one', charm_error=True, promulgated=True)
-        request = self.getRequest()
-        request.matchdict = {'series': 'one'}
-        response = series_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
+        request = self.getRequest()
+        request.matchdict = {'series': 'one'}
+        with self.assertRaises(HTTPMovedPermanently) as e:
+            series_collection(request)
+        self.assertIn('/q/one', e.exception.location)
 
     def test_personal_series_collection(self):
-        # personal_series_collection() returns only charms for a given
-        # series and person.
-        one_id, one = factory.makeCharm(self.db, series='one', owner='foo')
-        two_id, two = factory.makeCharm(self.db, series='two', owner='foo')
-        three_id, three = factory.makeCharm(self.db, series='one', owner='bar')
-        request = self.getRequest()
-        request.matchdict = {
-            'owner': 'foo',
-            'series': 'one',
-        }
-        response = personal_series_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
-
-    def test_personal_series_collection_sub_filter(self):
-        # If the parameter 'sub' is set, personal_series_collection()
-        # returns only subordinate charms for the given person ans series.
-        one_id, one = factory.makeCharm(self.db, series='one', owner='foo')
-        two_id, two = factory.makeCharm(self.db, series='two', owner='foo')
-        three_id, three = factory.makeCharm(self.db, series='one', owner='bar')
-        four_id, four = factory.makeCharm(
-            self.db, series='one', owner='foo', subordinate=True)
-        request = self.getRequest()
-        request.matchdict = {
-            'owner': 'foo',
-            'series': 'one',
-        }
-        request.GET['sub'] = 'true'
-        response = personal_series_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(four['name'], charms[0].name)
-
-    def test_personal_series_collection_charms_with_errors(self):
-        # Charms with errors are not returned by series_collection().
-        one_id, one = factory.makeCharm(self.db, series='one', owner='foo')
-        two_id, two = factory.makeCharm(self.db, series='two', owner='foo')
-        three_id, three = factory.makeCharm(self.db, series='one', owner='bar')
-        four_id, four = factory.makeCharm(
-            self.db, series='one', owner='foo', charm_error=True)
-        request = self.getRequest()
-        request.matchdict = {
-            'owner': 'foo',
-            'series': 'one',
-        }
-        response = personal_series_collection(request)
-        charms = response['charms']
-        self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0].name)
+        request = self.getRequest()
+        request.matchdict = {'series': 'one', 'owner': 'me'}
+        with self.assertRaises(HTTPMovedPermanently) as e:
+            personal_series_collection(request)
+        self.assertIn('/q/me/one', e.exception.location)
 
     def test_interface(self):
         # interface() returns all charms requiring or providing a given
@@ -429,12 +272,8 @@
             'charm': 'sample_charm',
             'series': 'precise',
         }
-        response = distro_charm(request)
-        expected_keys = set((
-            'charm', 'is_featured', 'format_change', 'format_proof', 'name',
-            'others', 'project', 'qadata', 'readme', 'readme_format',
-            'icon_path'))
-        self.assertEqual(expected_keys, set(response))
+        with self.assertRaises(HTTPMovedPermanently):
+            distro_charm(request)
 
     def test_distro_charm_json(self):
         ignore, charm = factory.makeCharm(
@@ -542,9 +381,8 @@
         two_id, two = factory.makeCharm(self.db, promulgated=True)
         del two['summary']
         self.db.charms.save(two)
-        # Test that the route works.
-        resp = self.app.get('/charms/precise', status=200)
-        self.assertIn(two['short_url'], resp.body)
+        response = self.app.get('/charms/precise', status=301)
+        self.assertIn('/q/precise', response.body)
 
     def test_route_personal_charm_with_revision(self):
         # The route /~owner/series/charm-1 finds charms using owner,
@@ -552,8 +390,8 @@
         # does not include the revision.
         ignore, charm = factory.makeCharm(
             self.db, owner='owner', series='series', name='charm', files={})
-        response = self.app.get('/~owner/series/charm-1', status=200)
-        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+        response = self.app.get('/~owner/series/charm-1', status=301)
+        self.assertIn('/u/owner/charm/series/1', response.body)
 
     def test_route_personal_charm_with_dash_and_revision(self):
         # /~owner/series/charm-name-1 route find the charm.
@@ -562,16 +400,17 @@
         ignore, charm = factory.makeCharm(
             self.db, owner='owner', series='series', name='charm-name',
             files={})
-        response = self.app.get('/~owner/series/charm-name-1', status=200)
-        self.assertIn('juju deploy cs:~owner/series/charm-name', response.body)
+        response = self.app.get('/~owner/series/charm-name-1', status=301)
+        self.assertIn('/u/owner/charm-name/series/1', response.body)
 
     def test_route_personal_charm_with_head_revision(self):
         # the /~owner/series/charm-name-HEAD route finds the charm.
         # The juju-gui apache rewrite versionless charm urls to help the GUI.
         ignore, charm = factory.makeCharm(
             self.db, owner='owner', series='series', name='charm', files={})
-        response = self.app.get('/~owner/series/charm-HEAD', status=200)
-        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+        response = self.app.get('/~owner/series/charm-HEAD', status=301)
+        self.assertIn('/u/owner/charm/series', response.body)
+        self.assertNotIn('HEAD', response.body)
 
     def test_route_personal_charm_without_revision(self):
         # The route /~owner/series/charm route finds the charm using owner
@@ -579,8 +418,8 @@
         # matches the route.
         ignore, charm = factory.makeCharm(
             self.db, owner='owner', series='series', name='charm', files={})
-        response = self.app.get('/~owner/series/charm', status=200)
-        self.assertIn('juju deploy cs:~owner/series/charm', response.body)
+        response = self.app.get('/~owner/series/charm', status=301)
+        self.assertIn('/u/owner/charm/series', response.body)
 
     def test_route_personal_charm_json_with_revision(self):
         # The route /~owner/series/charm-1/json finds charms using owner,
@@ -604,10 +443,10 @@
         # The route /series/charm-1 route finds promulgated charms using
         # The series, charm name and version. The deploy instruction does not
         # include the version.
-        ignore, charm = factory.makeCharm(
+        factory.makeCharm(
             self.db, promulgated=True, series='series', name='charm', files={})
-        response = self.app.get('/series/charm-1', status=200)
-        self.assertIn('juju deploy cs:series/charm', response.body)
+        response = self.app.get('/series/charm-1', status=301)
+        self.assertIn('/charm/series/1', response.body)
 
     def test_route_promulgated_charm_with_head_revision(self):
         # The route /series/charm-HEAD finds promulgated charms using
@@ -615,22 +454,22 @@
         # in the deploy instruction.
         ignore, charm = factory.makeCharm(
             self.db, promulgated=True, series='series', name='charm', files={})
-        response = self.app.get('/series/charm-HEAD', status=200)
-        self.assertIn('juju deploy cs:series/charm', response.body)
+        response = self.app.get('/series/charm-HEAD', status=301)
+        self.assertNotIn('HEAD', response.body)
+        self.assertIn('/charm/series', response.body)
 
     def test_route_promulgated_charm_without_revision(self):
         # The route /series/charm finds the promulgated charms using just
         # the series and charm name. The deploy instruction matches the route.
-        ignore, charm = factory.makeCharm(
+        factory.makeCharm(
             self.db, promulgated=True, series='series', name='charm', files={})
-        response = self.app.get('/series/charm', status=200)
-        self.assertIn('juju deploy cs:series/charm', response.body)
+        self.app.get('/series/charm', status=301)
 
     def test_charm_short_url_route(self):
         ignore, charm_data = factory.makeCharm(
             self.db, promulgated=True, series='series', name='charm', files={})
         charm = Charm(charm_data)
-        self.app.get(charm.short_url, status=200)
+        self.app.get(charm.short_url, status=301)
 
 
 class TestQAForm(ViewTestBase):

=== modified file 'default.ini'
--- default.ini	2014-01-14 14:37:56 +0000
+++ default.ini	2015-02-09 18:46:28 +0000
@@ -66,7 +66,10 @@
 run_test_builds = false
 charm_test_url = 
 charm_test_token = 
-charm_test_job = 
+charm_test_job =
+
+# New web site for redirection
+redirect_jujucharms = https://jujucharms.com
 
 [server:main]
 use = egg:Paste#http