Merge lp:~zematynnad/rnr-server/api_check_856808 into lp:rnr-server

[Danny Tamez]

Now calling sca's piston API for published apps.

Overview
=========
This branch switches to using SCA's piston api for retrieving applications.

Details
========
Now that SCA has this API available there is no need to call the view we were calling before. get_sca_publishings_for now calls api/2.0/applications/en/ubuntu/<distro>/<arch>/

To Test
========
$fab bootstrap test

[Michael Nelson (michael.nelson)]

Looks fine Danny. I'm assuming you looked at whether it was worth using the actual piston mini-client and decided against it for this one call?

Sheesh - I just checked the scaclient, and it's also using the old view-based available apps:

http://bazaar.launchpad.net/~canonical-ca-hackers/software-center/scaclient/view/head:/scaclient.py

for all api calls :/

In which case, it might be best to (1) update the scaclient to use the 2.0 api for all calls (it's tested in test_scaclient.py in sca), and then (2) update rnr to use the sca client? I think (1) is necessary, (2) is just a matter of us dogfooding our own tools - feel free to ignore (2) and land this branch as is, but given that scaclient is what where giving others to use (such as USC) I think we should update it asap. What do you think?

[Michael Nelson (michael.nelson)]

Hi Danny!

Thanks for updating to use the sca client. I'm not sure how I missed above that the scaclient *is* using the /2.0/ api already - I must have missed the 'default_service_root' property.

Two things that might be worth considering with this branch - see what you think.

1) In utilities.py, you don't need to go through the whole _sca_service, _set_sca_service() before providing your sca property. It's used for lazr.restful clients which actually ping for wadl when they're instantiated. But in the case of the sca client, it's cheap to instantiate, so you can just have the sca property doing it directly without any cached service.

2) You wouldn't need your DictToObj conversion if you instead patched SoftwareCenterAgentAPI._get (or whatever it's underlying code is calling) with self.factory.get_test_file('software_center_apps.json') and let the piston-mini-client do the conversion for you.

See what you think. Happy to +1 the MP if you've got reasons against the above 2 points.

[Danny Tamez (zematynnad)]

As per our conversation on mumble - 1) no need to cache the service just use it directly as it's cheap. 2) instead of patching available_apps, patch httplib2.Http.request with a return value of ({'status': '200', <value of serialized json from file>) instead.

[Michael Nelson (michael.nelson)]

Thanks Danny - looks great. I can see why we'd no longer need the test_retrieved_url (as we'd be testing the sca client implementation), but I'm not sure why we'd remove test_get_sca_publishings_cached. If we did want to keep it, we could switch the @patch('httplib2.Http.request') into the setUp(), with:

{{{
def setUp(self):
   self.patcher = patch('httplib2.Http.request')
   self.mock_http_request = self.patcher.start()
   self.addCleanup(self.patcher.stop)
}}}

that way we could keep test_get_sca_publishings_cached, asserting that self.mock_http_request.call_count == 0... if we wanted to, but maybe there's a reason for removing the test. Anyway, I'm adding a +1 and you can see what you think.

review: Approve

[Danny Tamez (zematynnad)]

Combined the branch and the dependent branch into this one as there was much overlap in the comments on the two.

[Michael Nelson (michael.nelson)]

Two small things - see if you think they're worth changes, but +1 :)

1:

16:04 <noodles> Hey there! Should the assertion on line 121 alse be on line 117 (it looks like the
                previous code had a typo, which you've fixed, but we're only checking after the
                second call)
16:04 * zematynnad takes a look

2: I don't *think* you should need to make assumptions about the caching backend being used to mock it, we've got similar tests elsewhere that patch out the cache.get, but no biggie.

review: Approve

