Hi Edwin,

Very interesting branch.  I've got some suggestions but overall it is great.

--Brad

> === added file 'lib/lp/registry/model/persontransferjob.py'
> --- lib/lp/registry/model/persontransferjob.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/model/persontransferjob.py	2010-09-17 18:59:05 +0000
> @@ -0,0 +1,322 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Job classes related to PersonTransferJob are in here."""

s/ are in here//

> +__metaclass__ = type
> +__all__ = [
> +    'MembershipNotificationJob',
> +    'PersonTransferJob',
> +    ]
> +
> +from lazr.delegates import delegates
> +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 (
> +    classProvides,
> +    implements,
> +    )
> +
> +

extra blank line

> +from canonical.config import config
> +from canonical.database.enumcol import EnumCol
> +from canonical.launchpad.helpers import (
> +    get_contact_email_addresses,
> +    get_email_template,
> +    )
> +from canonical.launchpad.interfaces.lpstorm import (
> +    IMasterStore,
> +    IStore,
> +    )
> +from canonical.launchpad.mail import (
> +    format_address,
> +    simple_sendmail,
> +    )
> +from canonical.launchpad.mailnotification import MailWrapper
> +from canonical.launchpad.webapp import canonical_url
> +
> +

Hmmm, two of them.  is there supposed to be two?  did you do that
manually or did henning's tool do it?  if the latter, then keep it.

> +from lp.registry.enum import PersonTransferJobType
> +from lp.registry.interfaces.person import (
> +    IPerson,
> +    IPersonSet,
> +    ITeam,
> +    )
> +from lp.registry.interfaces.persontransferjob import (
> +    IPersonTransferJob,
> +    IPersonTransferJobSource,
> +    IMembershipNotificationJob,
> +    IMembershipNotificationJobSource,
> +    )
> +from lp.registry.interfaces.teammembership import TeamMembershipStatus
> +from lp.registry.model.person import Person
> +from lp.services.job.model.job import Job
> +from lp.services.job.runner import BaseRunnableJob
> +
> +
> +class PersonTransferJob(Storm):
[...]
> +    def __init__(self, minor_person, major_person, job_type, metadata):
> +        """Constructor.
> +
> +        :param minor_person: The person or team being added to or removed
> +                             from the major_person.
> +        :param major_person: The person or team that is receiving or losing
> +                             the minor person.
> +        :param job_type: The specific membership action being performed.
> +        :param metadata: The type-specific variables, as a JSON-compatible
> +            dict.

Indent to match.

