launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/delete-packaging-link into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1033523 in Launchpad itself: "Users get 403 because Lp has contradictory rules to unlink packages"
  https://bugs.launchpad.net/launchpad/+bug/1033523

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/delete-packaging-link/+merge/124809

Launchpad's zcml states that anyone can unlink a package from a series.
The rule is used on the distribution and source package pages, and on
the project's package pages. When translation message sharing was added,
a condition was added to the code that will raise a forbidden error if
the user attempts to unlink a package from a series that is sharing
translations.

There is no role or action that a user can do that definitely qualify a
user to manage packaging. We know that project users are more likely to
create bogus packaging links and that users create more packaging links
then are deleted.

The Packaging.userCanDelete() restrictions are naive. The rules assume
that shared messages are more important then the true links to the
upstream project. The ubuntu community cannot fix bogus data created by
project maintainers who do not understand packaging. Ubuntu encourages
projects to link to them by making it easy to create the link, and the
community gardens the packaging links. The restrictive code is solving a
problem that never existed, and introduces a new problem that distro
communities cannot manage their data.

The broken rules and tests favoured the package or productseries owner,
but these properties recored registrants, not people with
responsibility. In fact, most of these Packaging permission checks are
asking if the user is ~sinzui or ~jelmer because we created most of the
packaging links :(

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

RULES

    Pre-implementation: jcsackett
    * Either remove Packaging.userCanDelete() or simplify it to verify
      if the user has launchpad experience.


QA

    * Visit https://qastaging.launchpad.net/ubuntu/q-series/+source/tweak
    * Verify there is a delete and edit icon next to the upstream series
    * Open each action and verify the pages loads without error
    * Visit https://translations.qastaging.launchpad.net/ubuntu/precise/+source/tweak/+sharing-details
    * Verify the edit and delete icons are show next to the package
    * Open each and verify the pages loads without error


LINT

    lib/lp/registry/interfaces/packaging.py
    lib/lp/registry/model/packaging.py
    lib/lp/translations/browser/tests/test_sharing_details.py


TEST

    ./bin/test -vvc -t packaging lp.translations.browser.tests.test_sharing


IMPLEMENTATION

I updated Packaging.userCanDelete() to check if the user is not
probationary, which is the experience test we use elsewhere when we do
not want novices creating bogus data. I updated the tests that care
about sharing and packaging to make it clear probationary users cannot
change collection packaging data.
    lib/lp/registry/interfaces/packaging.py
    lib/lp/registry/model/packaging.py
    lib/lp/translations/browser/tests/test_sharing_details.py
-- 
https://code.launchpad.net/~sinzui/launchpad/delete-packaging-link/+merge/124809
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/delete-packaging-link into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/packaging.py'
--- lib/lp/registry/interfaces/packaging.py	2011-12-24 16:54:44 +0000
+++ lib/lp/registry/interfaces/packaging.py	2012-09-17 21:44:23 +0000
@@ -98,9 +98,8 @@
         """True, if the current user is allowed to delete this packaging,
         else False.
 
-        People who created the packaging can delete it, as well as
-        people with upload rights for a source package, distribution
-        owners, members of the registry team and LP admin team.
+        Non-probationary users can delete packaging links that they believe
+        connect Ubuntu to bogus data.
         """
 
 

=== modified file 'lib/lp/registry/model/packaging.py'
--- lib/lp/registry/model/packaging.py	2011-12-30 06:14:56 +0000
+++ lib/lp/registry/model/packaging.py	2012-09-17 21:44:23 +0000
@@ -16,7 +16,6 @@
 from zope.interface import implements
 from zope.security.interfaces import Unauthorized
 
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.packaging import (
     IPackaging,
     IPackagingUtil,
@@ -71,15 +70,9 @@
     def userCanDelete(self):
         """See `IPackaging`."""
         user = getUtility(ILaunchBag).user
-        if user is None:
-            return False
-        admin = getUtility(ILaunchpadCelebrities).admin
-        registry_experts = (
-            getUtility(ILaunchpadCelebrities).registry_experts)
-        return (
-            user.inTeam(self.owner) or
-            user.canAccess(self.sourcepackage, 'setBranch') or
-            user.inTeam(registry_experts) or user.inTeam(admin))
+        if user is not None and not user.is_probationary:
+            return True
+        return False
 
     def destroySelf(self):
         if not self.userCanDelete():

=== modified file 'lib/lp/translations/browser/tests/test_sharing_details.py'
--- lib/lp/translations/browser/tests/test_sharing_details.py	2012-08-06 06:09:19 +0000
+++ lib/lp/translations/browser/tests/test_sharing_details.py	2012-09-17 21:44:23 +0000
@@ -5,7 +5,6 @@
 
 
 import re
-
 from lazr.restful.interfaces import IJSONRequestCache
 from soupmatchers import (
     HTMLContains,
@@ -13,6 +12,7 @@
     )
 
 from lp.app.enums import ServiceUsage
+from lp.registry.model.karma import KarmaTotalCache
 from lp.services.webapp import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
@@ -27,6 +27,7 @@
     extract_text,
     find_tag_by_id,
     )
+from lp.testing.dbuser import dbuser
 from lp.translations.browser.sourcepackage import (
     SourcePackageTranslationSharingDetailsView,
     )
@@ -107,6 +108,9 @@
         self.upstream_only_template = self.factory.makePOTemplate(
             productseries=self.productseries, name='upstream-only')
         self.view = make_initialized_view(self.sourcepackage)
+        self.privileged_user = self.productseries.owner
+        with dbuser('karma'):
+            KarmaTotalCache(person=self.privileged_user.id, karma_total=200)
 
     def configureSharing(self,
             set_upstream_branch=False,
@@ -553,7 +557,7 @@
             text='Set upstream link', visible=False)
         self.assertEqual(expected, self.view.set_packaging_link.escapedtext)
 
-    def test_set_packaging_link__no_packaging_any_user(self):
+    def test_set_packaging_link__no_packaging_probationary_user(self):
         # If packaging is not configured, any user sees the "set packaging"
         # link.
         expected = self._getExpectedPackagingLink(
@@ -566,7 +570,7 @@
             self.assertEqual(
                 expected, self.view.set_packaging_link.escapedtext)
 
-    def test_set_packaging_link__with_packaging_any_user(self):
+    def test_set_packaging_link__with_packaging_probationary_user(self):
         # If packaging is configured, arbitrary users do no see
         # the "set packaging" link.
         self.configureSharing()
@@ -588,7 +592,7 @@
         expected = self._getExpectedPackagingLink(
             id='set-packaging', url='+edit-packaging', icon='add',
             text='Set upstream link', visible=True)
-        with person_logged_in(self.sourcepackage.packaging.owner):
+        with person_logged_in(self.privileged_user):
             view = SourcePackageTranslationSharingDetailsView(
                 self.sourcepackage, LaunchpadTestRequest())
             view.initialize()
@@ -604,7 +608,7 @@
         self.assertEqual(
             expected, self.view.change_packaging_link.escapedtext)
 
-    def test_change_packaging_link__no_packaging_any_user(self):
+    def test_change_packaging_link__no_packaging_probationary_user(self):
         # If packaging is not configured, any user sees the "change packaging"
         # link.
         expected = self._getExpectedPackagingLink(
@@ -617,7 +621,7 @@
             self.assertEqual(
                 expected, self.view.change_packaging_link.escapedtext)
 
-    def test_change_packaging_link__with_packaging_any_user(self):
+    def test_change_packaging_link__with_packaging_probationary_user(self):
         # If packaging is configured, arbitrary users do no see
         # the "change packaging" link.
         self.configureSharing()
@@ -639,7 +643,7 @@
         expected = self._getExpectedPackagingLink(
             id='change-packaging', url='+edit-packaging', icon='edit',
             text='Change upstream link', visible=True)
-        with person_logged_in(self.sourcepackage.packaging.owner):
+        with person_logged_in(self.privileged_user):
             view = SourcePackageTranslationSharingDetailsView(
                 self.sourcepackage, LaunchpadTestRequest())
             view.initialize()
@@ -655,7 +659,7 @@
         self.assertEqual(
             expected, self.view.remove_packaging_link.escapedtext)
 
-    def test_remove_packaging_link__no_packaging_any_user(self):
+    def test_remove_packaging_link__no_packaging_probationary_user(self):
         # If packaging is not configured, any user sees the "remove packaging"
         # link.
         expected = self._getExpectedPackagingLink(
@@ -668,7 +672,7 @@
             self.assertEqual(
                 expected, self.view.remove_packaging_link.escapedtext)
 
-    def test_remove_packaging_link__with_packaging_any_user(self):
+    def test_remove_packaging_link__with_packaging_probationary_user(self):
         # If packaging is configured, arbitrary users do no see
         # the "remove packaging" link.
         self.configureSharing()
@@ -690,7 +694,7 @@
         expected = self._getExpectedPackagingLink(
             id='remove-packaging', url='+remove-packaging', icon='remove',
             text='Remove upstream link', visible=True)
-        with person_logged_in(self.sourcepackage.packaging.owner):
+        with person_logged_in(self.privileged_user):
             view = SourcePackageTranslationSharingDetailsView(
                 self.sourcepackage, LaunchpadTestRequest())
             view.initialize()