Code review comment for lp:~gmb/launchpad/jobbifiy-bug-heat-calculations-509193

Hi Graham-

  This branch looks good. I have a few comments.

=== added file 'cronscripts/calculate-bug-heat.py'
--- cronscripts/calculate-bug-heat.py 1970-01-01 00:00:00 +0000
+++ cronscripts/calculate-bug-heat.py 2010-01-21 18:28:18 +0000
@@ -0,0 +1,33 @@
+#!/usr/bin/python2.5
+#
+# Copyright 2009 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# pylint: disable-msg=W0403
+

It's 2010 now. :)

=== added file 'lib/lp/bugs/model/bugjob.py'
--- lib/lp/bugs/model/bugjob.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/model/bugjob.py 2010-01-21 18:28:18 +0000
@@ -0,0 +1,158 @@
+# Copyright 2009 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Job classes related to BugJobs are in here."""
+
+__metaclass__ = type
+__all__ = [
+ 'BugJob',
+ ]
+
+import simplejson
+
+from sqlobject import SQLObjectNotFound
+from storm.base import Storm
+from storm.expr import And
+from storm.locals import Int, Reference, Unicode
+
+from zope.component import getUtility
+from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.database.enumcol import EnumCol
+from canonical.launchpad.webapp.interfaces import (
+ DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
+
+from lazr.delegates import delegates
+
+from lp.bugs.interfaces.bugjob import BugJobType, IBugJob
+from lp.bugs.model.bug import Bug
+from lp.services.job.model.job import Job
+from lp.services.job.runner import BaseRunnableJob
+
+
+class BugJob(Storm):
+ """Base class for jobs related to Bugs."""
+
+ implements(IBugJob)
+
+ __storm_table__ = 'BugJob'
+
+ id = Int(primary=True)
+
+ job_id = Int(name='job')
+ job = Reference(job_id, Job.id)
+
+ bug_id = Int(name='bug')
+ bug = Reference(bug_id, Bug.id)
+
+ job_type = EnumCol(enum=BugJobType, notNull=True)
+
+ _json_data = Unicode('json_data')
+
+ @property
+ def metadata(self):
+ return simplejson.loads(self._json_data)
+
+ def __init__(self, bug, job_type, metadata):
+ """Constructor.
+
+ :param bug: The proposal this job relates to.
+ :param job_type: The BugJobType of this job.
+ :param metadata: The type-specific variables, as a JSON-compatible
+ dict.
+ """
+ Storm.__init__(self)
+ json_data = simplejson.dumps(metadata)
+ self.job = Job()
+ self.bug = bug
+ self.job_type = job_type
+ # XXX AaronBentley 2009-01-29 bug=322819: This should be a bytestring,
+ # but the DB representation is unicode.
+ self._json_data = json_data.decode('utf-8')
+
+ @classmethod
+ def get(cls, key):
+ """Return the instance of this class whose key is supplied."""
+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ instance = store.get(cls, key)
+ if instance is None:
+ raise SQLObjectNotFound(
+ 'No occurrence of %s has key %s' % (cls.__name__, key))
+ return instance
+
+
+class BugJobDerived(BaseRunnableJob):
+ """Intermediate class for deriving from BugJob."""
+ delegates(IBugJob)
+
+ def __init__(self, job):
+ self.context = job
+
+ # We need to define __eq__ and __ne__ here to prevent the security
+ # proxy from mucking up our comparisons in tests and elsewhere.
+
+ def __eq__(self, job):
+ return (
+ self.__class__ is removeSecurityProxy(job.__class__)
+ and self.job == job.job)
+
+ def __ne__(self, job):
+ return not (self == job)
+
+ @classmethod
+ def create(cls, bug):
+ """See `ICalculateBugHeatJobSource`."""

Why see ICalculateBugHeatJobSource when this delegates to IBugJob - Is this a class that could use a better name?

+ # If there's already a job for the bug, don't create a new one.
+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ job_for_bug = store.find(
+ BugJob,
+ BugJob.bug == bug,
+ BugJob.job_type == cls.class_job_type,
+ BugJob.job == Job.id,
+ Job.id.is_in(Job.ready_jobs)
+ ).any()
+
+ if job_for_bug is not None:
+ return cls(job_for_bug)
+ else:
+ job = BugJob(bug, cls.class_job_type, {})
+ return cls(job)

The branch jobs that only need one job raise AssertionErrors in the BranchJob code. What was your reasoning for returning the existing job?