launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-user-can-retry-build into launchpad:master.

Commit message:
Remove unused BuildView.user_can_retry_build

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/392031

This was replaced with a context menu item some years ago, but the property was never removed.  I updated the last remaining tests to check whether the corresponding context menu item is enabled instead.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-user-can-retry-build into launchpad:master.

diff --git a/lib/lp/soyuz/browser/build.py b/lib/lp/soyuz/browser/build.py
index fea84ab..16991ed 100644
--- a/lib/lp/soyuz/browser/build.py
+++ b/lib/lp/soyuz/browser/build.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for builds."""
@@ -64,7 +64,6 @@ from lp.services.webapp import (
     LaunchpadView,
     Link,
     )
-from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import (
     BatchNavigator,
     StormRangeFactory,
@@ -204,15 +203,6 @@ class BuildView(LaunchpadView):
 
     page_title = label
 
-    @property
-    def user_can_retry_build(self):
-        """Return True if the user is permitted to Retry Build.
-
-        The build must be re-tryable.
-        """
-        return (check_permission('launchpad.Edit', self.context)
-            and self.context.can_be_retried)
-
     @cachedproperty
     def package_upload(self):
         """Return the corresponding package upload for this build."""
diff --git a/lib/lp/soyuz/browser/tests/test_build_views.py b/lib/lp/soyuz/browser/tests/test_build_views.py
index 8e13a22..fd54d9b 100644
--- a/lib/lp/soyuz/browser/tests/test_build_views.py
+++ b/lib/lp/soyuz/browser/tests/test_build_views.py
@@ -1,4 +1,4 @@
-# Copyright 2011-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import, print_function, unicode_literals
@@ -36,6 +36,7 @@ from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.model.queue import PackageUploadBuild
 from lp.testing import (
     admin_logged_in,
+    ANONYMOUS,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -52,11 +53,10 @@ class TestBuildViews(TestCaseWithFactory):
         self.empty_request = LaunchpadTestRequest(form={})
         self.admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
 
-    def assertBuildViewRetryIsExpected(self, build, person, expected):
+    def assertBuildMenuRetryIsExpected(self, build, person, expected):
         with person_logged_in(person):
-            build_view = getMultiAdapter(
-                (build, self.empty_request), name="+index")
-            self.assertEqual(build_view.user_can_retry_build, expected)
+            build_menu = BuildContextMenu(build)
+            self.assertEqual(expected, build_menu.retry().enabled)
 
     def test_view_with_component(self):
         # The component name is provided when the component is known.
@@ -112,11 +112,9 @@ class TestBuildViews(TestCaseWithFactory):
             self.assertTrue(build_menu.cancel().enabled)
 
     def test_cannot_retry_stable_distroseries(self):
-        # 'BuildView.user_can_retry_build' property checks not only the
-        # user's permissions to retry but also if a build is in a status
-        # that it can be retried.
-        # The build cannot be retried (see IBuild) because it's targeted to a
-        # released distroseries.
+        # The 'retry' link is only enabled if the user has permissions to
+        # retry and the build has a status such that it can be retried.  A
+        # build for a released distroseries cannot be retried.
         archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
         build = self.factory.makeBinaryPackageBuild(
             archive=archive, status=BuildStatus.FAILEDTOBUILD)
@@ -129,19 +127,17 @@ class TestBuildViews(TestCaseWithFactory):
         self.assertEqual(build_view.buildqueue, None)
         self.assertEqual(build_view.component_name, 'multiverse')
         self.assertFalse(build.can_be_retried)
-        self.assertFalse(build_view.user_can_retry_build)
+        self.assertBuildMenuRetryIsExpected(build, build.archive.owner, False)
 
     def test_retry_ppa_builds(self):
         # PPA builds can always be retried, no matter what status the
         # distroseries has.
         build = self.factory.makeBinaryPackageBuild(
             status=BuildStatus.FAILEDTOBUILD)
-        build_view = getMultiAdapter(
-            (build, self.empty_request), name="+index")
         self.assertTrue(build.can_be_retried)
         # Anonymous, therefore supposed to be disallowed
-        self.assertFalse(build_view.user_can_retry_build)
-        self.assertBuildViewRetryIsExpected(build, build.archive.owner, True)
+        self.assertBuildMenuRetryIsExpected(build, ANONYMOUS, False)
+        self.assertBuildMenuRetryIsExpected(build, build.archive.owner, True)
 
     def test_buildd_admins_retry_builds(self):
         # Buildd admins can retry any build as long is it's in a state that
@@ -153,13 +149,13 @@ class TestBuildViews(TestCaseWithFactory):
             self.assertTrue(build.can_be_retried)
         nopriv = getUtility(IPersonSet).getByName("no-priv")
         # A person with no privileges can't retry
-        self.assertBuildViewRetryIsExpected(build, nopriv, False)
+        self.assertBuildMenuRetryIsExpected(build, nopriv, False)
         # But they can as a member of launchpad-buildd-admins
         buildd_admins = getUtility(IPersonSet).getByName(
             "launchpad-buildd-admins")
         with person_logged_in(self.admin):
             buildd_admins.addMember(nopriv, nopriv)
-        self.assertBuildViewRetryIsExpected(build, nopriv, True)
+        self.assertBuildMenuRetryIsExpected(build, nopriv, True)
 
     def test_packageset_upload_retry(self):
         # A person in a team that has rights to upload to a packageset can
@@ -174,11 +170,11 @@ class TestBuildViews(TestCaseWithFactory):
                 distroseries=build.distro_arch_series.distroseries)
             packageset.add((build.source_package_release.sourcepackagename,))
         # The team doesn't have permission until we grant it
-        self.assertBuildViewRetryIsExpected(build, team.teamowner, False)
+        self.assertBuildMenuRetryIsExpected(build, team.teamowner, False)
         with person_logged_in(self.admin):
             getUtility(IArchivePermissionSet).newPackagesetUploader(
                 archive, team, packageset)
-        self.assertBuildViewRetryIsExpected(build, team.teamowner, True)
+        self.assertBuildMenuRetryIsExpected(build, team.teamowner, True)
 
     def test_build_view_package_upload(self):
         # `BuildView.package_upload` returns the cached `PackageUpload`