launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-edit-import-for-you-redux into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1059324 in Launchpad itself: "Can't edit branch import"
  https://bugs.launchpad.net/launchpad/+bug/1059324

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-edit-import-for-you-redux/+merge/127613

https://code.launchpad.net/~stevenk/launchpad/no-edit-import-for-you/+merge/127417 was landed yesterday, but was rolled back due to the test failing on buildbot. This branch contains the same fix with a new test.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-edit-import-for-you-redux/+merge/127613
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-edit-import-for-you-redux into lp:launchpad.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2012-10-02 07:45:15 +0000
+++ lib/lp/code/browser/branch.py	2012-10-02 23:27:20 +0000
@@ -381,18 +381,14 @@
 
     def edit_import(self):
         text = 'Edit import source or review import'
-        enabled = True
         enabled = (
             self.context.branch_type == BranchType.IMPORTED and
             check_permission('launchpad.Edit', self.context.code_import))
-        return Link(
-            '+edit-import', text, icon='edit', enabled=enabled)
+        return Link('+edit-import', text, icon='edit', enabled=enabled)
 
     @enabled_with_permission('launchpad.Edit')
     def upgrade_branch(self):
-        enabled = False
-        if self.context.needs_upgrading:
-            enabled = True
+        enabled = self.context.needs_upgrading
         return Link(
             '+upgrade', 'Upgrade this branch', icon='edit', enabled=enabled)
 

=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py	2012-10-02 07:45:15 +0000
+++ lib/lp/code/browser/codeimport.py	2012-10-02 23:27:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for CodeImports."""
@@ -27,6 +27,7 @@
 from zope.formlib import form
 from zope.interface import Interface
 from zope.schema import Choice
+from zope.security.interfaces import Unauthorized
 
 from lp import _
 from lp.app.browser.launchpadform import (
@@ -35,7 +36,6 @@
     LaunchpadFormView,
     )
 from lp.app.errors import NotFoundError
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.widgets.itemswidgets import (
     LaunchpadDropdownWidget,
     LaunchpadRadioWidget,
@@ -67,6 +67,7 @@
     )
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
 from lp.registry.interfaces.product import IProduct
+from lp.registry.interfaces.role import IPersonRoles
 from lp.services.fields import URIField
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
@@ -155,9 +156,8 @@
     @cachedproperty
     def _super_user(self):
         """Is the user an admin or member of vcs-imports?"""
-        celebs = getUtility(ILaunchpadCelebrities)
-        return (self.user.inTeam(celebs.admin) or
-                self.user.inTeam(celebs.vcs_imports))
+        role = IPersonRoles(self.user)
+        return role.in_admin or role.in_vcs_imports
 
     def showOptionalMarker(self, field_name):
         """Don't show the optional marker for rcs locations."""
@@ -217,9 +217,7 @@
     """The fields presented on the form for editing a code import."""
 
     use_template(IBranch, ['owner'])
-    use_template(
-        ICodeImport,
-        ['rcs_type', 'cvs_root', 'cvs_module'])
+    use_template(ICodeImport, ['rcs_type', 'cvs_root', 'cvs_module'])
 
     svn_branch_url = URIField(
         title=_("Branch URL"), required=False,
@@ -306,10 +304,10 @@
 
     @property
     def label(self):
+        label = 'Request a code import'
         if self.context_is_product:
-            return 'Request a code import for %s' % self.context.displayname
-        else:
-            return 'Request a code import'
+            label += ' for %s' % self.context.displayname
+        return label
 
     @property
     def cancel_url(self):
@@ -338,7 +336,7 @@
             self.form_fields = any_owner_field + self.form_fields
 
     def setUpWidgets(self):
-        CodeImportBaseView.setUpWidgets(self)
+        super(CodeImportNewView, self).setUpWidgets()
 
         # Extract the radio buttons from the rcs_type widget, so we can
         # display them separately in the form.
@@ -542,9 +540,11 @@
         self.code_import = self.context.code_import
         if self.code_import is None:
             raise NotFoundError
+        if not self._super_user:
+            raise Unauthorized
         # The next and cancel location is the branch details page.
         self.cancel_url = self.next_url = canonical_url(self.context)
-        CodeImportBaseView.initialize(self)
+        super(CodeImportEditView, self).initialize()
 
     @property
     def adapters(self):

=== modified file 'lib/lp/code/browser/tests/test_codeimport.py'
--- lib/lp/code/browser/tests/test_codeimport.py	2012-10-02 07:45:15 +0000
+++ lib/lp/code/browser/tests/test_codeimport.py	2012-10-02 23:27:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the code import browser code."""
@@ -7,14 +7,20 @@
 
 import re
 
+from zope.security.interfaces import Unauthorized
+
 from lp.code.enums import RevisionControlSystems
 from lp.services.webapp import canonical_url
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.pages import (
     extract_text,
     find_tag_by_id,
     )
+from lp.testing.views import create_initialized_view
 
 
 class TestImportDetails(TestCaseWithFactory):
@@ -53,3 +59,11 @@
         svn_details = find_tag_by_id(browser.contents, 'svn-import-details')
         self.assertSvnDetailsDisplayed(
             svn_details, RevisionControlSystems.SVN)
+
+    def test_branch_owner_of_import_forbidden(self):
+        # The branch owner is forbidden to edit an import.
+        cimport = self.factory.makeCodeImport()
+        with person_logged_in(cimport.branch.owner):
+            self.assertRaises(
+                Unauthorized, create_initialized_view, cimport.branch,
+                '+edit-import')

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-10-02 07:45:15 +0000
+++ lib/lp/code/model/branch.py	2012-10-02 23:27:20 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212,W0141,F0401
-
 __metaclass__ = type
 __all__ = [
     'Branch',

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2012-10-02 07:45:15 +0000
+++ lib/lp/code/model/codeimport.py	2012-10-02 23:27:20 +0000
@@ -1,8 +1,6 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212
-
 """Database classes including and related to CodeImport."""
 
 __metaclass__ = type
@@ -148,13 +146,6 @@
         if job is not None:
             if job.state == CodeImportJobState.PENDING:
                 CodeImportJobWorkflow().deletePendingJob(self)
-            else:
-                # When we have job killing, we might want to kill a running
-                # job.
-                pass
-        else:
-            # No job, so nothing to do.
-            pass
 
     results = SQLMultipleJoin(
         'CodeImportResult', joinColumn='code_import',
@@ -239,7 +230,6 @@
         else:
             getUtility(ICodeImportJobWorkflow).requestJob(
                 self.import_job, requester)
-        return None
 
 
 class CodeImportSet: