launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/private-persontransferjob-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #665158 PersonTransferJob table entries prevent making a team private
  https://bugs.launchpad.net/bugs/665158


This is my branch to skip PersonTransferJob in team visibility checks.

    lp:~sinzui/launchpad/private-persontransferjob-0
    Diff size: 75
    Launchpad bug: https://bugs.launchpad.net/bugs/665158
    Test command: ./bin/test -vv -t TestVisibilityConsistencyWarning
    Pre-implementation: no one
    Target release: 10.12


Skip PersonTransferJob in team visibility checks
------------------------------------------------

It should be possible to make a team private even if there is a pending
PersonTransferJob for sending emails.


Rules
-----

    * Add PersonTransferJob to the skip list used by
      visibilityConsistencyWarning.


QA
--

In quick succession
    * Register a new team
    * Add a member
    * Use the Administer page to change the team to private
    * Verify the team is private. There should not be a warning:
      This team cannot be converted to Private since it is referenced by
      a persontransferjob.


Lint
----

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team.py


Implementation
--------------

Added PersonTransferJob to the visibilityConsistencyWarning skip list. Removed
comment about private-membership teams which no longer exist.
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team.py
-- 
https://code.launchpad.net/~sinzui/launchpad/private-persontransferjob-0/+merge/41614
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/private-persontransferjob-0 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-11-18 12:05:34 +0000
+++ lib/lp/registry/model/person.py	2010-11-23 15:41:34 +0000
@@ -2091,13 +2091,12 @@
             ('logintoken', 'requester'),
             ('personlanguage', 'person'),
             ('personlocation', 'person'),
+            ('persontransferjob', 'minor_person'),
+            ('persontransferjob', 'major_person'),
             ('signedcodeofconduct', 'owner'),
             ('sshkey', 'person'),
             ('structuralsubscription', 'subscriber'),
-            # Private-membership teams can have members, but they
-            # cannot be members of other teams.
             ('teammembership', 'team'),
-            # A private-membership team must be able to participate in itself.
             ('teamparticipation', 'person'),
             ('teamparticipation', 'team'),
             # Skip mailing lists because if the mailing list is purged, it's
@@ -2105,9 +2104,8 @@
             ('mailinglist', 'team'),
             ])
 
-        # Private teams may participate in more areas of Launchpad than
-        # Private Membership teams.  The following relationships are allowable
-        # for Private teams and thus should be skipped.
+        # The following relationships are allowable for Private teams and
+        # thus should be skipped.
         if new_value == PersonVisibility.PRIVATE:
             skip.update([('bugsubscription', 'person'),
                          ('bugtask', 'assignee'),

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_team.py	2010-11-23 15:41:34 +0000
@@ -13,11 +13,14 @@
 from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.enum import PersonTransferJobType
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import (
     ITeamPublic,
+    PersonVisibility,
     TeamMembershipRenewalPolicy,
     )
+from lp.registry.model.persontransferjob import PersonTransferJob
 from lp.testing import (
     login_celebrity,
     login_person,
@@ -186,3 +189,23 @@
 
     def test_default_membership_period_maximum(self):
         ITeamPublic['defaultmembershipperiod'].validate(3650)
+
+
+class TestVisibilityConsistencyWarning(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestVisibilityConsistencyWarning, self).setUp()
+        self.team = self.factory.makeTeam()
+        login_celebrity('admin')
+
+    def test_no_warning_for_PersonTransferJob(self):
+        member = self.factory.makePerson()
+        metadata = ('some', 'arbitrary', 'metadata')
+        person_transfer_job = PersonTransferJob(
+            member, self.team,
+            PersonTransferJobType.MEMBERSHIP_NOTIFICATION, metadata)
+        self.assertEqual(
+            None,
+            self.team.visibilityConsistencyWarning(PersonVisibility.PRIVATE))