Merge lp:~sinzui/charmworld/charm-object-damn-it-2 into lp:~juju-jitsu/charmworld/trunk

[Curtis Hovey]

Always pass charm objects to the charm-collection template.

Always pass charm objects to the charm-collection template.

RULES

    This branch, pre-implementation: abentley
    * Update all the charm-collection.py views to use a charm object.
    * Extract the common query and format logic in the views to a
      helper function

    Next Branches
    * The hooks template still uses a charm dict.

QA

    Verify these pages load. Each has the problem bonnie charm.
    * Visit http://staging.jujucharms.com/charms
    * Visit http://staging.jujucharms.com/charms/precise
    * Visit http://staging.jujucharms.com/~charmers
    * Visit http://staging.jujucharms.com/~charmers/precise

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'charmworld/templates/charm-collection.pt'
--- charmworld/templates/charm-collection.pt	2013-07-03 15:29:09 +0000
+++ charmworld/templates/charm-collection.pt	2013-07-03 20:14:25 +0000
@@ -33,9 +33,9 @@
         </tr>
     </thead>
     <tr tal:repeat="charm charms">
-      <td><a href="/charms/${charm['series']}/${charm['name']}">${charm["name"]}</a></td>
-      <td tal:content="charm['summary'] | nothing"></td>
-      <td tal:content="charm['series']"></td>
+      <td><a href="/charms/${charm.series}/${charm.name}">${charm.name}</a></td>
+      <td tal:content="charm.summary"></td>
+      <td tal:content="charm.series"></td>
     </tr>
   </table>
   </div>

=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2013-06-25 08:59:50 +0000
+++ charmworld/views/charms.py	2013-07-03 20:14:25 +0000
@@ -178,16 +178,38 @@
         content_type=u"application/json")
 
 
+def found_charm_collection(db, query, sub_only, only_promulgated=False):
+    """Return a dict of found charms with subordinates flag.
+
+    Build a list of Charm objects for the found charm data. The list is
+    added to a dict (as charms) with a sub_only key to indicate if the
+    query is for subordinates only. The dict is compatible with the
+    charm-collection.pt template.
+
+    :param db: The database to query.
+    :param query: The query to match charms.
+    :param sub_only: Is the query restricted to subordinates?
+    :param only_promulgated: restrict the query to promulgated charms.
+        Default False.
+    :return: A dict {charms: [<Charm>], sub_only: sub_only}.
+    """
+    collection_project = {
+        "name": 1, "owner": 1, "series": 1, "summary": 1, "short_url": 1}
+    collection_sort = [("name", pymongo.ASCENDING)]
+    results = found(find_charms(
+        db, query, collection_project,
+        sort=collection_sort, only_promulgated=only_promulgated))
+    charms = [Charm(charm_data) for charm_data in results]
+    return {"charms": charms, 'sub_only': sub_only}
+
+
 @cached_view_config(
     route_name="charm-collection",
     renderer="charmworld:templates/charm-collection.pt")
 def charm_collection(request):
     query, sub_only = sub_filter({}, request)
