Merge lp:~adeuring/charmworld/1199790-last-change-error into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

Return dummy data for Charm.last_change and Charm.first_change, if a branch does not have any commits.

This branch fixes bug 1199780: search requests errors when a charm has no
last_change

While the root cause of the error described in the bug report (a branch is
registered in LP but no real branch exists) may be fixed by recent changes
to the ingest job, a similar situation may still exist: A branch that
exists but that has no commits.

This problem affects not only the API views but also views which
use the function views.helpers.format_change().

I changed class Charm in models.py so that it does not return None in
this case but some dummy data.

The function format_change() still needs a change: Calling
datetime.strftime() obviously does not work if for non-existent
timestamp -- and using a default timestamp does not make sense either.

[Aaron Bentley (abentley)]

This is fine as a first step, but I hope there will be a follow-on that changes all the views to check for None when accessing Charm.last_change and Charm.first_change, since that will look better.

review: Approve

[Charmworld Lander (charmworld-lander)]

No commit message specified.

[Abel Deuring (adeuring)]

review: Approve

[Charmworld Lander (charmworld-lander)]

Attempt to merge into lp:charmworld failed due to conflicts:

text conflict in charmworld/testing/factory.py

[Abel Deuring (adeuring)]

review: Approve

[Charmworld Lander (charmworld-lander)]

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

[Abel Deuring (adeuring)]

review: Approve

[Abel Deuring (adeuring)]

I think there is no need for further changes. format_change() is used in templates/charm.pt, but not for first_change or last_change:

              <tr tal:repeat="change charm.changes">
                <td tal:content="structure: format_change(change, charm)"></td>
              </tr>

The second callsite of format_change is in views.feeds._recently_changed(), which in turn is used in recently_changed()
 serving the URL http://staging.jujucharms.com/recently-changed and in recently_changed_feed(), serving the URL http://staging.jujucharms.com/feed/charm-changes. IMHO, an empty string dor the last change does not matter on these pages.

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-07-30 19:14:07 +0000
+++ charmworld/models.py	2013-07-31 17:04:21 +0000
@@ -470,15 +470,30 @@
         """The list of revision info dicts from the charm's branch."""
         return self._representation['changes']
 
+    def _get_change(self, name):
+        # Some view functions assume that last_change and first_change
+        # always exist, but this is not true for empty branches. Return
+        # some default data in this case.
+        change = self._representation[name]
+        if change is None:
+            return {
+                'authors': [],
+                'revno': 0,
+                'committer': '',
+                'created': None,
+                'message': '',
+            }
+        return change
+
     @property
     def first_change(self):
         """The revision dict of the first change to the charm's branch."""
-        return self._representation['first_change']
+        return self._get_change('first_change')
 
     @property
     def last_change(self):
         """The revision dict of the last change to the charm's branch."""
-        return self._representation['last_change']
+        return self._get_change('last_change')
 
     @property
     def revision(self):

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-07-31 14:41:29 +0000
+++ charmworld/testing/factory.py	2013-07-31 17:04:21 +0000
@@ -172,7 +172,7 @@
                    date_created=None, downloads=0,
                    downloads_in_past_30_days=0, is_featured=False,
                    promulgated=False, categories=None, branch_deleted=False,
