Code review comment for lp:~bac/charmworld/add-stats-to-api

Reviewers: mp+195991_code.launchpad.net,

Message:
Please take a look.

Description:
Add download stats to bundle API call.

https://code.launchpad.net/~bac/charmworld/add-stats-to-api/+merge/195991

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/29850043/

Affected files (+58, -31 lines):
   A [revision details]
   M charmworld/views/api/__init__.py
   M charmworld/views/bundles.py
   M charmworld/views/tests/test_api.py
   M charmworld/views/tests/test_bundles.py
   M charmworld/views/utils.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131119203355-yu8j8hqzmyv8f9d1
+New revision: <email address hidden>

Index: charmworld/views/bundles.py
=== modified file 'charmworld/views/bundles.py'
--- charmworld/views/bundles.py 2013-11-13 17:40:11 +0000
+++ charmworld/views/bundles.py 2013-11-20 18:08:03 +0000
@@ -8,12 +8,8 @@
  from charmworld import cached_view_config
  from charmworld.models import (
      Bundle,
- DatedMetricSource,
-)
-from charmworld.utils import (
- build_metric_key,
- utc_today,
-)
+)
+from charmworld.views.utils import get_bundle_downloads
  from charmworld.views.api import json_response
  from charmworld.views.helpers import (
      find_bundle,
@@ -29,17 +25,6 @@
  SUPPORTED_README_EXTS = ('', '.txt', '.md', '.markdown', '.mkd', '.rst')

-class Statistics:
- """A template helper that wraps a DatedMetric instance."""
-
- def __init__(self, metric, day=None):
- day = day or utc_today()
- self.past_7_days = metric.get_range_total(7, day)
- self.past_30_days = metric.get_range_total(30, day)
- self.past_half_year = metric.get_range_total(365 / 2, day)
- self.total = metric.get_total()
-
-
  class BundleDetail(Bundle):

      service_keys = (
@@ -113,11 +98,8 @@

  def _bundle_view(request, bundle):
- source = DatedMetricSource.from_db(request.db)
- metric_key = build_metric_key('deployments', bundle.id)
- metric = source.retrieve(metric_key, create=True)
+ downloads = get_bundle_downloads(request.db, bundle.id)
      bundle = BundleDetail(dict(bundle))
- downloads = Statistics(metric)
      try:
          is_owner = bool((request.user is not None) and
                          (request.user.nickname == bundle.owner))

Index: charmworld/views/utils.py
=== modified file 'charmworld/views/utils.py'
--- charmworld/views/utils.py 2013-10-20 19:09:02 +0000
+++ charmworld/views/utils.py 2013-11-20 17:16:29 +0000
@@ -1,5 +1,10 @@
  import json
  from webob import Response
+from charmworld.models import DatedMetricSource
+from charmworld.utils import (
+ build_metric_key,
+ utc_today,
+)

  def json_response(status, value, headers=[]):
@@ -31,3 +36,22 @@

('Access-Control-Allow-Headers', 'X-Requested-With'),
                      ] + headers,
                      status_code=status)
+
+
+class Statistics:
+ """A template helper that wraps a DatedMetric instance."""
+
+ def __init__(self, metric, day=None):
+ day = day or utc_today()
+ self.past_7_days = metric.get_range_total(7, day)
+ self.past_30_days = metric.get_range_total(30, day)
+ self.past_half_year = metric.get_range_total(365 / 2, day)
+ self.total = metric.get_total()
+
+
+def get_bundle_downloads(db, bundle_id):
+ """Return a Statistics object for the bundle's downloads."""
+ source = DatedMetricSource.from_db(db)
+ metric_key = build_metric_key('deployments', bundle_id)
+ metric = source.retrieve(metric_key, create=True)
+ return Statistics(metric)

Index: charmworld/views/api/__init__.py
=== modified file 'charmworld/views/api/__init__.py'
--- charmworld/views/api/__init__.py 2013-11-13 17:40:11 +0000
+++ charmworld/views/api/__init__.py 2013-11-20 18:08:03 +0000
@@ -45,7 +45,10 @@
      timestamp,
  )
  from charmworld.views.api.proof import proof_deployer
-from charmworld.views.utils import json_response
+from charmworld.views.utils import (
+ get_bundle_downloads,
+ json_response,
+)

  @view_config(route_name="search-json-obsolete")
@@ -428,6 +431,12 @@
              owner=bundle_dict['owner'],
              rev=bundle_dict['basket_revision'],
          )
+
+ # Add in download data.
+ downloads = get_bundle_downloads(db, bundle.id)
+ bundle_dict['downloads'] = downloads.total
+ bundle_dict['downloads_in_past_30_days'] = downloads.past_30_days
+
          service_data = bundle_dict.get('data')
          # Now load the charm information we require for the services in the
          # bundle.

Index: charmworld/views/tests/test_api.py
=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py 2013-11-08 19:14:29 +0000
+++ charmworld/views/tests/test_api.py 2013-11-20 18:08:03 +0000
@@ -786,23 +786,29 @@
          response = self.get_response('bundle', 'byobu/bat')
          self.assertEqual(200, response.status_code)

- def test_results_match(self):
+ def test_results_are_correct(self):
          self.enable_routes()
          # Make the charm that we'll use as a service.
- _id, charm = factory.makeCharm(
- self.db,
- description=''
- )
+ _id, charm = factory.makeCharm(self.db, description='')
          services = {
              u'charm': {
                  u'branch': charm['branch_spec'],
- u'charm': charm['name']
+ u'charm': charm['name'],
              }
          }
          self.makeBundle(
              name='bat', owner='bac',
              basket_with_rev='byobu/4', services=services)
- response = self.get_response('bundle', '~bac/byobu/4/bat')
+
+ class FakeObject(object):
+ pass
+
+ fake_stat = FakeObject
+ fake_stat.total = 10
+ fake_stat.past_30_days = 5
+ mock_path = 'charmworld.views.api.get_bundle_downloads'
+ with patch(mock_path, return_value=fake_stat):
+ response = self.get_response('bundle', '~bac/byobu/4/bat')
          self.assertEqual(200, response.status_code)
          deployer_url = u'http://example.com/bundle/%7Ebac/byobu/4/bat/json'
          self.assertEqual(
@@ -812,7 +818,7 @@
                  u'basket_revision': 4,
                  u'branch_deleted': False,
                  u'charm_metadata': {
- u'charm': self.api_class._format_charm(Charm(charm))
+ u'charm': self.api_class._format_charm(Charm(charm)),
                  },
                  u'data': {
                      u'series': u'precise',
@@ -837,6 +843,8 @@
                  u'promulgated': False,
                  u'title': u'',
                  u'deployer_file_url': deployer_url,
+ u'downloads': 10,
+ u'downloads_in_past_30_days': 5,
              })

      def test_extracting_bundle_id_with_trailing_full_id(self):

Index: charmworld/views/tests/test_bundles.py
=== modified file 'charmworld/views/tests/test_bundles.py'
--- charmworld/views/tests/test_bundles.py 2013-11-13 17:40:11 +0000
+++ charmworld/views/tests/test_bundles.py 2013-11-20 18:08:03 +0000
@@ -17,12 +17,14 @@
  )
  from charmworld.views.bundles import (
      BundleDetail,
- Statistics,
      find_bundle,
      official_bundle_json,
      personal_bundle,
      personal_bundle_json,
  )
+from charmworld.views.utils import (
+ Statistics,
+)

  class TestBundleDetail(ViewTestBase):