launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/commercial-jobsource-zcml into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1012808 in Launchpad itself: "Expiration email job sources are not registered"
  https://bugs.launchpad.net/launchpad/+bug/1012808

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/commercial-jobsource-zcml/+merge/110365

process-job-source.py fails because the commercial notification classes
are not registered in ZCML. Clearly, a test is missing that demonstrates
the ZCML, schema-lazr.conf, and security.cfg are configured properly.

I really botched this. I new what test was missing when I discovered the
bug. There are two cronscripts used in this process, and I only wrote
tests for the script I wrote. I need to also write a test for the script
I reused.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Add the missing ZCML needed to register the secured utilities.
    * Add tests to verify the three kinds of job are run.
      * Fix any db permissions that are proven to fail.
    * Fix the ICommercialExpiredJob to be ICommercialExpiredJobSource
      in the schema.


QA

    On qastaging, as a webops to run
    * cronscripts/process-job-source.py ICommercialExpiredJobSource
    * cronscripts/process-job-source.py ISevenDayCommercialExpirationJobSource
    * cronscripts/process-job-source.py IThirtyDayCommercialExpirationJobSource


LINT

    cronscripts/daily_product_jobs.py
    database/schema/security.cfg
    lib/lp/registry/configure.zcml
    lib/lp/registry/model/productjob.py
    lib/lp/registry/tests/test_productjob.py
    lib/lp/services/config/schema-lazr.conf


TEST

    ./bin/test -vvc lp.registry.tests.test_productjob


IMPLEMENTATION

Fixed the class name used to lookup jobs
ICommercialExpiredJob => ICommercialExpiredJobSource
    cronscripts/daily_product_jobs.py
    lib/lp/services/config/schema-lazr.conf

Added a test to the test mixin to verify that the three kinds of
commercial notification job can be run by process-job-source.py. I
discovered that there is two run_script() test helpers with different
IO. I chose to use update an existing test to use the one from
lp.testing because it allows me to pass the environ to get better test
output.
    lib/lp/registry/model/productjob.py
    lib/lp/registry/tests/test_productjob.py

Added the missing ZCML registration that permits process-job-source.py
to lookup the job source class and then have permission to use the job
instance.
    lib/lp/registry/configure.zcml

Fixed the db permissions demonstrated to be need by the new test. The
packaging/distro tables are needed because there is a sanity check in
the product deactivation code that ensure Ubuntu does not look an
upstream packaging link
    database/schema/security.cfg
-- 
https://code.launchpad.net/~sinzui/launchpad/commercial-jobsource-zcml/+merge/110365
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/commercial-jobsource-zcml into lp:launchpad.

=== modified file 'cronscripts/daily_product_jobs.py'
--- cronscripts/daily_product_jobs.py	2012-06-07 19:14:04 +0000
+++ cronscripts/daily_product_jobs.py	2012-06-14 16:16:22 +0000
@@ -21,7 +21,7 @@
 
     def __init__(self):
         name = 'daily_product_jobs'
-        dbuser = config.ICommercialExpiredJob.dbuser
+        dbuser = config.ICommercialExpiredJobSource.dbuser
         LaunchpadCronScript.__init__(self, name, dbuser)
 
     def main(self):

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-06-08 15:46:51 +0000
+++ database/schema/security.cfg	2012-06-14 16:16:22 +0000
@@ -2367,11 +2367,17 @@
 [product-job]
 groups=script
 public.account                          = SELECT
+public.branch                           = SELECT
 public.commercialsubscription           = SELECT, UPDATE, DELETE
+public.distribution                     = SELECT
+public.distroseries                     = SELECT
 public.emailaddress                     = SELECT
 public.job                              = SELECT, INSERT, UPDATE
 public.person                           = SELECT
+public.packaging                        = SELECT
 public.product                          = SELECT, UPDATE
+public.productlicense                   = SELECT
 public.productseries                    = SELECT, UPDATE
 public.productjob                       = SELECT, INSERT
+public.sourcepackagename                = SELECT
 type=user

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-05-29 03:59:16 +0000
+++ lib/lp/registry/configure.zcml	2012-06-14 16:16:22 +0000
@@ -110,6 +110,37 @@
       <allow interface=".interfaces.persontransferjob.IPersonMergeJob"/>
     </class>
 
+    <!-- IProductNotificationJob -->
+    <securedutility
+        component=".model.productjob.CommercialExpiredJob"
+        provides=".interfaces.productjob.ICommercialExpiredJobSource">
+      <allow interface=".interfaces.productjob.ICommercialExpiredJobSource"/>
+    </securedutility>
+
+    <class class=".model.productjob.CommercialExpiredJob">
+      <allow interface=".interfaces.productjob.ICommercialExpiredJob"/>
+    </class>
+
+    <securedutility
+        component=".model.productjob.SevenDayCommercialExpirationJob"
+        provides=".interfaces.productjob.ISevenDayCommercialExpirationJobSource">
+      <allow interface=".interfaces.productjob.ISevenDayCommercialExpirationJobSource"/>
+    </securedutility>
+
+    <class class=".model.productjob.SevenDayCommercialExpirationJob">
+      <allow interface=".interfaces.productjob.ISevenDayCommercialExpirationJob"/>
+    </class>
+
+    <securedutility
+        component=".model.productjob.ThirtyDayCommercialExpirationJob"
+        provides=".interfaces.productjob.IThirtyDayCommercialExpirationJobSource">
+      <allow interface=".interfaces.productjob.IThirtyDayCommercialExpirationJobSource"/>
+    </securedutility>
+
+    <class class=".model.productjob.ThirtyDayCommercialExpirationJob">
+      <allow interface=".interfaces.productjob.IThirtyDayCommercialExpirationJob"/>
+    </class>
+
     <!-- INameBlacklist -->
     <securedutility
         class="lp.registry.model.nameblacklist.NameBlacklistSet"

