launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/code/browser/branch.py'
> --- lib/lp/code/browser/branch.py	2019-04-01 10:09:31 +0000
> +++ lib/lp/code/browser/branch.py	2019-04-15 14:58:20 +0000
> @@ -656,7 +656,9 @@
>      def rescan(self, action, data):
>          self.context.unscan(rescan=True)
>          self.request.response.addNotification("Branch scan scheduled")
> -        self.next_url = canonical_url(self.context)
> +        # This can be used by BMP, in which case we want to redirect back
> +        # from whence it came.

Ignorable pet grammar peeve: "whence" means "from where", so "from whence" is tautologous.

> +        self.next_url = self.request.headers.get('referer')

Hmm.  There isn't much precedent for this; all I see is in lp.bugs.browser.{bugsubscription,bugtask}, and those views seem to have some extra logic in them to ignore certain referrers.  On the other hand the Zope bug that the comments in those views refer to seems to be fixed now, so maybe this is OK.

Can I convince you to take on a side quest of grepping the LP codebase for references to bug 98437 and seeing if any of those workarounds can now be removed?

>  
>  
>  class BranchEditFormView(LaunchpadEditFormView):
> 
> === 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-15 14:58:20 +0000
> @@ -807,6 +807,16 @@
>              return False
>          return latest_preview.job.status == JobStatus.FAILED
>  
> +    def get_rescan_links(self):

As a method, this would have to be spelled getRescanLinks; but I'd suggest making it a property and calling it just rescan_links instead.  (Don't forget to rename the tests too.)

> +        repos = []
> +        source_job = self.context.parent.getLatestScanJob()

If you extend IGitRef as I suggest below, then this can use self.context.merge_source instead, which would feel more natural.

> +        target_job = self.context.target_branch_or_repo.getLatestScanJob()
> +        if source_job and source_job.job.status == JobStatus.FAILED:
> +            repos.append(self.context.parent)
> +        if target_job and target_job.job.status == JobStatus.FAILED:
> +            repos.append(self.context.target_branch_or_repo)
> +        return repos
> +
>  
>  @delegate_to(ICodeReviewVoteReference)
>  class DecoratedCodeReviewVoteReference:
> 
> === modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
> --- lib/lp/code/browser/tests/test_branchmergeproposal.py	2019-01-31 14:21:09 +0000
> +++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2019-04-15 14:58:20 +0000
> @@ -51,6 +51,7 @@
>  from zope.security.proxy import removeSecurityProxy
>  
>  from lp.app.enums import InformationType
> +from lp.code.model.branchjob import BranchScanJob

Sort imports.

>  from lp.code.browser.branch import RegisterBranchMergeProposalView
>  from lp.code.browser.branchmergeproposal import (
>      BranchMergeProposalAddVoteView,
> @@ -2177,6 +2179,33 @@
>          result = view.show_diff_update_link
>          self.assertTrue(result)
>  
> +    def test_get_rescan_links_git(self):
> +        bmp = self.factory.makeBranchMergeProposalForGit()
> +        target_job = GitRefScanJob.create(bmp.target_git_repository)

I normally prefer to use getUtility(IGitRefScanJobSource).create, and similar for BranchScanJob.create below.  It's not critical in test code, but it's a good habit to be in since it minimises circular import problems elsewhere.

If you do this then you'll also need to change the next line to "removeSecurityProxy(target_job).job._status = JobStatus.FAILED".

> +        target_job.job._status = JobStatus.FAILED
> +        view = create_initialized_view(bmp, '+index')
> +        result = view.get_rescan_links()
> +        self.assertEqual([bmp.target_git_repository], result)
> +
> +    def test_get_rescan_links_bzr(self):
> +        bmp = self.factory.makeBranchMergeProposal()
> +        target_job = BranchScanJob.create(bmp.target_branch)
> +        target_job.job._status = JobStatus.FAILED
> +        view = create_initialized_view(bmp, '+index')
> +        result = view.get_rescan_links()
> +        self.assertEqual([bmp.target_branch], result)
> +
> +    def test_get_rescan_links_both_failed(self):
> +        bmp = self.factory.makeBranchMergeProposalForGit()
> +        target_job = GitRefScanJob.create(bmp.target_git_repository)
> +        target_job.job._status = JobStatus.FAILED
> +        source_job = GitRefScanJob.create(bmp.source_git_repository)
> +        source_job.job._status = JobStatus.FAILED
> +        view = create_initialized_view(bmp, '+index')
> +        result = view.get_rescan_links()
> +        self.assertEqual(
> +            [bmp.source_git_repository, bmp.target_git_repository], result)
> +
>  
>  class TestLatestProposalsForEachBranchMixin:
>      """Confirm that the latest branch is returned."""
> 
> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py	2019-01-31 11:33:58 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py	2019-04-15 14:58:20 +0000
> @@ -222,6 +222,9 @@
>          "The parent object for use in navigation: source branch for Bazaar, "
>          "or source repository for Git.")
>  
> +    target_branch_or_repo = Attribute(
> +        "The target source control branch (bzr) or repository (git)")

I don't love the partial duplication of this with merge_target.  Could you instead add a getLatestScanJob method to IGitRef which passes it through to the repository (and whatever else you need), and then you can just use bmp.merge_target.getLatestScanJob and drop target_branch_or_repo?  That's the pattern we use elsewhere.

> +
>  
>  class IBranchMergeProposalView(Interface):
>  
> 
> === 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-15 14:58:20 +0000
> @@ -208,6 +208,25 @@
>          </p>
>        </div>
>      </div>
> +    <div class="yui-g" tal:repeat="repo view/get_rescan_links">
> +      <div class="pending-update" id="diff-pending-update">
> +        <span tal:condition="repeat/repo/start">
> +          <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>
> +        </span>
> +        <p>
> +          <form action="+rescan" tal:attributes="action repo/fmt:url/+rescan"
> +                name="launchpadform" method="post" enctype="multipart/form-data" accept-charset="UTF-8">
> +            <input id="field.actions.rescan" class="button" type="submit"
> +            name="field.actions.rescan" value="Rescan" />
> +            <span tal:content="structure repo/fmt:link/+rescan" />
> +          </form>
> +        </p>
> +      </div>
> +    </div>

While the use of repeat/repo/start is clever, this HTML structure isn't quite valid - if there's more than one rescan link then you'll have multiple divs and inputs that share an id, which is invalid per https://html.spec.whatwg.org/multipage/dom.html#the-id-attribute.  Also I don't think you're allowed to have h3 and p elements inside a span.

Can you instead do something like this?

  <div class="yui-g pending-update" id="diff-pending-update"
       tal:define="rescan_links view/rescan_links"
       tal:condition="rescan_links">
    <h3>Update scan failed</h3>
    <p>At least one ...</p>
    <p>
      <form tal:repeat="rescan_link rescan_links" ... />
    </p>
  </div>

... and drop the id from the input elements, because the name should be enough for the form submission algorithm.

Also consider whether you need to adjust the submit buttons to look right if there's more than one of them - it looks like they'll both say "Rescan", so how is the user going to tell the difference?  You'll probably need to look at this in a browser and experiment a little.

>      <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.