Merge lp:~twom/launchpad/sometimes-mirrors-are-broken into lp:launchpad

[Tom Wardill]

Add a BROKEN state for mirrors and allow them to be resubmitted

A mirror can be submitted for review that obviously does not work, even before probing (DNS failure, incorrect url, etc).
Currently the only option is to mark these as 'UNOFFICIAL' or they clutter the pendingreview view.

This MP adds a 'BROKEN' state, and allows the owner of the archive to resubmit the archive once it is fixed or the details edited.

[Colin Watson (cjwatson)]

This mostly looks good, but needs another pass to tighten things up.

Could you post screenshots somewhere of DistributionMirror:+index and DistributionMirror:+resubmit with these changes? I'd like to see how it'll be presented to mirror admins.

review: Needs Fixing

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2017-07-31 11:45:32 +0000
+++ lib/lp/registry/browser/configure.zcml	2019-09-03 07:12:58 +0000
@@ -2439,6 +2439,13 @@
         template="../templates/distributionmirror-edit.pt">
     </browser:page>
     <browser:page
+        name="+resubmit"
+        for="lp.registry.interfaces.distributionmirror.IDistributionMirror"
+        class="lp.registry.browser.distributionmirror.DistributionMirrorResubmitView"
+        permission="launchpad.Edit"
+        template="../templates/distributionmirror-resubmit.pt">
+    </browser:page>
+    <browser:page
         name="+review"
         for="lp.registry.interfaces.distributionmirror.IDistributionMirror"
         class="lp.registry.browser.distributionmirror.DistributionMirrorReviewView"

=== modified file 'lib/lp/registry/browser/distributionmirror.py'
--- lib/lp/registry/browser/distributionmirror.py	2015-10-13 13:22:08 +0000
+++ lib/lp/registry/browser/distributionmirror.py	2019-09-03 07:12:58 +0000
@@ -30,8 +30,12 @@
     )
 from lp.archivepublisher.debversion import Version
 from lp.registry.browser.objectreassignment import ObjectReassignmentView
+from lp.registry.errors import InvalidMirrorReviewState
 from lp.registry.interfaces.distribution import IDistributionMirrorMenuMarker
-from lp.registry.interfaces.distributionmirror import IDistributionMirror
+from lp.registry.interfaces.distributionmirror import (
+    IDistributionMirror,
+    MirrorStatus,
+    )
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     canonical_url,
@@ -52,7 +56,7 @@
 
     usedfor = IDistributionMirror
     facet = 'overview'
-    links = ['proberlogs', 'edit', 'review', 'reassign', 'delete']
+    links = ['proberlogs', 'edit', 'review', 'reassign', 'delete', 'resubmit']
 
     @enabled_with_permission('launchpad.Edit')
     def edit(self):
@@ -81,6 +85,12 @@
         text = 'Review mirror'
         return Link('+review', text, icon='edit')
 
+    @enabled_with_permission('launchpad.Edit')
+    def resubmit(self):
+        text = 'Resubmit for review'
+        enabled = self.context.status == MirrorStatus.BROKEN
+        return Link('+resubmit', text, icon='edit', enabled=enabled)
+
 
 class _FlavoursByDistroSeries:
     """A simple class to help when rendering a table of series and flavours
@@ -293,6 +303,29 @@
         self.next_url = canonical_url(self.context)
 
 
+class DistributionMirrorResubmitView(LaunchpadEditFormView):
+
+    schema = IDistributionMirror
+    field_names = []
+
+    @property
+    def label(self):
+        """See `LaunchpadFormView`."""
+        return 'Resubmit mirror %s' % self.context.title
+
+    page_title = label
+
+    @action(_("Resubmit"), name="resubmit")
+    def action_resubmit(self, action, data):
+        try:
+            self.context.resubmitForReview()
+        except InvalidMirrorReviewState:
+            self.request.response.addInfoNotification(
+                "The mirror is not in the correct state"
+                " (broken) and cannot be resubmitted.")
+        self.next_url = canonical_url(self.context)
+
+
 class DistributionMirrorReassignmentView(ObjectReassignmentView):
 
     @property

=== modified file 'lib/lp/registry/browser/tests/distributionmirror-views.txt'
--- lib/lp/registry/browser/tests/distributionmirror-views.txt	2019-05-22 14:57:45 +0000
+++ lib/lp/registry/browser/tests/distributionmirror-views.txt	2019-09-03 07:12:58 +0000
@@ -283,6 +283,32 @@
     False
 
 
+Resubmit distribution mirror
+----------------------------
+
+The mirror owner can resubmit a 'Broken' mirror for a new review.
+
+    >>> login('[e-mail]')
+    >>> review_form['field.status'] = 'BROKEN'
+    >>> review_form['field.whiteboard'] = 'This site is broken.'
+    >>> view =  create_initialized_view(mirror, '+review', form=review_form)
+    >>> form['field.actions.resubmit'] = 'Resubmit'
+    >>> view = create_initialized_view(mirror, '+resubmit', form=form)
+    >>> print mirror.status.name
+    PENDING_REVIEW
+
+The resubmit view should only be available to people with launchpad.Edit.
+
+    >>> login('[e-mail]')
+    >>> view = create_initialized_view(mirror, '+resubmit')
+    >>> check_permission('launchpad.Edit', view)
+    True
+
+    >>> login('[e-mail]')
+    >>> check_permission('launchpad.Edit', view)
+    False
+
+
 Delete distribution mirror
 --------------------------
 
@@ -376,7 +402,7 @@
     Antarctica
 
 The page shows which kind of mirror a mirror is:
-    
+
     >>> print extract_text(find_tag_by_id(content, 'type'))
     Type:
     Archive

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2018-04-22 23:30:37 +0000
+++ lib/lp/registry/configure.zcml	2019-09-03 07:12:58 +0000
@@ -2028,7 +2028,7 @@
             set_attributes="name display_name description whiteboard
                             http_base_url ftp_base_url rsync_base_url enabled
                             speed country content official_candidate owner"
-            attributes="official_candidate whiteboard" />
+            attributes="official_candidate whiteboard resubmitForReview" />
         <require
             permission="launchpad.Admin"
             set_attributes="status reviewer date_reviewed"

=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py	2016-05-24 03:39:36 +0000
+++ lib/lp/registry/errors.py	2019-09-03 07:12:58 +0000
@@ -13,6 +13,7 @@
     'CountryMirrorAlreadySet',
     'DeleteSubscriptionError',
     'InvalidFilename',
+    'InvalidMirrorReviewState',
     'InvalidName',
     'JoinNotAllowed',
     'MirrorNotOfficial',
@@ -223,6 +224,11 @@
     """The information type cannot be changed."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class InvalidMirrorReviewState(Exception):