=== modified file 'lib/lp/registry/model/productjob.py'
--- lib/lp/registry/model/productjob.py	2012-06-14 05:18:22 +0000
+++ lib/lp/registry/model/productjob.py	2012-06-14 16:16:22 +0000
@@ -71,6 +71,11 @@
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.mailwrapper import MailWrapper
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+<<<<<<< TREE
+=======
+from lp.services.mail.mailwrapper import MailWrapper
+from lp.services.scripts import log
+>>>>>>> MERGE-SOURCE
 from lp.services.mail.sendmail import (
     format_address,
     format_address_for_person,
@@ -332,6 +337,8 @@
             body, headers = self.getBodyAndHeaders(
                 email_template, address, self.reply_to)
             simple_sendmail(from_address, address, subject, body, headers)
+        log.debug("%s has sent email to the maintainer of %s.",
+            self.log_name, self.product.name)
 
     def run(self):
         """See `BaseRunnableJob`.

=== modified file 'lib/lp/registry/tests/test_productjob.py'
--- lib/lp/registry/tests/test_productjob.py	2012-06-14 05:18:22 +0000
+++ lib/lp/registry/tests/test_productjob.py	2012-06-14 16:16:22 +0000
@@ -12,6 +12,12 @@
 
 import pytz
 import transaction
+<<<<<<< TREE
+=======
+import pytz
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
+>>>>>>> MERGE-SOURCE
 from zope.component import getUtility
 from zope.interface import (
     classProvides,
@@ -53,6 +59,7 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     person_logged_in,
+    run_script,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -61,6 +68,14 @@
     ZopelessAppServerLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
+<<<<<<< TREE
+=======
+from lp.services.database.lpstorm import IStore
+from lp.services.job.interfaces.job import JobStatus
+from lp.services.log.logger import BufferLogger
+from lp.services.propertycache import clear_property_cache
+from lp.services.webapp.publisher import canonical_url
+>>>>>>> MERGE-SOURCE
 
 
 class CommercialHelpers:
@@ -147,8 +162,11 @@
         # ProductJobManagerTestCase.test_createAllDailyJobs
         self.make_test_products()
         transaction.commit()
-        retcode, stdout, stderr = run_script(
-            'cronscripts/daily_product_jobs.py', [])
+        stdout, stderr, retcode = run_script(
+            'cronscripts/daily_product_jobs.py')
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: stdout))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: stderr))
+        self.assertEqual(0, retcode)
         self.assertIn('Requested 3 total product jobs.', stderr)
 
 
@@ -556,6 +574,48 @@
         self.assertEqual(1, len(notifications))
         self.assertIn(iso_date, notifications[0].get_payload())
 
+    def test_run_cronscript(self):
+        # Everything is configured: ZCML, schema-lazr.conf, and security.cfg.
+        product, reviewer = self.make_notification_data()
+        private_branch = self.factory.makeBranch(
+            owner=product.owner, product=product,
+            information_type=InformationType.USERDATA)
+        with person_logged_in(product.owner):
+            product.setPrivateBugs(True, product.owner)
+            product.development_focus.branch = private_branch
+        self.expire_commercial_subscription(product)
+        job = self.JOB_CLASS.create(product, reviewer)
+        # Create a proprietary project which will have different DB relations.
+        proprietary_product = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        self.expire_commercial_subscription(proprietary_product)
+        proprietary_job = self.JOB_CLASS.create(proprietary_product, reviewer)
+        transaction.commit()
+
+        out, err, exit_code = run_script(
+            "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" %
+             self.JOB_SOURCE_INTERFACE.getName())
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.assertEqual(0, exit_code)
+        self.assertTrue(
+            'Traceback (most recent call last)' not in err)
+        message = (
+            '%s has sent email to the maintainer of %s.' % (
+                self.JOB_CLASS.__name__, product.name))
+        self.assertTrue(
+            message in err,
+            'Cound not find "%s" in err log:\n%s.' % (message, err))
+        message = (
+            '%s has sent email to the maintainer of %s.' % (
+                self.JOB_CLASS.__name__, proprietary_product.name))
+        self.assertTrue(
+            message in err,
+            'Cound not find "%s" in err log:\n%s.' % (message, err))
+        IStore(job.job).invalidate()
+        self.assertEqual(JobStatus.COMPLETED, job.job.status)
+        self.assertEqual(JobStatus.COMPLETED, proprietary_job.job.status)
+
 
 class SevenDayCommercialExpirationJobTestCase(CommericialExpirationMixin,
                                               TestCaseWithFactory):

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2012-06-13 01:05:42 +0000
+++ lib/lp/services/config/schema-lazr.conf	2012-06-14 16:16:22 +0000
@@ -1749,7 +1749,7 @@
 # dbuser, the crontab_group, and the module that the job source class
 # can be loaded from.
 job_sources:
-    ICommercialExpiredJob,
+    ICommercialExpiredJobSource,
     IMembershipNotificationJobSource,
     IPersonMergeJobSource,
     IPlainPackageCopyJobSource,
@@ -1759,7 +1759,7 @@
     ISevenDayCommercialExpirationJobSource,
     IThirtyDayCommercialExpirationJobSource
 
-[ICommercialExpiredJob]
+[ICommercialExpiredJobSource]
 module: lp.registry.interfaces.productjob
 dbuser: product-job
 crontab_group: MAIN