Merge lp:~sinzui/charmworld/api3-interesting into lp:~juju-jitsu/charmworld/trunk

[Curtis Hovey]

Update interesting to support bundles.

Update interesting to support bundles.

RULES

    Pre-implementation: no one
    * Change the calls in _interesting_charms() to pass the doctype
    * Update API2 to always pass CHARMS.
    * Update API3 to pass None (to get both CHARMS and BUNDLES)

    FUTURE
    * Add date_created to bundles?
    * Add downloads to bundles?
    * Update get_featured to work with the charms and bundles collections
      possibly by extending http://pastebin.ubuntu.com/6018943/

QA

    * Visit http://staging.jujucharms.com/heartbeat
    * Verify all four checks pass.
    * Visit http://staging.jujucharms.com/api/2/charms/interesting
    * Verify list contains charms.
    * Visit http://staging.jujucharms.com/api/3/charms/interesting
    * Verify the data contains a lists of new, popular and featured
      charms. The wiki bundle wont appear because it has no downloads
      nor is it new.

IMPLEMENTATION

I cut the scope of this branch when I realised other parts of the code
need to change to truly show bundles with charms in production. This
branch does allow API to include bundles in new, popular, and featured,
but there are underlying data issues that prevent bundles from appearing.

The tests are simplified versions of other "interesting" tests that
checks the specific rules of the content of new, popular, and featured.
These tests only verify that API3 includes bundles and charms while
preserving the charm-only rule of API2.

While the tests show that bundles are supported, they wont be in
production! Bundles don't have a date_created attr so they are ranked
last. They appear in test because there are only two things in the db.
Similarly, bundles don't have a downloads attr, they are ranked last and
appear because there are only two things in the test. Following the
example of charms, we can use the branch's date_created as the bundle's
date_created. We need to count bundle json requests and store them in a
downloads attr to collect popular data. Bundles can be featured (as
demonstrated by the test setup) but we cannot see them in results.

I tried to solve including featured bundles. I got stuck when I realised
that get_featured() takes the charms collection as an argument. I suspect
we want to pass both the charms and bundle collections to get the right
data, but I don't know ho we want to merge the two lists? bundles first,
charms first, or intermingle them based on some attr? This is the diff
if what I attempted to allow me to locate featured charms and bundles.
    http://pastebin.ubuntu.com/6018943/

[Curtis Hovey (sinzui)]

I have some additional thoughts about getting featured charms and bundles. The current implementation is a mongo collection find, but all other API search methods use elasticsearch. The current implementation uses the featured data to create a find spec. We could instead use the featured data to create a list of ids to pass the index_clients muiltget() function. We would get charms and bundles from ES as we do for new and popular.

[Brad Crittenden (bac)]

The code and tests look nice. Thanks.

review: Approve

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-08-22 20:33:45 +0000
+++ charmworld/views/api.py	2013-08-23 20:00:56 +0000
@@ -463,7 +463,7 @@
         if path is None or path == ['']:
             handler = self._search
         elif path[0] == 'interesting':
-            handler = self._interesting_charms
+            handler = self._interesting_charms_and_bundles
         else:
             raise HTTPNotFound(self.request.path)
         return self._handle(handler, args, kwargs)
@@ -520,15 +520,16 @@
             results.append(result)
         return results
 
-    def _interesting_charms(self):
+    def _interesting_charms_and_bundles(self, doctype=None):
         """Generate a JSON structure of interesting charms.
 
         Includes featured, new and popular charms.
         """
         db = self.request.db
         popular = self.request.index_client.api_search(
-            sort='downloaded', limit=10)
-        new = self.request.index_client.api_search(sort='new', limit=10)
+            sort='downloaded', doctype=doctype, limit=10)
+        new = self.request.index_client.api_search(
+            sort='new', doctype=doctype, limit=10)
         featured = FeaturedSource.from_db(db).get_featured(db.charms, 'charm')
         return {
             'result': {
@@ -550,11 +551,11 @@
     """
 
     def charms(self, path=None, *args, **kwargs):
+        kwargs['doctype'] = CHARM
         if path is None or path == ['']:
             handler = self._search
-            kwargs['doctype'] = CHARM
         elif path[0] == 'interesting':
-            handler = self._interesting_charms
+            handler = self._interesting_charms_and_bundles
         else:
             raise HTTPNotFound(self.request.path)
         return self._handle(handler, args, kwargs)

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-08-22 13:50:17 +0000
+++ charmworld/views/tests/test_api.py	2013-08-23 20:00:56 +0000
@@ -1436,6 +1436,16 @@
         self.assertEqual({}, response['result']['requires'])
 
 
+def list_names(items):
+    names = []
+    for item in items:
+        if item['metadata']['doctype'] == CHARM:
+            names.append(item['charm']['name'])
+        else:
+            names.append(item['bundle']['name'])
+    return names
+
+
 class TestAPI3(TestAPI, API3Mixin):
     """Test API 3."""
 
@@ -1459,6 +1469,24 @@
         self.assertEqual(CHARM, found_charm['metadata']['doctype'])
         self.assertEqual(charm['store_url'], found_charm['charm']['url'])
 
+    def test_search_interesting_contains_charms_and_bundles(self):
+        self.use_index_client()
+        bundle = self.make_indexed_bundle(name='bat')
+        FeaturedSource.from_db(self.db).set_featured(
+            bundle._representation, BUNDLE)
+        charm = self.make_indexed_charm()
+        FeaturedSource.from_db(self.db).set_featured(charm, CHARM)
+        result = self.get_response(
+            self.search_endpoint, 'interesting').json_body['result']
+        self.assertEqual(3, len(result))
+        expected = [charm['name'], bundle.name]
+        self.assertEqual(expected, list_names(result['popular']))
+        self.assertEqual(expected, list_names(result['new']))
+        # XXX sinzui 2013-08-23: We want this to include bundles, but that
+        # is somewhat hard to do with get_featured() limited to just the
+        # charms collection.
+        self.assertEqual([charm['name']], list_names(result['featured']))
+
 
 class TestAPI2(TestAPI, API2Mixin):
     """Test API 2."""
@@ -1485,3 +1513,18 @@
         result = self.get_response(self.search_endpoint).json_body['result']
         self.assertEqual(1, len(result))
         self.assertEqual(charm['store_url'], result[0]['charm']['url'])
+
+    def test_charms_interesting_only_contain_charms(self):
+        self.use_index_client()
+        bundle = self.make_indexed_bundle()
+        FeaturedSource.from_db(self.db).set_featured(
+            bundle._representation, BUNDLE)
+        charm = self.make_indexed_charm()
+        FeaturedSource.from_db(self.db).set_featured(charm, CHARM)
+        result = self.get_response(
+            self.search_endpoint, 'interesting').json_body['result']
+        self.assertEqual(3, len(result))
+        expected = [charm['name']]
+        self.assertEqual(expected, list_names(result['popular']))
+        self.assertEqual(expected, list_names(result['new']))
+        self.assertEqual(expected, list_names(result['featured']))