launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

This is definitely an improvement, thanks!  Just a bit more to go ...

Diff comments:

> === modified file 'lib/lp/code/browser/branchmergeproposal.py'
> --- lib/lp/code/browser/branchmergeproposal.py	2019-01-31 14:45:32 +0000
> +++ lib/lp/code/browser/branchmergeproposal.py	2019-04-16 15:26:42 +0000
> @@ -807,6 +807,33 @@
>              return False
>          return latest_preview.job.status == JobStatus.FAILED
>  
> +    @property
> +    def show_rescan_link(self):
> +        source_job = self.context.merge_source.getLatestScanJob()
> +        target_job = self.context.merge_target.getLatestScanJob()
> +        if source_job and source_job.job.status == JobStatus.FAILED:
> +            return True
> +        if target_job and target_job.job.status == JobStatus.FAILED:
> +            return True
> +        return False
> +
> +
> +class BranchMergeProposalRescanView(LaunchpadEditFormView):
> +    schema = Interface
> +
> +    field_names = []
> +
> +    @action('Rescan', name='rescan')
> +    def rescan(self, action, data):
> +        source_job = self.context.merge_source.getLatestScanJob()
> +        target_job = self.context.merge_target.getLatestScanJob()
> +        if source_job and source_job.job.status == JobStatus.FAILED:
> +            self.context.merge_source.rescan()
> +        if target_job and target_job.job.status == JobStatus.FAILED:
> +            self.context.merge_target.rescan()

I don't think this can currently work for bzr, because Branch has no rescan method, but rather Branch.unscan(rescan=True).  Perhaps add a rescan method to Branch as a compatibility layer so that you don't need to add conditional code here?

This would have been caught if this view had any tests.  Could you add some?

> +        self.request.response.addNotification("Rescan scheduled")
> +        self.next_url = canonical_url(self.context)
> +
>  
>  @delegate_to(ICodeReviewVoteReference)
>  class DecoratedCodeReviewVoteReference:
> 
> === modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
> --- lib/lp/code/templates/branchmergeproposal-index.pt	2019-01-31 13:48:34 +0000
> +++ lib/lp/code/templates/branchmergeproposal-index.pt	2019-04-16 15:26:42 +0000
> @@ -208,6 +208,21 @@
>          </p>
>        </div>
>      </div>
> +    <div class="yui-g pending-update" id="diff-pending-update"
> +       tal:condition="view/show_rescan_link">
> +        <h3>Update scan failed</h3>
> +        <p>
> +          At least one of the branches involved have failed to scan.
> +          You can manually schedule a rescan if required.
> +        </p>
> +        <p>
> +          <form action="+rescan" name="launchpadform" method="post"
> +                enctype="multipart/form-data" accept-charset="UTF-8">
> +            <input class="button" type="submit"
> +            name="field.actions.rescan" value="Rescan" />

Can you indent this line a bit more, perhaps lining up "name" vertically with "class" on the line above, or at least not aligning it with "<input"?

> +          </form>
> +        </p>
> +    </div>

And while we're on trivial indentation details, you can see here that the contents of the <div> have a four-space indent while everything else here has two.

>      <div id="review-diff" tal:condition="view/preview_diff">
>        <h2>Preview Diff </h2>
>  


-- 
https://code.launchpad.net/~twom/launchpad/add-rescan-link-to-merge-proposals/+merge/366060
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.