launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/code/browser/branchmergeproposal.py'
> --- lib/lp/code/browser/branchmergeproposal.py	2018-09-06 16:00:45 +0000
> +++ lib/lp/code/browser/branchmergeproposal.py	2019-01-30 16:19:23 +0000
> @@ -797,6 +798,13 @@
>                  'launchpad.Edit', self.context),
>              })
>  
> +    @property
> +    def show_rescan_link(self):
> +        latest_preview = self.context.getLatestScanJob()

They aren't scan jobs, so I wouldn't talk about them as such, and I wouldn't use the term "rescan".  Maybe getLatestDiffUpdateJob and show_diff_update_link?  Similarly for view names, URLs, user-visible text, etc.

> +        if not latest_preview:
> +            return True
> +        return latest_preview.job.status == JobStatus.FAILED
> +
>  
>  @delegate_to(ICodeReviewVoteReference)
>  class DecoratedCodeReviewVoteReference:
> @@ -922,6 +930,19 @@
>                        reverse=True)
>  
>  
> +class BranchMergeProposalRescanView(LaunchpadEditFormView):
> +
> +    schema = Interface
> +
> +    field_names = []
> +
> +    @action('Rescan', name='rescan')
> +    def rescan(self, action, data):
> +        self.context.rescan()
> +        self.request.response.addNotification("Diff scan scheduled")

"Diff update scheduled"

> +        self.next_url = canonical_url(self.context)
> +
> +
>  class IReviewRequest(Interface):
>      """Schema for requesting a review."""
>  
> 
> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py	2018-10-21 17:32:46 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py	2019-01-30 16:19:23 +0000
> @@ -516,6 +516,11 @@
>          :param previewdiff_id: The ID of the target `PreviewDiff`.
>          """
>  
> +    def getLatestScanJob():
> +        """Return the latest IGenerateIncrementalDiffJob and
> +        IUpdatePreviewDiffJob for this repository.
> +        """

This doesn't match the implementation, since you now only return the latest IUpdatePreviewDiffJob.

> +
>  
>  class IBranchMergeProposalEdit(Interface):
>  
> @@ -662,6 +667,14 @@
>              the current description).
>          """
>  
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def rescan():
> +        """Force a rescan of the diff for this branch merge proposal.
> +
> +        Thus may be helpful in cases where a previous scan has crashed.
> +        """

Let's please not have this indirection layer.  Just use scheduleDiffUpdates directly.

> +
>      @operation_parameters(
>          reviewer=Reference(
>              title=_("A reviewer."), schema=IPerson),


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