Merge lp:~abentley/charmworld/hide-same-series into lp:~juju-jitsu/charmworld/trunk

[Aaron Bentley]

Exclude related charms from different series.

It is confusing to show charms from a different series as "related", because it's very unusual to deploy charms from different series. This branch suppresses such results in the "related" endpoint (API 2).

As a driveby, it tweaks the "make test" command to suppress urllib and pyelasticsearch logs, making most debugging easier.

As another driveby, it moves API2._filter_charm_related into API1, since it is only used there.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'Makefile'
--- Makefile	2013-05-03 15:15:20 +0000
+++ Makefile	2013-06-07 19:47:31 +0000
@@ -5,7 +5,7 @@
 CACHE := download-cache/dist
 CSS_PATH := charmworld/static/css
 LESSC := node_modules/less/bin/lessc
-NOSE := INI="test.ini" bin/nosetests
+NOSE := INI="test.ini" bin/nosetests --logging-filter=-pyelasticsearch,-requests.packages.urllib3.connectionpool
 PSERVE := bin/pserve
 SYSTEM_DEPS := \
 	build-essential bzr charm-tools libyaml-dev python-dev python-virtualenv\

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-06-04 15:28:33 +0000
+++ charmworld/search.py	2013-06-07 19:47:31 +0000
@@ -313,11 +313,24 @@
             'matches_human_readable_estimate': '%d' % len(charms),
         }
 
-    def related_charms(self, requires, provides):
+    def related_charms(self, requires, provides, series=None):
+        """Determine the charms related to the specified interfaces.
+
+        Results are of the form:
+        {'data': {/*actual schema*/}, 'weight': 5.6}
+
+        :param requires: A set of interfaces that charms may require.
+        :param provides: A set of interfaces that charms may provide.
+        :param series: If provided, limit results to the specified series.
+        :return: requires, provides, each of which is a dict mapping
+            interfaces to lists of results.
+        """
         and_clauses = [{'or': [
             {'terms': {'i_provides': provides}},
             {'terms': {'i_requires': requires}},
         ]}]
+        if series is not None:
+            and_clauses.append({'term': {'series': series}})
         and_clauses.extend(self.valid_charms_clauses)
         dsl = {
             'filtered': {

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-06-04 16:09:44 +0000
+++ charmworld/tests/test_search.py	2013-06-07 19:47:31 +0000
@@ -541,6 +541,31 @@
                      for result in provides['telnet'])
         self.assertAlmostEqual(by_id[official_id], by_id[unofficial_id] * 10)
 
+    def test_related_charms_series_filter(self):
+        def filter_data(result):
+            return dict((key, [entry['data'] for entry in value])
+                        for key, value in result.items())
+        alpha_charm = factory.get_charm_json(
+            series='alpha', provides={'asdf': {'interface': 'foo'}})
+        beta_charm = factory.get_charm_json(
+            series='beta', requires={'asdf': {'interface': 'bar'}})
+        self.index_client.index_charms([alpha_charm, beta_charm])
+        requires, provides = self.index_client.related_charms({'bar'}, {'foo'})
+        self.assertEqual({'bar': [beta_charm]}, filter_data(requires))
+        self.assertEqual({'foo': [alpha_charm]}, filter_data(provides))
+        requires, provides = self.index_client.related_charms(
+            {'bar'}, {'foo'}, series='alpha')
+        self.assertEqual({}, filter_data(requires))
+        self.assertEqual({'foo': [alpha_charm]}, filter_data(provides))
+        requires, provides = self.index_client.related_charms(
+            {'bar'}, {'foo'}, series='beta')
+        self.assertEqual({'bar': [beta_charm]}, filter_data(requires))
+        self.assertEqual({}, filter_data(provides))
+        requires, provides = self.index_client.related_charms(
+            {'bar'}, {'foo'}, series='gamma')
+        self.assertEqual({}, filter_data(requires))
+        self.assertEqual({}, filter_data(provides))
+
 
 def put_mapping(client, properties):
     client._client.put_mapping(

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-06-07 13:08:39 +0000
+++ charmworld/views/api.py	2013-06-07 19:47:31 +0000
@@ -128,14 +128,6 @@
         }
 
     @classmethod
-    def _filter_charm_related(cls, charm_name, interfaces, relations):
-        for interface in interfaces:
-            related_charms = [ch for ch in relations.get(interface, [])
-                              if ch['name'] != charm_name]
-            if len(related_charms) > 0:
-                yield interface, related_charms
-
-    @classmethod
     def _charm_result(cls, charm_data, requires=None, provides=None):
         # requires and provides are unused, provided only for API1
         # compatibility.
@@ -146,7 +138,7 @@
             }
         }
 
