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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
