Merge lp:~bac/charmworld/bundle-redirect into lp:charmworld

[Brad Crittenden]

Redirect bundles.

Redirect bundles to jujucharms.com

QA: a bit hard as getting bundles to load that are visible in jujucharms.com is difficult since 1) many bundles name a specific version of the charm and that version may not be ingestable, 2) jujucharms.com does not seem to have a complete set of available bundles. (?)

But if you can find a local bundle that has a corresponding one in jujucharms.com, test that the redirect works.

In all honesty, I'm not going to feel 100% about this branch until we get it on staging and can do very thorough QA.

[Francesco Banconi (frankban)]

Looks good to me with minor suggestions.
Thank you!

review: Approve

[Fabrice Matrat (fabricematrat)]

I just wonder if redirecting to a search in jujucharms.com won't be safer ??

[Brad Crittenden (bac)]

QA:
# Personal bundle
http://127.0.0.1:2464/bundle/~a.rosales/storm/instantstorm
# Personal bundle with revision
http://127.0.0.1:2464/bundle/~a.rosales/storm/5/instantstorm
# Official bundle
http://127.0.0.1:2464/bundle/hadoop/7/cluster

[Fabrice Matrat (fabricematrat)]

Working great, did browse a few bundles and get redirected.
Code LGTM.

review: Approve

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

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~bac/charmworld/bundle-redirect/+merge/249420/+edit-commit-message

review: Needs Fixing (continuous-integration)

[Brad Crittenden (bac)]

Kick CI

review: Approve (code)

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

review: Approve (continuous-integration)

=== modified file 'charmworld/__init__.py'
--- charmworld/__init__.py	2013-11-14 13:38:24 +0000
+++ charmworld/__init__.py	2015-02-18 14:26:31 +0000
@@ -59,12 +59,12 @@
 
             if 'http_cache' not in settings:
                 http_cache = app_settings.get('http_cache')
-                if http_cache is not None:
+                if http_cache:
                     http_cache = int(http_cache)
                 settings['http_cache'] = http_cache
             if 'http_cache_modifier' in settings:
                 mod = settings['http_cache_modifier']
-                if settings['http_cache'] is not None:
+                if settings['http_cache']:
                     settings['http_cache'] = int(settings['http_cache'] * mod)
                 # We can't let the new setting to get add_view or it blows up
                 # with an unexpected kwarg.

=== modified file 'charmworld/jobs/config.py'
--- charmworld/jobs/config.py	2013-10-10 11:27:55 +0000
+++ charmworld/jobs/config.py	2015-02-18 14:26:31 +0000
@@ -7,12 +7,14 @@
 import shutil
 import tempfile
 
+from pyramid.settings import asbool
+
 from charmworld.utils import get_ini
 
 
 settings = get_ini()
 
-if settings.get('is_testing', 'false').lower() == 'true':
+if asbool(settings.get('is_testing')):
     CHARM_DIR = tempfile.mkdtemp()
     atexit.register(shutil.rmtree, CHARM_DIR, ignore_errors=True)
 else:

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2014-05-15 15:57:23 +0000
+++ charmworld/testing/factory.py	2015-02-18 14:26:31 +0000
@@ -375,10 +375,7 @@
 
 
 def make_bundle(db, *args, **kwargs):
-    with_basket = False
-    if 'with_basket' in kwargs:
-        with_basket = True
-        del kwargs['with_basket']
+    with_basket = kwargs.pop('with_basket', False)
 
     bundle_data = get_bundle_data(*args, **kwargs)
     bundle = Bundle(bundle_data)

=== modified file 'charmworld/views/bundles.py'
--- charmworld/views/bundles.py	2014-03-20 14:00:11 +0000
+++ charmworld/views/bundles.py	2015-02-18 14:26:31 +0000
@@ -5,6 +5,8 @@
 
 import os.path
 
+from pyramid.httpexceptions import HTTPMovedPermanently
+
 from charmworld import cached_view_config
 from charmworld.models import (
     Bundle,
@@ -15,8 +17,8 @@
 from charmworld.views.api import json_response
 from charmworld.views.helpers import (
     find_bundle,
-    found,
     format_change,
+    redirect_url,
 )
 
 
@@ -123,6 +125,14 @@
     )
 
 
+def _bundle_search_redirect(request):
+    """Return the redirect search location."""
+    return '{url}/q/{basket}?type=bundle'.format(
+        url=redirect_url(request),
+        basket=request.matchdict['basket'],
+    )
+
+
 @cached_view_config(
     route_name="personal-bundle-revision",
     renderer="charmworld:templates/bundle.pt")
@@ -130,8 +140,10 @@
     route_name="personal-bundle",
     renderer="charmworld:templates/bundle.pt")
 def personal_bundle(request):