-    def _related_charms(self, charms):
+    def _related_charms(self, charms, series=None):
         charms_provide = set()
         charms_require = set()
         for charm in charms:
@@ -156,7 +148,7 @@
         # what these charms provide, and the charms that *provide* what these
         # charms require.
         r_requires, r_provides = self.request.index_client.related_charms(
-            charms_provide, charms_require)
+            charms_provide, charms_require, series=series)
         f_requires = {}
         for key, value in r_requires.items():
             f_requires[key] = [
@@ -332,7 +324,8 @@
             status_code=200)
 
     def _charm_related(self, charm):
-        requires, provides = self._related_charms([charm])
+        requires, provides = self._related_charms(
+            [charm], series=charm['series'])
         return {'result': {'requires': requires, 'provides': provides}}
 
     @staticmethod
@@ -424,6 +417,14 @@
                 for charm in charms]
 
     @classmethod
+    def _filter_charm_related(cls, charm_name, interfaces, relations):
+        for interface in interfaces:
+            related_charms = [ch for ch in relations.get(interface, [])
+                              if ch['name'] != charm_name]
+            if len(related_charms) > 0:
+                yield interface, related_charms
+
+    @classmethod
     def _charm_result(cls, charm_data, requires, provides):
         charm = Charm(charm_data)
         return {

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-06-06 19:39:36 +0000
+++ charmworld/views/tests/test_api.py	2013-06-07 19:47:31 +0000
@@ -1182,7 +1182,7 @@
 
 
 class TestAPI2(TestAPI, API2Mixin):
-    """Test API 1."""
+    """Test API 2."""
 
     def test_charm_result_excludes_related(self):
         charm = factory.get_charm_json(promulgated=False, provides={
@@ -1217,6 +1217,10 @@
         result = self.api_class._charm_result(charm, requires, provides)
         self.assertNotIn('related', result['metadata'])
 
+    def get_charm_response(self, charm, suffix):
+        remainder = self.api_class._get_api_id(charm) + '/' + suffix
+        return self.get_response('charm', remainder=remainder).json
+
     def test_related(self):
         self.use_index_client()
         charm_source = CharmSource.from_request(self)
@@ -1246,8 +1250,7 @@
             'bar': {'interface': 'imap'}
         })
         self.index_client.index_charm(imap_provider)
-        remainder = self.api_class._get_api_id(charm) + '/related'
-        result_json = self.get_response('charm', remainder=remainder).json
+        result_json = self.get_charm_response(charm, 'related')
         self.assertEqual(
             self.weightless_relation(result_json['result']['requires']),
             {'imap': [self.related_weightless(imap_user)]})
@@ -1255,6 +1258,31 @@
             self.weightless_relation(result_json['result']['provides']),
             {'ssl': [self.related_weightless(ssl_provider)]})
 
+    def test_charm_related_excludes_different_series(self):
+        self.use_index_client()
+        charm_source = CharmSource.from_request(self)
+        charm_one = factory.get_charm_json(promulgated=False, provides={
+            'cookie-factory': {
+            'interface': 'imap',
+            }
+        })
+        charm_source.save(charm_one)
+        charm_two = factory.get_charm_json(promulgated=False, requires={
+            'cookie-factory': {
+            'interface': 'imap',
+            }
+        }, series=charm_one['series'])
+        charm_source.save(charm_two)
+        response = self.get_charm_response(charm_one, 'related')
+        charm_two_id = self.api_class._get_api_id(charm_two)
+        self.assertEqual(
+            [charm_two_id], [charm['id'] for charm
+                             in response['result']['requires']['imap']])
+        charm_two['series'] = factory.random_string(10)
+        charm_source.save(charm_two)
+        response = self.get_charm_response(charm_one, 'related')
+        self.assertEqual({}, response['result']['requires'])
+
 
 class TestAPI1(TestAPI, API1Mixin):
     """Test API 1."""