launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/celery-everywhere-8 into lp:launchpad with lp:~abentley/launchpad/celery-everywhere-7 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~abentley/launchpad/celery-everywhere-8/+merge/103545

= Summary =
Support running PersonTransferJob via Celery.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of a resourced arc that will ultimately remove code.

== Implementation details ==
PersonTransferJob now provides makeDerived.
PersonTransferJobDerived now uses the EnumeratedSubclass metatype, so it can turn a PersonTransferJob into the appropriate subclass.
PersonTransferJobDerived.create now requests the job to run via Celery.
UniversalJobSource now supports PersonTransferJob.
PersonMergeJob and MembershipNotificationJob both provide configs.

== Tests ==
bin/test --layer=CeleryJobLayer -m 'membership_notification|person_merge'

== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_pofilestatsjob.py
  lib/lp/registry/tests/test_membership_notification_job.py
  lib/lp/translations/model/pofilestatsjob.py
  lib/lp/registry/model/persontransferjob.py
  lib/lp/registry/tests/test_person_merge_job.py
  lib/lp/services/job/model/job.py
-- 
https://code.launchpad.net/~abentley/launchpad/celery-everywhere-8/+merge/103545
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/celery-everywhere-8 into lp:launchpad.

=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py	2012-01-27 11:12:53 +0000
+++ lib/lp/registry/model/persontransferjob.py	2012-04-25 18:50:05 +0000
@@ -52,7 +52,10 @@
     IStore,
     )
 from lp.services.database.stormbase import StormBase
-from lp.services.job.model.job import Job
+from lp.services.job.model.job import (
+    EnumeratedSubclass,
+    Job,
+    )
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.mail.helpers import (
     get_contact_email_addresses,
@@ -115,6 +118,9 @@
         # but the DB representation is unicode.
         self._json_data = json_data.decode('utf-8')
 
+    def makeDerived(self):
+        return PersonTransferJobDerived.makeSubclass(self)
+
 
 class PersonTransferJobDerived(BaseRunnableJob):
     """Intermediate class for deriving from PersonTransferJob.
@@ -126,6 +132,7 @@
     the run() method.
     """
 
+    __metaclass__ = EnumeratedSubclass
     delegates(IPersonTransferJob)
     classProvides(IPersonTransferJobSource)
 
@@ -146,7 +153,9 @@
             major_person=major_person,
             job_type=cls.class_job_type,
             metadata=metadata)
-        return cls(job)
+        derived = cls(job)
+        derived.celeryRunOnCommit()
+        return derived
 
     @classmethod
     def iterReady(cls):
@@ -176,6 +185,8 @@
 
     class_job_type = PersonTransferJobType.MEMBERSHIP_NOTIFICATION
 
+    config = config.IMembershipNotificationJobSource
+
     @classmethod
     def create(cls, member, team, reviewer, old_status, new_status,
                last_change_comment=None):
@@ -342,6 +353,8 @@
 
     class_job_type = PersonTransferJobType.MERGE
 
+    config = config.IPersonMergeJobSource
+
     @classmethod
     def create(cls, from_person, to_person, reviewer=None, delete=False):
         """See `IPersonMergeJobSource`."""

=== modified file 'lib/lp/registry/tests/test_membership_notification_job.py'
--- lib/lp/registry/tests/test_membership_notification_job.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_membership_notification_job.py	2012-04-25 18:50:05 +0000
@@ -19,14 +19,22 @@
     TeamMembershipStatus,
     )
 from lp.registry.model.persontransferjob import MembershipNotificationJob
+from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.tests import (
+    block_on_job,
+    pop_remote_notifications,
+    )
 from lp.testing import (
     login_person,
     person_logged_in,
     run_script,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    CeleryJobLayer,
+    DatabaseFunctionalLayer,
+    )
 from lp.testing.sampledata import ADMIN_EMAIL
 
 
@@ -110,3 +118,38 @@
         self.assertEqual(0, exit_code)
         self.assertTrue(job_repr in err, err)
         self.assertTrue("MembershipNotificationJob sent email" in err, err)
+
+
+class TestViaCelery(TestCaseWithFactory):
+
+    layer = CeleryJobLayer
+
+    def test_smoke_admining_team(self):
+        # Smoke test, primarily for DB permissions needed by queries to work
+        # with admining users and teams
+        # Check the oopses in /var/tmp/lperr.test if the assertions fail.
+        self.useFixture(FeatureFixture({
+                'jobs.celery.enabled_classes': 'MembershipNotificationJob'
+        }))
+        team = self.factory.makeTeam(name='a-team')
+        with person_logged_in(team.teamowner):
+            # This implicitly creates a job, but it is not the job under test.
+            admining_team = self.factory.makeTeam()
+            team.addMember(
+                admining_team, team.teamowner, force_team_add=True)
+            membership = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+                admining_team, team)
+            membership.setStatus(
+                TeamMembershipStatus.ADMIN, team.teamowner)
+        person = self.factory.makePerson(name='murdock')
+        with block_on_job(self):
+            transaction.commit()
+        pop_remote_notifications()
+        job = getUtility(IMembershipNotificationJobSource).create(
+            person, team, team.teamowner,
+            TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN)
+        with block_on_job(self):
+            transaction.commit()
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+        (notification,) = pop_remote_notifications()
+        self.assertIn('murdock made admin by', notification['Subject'])

