Skip to content

Making the version foo independent of code (Bug: 659102) #29

Merged
merged 1 commit into from Oct 24, 2011

3 participants

@nigelbabu

First cut of the first step of the change.

@tofumatt tofumatt and 1 other commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
@@ -91,3 +92,35 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ versions = (
+ Opinion.objects.filter(
+ created__gte=(datetime.datetime.now() - datetime.timedelta(30)))
+ .values('product', 'version').annotate(count=models.Count('id')))
+ for one_version in versions:
+ exists = True
@tofumatt
Mozilla member
tofumatt added a note Oct 20, 2011

You could move the if exists: block into the try and the code in else: into the except block.

@nigelbabu
nigelbabu added a note Oct 20, 2011

I thought it wasn't clean. Moving!

@tofumatt
Mozilla member
tofumatt added a note Oct 20, 2011

Well, exceptions for control flow are odd... you could do objects.filter or get_object_or_none so you didn't generate an exception then check to see if you got a version. That's probably best, but having both the if/else and try/except seems clunky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 1 other commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
+ exists = True
+ try:
+ version_count = VersionCount.objects.get(
+ product=one_version['product'],
+ version=one_version['version'])
+ except VersionCount.DoesNotExist:
+ exists = False
+ if exists:
+ version_count.count = one_version['count']
+ else:
+ v = Version(one_version['version'])
+ version_count = VersionCount(product=one_version['product'],
+ version=one_version['version'],
+ version_int=v._version_int,
+ count=one_version['count'])
+ print (version_count.product,
@tofumatt
Mozilla member
tofumatt added a note Oct 20, 2011

Is this printing because it's supposed to output somewhere for the cron job? That seems odd, but should at least be documented as to why there's a print statement here.

@nigelbabu
nigelbabu added a note Oct 20, 2011

Product of my debugging. I'll remove :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 2 others commented on an outdated diff Oct 20, 2011
apps/feedback/models.py
@@ -212,3 +212,15 @@ class Term(ModelBase):
class Meta:
ordering = ('term',)
+
+class VersionCount(ModelBase):
+ """Product versions and their usage."""
+ product = models.PositiveSmallIntegerField(db_index=True)
+ version = models.CharField(max_length=30, db_index=True)
+ version_int = models.BigIntegerField(db_index=True)
+ count = models.IntegerField()
@tofumatt
Mozilla member
tofumatt added a note Oct 20, 2011

What is this counting?

@nigelbabu
nigelbabu added a note Oct 20, 2011

The idea is upto a certain count, we count that version for that product as active. That's what its counting.

@davedash
Mozilla member
davedash added a note Oct 20, 2011

num_opinions?

@tofumatt
Mozilla member
tofumatt added a note Oct 21, 2011

Yeah, if this the (denormalized) count of opinions, label it a bit more explicitly? And/or add a comment; I don't think we often denormalize stuff like this so it'd be good to mark why we do it.

Or I've misunderstood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
@@ -7,8 +7,9 @@ from django.db import transaction, models
import cronjobs
import input
-from feedback.models import Opinion, extract_terms
+from feedback.models import Opinion, extract_terms, VersionCount
@davedash
Mozilla member
davedash added a note Oct 20, 2011

nitpick you say? sort the imports :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash and 1 other commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
@@ -91,3 +92,35 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ versions = (
@davedash
Mozilla member
davedash added a note Oct 20, 2011

This statement is 4 lines... maybe we should break it down.

also you can do this:

>>> Entry.objects.filter(mod_date__gt=F('pub_date') + timedelta(days=3))

Also the indenting just seems weird.

@nigelbabu
nigelbabu added a note Oct 20, 2011

Yeah, I don't know how to clean up the indentation while still obeying the style guide.

@davedash
Mozilla member
davedash added a note Oct 20, 2011

paste it in a gist and let's figure it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
@@ -91,3 +92,28 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ versions = (
+ Opinion.objects.filter(
+ created__gte=(datetime.datetime.now() - datetime.timedelta(30)))
+ .values('product', 'version').annotate(count=models.Count('id')))
@davedash
Mozilla member
davedash added a note Oct 20, 2011

one_Version seems odd...

I'd do version unless that's taken... or maybe v even...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
@@ -91,3 +92,28 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ versions = (
+ Opinion.objects.filter(
+ created__gte=(datetime.datetime.now() - datetime.timedelta(30)))
+ .values('product', 'version').annotate(count=models.Count('id')))
+ for one_version in versions:
+ try:
+ version_count = VersionCount.objects.get(
+ product=one_version['product'],
@davedash
Mozilla member
davedash added a note Oct 20, 2011

if these are the only items inthe dictionary you could do **v

@davedash
Mozilla member
davedash added a note Oct 20, 2011

oh but you've got count... nevermind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
+ versions = (
+ Opinion.objects.filter(
+ created__gte=(datetime.datetime.now() - datetime.timedelta(30)))
+ .values('product', 'version').annotate(count=models.Count('id')))
+ for one_version in versions:
+ try:
+ version_count = VersionCount.objects.get(
+ product=one_version['product'],
+ version=one_version['version'])
+ version_count.count = one_version['count']
+ except VersionCount.DoesNotExist:
+ v = Version(one_version['version'])
+ version_count = VersionCount(product=one_version['product'],
+ version=one_version['version'],
+ version_int=v._version_int,
+ count=one_version['count'])
@davedash
Mozilla member
davedash added a note Oct 20, 2011

calculate the activeness ahead of time

and then you can use ...objects.create()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
apps/feedback/cron.py
+ Opinion.objects.filter(
+ created__gte=(datetime.datetime.now() - datetime.timedelta(30)))
+ .values('product', 'version').annotate(count=models.Count('id')))
+ for one_version in versions:
+ try:
+ version_count = VersionCount.objects.get(
+ product=one_version['product'],
+ version=one_version['version'])
+ version_count.count = one_version['count']
+ except VersionCount.DoesNotExist:
+ v = Version(one_version['version'])
+ version_count = VersionCount(product=one_version['product'],
+ version=one_version['version'],
+ version_int=v._version_int,
+ count=one_version['count'])
+ if version_count.count > settings.MINIMUM_ENTRIES:
@davedash
Mozilla member
davedash added a note Oct 20, 2011

also version_count.active = (version_count.count >= settings.MINIMUM_ENTRIES)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
migrations/16-versions.sql
@@ -0,0 +1,9 @@
+CREATE TABLE `version_count` (
+ `id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY,
+ `product` smallint UNSIGNED NOT NULL,
+ `version` varchar(30) NOT NULL,
+ `version_int` bigint NOT NULL,
+ `count` integer NOT NULL,
+ `active` bool NOT NULL,
+ UNIQUE (`product`, `version`)
@davedash
Mozilla member
davedash added a note Oct 20, 2011

I don't see the index for active or version_int
also those could be indexed together... find out why django didn't produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 20, 2011
settings.py
@@ -347,3 +347,6 @@ ES_DISABLED = True
# (good for testing)
ENFORCE_USER_AGENT = True
DISABLE_TERMS = False
+
+# Minimum number of entries for active status
+MINIMUM_ENTRIES = 1000
@davedash
Mozilla member
davedash added a note Oct 20, 2011

MINIMUM_ENTRIES_PER_ACTIVE_VERSION <-- super verbose... but the above is more ambiguous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt
Mozilla member

Looking good.

Because I don't see the code being used anywhere yet it's still a bit weird; I can't actually see it in action. Also, VersionCount in general is a denormalized model in general if I understand; the reason for doing this should be clearly stated in comments somewhere (the VersionCount object docstring?), because denormalized model isn't "usual".

@tofumatt tofumatt commented on an outdated diff Oct 21, 2011
apps/feedback/models.py
@@ -212,3 +213,28 @@ class Term(ModelBase):
class Meta:
ordering = ('term',)
+
+
+class VersionCount(ModelBase):
+ """
+ Create a denormalized table of product, versions, number of
@tofumatt
Mozilla member
tofumatt added a note Oct 21, 2011

First line should be one line and a sentence, but this is the right idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 21, 2011
apps/feedback/cron.py
@@ -91,3 +92,25 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ """Cron to activate and deactivate product versions"""
@davedash
Mozilla member
davedash added a note Oct 21, 2011

docstrings should be sentences. with a full stop at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 21, 2011
apps/feedback/cron.py
@@ -91,3 +92,25 @@ def populate(num_opinions=None, product='mobile', type=None, locale=None):
models.signals.post_save.connect(extract_terms, sender=Opinion,
dispatch_uid='extract_terms')
+
+
+@cronjobs.register
+def version_counter():
+ """Cron to activate and deactivate product versions"""
+ thirtydaysago = datetime.datetime.now() - datetime.timedelta(30)
+ versions = (Opinion.objects.filter(created__gte=(thirtydaysago))
+ .values('product', 'version')
+ .annotate(count=models.Count('id')))
+ for version in versions:
+ try:
+ version_count = VersionCount.objects.get(
@davedash
Mozilla member
davedash added a note Oct 21, 2011

ah, now that I can follow this,

you could do

get_or_create()

and avoid the try/except. You may need to set a default for count, but that shouldn't be too hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on the diff Oct 21, 2011
apps/feedback/models.py
+ version = models.CharField(max_length=30, db_index=True)
+ version_int = models.BigIntegerField(db_index=True)
+ num_opinions = models.IntegerField()
+ active = models.BooleanField(db_index=True)
+
+ class Meta:
+ unique_together = (('product', 'version'))
+ db_table = 'version_count'
+
+
+def update_version_int(sender, instance, **kwargs):
+ if not instance.pk:
+ v = Version(instance.version)
+ instance.version_int = v._version_int
+
+
@davedash
Mozilla member
davedash added a note Oct 21, 2011

funfact, you can use decorator syntax too :)

but I wouldn't fault you for doing it this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 21, 2011
settings.py
@@ -347,3 +347,6 @@ ES_DISABLED = True
# (good for testing)
ENFORCE_USER_AGENT = True
DISABLE_TERMS = False
+
+# Minimum number of entries for active status
@davedash
Mozilla member
davedash added a note Oct 21, 2011

comment could be: "Min num of opinions in the last 30 days for version to be shown in dashboard"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 21, 2011
apps/feedback/cron.py
+ """Cron to activate and deactivate product versions."""
+ thirtydaysago = datetime.datetime.now() - datetime.timedelta(30)
+ versions = (Opinion.objects.filter(created__gte=(thirtydaysago))
+ .values('product', 'version')
+ .annotate(count=models.Count('id')))
+ for version in versions:
+ version_count, created = VersionCount.objects.get_or_create(
+ product=version['product'],
+ version=version['version'],
+ defaults={
+ 'num_opinions':version['count']})
+ if not created:
+ version_count.num_opinions = version['count']
+ version_count.active = (
+ version_count.num_opinions >= settings.MINIMUM_ENTRIES_PER_VERSION)
+ version_count.save()
@davedash
Mozilla member
davedash added a note Oct 21, 2011

5 -line statements are tough, let's try this:

https://gist.github.com/1304503

My heuristics are this:

  • use small variable names if the scope is fairly limited.
  • use the indent 8 spaces in rule of pep8 if you can't fit things nicely
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash davedash commented on an outdated diff Oct 21, 2011
apps/feedback/cron.py
+
+@cronjobs.register
+def version_counter():
+ """Cron to activate and deactivate product versions."""
+ thirtydaysago = datetime.datetime.now() - datetime.timedelta(30)
+ versions = (Opinion.objects.filter(created__gte=(thirtydaysago))
+ .values('product', 'version')
+ .annotate(count=models.Count('id')))
+ for version in versions:
+ vc, created = VersionCount.objects.get_or_create(
+ product=version['product'], version=version['version'],
+ defaults={'num_opinions':version['count']})
+ if not created:
+ vc.num_opinions = version['count']
+ vc.active = (
+ vc.num_opinions >= settings.MINIMUM_ENTRIES_PER_VERSION)
@davedash
Mozilla member
davedash added a note Oct 21, 2011

This doesn't fit on one line?

Oh because it's called num_opinions... sigh ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davedash
Mozilla member

Add this patch and rebase:

http://paste.pocoo.org/show/496264/

@davedash
Mozilla member

with the patch we get some awesome results, thanks for this, it'll save some headache.

@davedash davedash merged commit d619846 into mozilla:master Oct 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.