launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

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.

Diff comments:

> 
> === 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-08-29 17:15:53 +0000
> @@ -81,6 +81,11 @@
>          text = 'Review mirror'
>          return Link('+review', text, icon='edit')
>  
> +    @enabled_with_permission('launchpad.Edit')
> +    def resubmit(self):
> +        text = 'Resubmit for review'
> +        return Link('+resubmit', text, icon='edit')

We should arrange for this link to only be visible in the navigation menu if the mirror is currently marked as broken.

> +
>  
>  class _FlavoursByDistroSeries:
>      """A simple class to help when rendering a table of series and flavours
> @@ -293,6 +298,27 @@
>          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
> +
> +    @property
> +    def page_title(self):
> +        """The page title."""
> +        return self.label

Just "page_title = label" would do.

> +
> +    @action(_("Resubmit"), name="resubmit")
> +    def action_resubmit(self, action, data):
> +        self.context.resubmitForReview()
> +        self.next_url = canonical_url(self.context)
> +
> +
>  class DistributionMirrorReassignmentView(ObjectReassignmentView):
>  
>      @property
> 
> === 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-08-29 17:15:53 +0000
> @@ -2027,7 +2028,8 @@
>              permission="launchpad.Edit"
>              set_attributes="name display_name description whiteboard
>                              http_base_url ftp_base_url rsync_base_url enabled
> -                            speed country content official_candidate owner"
> +                            speed country content official_candidate owner
> +                            resubmitForReview"

Method calls don't work by setting the attribute, so this won't be effective.  I think you need to move it to attributes instead, but you should be able to confirm this by adding a model security test.

>              attributes="official_candidate whiteboard" />
>          <require
>              permission="launchpad.Admin"
> 
> === 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-08-29 17:15:53 +0000
> @@ -331,6 +331,12 @@
>          return (self.official_candidate
>                  and self.status == MirrorStatus.OFFICIAL)
>  
> +    def resubmitForReview(self):
> +        """See IDistributionMirror"""
> +        if self.status != MirrorStatus.BROKEN:
> +            raise AssertionError("DistributionMirror.status is not BROKEN")

I don't think this should raise an AssertionError as things stand, because there's nothing to stop you going to the +resubmit view when the mirror is in some other status, and that should turn into a proper user-visible error rather than an AssertionError.

> +        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-08-29 17:15:53 +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 archive to the review queue and set

In Launchpad's terminology, it's a mirror, not an archive.

> +          the status to 'Pending Review'.

Nit: the text of the relevant status item in MirrorStatus is in sentence case, i.e. "Pending review", so this should match that.

> +        </p>
> +      </div>
> +    </div>
> +  </body>
> +</html>


-- 
https://code.launchpad.net/~twom/launchpad/sometimes-mirrors-are-broken/+merge/372015
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.