-    bundle = find_bundle(request)
-    return _bundle_view(request, found(bundle))
+    # For simplicity and safety, all bundles will redirect to a search on the
+    # basket name.
+    location = _bundle_search_redirect(request)
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(
@@ -159,8 +171,10 @@
     route_name="official-bundle-revision",
     renderer="charmworld:templates/bundle.pt")
 def official_bundle(request):
-    bundle = find_bundle(request, promulgated=True)
-    return _bundle_view(request, found(bundle))
+    # For simplicity and safety, all bundles will redirect to a search on the
+    # basket name.
+    location = _bundle_search_redirect(request)
+    raise HTTPMovedPermanently(location=location)
 
 
 @cached_view_config(

=== modified file 'charmworld/views/helpers.py'
--- charmworld/views/helpers.py	2014-02-10 21:30:20 +0000
+++ charmworld/views/helpers.py	2015-02-18 14:26:31 +0000
@@ -161,3 +161,8 @@
     else:
         spec["owner"] = request.matchdict['owner']
     return Bundle.from_query(spec, request.db)
+
+
+def redirect_url(request):
+    """Get the redirect_jujucharms configuration setting."""
+    return request.registry.settings.get('redirect_jujucharms')

=== modified file 'charmworld/views/tests/test_bundles.py'
--- charmworld/views/tests/test_bundles.py	2014-03-06 16:01:13 +0000
+++ charmworld/views/tests/test_bundles.py	2015-02-18 14:26:31 +0000
@@ -1,10 +1,12 @@
-# Copyright 2013-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from StringIO import StringIO
 import datetime
 import unittest
 
+from pyramid.httpexceptions import HTTPMovedPermanently
+
 from charmworld.models import (
     Bundle,
     DatedMetric,
@@ -152,7 +154,7 @@
         found = find_bundle(request)
         self.assertEqual(bundle, found)
 
-    def test_multiple_bundles_picks_newest(self):
+    def _multiple_bundle_setup(self):
         basket_rev1 = 'riak/1'
         name = 'xyz'
         owner = 'matt'
@@ -161,12 +163,16 @@
         basket_rev2 = 'riak/10'
         bundle2, bundle2_data = factory.makeBundle(
             self.db, name=name, owner=owner, basket_with_rev=basket_rev2)
+        return name, owner, bundle2
+
+    def test_multiple_bundles_picks_newest(self):
+        name, owner, expected_bundle = self._multiple_bundle_setup()
         request = self.getRequest()
         request.matchdict = dict(bundle=name,
                                  basket='riak',
                                  owner=owner)
         found = find_bundle(request)
-        self.assertEqual(bundle2, found)
+        self.assertEqual(expected_bundle, found)
 
     def test_found_bundle_promulgated(self):
         bundle, bundle_data = factory.makeBundle(self.db, promulgated=True)
@@ -198,13 +204,10 @@
             basket=basket,
             bundle=bundle.name,
         )
-        expected = dict(
-            series='raring',
-            relations={},
-            services={},
-        )
-        response = personal_bundle(request)
-        self.assertEqual(expected, response['bundle'].data)
+        with self.assertRaises(HTTPMovedPermanently) as ctx:
+            personal_bundle(request)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(ctx.exception.location.endswith(expected))
 
     def test_personal_bundle_json(self):
         basket = 'ball'
@@ -298,37 +301,63 @@
         # Test that the route works.
         path = '/bundle/~{owner}/{basket}/{bundle}'.format(
             owner=owner, basket=basket, bundle=bundle_name)
-        resp = self.app.get(path, status=200)
-        self.assertIn(bundle.owner, resp.body)
-        self.assertIn(bundle.data['series'], resp.body)
-        self.assertIn(bundle.basket_name, resp.body)
-        self.assertIn(bundle.owner, resp.body)
-        # Ensure the short url to the branch on Launchpad is in the body.
-        lp_path = '~{owner}/charms/bundles/{basket}/bundle'.format(
-            owner=owner, basket=basket)
-        lp_short_url = 'lp:{path}'.format(path=lp_path)
-        self.assertIn(lp_short_url, resp.body)
-        # Ensure the long url to the branch on Launchpad is in the body.
-        lp_long_url = 'https://code.launchpad.net/{path}'.format(path=lp_path)
-        self.assertIn(lp_long_url, resp.body)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
+
+    def test_personal_bundle_route_multi_bundle_basket(self):
+        owner = 'ladonna'
+        basket = 'treme'
+        bundle_name = 'gigis'
+        rev = 1
+        basket_with_rev = '%s/%d' % (basket, rev)
+        bundle_1, bundle_data_1 = factory.makeBundle(
+            self.db, name=bundle_name, owner=owner,
+            basket_with_rev=basket_with_rev, series='raring',
+            with_basket=True,
+        )
+        bundle_2, bundle_data_2 = factory.makeBundle(
+            self.db, name='other_bundle', owner=owner,
+            basket_with_rev=basket_with_rev, series='raring',
+            with_basket=True,
+        )
+        # Test that the route works.
+        path = '/bundle/~{owner}/{basket}/{bundle}'.format(
+            owner=owner, basket=basket, bundle=bundle_name)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
+
+    def test_personal_bundle_route_not_found(self):
+        # We just forward without checking the database, so a 404 is not
+        # thrown.
+        basket = 'nobasket'
+        path = '/bundle/~noowner/{}/nobundle'.format(basket)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
 
     def test_personal_bundle_route_with_revision(self):
+        basket = 'wicker'
         bundle, bundle_data = factory.makeBundle(
             self.db, name='gigis', owner='ladonna',
-            basket_with_rev='basket/1', series='raring',
+            basket_with_rev='{}/1'.format(basket), series='raring',
         )
-        path = '/bundle/~ladonna/basket/1/gigis'
-        response = self.app.get(path, status=200)
-        self.assertIn(
-            'lp:~ladonna/charms/bundles/basket/bundle', response.body)
+        path = '/bundle/~ladonna/{}/1/gigis'.format(basket)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
 
     def test_personal_bundle_route_404(self):
+        basket = 'wicker'
         bundle, bundle_data = factory.makeBundle(
             self.db, name='gigis', owner='ladonna',
-            basket_with_rev='basket/1', series='raring',
+            basket_with_rev='{}/1'.format(basket), series='raring',
         )
-        path = '/bundle/~ladonna/basket/1/XXX'
-        self.app.get(path, status=404)
+        path = '/bundle/~ladonna/{}/1/XXX'.format(basket)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
 
     def test_personal_bundle_with_revision_json_route(self):
         # A JSON-encoded bundle can be requested in which the ID includes both
@@ -364,7 +393,7 @@
         path = '/bundle/~ladonna/basket/1/XXX/json'
         self.app.get(path, status=404)
 
-    def test_official_bundle_route(self):
+    def test_official_bundle_json_route(self):
         bundle, bundle_data = factory.makeBundle(
             self.db, name='wiki', owner='charmers',
             basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
@@ -376,14 +405,16 @@
         self.assertIn(str(bundle.data['services']), resp.body)
 
     def test_official_bundle_with_revision_route(self):
+        basket = 'wiki-basket'
         bundle, bundle_data = factory.makeBundle(
             self.db, name='wiki', owner='charmers',
-            basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
+            basket_with_rev='{}/1'.format(basket), series='raring',
+            promulgated=True,
         )
-        path = '/bundle/wiki-basket/1/wiki'
-        response = self.app.get(path, status=200)
-        self.assertIn(
-            'lp:~charmers/charms/bundles/wiki-basket/bundle', response.body)
+        path = '/bundle/{}/1/wiki'.format(basket)
+        resp = self.app.get(path, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
 
     def test_official_bundle_route_404(self):
         bundle, bundle_data = factory.makeBundle(
@@ -394,25 +425,15 @@
         self.app.get(path, status=404)
 
     def test_bundle_short_url_route(self):
-        bundle, bundle_data = factory.makeBundle(
-            self.db, name='gigis', owner='ladonna',
-            basket_with_rev='basket/1', series='raring', promulgated=True,
-        )
-        self.app.get('/' + bundle.short_url, status=200)
-
-
-class BundleDisplayTests(WebTestBase):
-    """Verify that bundles are displayed correctly."""
-
-    def test_download_statistics_are_displayed(self):
-        # The rendered page includes details about the number of times this
-        # bundle has been "downloaded" (actually, it means "deployed").
-        bundle, bundle_data = factory.makeBundle(
-            self.db, name='gigis', owner='ladonna',
-            basket_with_rev='basket/1', series='raring', promulgated=True,
-        )
-        response = self.app.get('/' + bundle.short_url, status=200)
-        self.assertIn('>Downloads<', response.body)
+        basket = 'wicker'
+        bundle, bundle_data = factory.makeBundle(
+            self.db, name='gigis', owner='ladonna',
+            basket_with_rev='{}/1'.format(basket), series='raring',
+            promulgated=True,
+        )
+        resp = self.app.get('/' + bundle.short_url, status=301)
+        expected = '/q/{}?type=bundle'.format(basket)
+        self.assertTrue(resp.location.endswith(expected))
 
 
 class StatisticsTests(unittest.TestCase):

=== modified file 'default.ini'
--- default.ini	2015-02-09 14:16:33 +0000
+++ default.ini	2015-02-18 14:26:31 +0000
@@ -29,6 +29,9 @@
 # App Settings
 project_name = Charm Browser
 
+# Caching settings
+http_cache =
+
 # List of groups/teams that have edit access.
 openid_teams = charmers,juju-jitsu
 
@@ -71,6 +74,9 @@
 # New web site for redirection
 redirect_jujucharms = https://jujucharms.com
 
+# This signals that we are running under test.
+is_testing = false
+
 [server:main]
 use = egg:Paste#http
 host = 0.0.0.0
@@ -134,3 +140,4 @@
 format = %(asctime)s %(message)s
 
 # End logging configuration
+