Merge lp:~edwin-grubbs/launchpad/bug-615654-queue-addmember-emails into lp:launchpad/db-devel

[Edwin Grubbs]

Summary
-------

This branch adds the MembershipNotificationJob class to send emails
after the team membership status has changed, e.g. proposed, approved,
admin, expired.

A follow-up branch will be needed in order to add a cronjob to actually
process the job queue. This branch can't land until then.

Implementation details
----------------------

lib/lp/registry/model/teammembership.py

    lib/lp/registry/configure.zcml
    lib/lp/registry/enum.py
    lib/lp/registry/interfaces/persontransferjob.py
    lib/lp/registry/model/persontransferjob.py

Tests:
    lib/lp/registry/tests/test_persontransferjob.py
    lib/lp/testing/mail_helpers.py
    lib/lp/registry/doc/teammembership-email-notification.txt

Tests
-----

./bin/test -vv -t 'teammembership-email-notification.txt|test_persontransferjob'

[Brad Crittenden (bac)]

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.dump...

review: Approve (code)

[Edwin Grubbs (edwin-grubbs)]

On Fri, Sep 17, 2010 at 3:12 PM, Brad Crittenden <email address hidden> wrote:
> Review: Approve code
> Hi Edwin,
>
> Very interesting branch.  I've got some suggestions but overall it is great.
>
> --Brad

Thanks for the review.

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

Fixed. That's just odd. I'm blaming it on my rampant plagiarism in this branch.

>> +__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

Fixed with henning's tool.

>> +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.

Fixed. I really need to update my vim plugin.

>> +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 b...

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2010-09-14 10:51:25 +0000
+++ lib/lp/registry/configure.zcml	2010-09-17 22:26:48 +0000
@@ -84,6 +84,23 @@
                 __call__"/>
     </class>
 
+    <!-- IPersonTransferJob -->
+    <securedutility
+        component="lp.registry.model.persontransferjob.MembershipNotificationJob"
+        provides="lp.registry.interfaces.persontransferjob.IMembershipNotificationJobSource">
+        <allow
+            interface="lp.registry.interfaces.persontransferjob.IMembershipNotificationJobSource"/>
+    </securedutility>
+
+    <class class="lp.registry.model.persontransferjob.PersonTransferJob">
+        <allow interface="lp.registry.interfaces.persontransferjob.IPersonTransferJob"/>
+    </class>
+
+    <class class="lp.registry.model.persontransferjob.MembershipNotificationJob">
+        <allow
+            interface="lp.registry.interfaces.persontransferjob.IMembershipNotificationJob"/>
+    </class>
+
     <!-- Location -->
 
     <class