[ISD Branch Mangler (isd-branches-mangler)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file '.bzrignore'
--- .bzrignore	2011-10-06 13:08:27 +0000
+++ .bzrignore	2012-04-11 16:37:30 +0000
@@ -18,4 +18,5 @@
 .virtualenvs
 branches/*
 virtualenv/*
-sourcecode/*
\ No newline at end of file
+sourcecode/*
+django_project/scaclient.py

=== modified file 'fabtasks/bootstrap.py'
--- fabtasks/bootstrap.py	2012-01-27 12:39:58 +0000
+++ fabtasks/bootstrap.py	2012-04-11 16:37:30 +0000
@@ -17,7 +17,7 @@
 import os
 import os.path
 
-from fabric.api import local, cd
+from fabric.api import local
 
 
 def _get_or_pull_bzr_branch(repo, name):
@@ -67,6 +67,13 @@
         "branches/wsgi-oops/canonical",
         "django_project/canonical")
 
+    _get_or_pull_bzr_branch(
+        "lp:~canonical-ca-hackers/software-center/scaclient",
+        "scaclient")
+    _symlink(
+        "branches/scaclient/scaclient.py",
+        "django_project/scaclient.py")
+
 
 def download_multi_mechanize():
     multi_mechanize = "multi-mechanize_1.010.zip"

=== modified file 'src/reviewsapp/tests/test_utilities.py'
--- src/reviewsapp/tests/test_utilities.py	2012-01-25 16:18:35 +0000
+++ src/reviewsapp/tests/test_utilities.py	2012-04-11 16:37:30 +0000
@@ -45,8 +45,8 @@
 from reviewsapp.utilities import (
     cache_key_for_url,
     invalidate_paginated_reviews_for,
+    WebServices,
     WebServiceError,
-    WebServices,
     full_claimed_id,
 )
 
@@ -238,24 +238,28 @@
 
 class SCATestCase(TestCaseWithFactory):
 
+    @patch('httplib2.Http.request')
     def _request_sca_pubs_for(self, distroseries, arch_tag,
-                              disable_cache=True):
-        with patch('urllib.urlretrieve') as mock_urlretrieve:
-            mock_urlretrieve.return_value = (
-                self.factory.get_test_file('software_center_apps.json'), None)
-            if disable_cache:
-                with patch('django.core.cache') as mock_cache:
-                    mock_cache.get.return_value = False
-                    published_apps = WebServices().get_sca_publishings_for(
-                        distroseries, arch_tag)
-            else:
+                              disable_cache, mock_request):
+        data_file = self.factory.get_test_file('software_center_apps.json')
+        with open(data_file) as test_data:
+            data = test_data.read()
+            mock_request.return_value = ({'status': '200'}, data)
+
+        if disable_cache:
+            func = 'django.core.cache.cache.get'
+            with patch(func) as mock_get:
+                mock_get.return_value = False
                 published_apps = WebServices().get_sca_publishings_for(
                     distroseries, arch_tag)
-        return published_apps, mock_urlretrieve
+        else:
+            published_apps = WebServices().get_sca_publishings_for(
+                distroseries, arch_tag)
+        return published_apps, mock_request
 
     def test_get_sca_publishings(self):
-        published_apps, ignored = self._request_sca_pubs_for(
-            'maverick', 'i386')
+        published_apps, _ = self._request_sca_pubs_for(
+            'maverick', 'i386', True)
 
         self.assertEqual({
             'lp-ppa-commercial-ppa-uploaders-brukkon': 'brukkon',
@@ -272,19 +276,23 @@
 
     def test_retrieved_url(self):
         with patch_settings(SCA_HOST_URL='http://example.com/'):
-            ignored, mock_urlretrieve = self._request_sca_pubs_for(
-                'natty', 'amd64')
+            ignored, mock_request = self._request_sca_pubs_for(
+            'natty', 'amd64', True)
 
-        mock_urlretrieve.assert_called_with(
-            'http://example.com/apps/en/ubuntu/natty/amd64/')
+        mock_request.assert_called_with(
+            'http://example.com/api/2.0/applications/any/ubuntu/any/any/',
+            body='', headers={}, method='GET')
 
     def test_get_sca_publishings_cached(self):
-        published_apps, mock_url_retrieve = self._request_sca_pubs_for(
-            'maverick', 'i386', disable_cache=False)
-        published_apps, mock_urlretrieve = self._request_sca_pubs_for(
-            'maverick', 'i386', disable_cache=False)
-
-        self.assertEqual(0, mock_urlretrieve.call_count)
+        cache.clear()
+        published_apps, mock_request = self._request_sca_pubs_for(
+            'maverick', 'i386', False)
+        self.assertEqual(1, mock_request.call_count)
+
+        published_apps, mock_request = self._request_sca_pubs_for(
+            'natty', 'i386', False)
+
+        self.assertEqual(0, mock_request.call_count)
 
     def _request_sca_verify(self, pkgname, origin, distroseries, arch_tag):
         with patch('urllib.urlretrieve') as mock_urlretrieve:

=== modified file 'src/reviewsapp/utilities.py'
--- src/reviewsapp/utilities.py	2012-01-27 12:50:26 +0000
+++ src/reviewsapp/utilities.py	2012-04-11 16:37:30 +0000
@@ -37,7 +37,6 @@
 from django.http import HttpRequest
 from django.utils.cache import get_cache_key
 from django.utils.http import urlquote_plus
-from django.utils import simplejson
 
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.uris import lookup_service_root
@@ -46,6 +45,7 @@
 from lazr.restfulclient.errors import HTTPError
 from lazr.restfulclient.resource import ServiceRoot
 from oauth.oauth import OAuthToken
+from scaclient import SoftwareCenterAgentAPI
 
 
 class WebServiceError(Exception):
@@ -222,40 +222,20 @@
         return False
 
     def get_sca_publishings_for(self, distroseries, arch_tag):
-        """A helper that builds a dict of publishings for sca-sold apps.
-
-        XXX michaeln 2011-03-14 bug=734747 Optional parms for available_apps.
-        It would be great if we could just pull /apps/en/ubuntu/ and use
-        each items 'series' dict to populate this, rather than pulling each
-        distroserie/arch separately. If we can assume that an app sold via
-        softwarecenter will always be available for maverick i386, then we
-        could do just the one call too.
-        """
-        cache_key = 'sca_{0}_{1}'.format(distroseries, arch_tag)
-        result = cache.get(cache_key)
-        if result is not None:
-            return result
-
-        # XXX michaeln 2011-03-14 Update to use sca piston client.
-        # Once production SCA includes the piston client version we can
-        # use that instead. At time of writing the following url works
-        # on staging but not production sca:
-        #     api/2.0/applications/en/ubuntu/maverick/i386/
-        filename, headers = urllib.urlretrieve(
-            settings.SCA_HOST_URL +
-            'apps/en/ubuntu/{0}/{1}/'.format(distroseries, arch_tag))
-
-        with open(filename, 'r') as sca_apps_file:
-            sca_apps = simplejson.load(sca_apps_file)
-
-        # We ensure the keys match Launchpad's current (ambiguous)
-        # origin identifiers - Launchpad bug 733170.
-        app_names = dict([(
-            'lp-ppa-' + app['archive_id'].replace('/', '-'),
-            app['package_name'],
-            ) for app in sca_apps])
-        cache.set(cache_key, app_names, settings.SCA_APPS_CACHE_TIMEOUT)
-        return app_names
+        cache_key = 'sca_all_available'
+        apps = cache.get(cache_key)
+
+        if not apps:
+            api = SoftwareCenterAgentAPI(service_root="{0}api/2.0/"
+                                        .format(settings.SCA_HOST_URL))
+            apps = api.available_apps(lang='any', series='any', arch='any')
+            cache.set(cache_key, apps, settings.SCA_APPS_CACHE_TIMEOUT)
+
+        return dict([(
+            'lp-ppa-' + app.archive_id.replace('/', '-'),
+            app.package_name,
+            ) for app in apps if distroseries in app.series and
+            arch_tag in app.series[distroseries]])
 
 
 def cache_key_for_url(url):