-    charms = found(find_charms(
-        request.db, query,
-        {"name": 1, "owner": 1, "series": 1, "summary": 1, "short_url": 1},
-        sort=[("name", pymongo.ASCENDING)], only_promulgated=True))
-    return {"charms": list(charms), 'sub_only': sub_only}
+    return found_charm_collection(
+        request.db, query, sub_only, only_promulgated=True)
 
 
 @cached_view_config(
@@ -197,11 +219,7 @@
     query, sub_only = sub_filter({
         "owner": request.matchdict["owner"]
     }, request)
-    charms = found(find_charms(
-        request.db, query,
-        {"name": 1, "owner": 1, "series": 1, "summary": 1, "short_url": 1},
-        sort=[("name", pymongo.ASCENDING)]))
-    return {"charms": list(charms), "sub_only": sub_only}
+    return found_charm_collection(request.db, query, sub_only)
 
 
 @cached_view_config(
@@ -210,19 +228,8 @@
 def series_collection(request):
     query, sub_only = sub_filter(
         {"series": request.matchdict["series"]}, request)
-    charms = found(find_charms(
-        request.db, query,
-        {
-            "name": 1,
-            "owner": 1,
-            "series": 1,
-            "summary": 1,
-            "short_url": 1,
-            'store_url': True,
-        },
-        sort=[("name", pymongo.ASCENDING)],
-        only_promulgated=True))
-    return {"charms": list(charms), 'sub_only': sub_only}
+    return found_charm_collection(
+        request.db, query, sub_only, only_promulgated=True)
 
 
 @cached_view_config(
@@ -235,12 +242,7 @@
             "series": request.matchdict["series"]
         },
         request)
-
-    charms = found(find_charms(
-        request.db, query,
-        {"name": 1, "owner": 1, "series": 1, "summary": 1, "short_url": 1},
-        sort=[("name", pymongo.ASCENDING)]))
-    return {"charms": list(charms), 'sub_only': sub_only}
+    return found_charm_collection(request.db, query, sub_only)
 
 
 def find_charm(request, promulgated=False):

=== modified file 'charmworld/views/tests/test_charms.py'
--- charmworld/views/tests/test_charms.py	2013-07-03 15:29:09 +0000
+++ charmworld/views/tests/test_charms.py	2013-07-03 20:14:25 +0000
@@ -24,6 +24,7 @@
     config,
     distro_charm,
     distro_charm_json,
+    found_charm_collection,
     hook,
     interface,
     featured_edit,
@@ -39,6 +40,27 @@
 
 class TestCharms(ViewTestBase):
 
+    def test_found_charm_collection(self):
+        """found_charm_collection() returns a dict of charm objects."""
+        _id, one = factory.makeCharm(self.db, name='bee', promulgated=True)
+        _id, two = factory.makeCharm(self.db, name='awk', promulgated=True)
+        _id, three = factory.makeCharm(self.db, name='cat', promulgated=False)
+        query = {"series": 'precise'}
+        sub_only = False
+        # The only_promulgated kwarg limits the sorted charms.
+        collection = found_charm_collection(
+            self.db, dict(query), sub_only, only_promulgated=True)
+        promulgated_names = [charm.name for charm in collection['charms']]
+        expected = [two['name'], one['name']]
+        self.assertEqual(expected, promulgated_names)
+        self.assertIs(False, collection['sub_only'])
+        # The only_promulgated kwarg defaults to False, getting all sorted
+        # charms.
+        collection = found_charm_collection(self.db, dict(query), sub_only)
+        all_names = [charm.name for charm in collection['charms']]
+        expected = [two['name'], one['name'], three['name']]
+        self.assertEqual(expected, all_names)
+
     def test_charm_collection_basic(self):
         """Simple sanity check."""
         one_id, one = factory.makeCharm(self.db, promulgated=True)
@@ -46,7 +68,7 @@
         three_id, three = factory.makeCharm(self.db, promulgated=False)
 
         response = charm_collection(self.getRequest())
-        charm_names = set(charm['name'] for charm in response['charms'])
+        charm_names = set(charm.name for charm in response['charms'])
         expected = set((one['name'], two['name']))
         self.assertEqual(expected, charm_names)
 
@@ -61,7 +83,7 @@
         response = charm_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_charm_collection_charms_with_errors(self):
         """Charms with errors are not returned by charm_collection()."""
@@ -71,7 +93,7 @@
         response = charm_collection(self.getRequest())
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_personal_collection(self):
         # personal_collection() returns only charms owned by the given
@@ -83,7 +105,7 @@
         response = personal_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual('foo', charms[0]['owner'])
+        self.assertEqual('foo', charms[0].owner)
 
     def test_personal_collection_sub_filter(self):
         # If the request parameter 'sub' is set, only subordinate charms
@@ -98,15 +120,15 @@
         response = personal_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(three['name'], charms[0]['name'])
+        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'])
+        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().
@@ -119,7 +141,7 @@
         response = personal_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_series_collection(self):
         # series_collection() returns only promulgated charms for a
@@ -138,8 +160,8 @@
         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'])
+            ['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
@@ -156,7 +178,7 @@
         response = series_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(three['name'], charms[0]['name'])
+        self.assertEqual(three['name'], charms[0].name)
 
     def test_series_collection_charms_with_errors(self):
         # Charms with errors are not returned by series_collection().
@@ -171,7 +193,7 @@
         response = series_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_personal_series_collection(self):
         # personal_series_collection() returns only charms for a given
@@ -187,7 +209,7 @@
         response = personal_series_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_personal_series_collection_sub_filter(self):
         # If the parameter 'sub' is set, personal_series_collection()
@@ -206,7 +228,7 @@
         response = personal_series_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(four['name'], charms[0]['name'])
+        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().
@@ -223,7 +245,7 @@
         response = personal_series_collection(request)
         charms = response['charms']
         self.assertEqual(1, len(charms))
-        self.assertEqual(one['name'], charms[0]['name'])
+        self.assertEqual(one['name'], charms[0].name)
 
     def test_interface(self):
         # interface() returns all charms reqiring or providing a given