=== modified file 'lib/lp/registry/doc/teammembership-email-notification.txt'
--- lib/lp/registry/doc/teammembership-email-notification.txt	2010-03-26 00:45:36 +0000
+++ lib/lp/registry/doc/teammembership-email-notification.txt	2010-09-17 22:26:48 +0000
@@ -24,7 +24,7 @@
 
     >>> from lp.services.mail import stub
     >>> from lp.testing.mail_helpers import (
-    ...     pop_notifications, print_distinct_emails)
+    ...     pop_notifications, print_distinct_emails, run_mail_jobs)
 
     >>> from zope.component import getUtility
     >>> from canonical.launchpad.interfaces import (
@@ -49,7 +49,7 @@
     >>> membership.status.title
     'Proposed'
 
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     5
     >>> print_distinct_emails(include_reply_to=True)
@@ -87,6 +87,11 @@
     # that team.
     >>> login('[e-mail]')
     >>> setStatus(membership, TeamMembershipStatus.DECLINED, reviewer=mark)
+
+addMember() has queued up a job to send out the emails. We'll run the
+job now.
+
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     6
 
@@ -125,6 +130,7 @@
 
     >>> setStatus(daf_membership, TeamMembershipStatus.APPROVED,
     ...           reviewer=mark, comment='This is a nice guy; I like him')
+    >>> run_mail_jobs()
     >>> stub.test_emails.sort(by_to_addrs)
     >>> len(stub.test_emails)
     6
@@ -158,6 +164,7 @@
 
     >>> setStatus(daf_membership, TeamMembershipStatus.DEACTIVATED,
     ...           reviewer=mark)
+    >>> run_mail_jobs()
     >>> stub.test_emails.sort(by_to_addrs)
     >>> len(stub.test_emails)
     6
@@ -187,7 +194,7 @@
 
     >>> admins = personset.getByName('admins')
     >>> admins.join(ubuntu_team, requester=mark)
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     5
     >>> print_distinct_emails(include_reply_to=True)
@@ -228,7 +235,10 @@
     >>> cprov = personset.getByName('cprov')
     >>> marilize = personset.getByName('marilize')
     >>> ignored = ubuntu_team.addMember(marilize, reviewer=cprov)
-    >>> transaction.commit()
+    >>> run_mail_jobs()
+
+Now, the emails have been sent.
+
     >>> len(stub.test_emails)
     6
     >>> print_distinct_emails()
@@ -277,7 +287,7 @@
     >>> mirror_admins.getTeamAdminsEmailAddresses()
     ['[e-mail]']
     >>> ignored = ubuntu_team.addMember(mirror_admins, reviewer=cprov)
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     1
     >>> print_distinct_emails()
@@ -304,7 +314,7 @@
     >>> comment = "Of course I want to be part of ubuntu!"
     >>> mirror_admins.acceptInvitationToBeMemberOf(ubuntu_team, comment)
     >>> flush_database_updates()
-    >>> transaction.commit()
+    >>> run_mail_jobs()
 
     >>> len(stub.test_emails)
     6
@@ -337,7 +347,7 @@
     >>> comment = "Landscape has nothing to do with ubuntu, unfortunately."
     >>> landscape.declineInvitationToBeMemberOf(ubuntu_team, comment)
     >>> flush_database_updates()
-    >>> transaction.commit()
+    >>> run_mail_jobs()
 
     >>> len(stub.test_emails)
     7
@@ -363,7 +373,7 @@
     >>> ignored = ubuntu_team.addMember(
     ...     launchpad, reviewer=cprov, force_team_add=True)
     >>> flush_database_updates()
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     5
     >>> print_distinct_emails()
@@ -610,7 +620,7 @@
     >>> membershipset.handleMembershipsExpiringToday(
     ...     reviewer=getUtility(ILaunchpadCelebrities).janitor)
     >>> flush_database_updates()
-    >>> transaction.commit()
+    >>> run_mail_jobs()
 
     >>> len(stub.test_emails)
     8
@@ -690,7 +700,7 @@
 
     >>> login_person(karl)
     >>> karl.renewTeamMembership(mirror_admins)
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     1
 
@@ -713,7 +723,7 @@
 approved to admin, but he won't get a notification of that.
 
     >>> team = personset.newTeam(mark, 'testteam', 'Test')
-    >>> transaction.commit()
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     0
 
@@ -730,6 +740,7 @@
     >>> login('[e-mail]')
     >>> setStatus(
     ...     cprov_membership, TeamMembershipStatus.ADMIN, reviewer=mark)
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     6
     >>> print_distinct_emails()
@@ -760,6 +771,7 @@
     >>> jdub_membership = membershipset.getByPersonAndTeam(jdub, ubuntu_team)
     >>> setStatus(jdub_membership, TeamMembershipStatus.APPROVED,
     ...           reviewer=jdub)
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     5
     >>> print_distinct_emails()
@@ -784,6 +796,7 @@
     ...     mirror_admins, ubuntu_team)
     >>> setStatus(mirror_admins_membership, TeamMembershipStatus.DEACTIVATED,
     ...           reviewer=mark, silent=False)
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     6
 
@@ -811,6 +824,7 @@
     Approved
     >>> setStatus(dumper_hwdb_membership,
     ...     TeamMembershipStatus.DEACTIVATED, reviewer=mark, silent=True)
+    >>> run_mail_jobs()
     >>> len(stub.test_emails)
     0
     >>> print dumper_hwdb_membership.status.title

=== modified file 'lib/lp/registry/enum.py'
--- lib/lp/registry/enum.py	2010-09-10 10:08:58 +0000
+++ lib/lp/registry/enum.py	2010-09-17 22:26:48 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'PersonTransferJobType',
     'BugNotificationLevel',
     'DistroSeriesDifferenceStatus',
     'DistroSeriesDifferenceType',
@@ -82,6 +83,7 @@
         This difference has been resolved and versions are now equal.
         """)
 
+
 class DistroSeriesDifferenceType(DBEnumeratedType):
     """Distribution series difference type."""
 
@@ -104,3 +106,13 @@
 
         This package is present in both series with different versions.
         """)
+
+
+class PersonTransferJobType(DBEnumeratedType):
+    """Values that IPersonTransferJob.job_type can take."""
+
+    MEMBERSHIP_NOTIFICATION = DBItem(0, """
+        Add-member notification
+
+        Notify affected users of new team membership.
+        """)

