On Thu, Jan 21, 2010 at 07:17:01PM -0000, Paul Hummer wrote:
> Review: Needs Information code
> 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.  :)
> 

Erk. Fixed.

> === 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?

Because this should actually be on the CalculateBugHeatJob class :).
I've moved it and added a nice generic create() method to BugJobDerived.

> +        # 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?

See above about this being in the wrong place.

This is used specifically when creating new CalculateBugHeatJobs. These
will be created by the malone UI whenever a bug gets touched, so if we
don't guard against multiple jobs for a bug the queue would get very
long very quickly. However, we don't want to raise an AssertionError
here because the assertion is pointless in an environment where we
expect something to try to create more than one job for a bug.