-                   proof=None, payload=None):
+                   proof=None, payload=None, empty_branch=False):
     """Return the json of a charm."""
     if not description:
         description = """byobu-class provides remote terminal access through
@@ -254,12 +254,6 @@
         "changes": changes,
         "description": description,
         "doctype": "charm",
-        "first_change": {
-            "committer": "Jane Doe <[e-mail]>",
-            "created": 1308093138.892,
-            "message": "initial checkin\n",
-            "revno": 1
-        },
         "hooks": [
             "install",
             "start",
@@ -267,12 +261,6 @@
             "website-relation-joined"
         ],
         'is_featured': is_featured,
-        "last_change": {
-            "committer": "John Doe <[e-mail]>",
-            "created": 1343082725.06,
-            "message": commit_message,
-            "revno": revno,
-        },
         "maintainer": maintainer,
         "proof": proof,
         "provides": provides,
@@ -287,6 +275,22 @@
         'downloads_in_past_30_days': downloads_in_past_30_days,
         'date_created': date_created.isoformat(),
     })
+    if empty_branch:
+        charm["first_change"] = None
+        charm["last_change"] = None
+    else:
+        charm["first_change"] = {
+            "committer": "Jane Doe <[e-mail]>",
+            "created": 1308093138.892,
+            "message": "initial checkin\n",
+            "revno": 1
+        }
+        charm["last_change"] = {
+            "committer": "John Doe <[e-mail]>",
+            "created": 1343082725.06,
+            "message": commit_message,
+            "revno": revno,
+        }
     if tests is not None:
         charm['tests'] = tests
         charm['test_results'] = {}

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-07-30 19:14:07 +0000
+++ charmworld/tests/test_models.py	2013-07-31 17:04:21 +0000
@@ -346,9 +346,16 @@
         charm_data['first_change'] = change
         charm = Charm(charm_data)
         self.assertIs(change, charm.first_change)
-        # The default is None.
+        # The default is some dummy data.
         charm = Charm({})
-        self.assertIs(None, charm.first_change)
+        expected = {
+            'authors': [],
+            'committer': '',
+            'created': None,
+            'message': '',
+            'revno': 0,
+        }
+        self.assertEqual(expected, charm.first_change)
 
     def test_last_change(self):
         # The last_change property is a revision info dict.
@@ -357,9 +364,16 @@
         charm_data['last_change'] = change
         charm = Charm(charm_data)
         self.assertIs(change, charm.last_change)
-        # The default is None.
+        # The default is some dummy data.
         charm = Charm({})
-        self.assertIs(None, charm.last_change)
+        expected = {
+            'authors': [],
+            'committer': '',
+            'created': None,
+            'message': '',
+            'revno': 0,
+        }
+        self.assertEqual(expected, charm.last_change)
 
     def test_revision(self):
         # The revision property is an int.

=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2013-07-24 15:05:38 +0000
+++ charmworld/views/charms.py	2013-07-31 17:04:21 +0000
@@ -124,7 +124,6 @@
             endpoint='charm'
         )
         icon_path = icon_path + '/' + remainder
-
     return {
         "charm": charm,
         'format_change': format_change,

=== modified file 'charmworld/views/helpers.py'
--- charmworld/views/helpers.py	2013-07-24 20:01:52 +0000
+++ charmworld/views/helpers.py	2013-07-31 17:04:21 +0000
@@ -13,8 +13,13 @@
 def format_change(change, charm=None):
     """Format a bzr commit."""
     parts = []
-    date = datetime.utcfromtimestamp(change["created"])
-    parts.append(unicode(date.strftime("%Y/%m/%d")))
+    date = change["created"]
+    # bug 1199790: We may have a branch without any commits.
+    # In this case, class Charm returns dummy data for its attributes
+    # first_change and last_change.
+    if date is not None:
+        date = datetime.utcfromtimestamp(date)
+        parts.append(unicode(date.strftime("%Y/%m/%d")))
     parts.append(name_filter(change['committer']))
     parts.append(change["message"][:100])
     if charm is not None:

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-07-26 14:33:11 +0000
+++ charmworld/views/tests/test_api.py	2013-07-31 17:04:21 +0000
@@ -8,6 +8,7 @@
     datetime,
 )
 import hashlib
+import json
 
 from pyramid.httpexceptions import HTTPNotFound
 from webob.etag import ETagMatcher
@@ -607,6 +608,17 @@
         self.assertNotIn('related', data['result']['popular'][0]['metadata'])
         self.assertNotIn('related', data['result']['featured'][0]['metadata'])
 
+    def test_charm_with_empty_branch(self):
+        # Empty branches do not have real data for the attributes
+        # first_change and last_change. The view is nevertheless properly
+        # rendered.
+        self.makeCharm(empty_branch=True)
+        result = json.loads(self.get_response('charms').body)['result'][0]
+        # But the returned data does contain only dummy revision related
+        # data.
+        self.assertEqual('0', result['charm']['code_source']['revision'])
+        self.assertEqual('', result['charm']['code_source']['last_log'])
+
 
 class TestAPI2Charms(TestAPICharms, API2Mixin):
     """Test API 2's "charms" endpoint."""

=== modified file 'charmworld/views/tests/test_helpers.py'
--- charmworld/views/tests/test_helpers.py	2013-06-26 12:20:17 +0000
+++ charmworld/views/tests/test_helpers.py	2013-07-31 17:04:21 +0000
@@ -3,6 +3,7 @@
 
 from charmworld.models import Charm
 from charmworld.views.helpers import (
+    format_change,
     interfaces,
     result_sorter,
 )
@@ -129,3 +130,13 @@
         charm_b = factory.get_charm_json(series='raring')
         with self.assertNoException():
             result_sorter(charm_a, charm_b)
+
+    def test_format_change(self):
+        charm = Charm(factory.get_charm_json())
+        result = format_change(charm.last_change)
+        self.assertEqual('2012/07/23 John Doe maintainer', result)
+
+    def test_format_change_empty_branch(self):
+        charm = Charm(factory.get_charm_json(empty_branch=True))
+        result = format_change(charm.last_change)
+        self.assertEqual(' ', result)