launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1083709 into lp:launchpad.

Commit message:
Fix Milestone's launchpad.Edit adapter to respect driver inheritance, rather than just checking participation in the series driver or product owner. Now product drivers can release milestones.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1083709 in Launchpad itself: "Project driver can not edit dateexpected on milestone"
  https://bugs.launchpad.net/launchpad/+bug/1083709

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1083709/+merge/222895

This branch fixes the IMilestone launchpad.Edit adapter to allow product drivers, not just series drivers.

Additionally, I removed isDriver and renamed isOneOfDrivers into its place, as there's never a reason to just check the single driver field. Only specs used that method deliberately, and they only have a single driver, so isOneOfDrivers is fine for everything.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1083709/+merge/222895
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1083709 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2014-01-30 15:04:06 +0000
+++ lib/lp/bugs/model/bugtask.py	2014-06-12 01:38:34 +0000
@@ -1294,8 +1294,7 @@
         owner_context = context
         if IBugTarget.providedBy(context):
             owner_context = context.pillar
-        return (
-            role.isOwner(owner_context) or role.isOneOfDrivers(context))
+        return role.isOwner(owner_context) or role.isDriver(context)
 
     @classmethod
     def userHasBugSupervisorPrivilegesContext(cls, context, user):

=== modified file 'lib/lp/registry/doc/personroles.txt'
--- lib/lp/registry/doc/personroles.txt	2011-05-27 20:10:36 +0000
+++ lib/lp/registry/doc/personroles.txt	2014-06-12 01:38:34 +0000
@@ -90,29 +90,22 @@
 
 == isOwner, isDriver ==
 
-We can easily check for ownership and drivership. This is admittedly not much
-shorter than calling inTeam but clearer to read.
+We can easily check for ownership and drivership. If an object
+provides IHasDrivers, its ancestors' drivers will be checked too.
 
     >>> product = factory.makeProduct(owner=person)
     >>> print roles.isOwner(product)
     True
 
-    >>> print roles.isDriver(product)
+    >>> driver = factory.makePerson()
+    >>> driver_roles = IPersonRoles(driver)
+    >>> print driver_roles.isDriver(product)
     False
-    >>> product.driver = person
-    >>> print roles.isDriver(product)
+    >>> product.driver = driver
+    >>> print driver_roles.isDriver(product)
     True
-
-
-== isOneOfDrivers ==
-
-If an object implements IHasDrivers it lists all drivers of the object and
-possible parent objects. The method isOneOfDrivers lets us check for those.
-
     >>> productseries = factory.makeProductSeries(product=product)
-    >>> print roles.isDriver(productseries)
-    False
-    >>> print roles.isOneOfDrivers(productseries)
+    >>> print driver_roles.isDriver(productseries)
     True
 
 

=== modified file 'lib/lp/registry/interfaces/role.py'
--- lib/lp/registry/interfaces/role.py	2013-05-10 06:28:17 +0000
+++ lib/lp/registry/interfaces/role.py	2014-06-12 01:38:34 +0000
@@ -127,18 +127,15 @@
         """Is this person the owner of the object?"""
 
     def isDriver(obj):
-        """Is this person the driver of the object?"""
+        """Is this person one of the drivers of the object?
+
+        Works on objects that implement 'IHasDrivers', but will check the
+        driver attribute otherwise.
+        """
 
     def isBugSupervisor(obj):
         """Is this person the bug supervisor of the object?"""
 