=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
--- lib/lp/registry/tests/test_person_merge_job.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py	2012-04-25 18:50:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests of `PersonMergeJob`."""
@@ -20,9 +20,11 @@
     IMasterObject,
     IStore,
     )
+from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.job.tests import block_on_job
 from lp.services.log.logger import BufferLogger
 from lp.services.mail.sendmail import format_address_for_person
 from lp.services.scripts import log
@@ -31,7 +33,33 @@
     run_script,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    CeleryJobLayer,
+    DatabaseFunctionalLayer,
+    )
+
+
+def create_job(factory):
+    """Create a PersonMergeJob for testing purposes.
+
+    :param factory: A LaunchpadObjectFactory.
+    """
+    from_person = factory.makePerson(name='void')
+    to_person = factory.makePerson(name='gestalt')
+    return getUtility(IPersonMergeJobSource).create(
+        from_person=from_person, to_person=to_person)
+
+
+def transfer_email(job):
+    """Reassign email address using the people specified in the job.
+
+    IPersonSet.merge() does not (yet) promise to do this.
+    """
+    from_email = IMasterObject(job.from_person.preferredemail)
+    removeSecurityProxy(from_email).personID = job.to_person.id
+    removeSecurityProxy(from_email).accountID = job.to_person.accountID
+    removeSecurityProxy(from_email).status = EmailAddressStatus.NEW
+    IStore(from_email).flush()
 
 
 class TestPersonMergeJob(TestCaseWithFactory):
@@ -40,11 +68,10 @@
 
     def setUp(self):
         super(TestPersonMergeJob, self).setUp()
-        self.from_person = self.factory.makePerson(name='void')
-        self.to_person = self.factory.makePerson(name='gestalt')
         self.job_source = getUtility(IPersonMergeJobSource)
-        self.job = self.job_source.create(
-            from_person=self.from_person, to_person=self.to_person)
+        self.job = create_job(self.factory)
+        self.from_person = self.job.from_person
+        self.to_person = self.job.to_person
 
     def test_interface(self):
         # PersonMergeJob implements IPersonMergeJob.
@@ -90,11 +117,7 @@
     def transfer_email(self):
         # Reassign from_person's email address over to to_person because
         # IPersonSet.merge() does not (yet) promise to do that.
-        from_email = IMasterObject(self.from_person.preferredemail)
-        removeSecurityProxy(from_email).personID = self.to_person.id
-        removeSecurityProxy(from_email).accountID = self.to_person.accountID
-        removeSecurityProxy(from_email).status = EmailAddressStatus.NEW
-        IStore(from_email).flush()
+        transfer_email(self.job)
 
     def test_run(self):
         # When run it merges from_person into to_person.
@@ -179,3 +202,21 @@
                 self.assertEqual([self.job], self.find())
             else:
                 self.assertEqual([], self.find())
+
+
+class TestViaCelery(TestCaseWithFactory):
+    """Test that PersonMergeJob runs under Celery."""
+
+    layer = CeleryJobLayer
+
+    def test_run(self):
+        # When run it merges from_person into to_person.
+        self.useFixture(FeatureFixture({
+            'jobs.celery.enabled_classes': 'PersonMergeJob',
+        }))
+        job = create_job(self.factory)
+        transfer_email(job)
+        from_person = job.from_person
+        with block_on_job(self):
+            transaction.commit()
+        self.assertEqual(job.to_person, from_person.merged)

=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py	2012-04-25 18:49:35 +0000
+++ lib/lp/services/job/model/job.py	2012-04-25 18:50:05 +0000
@@ -276,6 +276,7 @@
         from lp.code.model.branchmergeproposaljob import (
             BranchMergeProposalJob,
             )
+        from lp.registry.model.persontransferjob import PersonTransferJob
         from lp.soyuz.model.distributionjob import DistributionJob
         from lp.translations.model.pofilestatsjob import POFileStatsJob
         from lp.translations.model.translationsharingjob import (
@@ -286,7 +287,7 @@
 
         for baseclass in [
             ApportJob, BranchJob, BranchMergeProposalJob, DistributionJob,
-            POFileStatsJob, TranslationSharingJob,
+            PersonTransferJob, POFileStatsJob, TranslationSharingJob,
             ]:
             derived, base_class, store = cls._getDerived(job_id, baseclass)
             if derived is not None: