Add download ("deployment") stats to bundle pages.

Add download ("deployment") stats to bundle pages.



https://code.launchpad.net/~benji/charmworld/display-bundle-deployment-stats/+merge/195104

(do not edit description out of merge proposal)

[benji, 2013-11-13 17:48:49]

Please take a look.

[benji, 2013-11-13 18:01:36]

I've added some comments for the reviewer.

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py#newcode2008
charmworld/models.py:2008: def retrieve(self, key, create=False):
Since multiple callers now want to create the metric if it doesn't exist I moved
the code into this method.

https://codereview.appspot.com/26140043/diff/1/charmworld/templates/bundle.pt
File charmworld/templates/bundle.pt (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/templates/bundle.pt...
charmworld/templates/bundle.pt:26: <metal:content fill-slot="content">
I found all the "metal:block"s in this template confusing so I changed this one
which surrounds the code I added.

https://codereview.appspot.com/26140043/diff/1/charmworld/tests/test_utils.py
File charmworld/tests/test_utils.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/tests/test_utils.py...
charmworld/tests/test_utils.py:338: 
I extracted the code to generate metrics keys into a function.  These are the
tests for that new function.

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py
File charmworld/utils.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode12
charmworld/utils.py:12: import os
The code below was confusing just using the bare name "path" so I changed it to
just import the "os" module.

https://codereview.appspot.com/26140043/diff/1/charmworld/views/bundles.py
File charmworld/views/bundles.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/views/bundles.py#ne...
charmworld/views/bundles.py:31: 
Instead of piling more junk into the BundleDetail class I made a separate helper
for exposing the metric statistics to the template.

[bac, 2013-11-13 18:45:35]

LGTM but I'm in the middle of something and cannot QA right now.

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py#newcode1867
charmworld/models.py:1867: bundle_id_revision_regex = re.compile(r'/\d+(?=/)')
Please comment the regex for your slower colleagues and future self.  :)

https://codereview.appspot.com/26140043/diff/1/charmworld/templates/bundle.pt
File charmworld/templates/bundle.pt (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/templates/bundle.pt...
charmworld/templates/bundle.pt:26: <metal:content fill-slot="content">
thanks.  i really like using <metal:unique_thing> but i think many people think
'block' is magic.

https://codereview.appspot.com/26140043/diff/1/charmworld/tests/test_utils.py
File charmworld/tests/test_utils.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/tests/test_utils.py...
charmworld/tests/test_utils.py:355: def
test_not_confusing_numbers_with_versions(self):
That answers the question I had about incrementing specific versions of bundle
ids.

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py
File charmworld/utils.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode12
charmworld/utils.py:12: import os
thanks.  i've never seen from os import path.  it's always 'os.path'!

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode214
charmworld/utils.py:214: id_revision_regex = re.compile(r'/\d+(?=/)')
This looks a lot like the one you just added to models.py (though I can't say
for sure since RV won't let me see both at once).

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode223
charmworld/utils.py:223: # Metrics are stored for the versionless bundle ID
full-stop

https://codereview.appspot.com/26140043/diff/1/charmworld/views/bundles.py
File charmworld/views/bundles.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/views/bundles.py#ne...
charmworld/views/bundles.py:31: 
Good idea.

[benji, 2013-11-13 19:25:18]

These were good review comments.  I have addressed them all and will submit the
branch.

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/models.py#newcode1867
charmworld/models.py:1867: bundle_id_revision_regex = re.compile(r'/\d+(?=/)')
On 2013/11/13 18:45:35, bac wrote:
> Please comment the regex for your slower colleagues and future self.  :)

My future self can read regexes even better than my present self, but for the
sake of the other set I have commented the regex.

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py
File charmworld/utils.py (right):

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode214
charmworld/utils.py:214: id_revision_regex = re.compile(r'/\d+(?=/)')
On 2013/11/13 18:45:35, bac wrote:
> This looks a lot like the one you just added to models.py (though I can't say
> for sure since RV won't let me see both at once).

Yeah, this is the right place for it.  The other was a forlorn copy left when I
moved it into here.  I have removed the other (and moved its shiny new comment
over here).

https://codereview.appspot.com/26140043/diff/1/charmworld/utils.py#newcode223
charmworld/utils.py:223: # Metrics are stored for the versionless bundle ID
On 2013/11/13 18:45:35, bac wrote:
> full-stop

Good catch.

[benji, 2013-11-13 19:40:22]

Please take a look.