> +        """
> +        super(PersonTransferJob, self).__init__()
> +        self.job = Job()
> +        self.job_type = job_type
> +        self.major_person = major_person
> +        self.minor_person = minor_person
> +
> +        json_data = simplejson.dumps(metadata)
> +        # 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 = IMasterStore(PersonTransferJob)
> +        instance = store.get(cls, key)
> +        if instance is None:
> +            raise SQLObjectNotFound(
> +                'No occurrence of %s has key %s' % (cls.__name__, key))
> +        return instance
> +
> +
> +class PersonTransferJobDerived(BaseRunnableJob):
> +    """Intermediate class for deriving from PersonTransferJob."""

Can you add to the docstring what it is used for?  I haven't looked at
it yet and I'm unsure.
Add a blank line here.

> +    delegates(IPersonTransferJob)
> +    classProvides(IPersonTransferJobSource)
> +
> +    def __init__(self, job):
> +        self.context = job
> +
> +    def __repr__(self):
> +        return (
> +            '<%(job_type)s branch job (%(id)s) for %(minor_person)s '
> +            'as part of %(major_person)s. status=%(status)s>' % {
> +                'job_type': self.context.job_type.name,
> +                'id': self.context.id,
> +                'minor_person': self.minor_person.name,
> +                'major_person': self.major_person.name,
> +                'status': self.job.status,
> +                })
> +
> +    @classmethod
> +    def create(cls, minor_person, major_person, metadata):
> +        """See `IPersonTransferJob`."""
> +        assert IPerson.providedBy(minor_person)
> +        assert IPerson.providedBy(major_person)

These asserts should be 'raise ValueError'

> +        job = PersonTransferJob(
> +            minor_person=minor_person,
> +            major_person=major_person,
> +            job_type=cls.class_job_type,
> +            metadata=metadata)
> +        return cls(job)
> +
> +    @classmethod
> +    def iterReady(cls):
> +        """Iterate through all ready PersonTransferJobs."""
> +        store = IMasterStore(PersonTransferJob)
> +        jobs = store.find(
> +            PersonTransferJob,
> +            And(PersonTransferJob.job_type == cls.class_job_type,
> +                PersonTransferJob.job_id.is_in(Job.ready_jobs)))
> +        return (cls(job) for job in jobs)
> +
> +    def getOopsVars(self):
> +        """See `IRunnableJob`."""
> +        vars = BaseRunnableJob.getOopsVars(self)
> +        vars.extend([
> +            ('major_person_name', self.context.major_person.name),
> +            ('minor_person_name', self.context.minor_person.name),
> +            ])
> +        return vars
> +
> +
> +class MembershipNotificationJob(PersonTransferJobDerived):
> +    """A Job that sends notifications about team membership changes."""
> +
> +    implements(IMembershipNotificationJob)
> +    classProvides(IMembershipNotificationJobSource)
> +
> +    class_job_type = PersonTransferJobType.MEMBERSHIP_NOTIFICATION
> +
> +    @classmethod
> +    def create(cls, member, team, reviewer, old_status, new_status,
> +               last_change_comment=None):
> +        assert ITeam.providedBy(team)
> +        assert IPerson.providedBy(reviewer)
> +        assert old_status in TeamMembershipStatus
> +        assert new_status in TeamMembershipStatus

ValueErrors

> +        metadata = {
> +            'reviewer': reviewer.id,
> +            'old_status': old_status.name,
> +            'new_status': new_status.name,
> +            'last_change_comment': last_change_comment,
> +            }
> +        return super(MembershipNotificationJob, cls).create(
> +            minor_person=member, major_person=team, metadata=metadata)
> +

[....]
> +        elif self.new_status == TeamMembershipStatus.DECLINED:
> +            subject = '%s declined by %s' % (
> +                self.member.name, self.reviewer.name)
> +        else:
> +            # Use the default template and subject.
> +            pass
> +
> +        if admins_emails:

Please check len(admin_emails) != 0


> +            admins_template = get_email_template(
> +                "%s-bulk.txt" % template_name)
> +            for address in admins_emails:
> +                recipient = getUtility(IPersonSet).getByEmail(address)
> +                replacements['recipient_name'] = recipient.displayname
> +                msg = MailWrapper().format(
> +                    admins_template % replacements, force_wrap=True)
> +                simple_sendmail(from_addr, address, subject, msg)
> +
> +        # The self.member can be a self.self.team without any
> +        # self.members, and in this case we won't have a single email
> +        # address to send this notification to.
> +        if self.member_email and self.reviewer != self.member:
> +            if self.member.isTeam():
> +                template = '%s-bulk.txt' % template_name
> +            else:
> +                template = '%s-personal.txt' % template_name
> +            self.member_template = get_email_template(template)
> +            for address in self.member_email:
> +                recipient = getUtility(IPersonSet).getByEmail(address)
> +                replacements['recipient_name'] = recipient.displayname
> +                msg = MailWrapper().format(
> +                    self.member_template % replacements, force_wrap=True)
> +                simple_sendmail(from_addr, address, subject, msg)
> +        log.debug('MembershipNotificationJob sent email')

> === added file 'lib/lp/registry/tests/test_persontransferjob.py'
> --- lib/lp/registry/tests/test_persontransferjob.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/tests/test_persontransferjob.py	2010-09-17 18:59:05 +0000
> @@ -0,0 +1,62 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for PersonTransferJobs."""
> +
> +__metaclass__ = type
> +
> +from canonical.testing import LaunchpadZopelessLayer
> +from lp.registry.enum import PersonTransferJobType
> +from lp.registry.model.persontransferjob import (
> +    PersonTransferJob,
> +    PersonTransferJobDerived,
> +    )
> +from lp.testing import TestCaseWithFactory
> +
> +
> +class PersonTransferJobTestCase(TestCaseWithFactory):
> +    """Test case for basic PersonTransferJob class."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def test_instantiate(self):
> +        # PersonTransferJob.__init__() instantiates a
> +        # PersonTransferJob instance.
> +        person = self.factory.makePerson()
> +        team = self.factory.makeTeam()
> +
> +        metadata = ('some', 'arbitrary', 'metadata')
> +        person_transfer_job = PersonTransferJob(
> +            person,
> +            team,
> +            PersonTransferJobType.MEMBERSHIP_NOTIFICATION,
> +            metadata)
> +
> +        self.assertEqual(person, person_transfer_job.minor_person)
> +        self.assertEqual(team, person_transfer_job.major_person)
> +        self.assertEqual(
> +            PersonTransferJobType.MEMBERSHIP_NOTIFICATION,
> +            person_transfer_job.job_type)
> +
> +        # When we actually access the PersonTransferJob's metadata it gets
> +        # unserialized from JSON, so the representation returned by
> +        # bug_heat.metadata will be different from what we originally
> +        # passed in.

Is the reference to bug heat correct?

> +        metadata_expected = [u'some', u'arbitrary', u'metadata']
> +        self.assertEqual(metadata_expected, person_transfer_job.metadata)
> +
> +
> +class PersonTransferJobDerivedTestCase(TestCaseWithFactory):
> +    """Test case for the PersonTransferJobDerived class."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def test_create_explodes(self):
> +        # PersonTransferJobDerived.create() will blow up because it
> +        # needs to be subclassed to work properly.
> +        person = self.factory.makePerson()
> +        team = self.factory.makeTeam()
> +        metadata = {'foo': 'bar'}
> +        self.assertRaises(
> +            AttributeError,
> +            PersonTransferJobDerived.create, person, team, metadata)

