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 being performed.
>> +        :param metadata: The type-specific variables, as a JSON-compatible
>> +            dict.
>
> Indent to match.


Fixed.


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


Fixed.


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


Actually, I think they should be TypeErrors, even though the
difference between ITeam and IPerson blurs what counts as a type.


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


Changed to TypeErrors.


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


Fixed. I also removed the extra "s" in adminS_emails, since it's
annoying despite being logical.


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


Fixed.


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


Fixed.


>> +    """
>> +    # 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()
>
> --
> https://code.launchpad.net/~edwin-grubbs/launchpad/bug-615654-queue-addmember-emails/+merge/35862
> You are the owner of lp:~edwin-grubbs/launchpad/bug-615654-queue-addmember-emails.
>