launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/person-transfer-permission-0 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #751995 in Launchpad itself: "person-transfer-job,  ProgrammingError: permission denied for relation account"
  https://bugs.launchpad.net/launchpad/+bug/751995

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/person-transfer-permission-0/+merge/58561

Fix person-transfer-job db permissions.

    Launchpad bug: https://bugs.launchpad.net/bugs/751995
    Pre-implementation: jcsackett

ProgrammingError: permission denied for relation account
was caused by a queue team membership change.

The script was broken by a refactoring to get email recipients a month ago.
The new code will ensures that emails to team members are deduped and that
the user is valid; requiring access to account.

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

RULES

    * Add a test to reproduce the condition
    * Add public.account to person-transfer-job in security.cfg


QA

    * On qastaging, make a team and admin for a team that you control.
      eg make delete-me-team and admin for
          https://qastaging.launchpad.net/~super-team/+members
    * Ask an LOSA to run
      bin/py cronscripts/process-job-source.py -vv \
          IMembershipNotificationJobSource > _test.log
    * Verify the log reports no oopses, specifically
      ProgrammingError: permission denied for relation account
      INFO    Job resulted in OOPS: OOPS-1936T14
      INFO    2 MembershipNotificationJob jobs did not complete.


LINT

  database/schema/security.cfg
  lib/lp/registry/tests/test_membership_notification_job.py



TEST

    ./bin/test -vv -t MembershipNotificationJobTest


IMPLEMENTATION

Added a test to reproduce the oops. Updated person-transfer-job to have
access to account.
  database/schema/security.cfg
  lib/lp/registry/tests/test_membership_notification_job.py
-- 
https://code.launchpad.net/~sinzui/launchpad/person-transfer-permission-0/+merge/58561
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/person-transfer-permission-0 into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-04-19 12:20:36 +0000
+++ database/schema/security.cfg	2011-04-20 19:20:59 +0000
@@ -2080,6 +2080,7 @@
 [person-transfer-job]
 type=user
 groups=script
+public.account                          = SELECT
 public.emailaddress                     = SELECT
 public.job                              = SELECT, UPDATE
 public.person                           = SELECT

=== modified file 'lib/lp/registry/tests/test_membership_notification_job.py'
--- lib/lp/registry/tests/test_membership_notification_job.py	2011-02-23 10:26:23 +0000
+++ lib/lp/registry/tests/test_membership_notification_job.py	2011-04-20 19:20:59 +0000
@@ -5,9 +5,13 @@
 
 __metaclass__ = type
 
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
+import transaction
+
 from zope.component import getUtility
 
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.persontransferjob import (
     IMembershipNotificationJobSource,
@@ -21,6 +25,7 @@
 from lp.testing import (
     login_person,
     person_logged_in,
+    run_script,
     TestCaseWithFactory,
     )
 from lp.testing.sampledata import ADMIN_EMAIL
@@ -28,7 +33,7 @@
 
 class MembershipNotificationJobTest(TestCaseWithFactory):
 
-    layer = LaunchpadFunctionalLayer
+    layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(MembershipNotificationJobTest, self).setUp()
@@ -79,3 +84,30 @@
             ("<MembershipNotificationJob about "
              "~murdock in ~a-team; status=Waiting>"),
             repr(job))
+
+    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.
+        with person_logged_in(self.team.teamowner):
+            # This implicitly creates a job, but it is not the job under test.
+            admining_team = self.factory.makeTeam()
+            self.team.addMember(
+                admining_team, self.team.teamowner, force_team_add=True)
+            membership = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+                admining_team, self.team)
+            membership.setStatus(
+                TeamMembershipStatus.ADMIN, self.team.teamowner)
+        job = self.job_source.create(
+            self.person, self.team, self.team.teamowner,
+            TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN)
+        job_repr = repr(job)
+        transaction.commit()
+        out, err, exit_code = run_script(
+            "LP_DEBUG_SQL=1 cronscripts/process-job-source.py -vv %s" % (
+                IMembershipNotificationJobSource.getName()))
+        self.addDetail("stdout", Content(UTF8_TEXT, lambda: out))
+        self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
+        self.assertEqual(0, exit_code)
+        self.assertTrue(job_repr in err, err)
+        self.assertTrue("MembershipNotificationJob sent email" in err, err)