> === modified file 'lib/lp/testing/mail_helpers.py'
> --- lib/lp/testing/mail_helpers.py	2010-08-20 20:31:18 +0000
> +++ lib/lp/testing/mail_helpers.py	2010-09-17 18:59:05 +0000
> @@ -10,7 +10,14 @@
>
>  import transaction
>
> +from zope.component import getUtility
> +
> +from lp.registry.interfaces.persontransferjob import (
> +    IMembershipNotificationJobSource,
> +    )
> +from lp.services.job.runner import JobRunner
>  from lp.services.mail import stub
> +from lp.testing.logger import MockLogger
>
>
>  def pop_notifications(sort_key=None, commit=True):
> @@ -91,8 +98,23 @@
>          print body
>          print "-"*40
>
> +
>  def print_distinct_emails(include_reply_to=False, include_rationale=True):
>      """A convenient shortcut for `print_emails`(group_similar=True)."""
>      return print_emails(group_similar=True,
>                          include_reply_to=include_reply_to,
>                          include_rationale=include_rationale)
> +
> +
> +def run_mail_jobs():
> +    """Process job queues that send out emails.
> +
> +    New job subclasses need to be added to this function.

Edwin I don't understsand the last comment.

> +    """
> +    # Commit the transaction to make sure that the JobRunner can find
> +    # the queued jobs.
> +    transaction.commit()
> +    job_source = getUtility(IMembershipNotificationJobSource)
> +    logger = MockLogger()
> +    runner = JobRunner.fromReady(job_source, logger)
> +    runner.runAll()