+    """The mirror is in an invalid state in the review workflow."""
+
+
 class CannotPackageProprietaryProduct(Exception):
     """Raised when a non-PUBLIC product's series is linked to a package."""
 

=== modified file 'lib/lp/registry/interfaces/distributionmirror.py'
--- lib/lp/registry/interfaces/distributionmirror.py	2015-10-26 14:54:43 +0000
+++ lib/lp/registry/interfaces/distributionmirror.py	2019-09-03 07:12:58 +0000
@@ -244,6 +244,13 @@
         the official mirrors for its distribution.
         """)
 
+    BROKEN = DBItem(40, """
+        Broken
+
+        This mirror has been reviewed and seems to not respond or
+        is otherwise misconfigured.
+        """)
+
 
 class DistributionMirrorNameField(ContentNameField):
     errormessage = _("%s is already in use by another distribution mirror.")
@@ -502,6 +509,12 @@
         previously enabled or if it was probed only once.
         """
 
+    def resubmitForReview():
+        """Allow the owner (launchpad.Edit) to resubmit for review.
+
+        Only allows the transition state from 'Broken' to 'Pending Review'.
+        """
+
     def newProbeRecord(log_file):
         """Create and return a new MirrorProbeRecord for this mirror."""
 

=== modified file 'lib/lp/registry/model/distributionmirror.py'
--- lib/lp/registry/model/distributionmirror.py	2015-10-13 13:22:08 +0000
+++ lib/lp/registry/model/distributionmirror.py	2019-09-03 07:12:58 +0000
@@ -39,6 +39,7 @@
 from lp.registry.errors import (
     CannotTransitionToCountryMirror,
     CountryMirrorAlreadySet,
+    InvalidMirrorReviewState,
     MirrorHasNoHTTPURL,
     MirrorNotOfficial,
     MirrorNotProbed,
@@ -331,6 +332,13 @@
         return (self.official_candidate
                 and self.status == MirrorStatus.OFFICIAL)
 
+    def resubmitForReview(self):
+        """See IDistributionMirror"""
+        if self.status != MirrorStatus.BROKEN:
+            raise InvalidMirrorReviewState(
+                "DistributionMirror.status is not BROKEN")
+        self.status = MirrorStatus.PENDING_REVIEW
+
     def shouldDisable(self, expected_file_count=None):
         """See IDistributionMirror"""
         if self.content == MirrorContent.RELEASE:

=== added file 'lib/lp/registry/templates/distributionmirror-resubmit.pt'
--- lib/lp/registry/templates/distributionmirror-resubmit.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/templates/distributionmirror-resubmit.pt	2019-09-03 07:12:58 +0000
@@ -0,0 +1,18 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad">
+  <body>
+    <div metal:fill-slot="main">
+      <div metal:use-macro="context/@@launchpad_form/form">
+        <p metal:fill-slot="extra_top" class="documentDescription">
+          This will resubmit this mirror to the review queue and set
+          the status to 'Pending review'.
+        </p>
+      </div>
+    </div>
+  </body>
+</html>

=== modified file 'lib/lp/registry/tests/test_distributionmirror.py'
--- lib/lp/registry/tests/test_distributionmirror.py	2018-01-02 10:54:31 +0000
+++ lib/lp/registry/tests/test_distributionmirror.py	2019-09-03 07:12:58 +0000
@@ -5,6 +5,7 @@
 
 import transaction
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
@@ -13,11 +14,13 @@
     IDistributionMirrorSet,
     MirrorContent,
     MirrorFreshness,
+    MirrorStatus,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distributionmirror import DistributionMirror
 from lp.services.database.sqlbase import flush_database_updates
 from lp.services.mail import stub
+from lp.services.webapp.authorization import check_permission
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.testing import (
     login,
@@ -224,6 +227,15 @@
         transaction.commit()
         self.assertEqual(len(stub.test_emails), 1)
 
+    def test_permissions_for_resubmit(self):
+        self.assertRaises(
+            Unauthorized, getattr,  self.archive_mirror, 'resubmitForReview')
+        login_as(self.archive_mirror.owner)
+        self.archive_mirror.status = MirrorStatus.BROKEN
+        self.archive_mirror.resubmitForReview()
+        self.assertEqual(
+            MirrorStatus.PENDING_REVIEW, self.archive_mirror.status)
+
 
 class TestDistributionMirrorSet(TestCase):
     layer = LaunchpadFunctionalLayer