Merge lp:~benji/charmworld/expose-bundle-changes into lp:~juju-jitsu/charmworld/trunk

[Benji York]

Add recent bzr changes to bundles.

https://codereview.appspot.com/15100046/

[Brad Crittenden (bac)]

LGTM

https://codereview.appspot.com/15100046/diff/1/charmworld/jobs/ingest.py
File charmworld/jobs/ingest.py (right):

https://codereview.appspot.com/15100046/diff/1/charmworld/jobs/ingest.py#newcode424
charmworld/jobs/ingest.py:424: if revision.timestamp < since:
So since cannot be None?

https://codereview.appspot.com/15100046/diff/1/charmworld/jobs/ingest.py#newcode424
charmworld/jobs/ingest.py:424: if revision.timestamp < since:
So we're sure since can never be None?

https://codereview.appspot.com/15100046/

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-10-17 21:32:57 +0000
+++ charmworld/jobs/ingest.py	2013-10-23 18:29:57 +0000
@@ -261,7 +261,7 @@
         update_download_count(store, charm_data)
         update_proof_data(charm_data, log)
         update_jenkins_data(db, charm_data, log)
-        update_from_revisions(charm_data, newest_revision=revision)
+        update_revision_details(charm_data, newest_revision=revision)
         update_date_created(charm_data, log)
         scan_charm(charm_data, fs, log)
         update_hash(charm_data)
@@ -309,8 +309,11 @@
         self.working_dir = working_dir
         super(UpdateBundleJob, self).__init__()
 
-    def store_bundles(self, deployer_config, owner, basket_id):
-        store_bundles(self.db.bundles, deployer_config, owner, basket_id)
+    def store_bundles(self, deployer_config, owner, basket_id, first_change,
+                      last_change, changes):
+        store_bundles(
+            self.db.bundles, deployer_config, owner, basket_id, first_change,
+            last_change, changes)
 
     @staticmethod
     def set_basket_info(data, revno):
@@ -319,6 +322,9 @@
     def decorate_basket(self, basket_data, fs):
         branch_dir = fetch_branch(self.working_dir, basket_data, self.log,
                                   revisionId=basket_data['commit'])
+        basket_data['branch_dir'] = branch_dir
+        update_revision_details(
+            basket_data, newest_revision=basket_data['commit'])
         branch = Branch.open(branch_dir)
         revno = branch.revision_id_to_revno(basket_data['commit'])
         self.set_basket_info(basket_data, revno)
@@ -340,7 +346,8 @@
         self.db.baskets.save(basket_data)
         deployer_config = self.get_deployer_config(fs, basket_data)
         self.store_bundles(
-            deployer_config, basket_data['owner'], basket_data['name_revno'])
+            deployer_config, basket_data['owner'], basket_data['name_revno'],
+            None, None, None)
 
 
 def _rev_info(r, branch):
@@ -354,27 +361,34 @@
     return d
 
 
-def update_from_revisions(charm_data, limit=10, since=None,
-                          newest_revision=None):
-    if charm_data['branch_deleted']:
+def find_revision_cutoff(now=None, settings=settings):
+    days_of_revisions = settings.get('days_of_revisions')
+    if days_of_revisions is None:
+        days_of_revisions = 30
+    if now is None:
+        now = datetime.utcnow()
+    cutoff = now - timedelta(int(days_of_revisions))
+    return calendar.timegm(cutoff.timetuple())
+
+
+def update_revision_details(data, limit=10, newest_revision=None):
+    # If the branch has been deleted, there can be no new revisions to pull in.
+    if data['branch_deleted']:
         return
-    if since is None:
-        days_of_revisions = settings.get('days_of_revisions')
-        if days_of_revisions is not None:
-            cutoff = datetime.utcnow() - timedelta(int(days_of_revisions))
-            since = calendar.timegm(cutoff.timetuple())
-    branch_dir = charm_data['branch_dir']
-    changes = get_changes(branch_dir, limit, since, newest_revision)
-    charm_data.update(changes)
+    since = find_revision_cutoff()
+    changes, first_change, last_change = get_changes(
+        data['branch_dir'], limit, since, newest_revision)
+    data['changes'] = changes
+    data['first_change'] = first_change
+    data['last_change'] = last_change
 
 
 def get_changes(branch_dir, limit, since, newest_revision=None):
-    charm_data = {}
     branch = Branch.open(branch_dir)
     branch.lock_read()
     try:
         revisions = get_revisions(branch, limit, since)