=== added file 'lib/lp/registry/interfaces/persontransferjob.py'
--- lib/lp/registry/interfaces/persontransferjob.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/interfaces/persontransferjob.py	2010-09-17 22:26:48 +0000
@@ -0,0 +1,70 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interface for the Jobs system to change memberships or merge persons."""
+
+__metaclass__ = type
+__all__ = [
+    'IMembershipNotificationJob',
+    'IMembershipNotificationJobSource',
+    'IPersonTransferJob',
+    'IPersonTransferJobSource',
+    ]
+
+from zope.interface import Attribute
+from zope.schema import (
+    Int,
+    Object,
+    )
+
+from canonical.launchpad import _
+from lp.services.fields import PublicPersonChoice
+from lp.services.job.interfaces.job import (
+    IJob,
+    IJobSource,
+    IRunnableJob,
+    )
+
+
+class IPersonTransferJob(IRunnableJob):
+    """A Job related to team membership or a person merge."""
+
+    id = Int(
+        title=_('DB ID'), required=True, readonly=True,
+        description=_("The tracking number for this job."))
+
+    job = Object(
+        title=_('The common Job attributes'),
+        schema=IJob,
+        required=True)
+
+    minor_person = PublicPersonChoice(
+        title=_('The person being added to the major person/team'),
+        vocabulary='ValidPersonOrTeam',
+        required=True)
+
+    major_person = PublicPersonChoice(
+        title=_('The person or team receiving the minor person'),
+        vocabulary='ValidPersonOrTeam',
+        required=True)
+
+    metadata = Attribute('A dict of data about the job.')
+
+
+class IPersonTransferJobSource(IJobSource):
+    """An interface for acquiring IPersonTransferJobs."""
+
+    def create(minor_person, major_person, metadata):
+        """Create a new IPersonTransferJob."""
+
+
+class IMembershipNotificationJob(IPersonTransferJob):
+    """A Job to notify new members of a team of that change."""
+
+
+class IMembershipNotificationJobSource(IJobSource):
+    """An interface for acquiring IMembershipNotificationJobs."""
+
+    def create(member, team, reviewer, old_status, new_status,
+               last_change_comment=None):
+        """Create a new IMembershipNotificationJob."""

=== 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 22:26:48 +0000
@@ -0,0 +1,334 @@
+# 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."""
+
+__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,
+    )
+
+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
+from canonical.launchpad.mail import (
+    format_address,
+    simple_sendmail,
+    )
+from canonical.launchpad.mailnotification import MailWrapper
+from canonical.launchpad.webapp import canonical_url
+from lp.registry.enum import PersonTransferJobType
+from lp.registry.interfaces.person import (
+    IPerson,
+    IPersonSet,
+    ITeam,
+    )
+from lp.registry.interfaces.persontransferjob import (
+    IMembershipNotificationJob,
+    IMembershipNotificationJobSource,
+    IPersonTransferJob,
+    IPersonTransferJobSource,
+    )
+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):
+    """Base class for team membership and person merge jobs."""
+
+    implements(IPersonTransferJob)
+
+    __storm_table__ = 'PersonTransferJob'
+
+    id = Int(primary=True)
+
+    job_id = Int(name='job')
+    job = Reference(job_id, Job.id)
+
+    major_person_id = Int(name='major_person')
+    major_person = Reference(major_person_id, Person.id)
+
+    minor_person_id = Int(name='minor_person')
+    minor_person = Reference(minor_person_id, Person.id)
+
+    job_type = EnumCol(enum=PersonTransferJobType, notNull=True)
+
+    _json_data = Unicode('json_data')
+
+    @property
+    def metadata(self):
+        return simplejson.loads(self._json_data)
+
+    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.
+        """
+        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.
+
+    Storm classes can't simply be subclassed or you can end up with
+    multiple objects referencing the same row in the db. This class uses
+    lazr.delegates, which is a little bit simpler than storm's
+    infoheritance solution to the problem. Subclasses need to override
+    the run() method.
+    """
+
+    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`."""
+        if not IPerson.providedBy(minor_person):
+            raise TypeError("minor_person must be IPerson: %s"
+                            % repr(minor_person))
+        if not IPerson.providedBy(major_person):
+            raise TypeError("major_person must be IPerson: %s"
+                            % repr(major_person))
+        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):
+        if not ITeam.providedBy(team):
+            raise TypeError('team must be ITeam: %s' % repr(team))
+        if not IPerson.providedBy(reviewer):
+            raise TypeError('reviewer must be IPerson: %s' % repr(reviewer))
+        if old_status not in TeamMembershipStatus:
+            raise TypeError("old_status must be TeamMembershipStatus: %s"
+                            % repr(old_status))
+        if new_status not in TeamMembershipStatus:
+            raise TypeError("new_status must be TeamMembershipStatus: %s"
+                            % repr(new_status))
+        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)
+
+    @property
+    def member(self):
+        return self.minor_person
+
+    @property
+    def team(self):
+        return self.major_person
+
+    @property
+    def reviewer(self):
+        return getUtility(IPersonSet).get(self.metadata['reviewer'])
+
+    @property
+    def old_status(self):
+        return TeamMembershipStatus.items[self.metadata['old_status']]
+
+    @property
+    def new_status(self):
+        return TeamMembershipStatus.items[self.metadata['new_status']]
+
+    @property
+    def last_change_comment(self):
+        return self.metadata['last_change_comment']
+
+    def run(self):
+        """See `IMembershipNotificationJob`."""
+        from canonical.launchpad.scripts import log
+        from_addr = format_address(
+            self.team.displayname, config.canonical.noreply_from_address)
+        admin_emails = self.team.getTeamAdminsEmailAddresses()
+        # person might be a self.team, so we can't rely on its preferredemail.
+        self.member_email = get_contact_email_addresses(self.member)
+        # Make sure we don't send the same notification twice to anybody.
+        for email in self.member_email:
+            if email in admin_emails:
+                admin_emails.remove(email)
+
+        if self.reviewer != self.member:
+            self.reviewer_name = self.reviewer.unique_displayname
+        else:
+            # The user himself changed his self.membership.
+            self.reviewer_name = 'the user himself'
+
+        if self.last_change_comment:
+            comment = ("\n%s said:\n %s\n" % (
+                self.reviewer.displayname, self.last_change_comment.strip()))
+        else:
+            comment = ""
+
+        replacements = {
+            'member_name': self.member.unique_displayname,
+            'recipient_name': self.member.displayname,
+            'team_name': self.team.unique_displayname,
+            'team_url': canonical_url(self.team),
+            'old_status': self.old_status.title,
+            'new_status': self.new_status.title,
+            'reviewer_name': self.reviewer_name,
+            'comment': comment}
+
+        template_name = 'membership-statuschange'
+        subject = (
+            'Membership change: %(member)s in %(team)s'
+            % {
+                'member': self.member.name,
+                'team': self.team.name,
+              })
+        if self.new_status == TeamMembershipStatus.EXPIRED:
+            template_name = 'membership-expired'
+            subject = '%s expired from team' % self.member.name
+        elif (self.new_status == TeamMembershipStatus.APPROVED and
+            self.old_status != TeamMembershipStatus.ADMIN):
+            if self.old_status == TeamMembershipStatus.INVITED:
+                subject = ('Invitation to %s accepted by %s'
+                        % (self.member.name, self.reviewer.name))
+                template_name = 'membership-invitation-accepted'
+            elif self.old_status == TeamMembershipStatus.PROPOSED:
+                subject = '%s approved by %s' % (
+                    self.member.name, self.reviewer.name)
+            else:
+                subject = '%s added by %s' % (
+                    self.member.name, self.reviewer.name)
+        elif self.new_status == TeamMembershipStatus.INVITATION_DECLINED:
+            subject = ('Invitation to %s declined by %s'
+                    % (self.member.name, self.reviewer.name))
+            template_name = 'membership-invitation-declined'
+        elif self.new_status == TeamMembershipStatus.DEACTIVATED:
+            subject = '%s deactivated by %s' % (
+                self.member.name, self.reviewer.name)
+        elif self.new_status == TeamMembershipStatus.ADMIN:
+            subject = '%s made admin by %s' % (
+                self.member.name, self.reviewer.name)
+        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 len(admin_emails) != 0:
+            admin_template = get_email_template(
+                "%s-bulk.txt" % template_name)
+            for address in admin_emails:
+                recipient = getUtility(IPersonSet).getByEmail(address)
+                replacements['recipient_name'] = recipient.displayname
+                msg = MailWrapper().format(
+                    admin_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')

=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/teammembership.py	2010-09-17 22:26:48 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'sendStatusChangeNotification',
     'TeamMembership',
     'TeamMembershipSet',
     'TeamParticipation',
@@ -51,6 +52,9 @@
     TeamMembershipRenewalPolicy,
     validate_public_person,
     )
+from lp.registry.interfaces.persontransferjob import (
+    IMembershipNotificationJobSource,
+    )
 from lp.registry.interfaces.teammembership import (
     CyclicalTeamMembershipError,
     DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
@@ -400,96 +404,11 @@
         """Send a status change notification to all team admins and the
         member whose membership's status changed.
         """
-        team = self.team
-        member = self.person
         reviewer = self.last_changed_by
-        from_addr = format_address(
-            team.displayname, config.canonical.noreply_from_address)
         new_status = self.status
-        admins_emails = team.getTeamAdminsEmailAddresses()
-        # self.person might be a team, so we can't rely on its preferredemail.
-        member_email = get_contact_email_addresses(member)
-        # Make sure we don't send the same notification twice to anybody.
-        for email in member_email:
-            if email in admins_emails:
-                admins_emails.remove(email)
-
-        if reviewer != member:
-            reviewer_name = reviewer.unique_displayname
-        else:
-            # The user himself changed his membership.
-            reviewer_name = 'the user himself'
-
-        if self.last_change_comment:
-            comment = ("\n%s said:\n %s\n" % (
-                reviewer.displayname, self.last_change_comment.strip()))
-        else:
-            comment = ""
-
-        replacements = {
-            'member_name': member.unique_displayname,
-            'recipient_name': member.displayname,
-            'team_name': team.unique_displayname,
-            'team_url': canonical_url(team),
-            'old_status': old_status.title,
-            'new_status': new_status.title,
-            'reviewer_name': reviewer_name,
-            'comment': comment}
-
-        template_name = 'membership-statuschange'
-        subject = ('Membership change: %(member)s in %(team)s'
-                   % {'member': member.name, 'team': team.name})
-        if new_status == TeamMembershipStatus.EXPIRED:
-            template_name = 'membership-expired'
-            subject = '%s expired from team' % member.name
-        elif (new_status == TeamMembershipStatus.APPROVED and
-              old_status != TeamMembershipStatus.ADMIN):
-            if old_status == TeamMembershipStatus.INVITED:
-                subject = ('Invitation to %s accepted by %s'
-                           % (member.name, reviewer.name))
-                template_name = 'membership-invitation-accepted'
-            elif old_status == TeamMembershipStatus.PROPOSED:
-                subject = '%s approved by %s' % (member.name, reviewer.name)
-            else:
-                subject = '%s added by %s' % (member.name, reviewer.name)
-        elif new_status == TeamMembershipStatus.INVITATION_DECLINED:
-            subject = ('Invitation to %s declined by %s'
-                       % (member.name, reviewer.name))
-            template_name = 'membership-invitation-declined'
-        elif new_status == TeamMembershipStatus.DEACTIVATED:
-            subject = '%s deactivated by %s' % (member.name, reviewer.name)
-        elif new_status == TeamMembershipStatus.ADMIN:
-            subject = '%s made admin by %s' % (member.name, reviewer.name)
-        elif new_status == TeamMembershipStatus.DECLINED:
-            subject = '%s declined by %s' % (member.name, reviewer.name)
-        else:
-            # Use the default template and subject.
-            pass
-
-        if admins_emails:
-            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 member can be a team without any members, and in this case we
-        # won't have a single email address to send this notification to.
-        if member_email and reviewer != member:
-            if member.isTeam():
-                template = '%s-bulk.txt' % template_name
-            else:
-                template = '%s-personal.txt' % template_name
-            member_template = get_email_template(template)
-            for address in member_email:
-                recipient = getUtility(IPersonSet).getByEmail(address)
-                replacements['recipient_name'] = recipient.displayname
-                msg = MailWrapper().format(
-                    member_template % replacements, force_wrap=True)
-                simple_sendmail(from_addr, address, subject, msg)
+        getUtility(IMembershipNotificationJobSource).create(
+            self.person, self.team, reviewer, old_status, new_status,
+            self.last_change_comment)
 
 
 class TeamMembershipSet:

=== 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 22:26:48 +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
+        # person_transfer_job.metadata will be different from what we
+        # originally passed in.
+        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 22:26:48 +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,25 @@
         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.
+
+    If a new job type is added that sends emails, this function can be
+    extended to run those jobs, so that testing emails doesn't require a
+    bunch of different function calls to process different queues.
+    """
+    # 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()