launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/transitionToInformationType-regression-1024148 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1024148 in Launchpad itself: "Branch.transitionToInformationType breaks when making a subscriberless branch private"
  https://bugs.launchpad.net/launchpad/+bug/1024148

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/transitionToInformationType-regression-1024148/+merge/115056

== Implementation ==

Add a check so that in branch's transitionToInformationType(), getPeopleWithoutAccess() is only called with a non empty people parameter.
Also do the check for bugs, even though at the moment bugs will always have subscribers. If things change, the code will be robust.

== Tests ==

Add a new test to TestBranchSetPrivate:
- test_can_transition_with_no_subscribers

== Lint ==

Fix some existing lint in test_branch

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/code/model/branch.py
  lib/lp/code/model/tests/test_branch.py

-- 
https://code.launchpad.net/~wallyworld/launchpad/transitionToInformationType-regression-1024148/+merge/115056
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/transitionToInformationType-regression-1024148 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-10 04:07:25 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-16 03:02:18 +0000
@@ -1815,14 +1815,15 @@
         # information type value.
         if information_type in PRIVATE_INFORMATION_TYPES:
             # Grant the subscriber access if they can't see the bug.
-            service = getUtility(IService, 'sharing')
             subscribers = self.getDirectSubscribers()
-            blind_subscribers = service.getPeopleWithoutAccess(
-                self, subscribers)
-            if len(blind_subscribers):
-                service.ensureAccessGrants(
-                    blind_subscribers, who, bugs=[self],
-                    ignore_permissions=True)
+            if subscribers:
+                service = getUtility(IService, 'sharing')
+                blind_subscribers = service.getPeopleWithoutAccess(
+                    self, subscribers)
+                if len(blind_subscribers):
+                    service.ensureAccessGrants(
+                        blind_subscribers, who, bugs=[self],
+                        ignore_permissions=True)
 
         self.updateHeat()
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/branch.py	2012-07-16 03:02:18 +0000
@@ -264,7 +264,7 @@
             raise BranchCannotChangeInformationType()
         self.information_type = information_type
         self._reconcileAccess()
-        if information_type in PRIVATE_INFORMATION_TYPES:
+        if information_type in PRIVATE_INFORMATION_TYPES and self.subscribers:
             # Grant the subscriber access if they can't see the branch.
             service = getUtility(IService, 'sharing')
             blind_subscribers = service.getPeopleWithoutAccess(

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-07-14 07:50:13 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-07-16 03:02:18 +0000
@@ -2543,6 +2543,19 @@
             InformationType.EMBARGOEDSECURITY,
             get_policies_for_artifact(branch)[0].type)
 
+    def test_can_transition_with_no_subscribers(self):
+        # Ensure that a branch can transition to another private type when
+        # there are no subscribers to the branch.
+        owner = self.factory.makePerson()
+        branch = self.factory.makeBranch(
+            owner=owner, information_type=InformationType.USERDATA)
+        with person_logged_in(owner):
+            branch.unsubscribe(owner, owner)
+        branch.transitionToInformationType(
+            InformationType.EMBARGOEDSECURITY, owner, verify_policy=False)
+        self.assertEqual(
+            InformationType.EMBARGOEDSECURITY, branch.information_type)
+
 
 class TestBranchCommitsForDays(TestCaseWithFactory):
     """Tests for `Branch.commitsForDays`."""
@@ -2896,9 +2909,9 @@
     else:
         information_type = InformationType.PUBLIC
     target_branch = factory.makeAnyBranch(information_type=information_type)
-    revision = factory.makeBranchRevision(revision_id=revision_id,
-                                          branch=target_branch,
-                                          sequence=revno)
+    factory.makeBranchRevision(revision_id=revision_id,
+                               branch=target_branch,
+                               sequence=revno)
     return factory.makeBranchMergeProposal(merged_revno=revno,
                                            target_branch=target_branch)