-    def isOneOfDrivers(obj):
-        """Is this person on of the drivers of the object?
-
-        Works on objects that implement 'IHasDrivers' but will default to
-        isDriver if it doesn't, i.e. check the driver attribute.
-        """
-
     def isOneOf(obj, attributes):
         """Is this person one of the roles in relation to the object?
 

=== modified file 'lib/lp/registry/model/hasdrivers.py'
--- lib/lp/registry/model/hasdrivers.py	2011-05-27 20:10:36 +0000
+++ lib/lp/registry/model/hasdrivers.py	2014-06-12 01:38:34 +0000
@@ -17,6 +17,6 @@
     def personHasDriverRights(self, person):
         """See `IHasDrivers`."""
         person_roles = IPersonRoles(person)
-        return (person_roles.isOneOfDrivers(self) or
+        return (person_roles.isDriver(self) or
                 person_roles.isOwner(self) or
                 person_roles.in_admin)

=== modified file 'lib/lp/registry/model/personroles.py'
--- lib/lp/registry/model/personroles.py	2012-08-21 00:34:02 +0000
+++ lib/lp/registry/model/personroles.py	2014-06-12 01:38:34 +0000
@@ -59,12 +59,8 @@
 
     def isDriver(self, obj):
         """See IPersonRoles."""
-        return self.inTeam(obj.driver)
-
-    def isOneOfDrivers(self, obj):
-        """See IPersonRoles."""
         if not IHasDrivers.providedBy(obj):
-            return self.isDriver(obj)
+            return self.inTeam(obj.driver)
         for driver in obj.drivers:
             if self.inTeam(driver):
                 return True

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2013-04-03 05:04:11 +0000
+++ lib/lp/registry/tests/test_milestone.py	2014-06-12 01:38:34 +0000
@@ -409,6 +409,15 @@
             for permission, names in self.expected_set_permissions.items():
                 self.assertChangeAuthorized(names, self.proprietary_milestone)
 
+    def test_access_for_product_driver(self):
+        # The driver of a private product can changeattributes.
+        driver = self.factory.makePerson()
+        with person_logged_in(self.proprietary_product_owner):
+            self.proprietary_product.driver = driver
+        with person_logged_in(driver):
+            for permission, names in self.expected_set_permissions.items():
+                self.assertChangeAuthorized(names, self.proprietary_milestone)
+
 
 class HasMilestonesSnapshotTestCase(TestCaseWithFactory):
     """A TestCase for snapshots of pillars with milestones."""

=== modified file 'lib/lp/registry/tests/test_personroles.py'
--- lib/lp/registry/tests/test_personroles.py	2012-08-21 00:34:02 +0000
+++ lib/lp/registry/tests/test_personroles.py	2014-06-12 01:38:34 +0000
@@ -100,20 +100,14 @@
         roles = IPersonRoles(self.person)
         self.assertTrue(roles.isDriver(sprint))
 
-    def test_isOneOfDrivers(self):
+    def test_isDriver_parent(self):
         # The person can be one of multiple drivers of if an object
         # implements IHasDrivers.
         productseries = self.factory.makeProductSeries()
         productseries.product.driver = self.person
         productseries.driver = self.factory.makePerson()
         roles = IPersonRoles(self.person)
-        self.assertTrue(roles.isOneOfDrivers(productseries))
-
-    def test_isOneOfDrivers_no_drivers(self):
-        # If the object does not implement IHasDrivers, False is returned.
-        sprint = self.factory.makeSprint()
-        roles = IPersonRoles(self.person)
-        self.assertFalse(roles.isOneOfDrivers(sprint))
+        self.assertTrue(roles.isDriver(productseries))
 
     def test_isBugSupervisor(self):
         # The person can be the bug supervisor of something, e.g. a product.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2014-03-17 21:50:33 +0000
+++ lib/lp/security.py	2014-06-12 01:38:34 +0000
@@ -379,7 +379,7 @@
         """Maintainers, drivers, and admins can drive projects."""
         return (user.in_admin or
                 user.isOwner(self.obj.pillar) or
-                user.isOneOfDrivers(self.obj.pillar))
+                user.isDriver(self.obj.pillar))
 
 
 class EditAccountBySelfOrAdmin(AuthorizationBase):
@@ -588,11 +588,11 @@
         assert self.obj.target
         goal = self.obj.goal
         if goal is not None:
-            if user.isOwner(goal) or user.isOneOfDrivers(goal):
+            if user.isOwner(goal) or user.isDriver(goal):
                 return True
         return (user.in_admin or
                 user.isOwner(self.obj.target) or
-                user.isOneOfDrivers(self.obj.target) or
+                user.isDriver(self.obj.target) or
                 user.isOneOf(
                     self.obj, ['owner', 'drafter', 'assignee', 'approver']))
 
@@ -604,7 +604,7 @@
     def checkAuthenticated(self, user):
         assert self.obj.target
         return (user.isOwner(self.obj.target) or
-                user.isOneOfDrivers(self.obj.target) or
+                user.isDriver(self.obj.target) or
                 user.in_admin)
 
 
@@ -669,10 +669,10 @@
 
     def checkAuthenticated(self, user):
         if self.obj.specification.goal is not None:
-            if user.isOneOfDrivers(self.obj.specification.goal):
+            if user.isDriver(self.obj.specification.goal):
                 return True
         else:
-            if user.isOneOfDrivers(self.obj.specification.target):
+            if user.isDriver(self.obj.specification.target):
                 return True
         return (user.inTeam(self.obj.person) or
                 user.isOneOf(
@@ -716,7 +716,7 @@
         able to change translation settings for a product.
         """
         return (user.isOwner(self.obj) or
-                user.isOneOfDrivers(self.obj) or
+                user.isDriver(self.obj) or
                 user.in_rosetta_experts or
                 user.in_admin)
 
@@ -1373,7 +1373,7 @@
         # Project drivers can view any project announcements.
         # Launchpad admins can view any announcement.
         assert self.obj.target
-        return (user.isOneOfDrivers(self.obj.target) or
+        return (user.isDriver(self.obj.target) or
                 user.isOwner(self.obj.target) or
                 user.in_admin)
 
@@ -1386,7 +1386,7 @@
         """Allow the project owner and drivers to edit any project news."""
 
         assert self.obj.target
-        return (user.isOneOfDrivers(self.obj.target) or
+        return (user.isDriver(self.obj.target) or
                 user.isOwner(self.obj.target) or
                 user.in_admin)
 
@@ -2178,7 +2178,7 @@
         Distribution translation managers and distribution series drivers
         can manage IDistroSeries translations.
         """
-        return (user.isOneOfDrivers(self.obj) or
+        return (user.isDriver(self.obj) or
                 self.forwardCheckAuthenticated(user, self.obj.distribution))
 
 
@@ -2200,7 +2200,7 @@
         """Is the user able to manage `IProductSeries` translations."""
 
         return (user.isOwner(self.obj) or
-                user.isOneOfDrivers(self.obj) or
+                user.isDriver(self.obj) or
                 self.forwardCheckAuthenticated(user, self.obj.product))