launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/source-package-owner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/source-package-owner/+merge/86122

Bug nominations can oops when checking if a user has driver permission.

    Launchpad bug: https://bugs.launchpad.net/bugs/id
    Pre-implementation: who

OOPS-ed96d2f6a36a2891db57a41f51bc40bd logged an AttributeError:
'SourcePackage' object has no attribute 'owner' nominating a
sourcepackage. This is similar to bug 901332 where the object does not
implement all the attributes assumed to exist during permission checks.

In this case, the user is a bug supervisor, so the driver permission
check must complete and return false so that the page knows that
the user is nominating only, not nominating and accepting.

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

RULES

    * Add test_personHasDriverRights_false that tests the false case to
      exercise all lines in personHasDriverRights()
    * Add IHasOwner to ISourcePackage.
    * Like SP.driver, the owner attribute will differ to the distroseries.
      * The distroseries differs to the distribution, so no new rights are
        actually being conferred.


QA

    * As an ubuntu bug supervisor (~ubuntu-bugs),
    * Visit https://bugs.qastaging.launchpad.net/ubuntu/oneiric/+source/linux/+bug/861296/+nominate
    * Verify the page loads


LINT

    lib/lp/registry/interfaces/sourcepackage.py
    lib/lp/registry/model/sourcepackage.py
    lib/lp/registry/tests/test_sourcepackage.py


TEST

    ./bin/test -vv -t 'driver|owner' lp.registry.tests.test_sourcepackage


IMPLEMENTATION

Added a negative test for personHasDriverRights(), then a test for the
owner attribute. Added IHasOwner to ISourcePackage, 
    lib/lp/registry/interfaces/sourcepackage.py
    lib/lp/registry/model/sourcepackage.py
    lib/lp/registry/tests/test_sourcepackage.py
-- 
https://code.launchpad.net/~sinzui/launchpad/source-package-owner/+merge/86122
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/source-package-owner into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/sourcepackage.py'
--- lib/lp/registry/interfaces/sourcepackage.py	2011-11-23 03:09:55 +0000
+++ lib/lp/registry/interfaces/sourcepackage.py	2011-12-16 21:53:26 +0000
@@ -58,7 +58,10 @@
     IHasMergeProposals,
     )
 from lp.registry.interfaces.productseries import IProductSeries
-from lp.registry.interfaces.role import IHasDrivers
+from lp.registry.interfaces.role import (
+    IHasDrivers,
+    IHasOwner,
+    )
 from lp.soyuz.interfaces.component import IComponent
 from lp.translations.interfaces.hastranslationtemplates import (
     IHasTranslationTemplates,
@@ -71,7 +74,7 @@
 class ISourcePackagePublic(IBugTarget, IHasBranches, IHasMergeProposals,
                            IHasOfficialBugTags, IHasCodeImports,
                            IHasTranslationImports, IHasTranslationTemplates,
-                           IHasDrivers):
+                           IHasDrivers, IHasOwner):
     """Public attributes for SourcePackage."""
 
     id = Attribute("ID")
@@ -175,6 +178,9 @@
     drivers = Attribute(
         "The drivers for the distroseries for this source package.")
 
+    owner = Attribute(
+        "The owner of the distroseries for this source package.")
+
     def __getitem__(version):
         """Return the source package release with the given version in this
         distro series, or None."""

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2011-12-07 21:32:11 +0000
+++ lib/lp/registry/model/sourcepackage.py	2011-12-16 21:53:26 +0000
@@ -535,6 +535,11 @@
         """See `IHasDrivers`."""
         return self.distroseries.drivers
 
+    @property
+    def owner(self):
+        """See `IHasOwner`."""
+        return self.distroseries.owner
+
     def createBug(self, bug_params):
         """See canonical.launchpad.interfaces.IBugTarget."""
         # We don't currently support opening a new bug directly on an

=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py	2011-12-07 21:32:11 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py	2011-12-16 21:53:26 +0000
@@ -502,7 +502,7 @@
         self.assertNotEqual([], distroseries.drivers)
         self.assertEqual(sourcepackage.drivers, distroseries.drivers)
 
-    def test_personHasDriverRights(self):
+    def test_personHasDriverRights_true(self):
         # A distroseries driver has driver permissions on source packages.
         distroseries = self.factory.makeDistroSeries()
         sourcepackage = self.factory.makeSourcePackage(
@@ -510,6 +510,24 @@
         driver = distroseries.drivers[0]
         self.assertTrue(sourcepackage.personHasDriverRights(driver))
 
+    def test_personHasDriverRights_false(self):
+        # A non-owner/driver/admin does not have driver rights.
+        distroseries = self.factory.makeDistroSeries()
+        sourcepackage = self.factory.makeSourcePackage(
+            distroseries=distroseries)
+        non_priv_user = self.factory.makePerson()
+        self.assertFalse(sourcepackage.personHasDriverRights(non_priv_user))
+
+    def test_owner_is_distroseries_owner(self):
+        # The source package owner differs to the ditroseries owner.
+        distroseries = self.factory.makeDistroSeries()
+        sourcepackage = self.factory.makeSourcePackage(
+            distroseries=distroseries)
+        self.assertIsNot(None, sourcepackage.owner)
+        self.assertEqual(distroseries.owner, sourcepackage.owner)
+        self.assertTrue(
+            sourcepackage.personHasDriverRights(distroseries.owner))
+
 
 class TestSourcePackageWebService(WebServiceTestCase):