launchpad-reviewers team mailing list archive

[Ioana Lasc <mp+433366@xxxxxxxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:registry-can-delete-branch into launchpad:master.

Commit message:
Registry experts can delete bzr branch

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/433366
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:registry-can-delete-branch into launchpad:master.

diff --git a/lib/lp/code/browser/branch.py b/lib/lp/code/browser/branch.py
index 5ecce7e..0645172 100644
--- a/lib/lp/code/browser/branch.py
+++ b/lib/lp/code/browser/branch.py
@@ -219,7 +219,7 @@ class BranchEditMenu(NavigationMenu):
         text = "Change branch details"
         return Link("+edit", text, icon="edit")
 
-    @enabled_with_permission("launchpad.Edit")
+    @enabled_with_permission("launchpad.Moderate")
     def delete(self):
         text = "Delete branch"
         return Link("+delete", text, icon="trash-icon")
@@ -983,7 +983,7 @@ class BranchDeletionView(LaunchpadFormView):
         for item, (operation, reason) in self.context.deletionRequirements(
             eager_load=True
         ).items():
-            allowed = check_permission("launchpad.Edit", item)
+            allowed = check_permission("launchpad.Moderate", item)
             reqs.append((item, operation, reason, allowed))
         return reqs
 
diff --git a/lib/lp/code/browser/configure.zcml b/lib/lp/code/browser/configure.zcml
index d0e3d9a..4d009ef 100644
--- a/lib/lp/code/browser/configure.zcml
+++ b/lib/lp/code/browser/configure.zcml
@@ -468,7 +468,7 @@
         name="+delete"
         for="lp.code.interfaces.branch.IBranch"
         class="lp.code.browser.branch.BranchDeletionView"
-        permission="launchpad.Edit"
+        permission="launchpad.Moderate"
         template="../templates/branch-delete.pt"/>
     <browser:pages
         for="lp.code.interfaces.branch.IBranch"
diff --git a/lib/lp/code/browser/tests/test_branch.py b/lib/lp/code/browser/tests/test_branch.py
index e8d914e..3276f09 100644
--- a/lib/lp/code/browser/tests/test_branch.py
+++ b/lib/lp/code/browser/tests/test_branch.py
@@ -29,7 +29,7 @@ from lp.code.model.branchjob import BranchScanJob
 from lp.code.tests.helpers import BranchHostingFixture
 from lp.registry.enums import BranchSharingPolicy
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import IPersonSet, PersonVisibility
 from lp.services.beautifulsoup import BeautifulSoup
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
@@ -208,6 +208,36 @@ class TestBranchView(BrowserTestCase):
         view = create_initialized_view(branch, "+index")
         self.assertFalse(view.show_merge_links)
 
+    def testDeleteBranch(self):
+        expert = self.factory.makePerson(
+            member_of=[getUtility(IPersonSet).getByName("registry")]
+        )
+        commercial_admin = self.factory.makeCommercialAdmin()
+        random_user = self.factory.makePerson()
+
+        branch = self.factory.makeAnyBranch()
+        branch_url = canonical_url(branch, view_name="+index", rootsite="code")
+
+        # the branch owner sees the Delete Branch link
+        browser = self.getUserBrowser(branch_url, user=branch.owner)
+        browser.open(branch_url)
+        self.assertIn("Delete branch", browser.contents)
+
+        # a commercial admin sees the Delete Branch link
+        browser = self.getUserBrowser(branch_url, user=commercial_admin)
+        browser.open(branch_url)
+        self.assertIn("Delete branch", browser.contents)
+
+        # a registry expert sees the Delete Branch link
+        browser = self.getUserBrowser(branch_url, user=expert)
+        browser.open(branch_url)
+        self.assertIn("Delete branch", browser.contents)
+
+        # random user does not see the Delete Branch link
+        browser = self.getUserBrowser(branch_url, user=random_user)
+        browser.open(branch_url)
+        self.assertNotIn("Delete branch", browser.contents)
+
     def testNoProductSeriesPushingTranslations(self):
         # By default, a branch view shows no product series pushing
         # translations to the branch.
@@ -673,6 +703,68 @@ class TestBranchView(BrowserTestCase):
         self.assertThat(recorder, HasQueryCount(Equals(30)))
 
 
+class TestBranchDeletionView(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_owner_can_delete(self):
+        branch = self.factory.makeAnyBranch()
+        branch_name = branch.displayname
+        branch_unique_name = branch.unique_name
+        branch_url = canonical_url(
+            branch, view_name="+delete", rootsite="code"
+        )
+        browser = self.getUserBrowser(branch_url, user=branch.owner)
+        browser.open(branch_url)
+        self.assertIn("Delete branch %s" % branch_name, browser.contents)
+
+        browser.getControl("Delete").click()
+        self.assertIn(
+            "Branch %s deleted." % branch_unique_name, browser.contents
+        )
+
+    def test_registry_expert_can_delete(self):
+        expert = self.factory.makePerson(
+            member_of=[getUtility(IPersonSet).getByName("registry")]
+        )
+        branch = self.factory.makeAnyBranch()
+        branch_name = branch.displayname
+        branch_unique_name = branch.unique_name
+        branch_url = canonical_url(
+            branch, view_name="+delete", rootsite="code"
+        )
+        browser = self.getUserBrowser(branch_url, user=expert)
+        browser.open(branch_url)
+        self.assertIn("Delete branch %s" % branch_name, browser.contents)
+        browser.getControl("Delete").click()
+        self.assertIn(
+            "Branch %s deleted." % branch_unique_name, browser.contents
+        )
+
+    def test_commercial_admin_can_delete(self):
+        commercial_admin = self.factory.makeCommercialAdmin()
+        branch = self.factory.makeAnyBranch()
+        branch_name = branch.displayname
+        branch_unique_name = branch.unique_name
+        branch_url = canonical_url(
+            branch, view_name="+delete", rootsite="code"
+        )
+        browser = self.getUserBrowser(branch_url, user=commercial_admin)
+        browser.open(branch_url)
+        self.assertIn("Delete branch %s" % branch_name, browser.contents)
+        browser.getControl("Delete").click()
+        self.assertIn(
+            "Branch %s deleted." % branch_unique_name, browser.contents
+        )
+
+    def test_other_user_can_not_delete(self):
+        branch = self.factory.makeAnyBranch()
+        branch_url = canonical_url(
+            branch, view_name="+delete", rootsite="code"
+        )
+        self.assertRaises(Unauthorized, self.getUserBrowser, branch_url)
+
+
 class TestBranchRescanView(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer
diff --git a/lib/lp/code/interfaces/branch.py b/lib/lp/code/interfaces/branch.py
index 8caedc9..d579132 100644
--- a/lib/lp/code/interfaces/branch.py
+++ b/lib/lp/code/interfaces/branch.py
@@ -1266,6 +1266,20 @@ class IBranchModerate(Interface):
         A convenience function wrapper around unscan().
         """
 
+    @call_with(break_references=True)
+    @export_destructor_operation()
+    @operation_for_version("beta")
+    def destroySelf(break_references=False):
+        """Delete the specified branch.
+
+        BranchRevisions associated with this branch will also be deleted.
+
+        :param break_references: If supplied, break any references to this
+            branch by deleting items with mandatory references and
+            NULLing other references.
+        :raise: CannotDeleteBranch if the branch cannot be deleted.
+        """
+
 
 class IBranchEditableAttributes(Interface):
     """IBranch attributes that can be edited.
@@ -1401,20 +1415,6 @@ class IBranchEdit(IWebhookTarget):
             branch.
         """
 
-    @call_with(break_references=True)
-    @export_destructor_operation()
-    @operation_for_version("beta")
-    def destroySelf(break_references=False):
-        """Delete the specified branch.
-
-        BranchRevisions associated with this branch will also be deleted.
-
-        :param break_references: If supplied, break any references to this
-            branch by deleting items with mandatory references and
-            NULLing other references.
-        :raise: CannotDeleteBranch if the branch cannot be deleted.
-        """
-
 
 @exported_as_webservice_entry(plural_name="branches", as_of="beta")
 class IBranch(
diff --git a/lib/lp/code/security.py b/lib/lp/code/security.py
index d6852ed..ddb230f 100644
--- a/lib/lp/code/security.py
+++ b/lib/lp/code/security.py
@@ -206,7 +206,7 @@ class ModerateBranch(EditBranch):
         pillar = branch.product or branch.distribution
         if pillar is not None and user.inTeam(pillar.owner):
             return True
-        return user.in_commercial_admin
+        return user.in_commercial_admin or user.in_registry_experts
 
 
 def can_upload_linked_package(person_role, branch):