-        charm_data['changes'] = changes = []
+        changes = []
         # If there is no limit on how new a revision we can include, then
         # consider the starting revision to be already found.
         found_start = newest_revision is None
@@ -391,11 +405,7 @@
             last_change = changes[0]
             first = branch.repository.get_revision(branch.get_rev_id(1))
             first_change = _rev_info(first, branch)
-        charm_data.update({
-            'last_change': last_change,
-            'first_change': first_change,
-        })
-        return charm_data
+        return changes, first_change, last_change
     finally:
         branch.unlock()
 
@@ -411,7 +421,7 @@
             break
         revision = branch.repository.get_revision(revision_id)
         if num >= limit:
-            if since is None or revision.timestamp < since:
+            if revision.timestamp < since:
                 break
         revs.append(revision)
     return revs
 
=== modified file 'charmworld/jobs/tests/test_bzr.py'
--- charmworld/jobs/tests/test_bzr.py	2013-10-17 20:19:55 +0000
+++ charmworld/jobs/tests/test_bzr.py	2013-10-23 18:29:57 +0000
@@ -3,6 +3,7 @@
 
 
 from contextlib import contextmanager
+import datetime
 from logging import getLogger
 import os
 import time
@@ -13,13 +14,14 @@
 
 from charmworld.jobs import ingest
 from charmworld.jobs.ingest import (
+    _rev_info,
     add_files,
     checkout_branch,
     do_bzr_update,
+    find_revision_cutoff,
     get_changes,
     get_revisions,
-    _rev_info,
-    update_from_revisions,
+    update_revision_details,
 )
 from charmworld.models import getfs
 from charmworld.testing import (
@@ -268,36 +270,63 @@
         self.assertEqual([YRH, ZRH], rev_info['authors'])
         self.assertEqual(JRH, rev_info['committer'])
 
-    def test_get_changes(self):
+    def test_get_changes_with_one_revision(self):
+        tree = self.make_locked_tree()
+        self.num_commit(tree, 1)
+        changes, first_change, last_change = get_changes(
+            'tree', 10, time.time())
+        self.assertEqual(first_change, last_change)
+        self.assertEqual([first_change], changes)
+
+    def test_get_changes_with_multiple_revisions(self):
         tree = self.make_locked_tree()
         first_id = self.num_commit(tree, 1)
-        charm_data = get_changes('tree', 10, time.time())
-        self.assertItemsEqual(['changes', 'first_change', 'last_change'],
-                              charm_data.keys())
-        self.assertEqual(charm_data['first_change'], charm_data['last_change'])
-        self.assertEqual([charm_data['first_change']], charm_data['changes'])
         for num in range(1, 11):
             self.num_commit(tree, 1)
-        charm_data = get_changes('tree', 10, time.time())
-        self.assertNotEqual(charm_data['first_change'],
-                            charm_data['last_change'])
-        self.assertEqual(charm_data['last_change'], charm_data['changes'][0])
-        self.assertNotEqual(charm_data['first_change'],
-                            charm_data['changes'][-1])
-        self.assertEqual(self.get_rev_info(tree.branch, first_id),
-                         charm_data['first_change'])
+        changes, first_change, last_change = get_changes(
+            'tree', 10, time.time())
+        self.assertNotEqual(first_change, last_change)
+        self.assertEqual(last_change, changes[0])
+        self.assertNotEqual(first_change, changes[-1])
+        self.assertEqual(
+            self.get_rev_info(tree.branch, first_id),
+            first_change)
 
     def test_get_changes_no_revisions(self):
         self.make_locked_tree()
-        charm_data = get_changes('tree', 10, time.time())
-        self.assertEqual([], charm_data['changes'])
-        self.assertIs(None, charm_data['first_change'])
-        self.assertIs(None, charm_data['last_change'])
+        changes, first_change, last_change = get_changes(
+            'tree', 10, time.time())
+        self.assertEqual([], changes)
+        self.assertIs(None, first_change)
+        self.assertIs(None, last_change)
 
     def test_branch_deleted(self):
-        # update_from_revisions does not do anything if the Launchpad branch
+        # update_revision_details does not do anything if the Launchpad branch
         # of a charm is deleted.
         charm = factory.get_charm_json(branch_deleted=True)
         with patch.object(ingest, 'get_changes') as mock:
-            update_from_revisions(charm)
+            update_revision_details(charm)
         self.assertFalse(mock.called)
+
+
+class TestFindRevisionCutoff(TestCase):
+    """Tests for charmworld.jobs.ingest.find_revision_cutoff."""
+
+    def test_return_type(self):
+        # find_revision_cutoff returns seconds since the epoch (an int).
+        self.assertEqual(type(find_revision_cutoff()), int)
+
+    def test_days_of_revisions_is_respected(self):
+        # If the cutoff date is further in the past, the resulting seconds
+        # since the epoch is further in the past too.
+        old = find_revision_cutoff(settings=dict(days_of_revisions=10))
+        older = find_revision_cutoff(settings=dict(days_of_revisions=20))
+        self.assertGreater(old, older)
+
+    def test_days_are_the_right_size(self):
+        # If we start at time 0 and ask for one day in the past, we get the
+        # obvious value.
+        time = find_revision_cutoff(
+            now=datetime.datetime.utcfromtimestamp(0),
+            settings=dict(days_of_revisions=1))
+        self.assertEqual(time, -86400)
 
=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-10-17 21:32:57 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-10-23 18:29:57 +0000
@@ -533,8 +533,8 @@
         """)
         fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH)
         job.store_bundles(
-            yaml.safe_load(deployer_config),
-            basket_data['owner'], basket_data['name_revno'])
+            yaml.safe_load(deployer_config), basket_data['owner'],
+            basket_data['name_revno'], None, None, None)
         self.assertIsNotNone(self.db.bundles.find_one(bundle_id))
 
     def test_job_run_stores_bundles_in_the_db(self):
@@ -559,7 +559,7 @@
                 with patch.object(job, 'store_bundles') as store_bundles:
                     job.run(basket_data)
                     store_bundles.assert_called_with(
-                        {'foo': {}}, 'charmers', 'dummy')
+                        {'foo': {}}, 'charmers', 'dummy', None, None, None)
 
 
 class TestUpdateCharm(MongoTestBase):
 
=== modified file 'charmworld/migrations/versions/tests/test_migrations.py'
--- charmworld/migrations/versions/tests/test_migrations.py	2013-09-23 15:58:47 +0000
+++ charmworld/migrations/versions/tests/test_migrations.py	2013-10-23 18:29:57 +0000
@@ -60,6 +60,7 @@
             owner, basket_name, bundle_name)
         store_bundles(
             self.db.bundles, parsed, owner, basket_id,
+            None, None, None,
             index_client=self.index_client)
 
     def test_bundles_are_removed_from_elastic_search(self):
 
=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-10-22 22:57:51 +0000
+++ charmworld/models.py	2013-10-23 18:29:57 +0000
@@ -1743,7 +1743,8 @@
     return options
 
 
-def make_bundle_doc(data, owner, basket_id, bundle_name):
+def make_bundle_doc(data, owner, basket_id, bundle_name, first_change,
+                    last_change, changes):
     basket_name, basket_revision = basket_id.split('/')
     _id = Bundle.construct_id(owner, basket_name, bundle_name, basket_revision)
     return {
@@ -1753,11 +1754,14 @@
         'basket_name': basket_name,
         'basket_revision': int(basket_revision),
         'data': data,
+        'first_change': first_change,
+        'last_change': last_change,
+        'changes': changes,
     }
 
 
-def store_bundles(collection, deployer_config, owner, basket_id,
-                  index_client=None):
+def store_bundles(collection, deployer_config, owner, basket_id, first_change,
+                  last_change, changes, index_client=None):
     """Store a basket of bundles into MongoDB and/or ElasticSearch.
 
     :param: collection: A db bundles collection.  If None the bundles are not
@@ -1779,8 +1783,9 @@
     for bundle_name in deployer_config:
         # Set up indexing.
         data = get_flattened_deployment(deployer_config, bundle_name)
-        index_data[bundle_name] = make_bundle_doc(data, owner, basket_id,
-                                                  bundle_name)
+        index_data[bundle_name] = make_bundle_doc(
+            data, owner, basket_id, bundle_name, first_change, last_change,
+            changes)
         if collection is not None:
             collection.save(index_data[bundle_name])
 
 
=== modified file 'charmworld/templates/bundle.pt'
--- charmworld/templates/bundle.pt	2013-10-11 13:51:05 +0000
+++ charmworld/templates/bundle.pt	2013-10-23 18:29:57 +0000
@@ -103,14 +103,6 @@
           </li>
         </ul>
       </div>
-      <tal:comment condition="nothing">
-        <div tal:condition="is_owner">
-          You are the owner.  In the future we'll show you extra stuff here.
-        </div>
-        <div tal:condition="not: is_owner">
-          If you were the author/owner you'd see extra stuff here.
-        </div>
-      </tal:comment>
     </metal:block>
   </body>
 </html>
 
=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-10-04 19:45:06 +0000
+++ charmworld/testing/factory.py	2013-10-23 18:29:57 +0000
@@ -331,7 +331,8 @@
     data = dict(series=series,
                 relations=relations,
                 services=services)
-    bundle_doc = make_bundle_doc(data, owner, basket_with_rev, name)
+    bundle_doc = make_bundle_doc(
+        data, owner, basket_with_rev, name, None, None, None)
     bundle_doc.update(dict(branch_deleted=branch_deleted,
                            data=data,
                            description=description,
 
=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-10-22 22:57:51 +0000
+++ charmworld/tests/test_models.py	2013-10-23 18:29:57 +0000
@@ -1595,7 +1595,7 @@
         basket_id = "%s/%d" % (basket_name, basket_rev)
         _id = Bundle.construct_id(owner, basket_name, bundle_name, basket_rev)
         store_bundles(
-            self.db.bundles, parsed, 'bac', basket_id)
+            self.db.bundles, parsed, 'bac', basket_id, None, None, None)
         self.assertEqual(
             {
                 '_id': _id,
@@ -1604,6 +1604,9 @@
                 'name': bundle_name,
                 'owner': owner,
                 'data': parsed['wordpress-stage'],
+                'first_change': None,
+                'last_change': None,
+                'changes': None,
             },
             self.db.bundles.find_one(_id))
 
@@ -1638,7 +1641,9 @@
 
         with patch('charmworld.models.get_flattened_deployment',
                    get_flattened_deployment):
-            store_bundles(self.db.bundles, parsed, 'bac', 'wordpress-basket/5')
+            store_bundles(
+                self.db.bundles, parsed, 'bac', 'wordpress-basket/5', None,
+                None, None)
         self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys)
 
     def test_storing_a_bundle_includes_indexing_it(self):
@@ -1656,7 +1661,9 @@
         with patch(
                 'charmworld.models.ElasticSearchClient',
                 FauxElasticSearchClient):
-            store_bundles(self.db.bundles, {}, 'owner', 'wordpress-basket/5')
+            store_bundles(
+                self.db.bundles, {}, 'owner', 'wordpress-basket/5', None,
+                None, None)
 
         self.assertTrue(FauxElasticSearchClient.index_bundles_called)
 
@@ -2002,7 +2009,8 @@
 class TestMakeBundleDoc(TestCase):
 
     def test_bundle_doc(self):
-        doc = make_bundle_doc({'a': 'b'}, 'foo', 'bar/9', 'baz')
+        doc = make_bundle_doc(
+            {'a': 'b'}, 'foo', 'bar/9', 'baz', None, None, None)
         self.assertEqual({
             'owner': 'foo',
             'basket_name': 'bar',
@@ -2010,6 +2018,9 @@
             'name': 'baz',
             'data': {'a': 'b'},
             '_id': '~foo/bar/9/baz',
+            'first_change': None,
+            'last_change': None,
+            'changes': None,
         }, doc)
 
 
 
=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-09-16 21:14:06 +0000
+++ charmworld/tests/test_search.py	2013-10-23 18:29:57 +0000
@@ -166,8 +166,9 @@
                         charm: cs:precise/mysql
         """)
         parsed = yaml.safe_load(deployer_config)
-        store_bundles(None, parsed, 'abentley', 'wordpress-basket/5',
-                      index_client=self.index_client)
+        store_bundles(
+            None, parsed, 'abentley', 'wordpress-basket/5', None, None, None,
+            index_client=self.index_client)
         return _id
 
     def test_store_bundles_bundle_name_indexed(self):
 
=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-10-22 21:02:10 +0000
+++ charmworld/views/tests/test_api.py	2013-10-23 18:29:57 +0000
@@ -732,6 +732,9 @@
             description='',
             promulgated=False,
             branch_deleted=False,
+            first_change=None,
+            last_change=None,
+            changes=None,
         )
         self.assertEqual(expected, bundle._representation)
 
@@ -858,7 +861,7 @@
         self.assertEqual(u'text/plain', response.content_type)
 
     def test_bundle_icon(self):
-        # The current design is that all bundles, regardless of their id, will
+        # The current design is that all bundles, regardless of their ID, will
         # get the default bundle icon.
         bundle = self.makeBundle()
         response = self.get_response(