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-07-16 00:49:00 +0000
> +++ lib/lp/code/browser/branchmergeproposal.py	2018-09-06 14:26:48 +0000
> @@ -266,7 +266,7 @@
>          if (self.context.queue_status ==
>              BranchMergeProposalStatus.NEEDS_REVIEW):
>              enabled = True
> -            if (self.context.votes.count()) > 0:
> +            if (len(self.context.votes)) > 0:

Unnecessary parens.

>                  text = 'Request another review'
>          return Link('+request-review', text, icon='add', enabled=enabled)
>  
> @@ -890,8 +890,14 @@
>      @cachedproperty
>      def reviews(self):
>          """Return the decorated votes for the proposal."""
> -        users_vote = self.context.getUsersVoteReference(self.user)
> -        return [DecoratedCodeReviewVoteReference(vote, self.user, users_vote)
> +
> +        # This would use getUsersVoteReference, but we need to
> +        # be able to cache the property. We don't need to normalize
> +        # the review types.
> +        users_vote = sorted((uv for uv in self.context.votes
> +                             if uv.reviewer == self.user), 'date_created')

I don't think that's how `sorted` works.  Don't you need `key=attrgetter('date_created')` instead?

> +        final_vote = users_vote[0] if users_vote else None
> +        return [DecoratedCodeReviewVoteReference(vote, self.user, final_vote)
>                  for vote in self.context.votes
>                  if check_permission('launchpad.LimitedView', vote.reviewer)]
>  
> 
> === modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
> --- lib/lp/code/browser/tests/test_gitrepository.py	2018-08-31 11:35:52 +0000
> +++ lib/lp/code/browser/tests/test_gitrepository.py	2018-09-06 14:26:48 +0000
> @@ -275,19 +278,44 @@
>          self.assertEqual('1 branch', view._getBranchCountText(1))
>          self.assertEqual('2 branches', view._getBranchCountText(2))
>  
> +    def test_landing_candidates_query_count(self):
> +        repository = self.factory.makeGitRepository()
> +        git_refs = self.factory.makeGitRefs(
> +            repository,
> +            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
> +
> +        def login_and_view():
> +            with FeatureFixture({"code.git.show_repository_mps": "on"}):
> +                with person_logged_in(repository.owner):
> +                    browser = self.getViewBrowser(repository)
> +                    self.assertIsNotNone(
> +                        find_tag_by_id(browser.contents, 'landing-candidates'))
> +
> +        def create_merge_proposal():
> +            bmp = self.factory.makeBranchMergeProposalForGit(
> +                target_ref=git_refs[0],
> +                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
> +            self.factory.makePreviewDiff(merge_proposal=bmp)
> +
> +        recorder1, recorder2 = record_two_runs(
> +            login_and_view,
> +            create_merge_proposal,
> +            2)
> +        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
> +
>      def test_view_with_landing_targets(self):
>          product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
>          target_repository = self.factory.makeGitRepository(target=product)
>          source_repository = self.factory.makeGitRepository(target=product)
> -        target_git_refs = self.factory.makeGitRefs(
> +        [target_git_refs] = self.factory.makeGitRefs(

`[target_git_refs]` is confusing, because the thing inside the brackets is a single ref.  This should be `[target_git_ref]` (and similarly `[source_git_ref]` below).

>              target_repository,
> -            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
> -        source_git_refs = self.factory.makeGitRefs(
> +            paths=["refs/heads/master"])
> +        [source_git_refs] = self.factory.makeGitRefs(
>              source_repository,
>              paths=["refs/heads/master"])
>          self.factory.makeBranchMergeProposalForGit(
> -            target_ref=target_git_refs[0],
> -            source_ref=source_git_refs[0],
> +            target_ref=target_git_refs,
> +            source_ref=source_git_refs,
>              set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
>          with FeatureFixture({"code.git.show_repository_mps": "on"}):
>              with person_logged_in(target_repository.owner):
> 
> === modified file 'lib/lp/code/model/branchmergeproposal.py'
> --- lib/lp/code/model/branchmergeproposal.py	2018-04-25 12:01:33 +0000
> +++ lib/lp/code/model/branchmergeproposal.py	2018-09-06 14:26:48 +0000
> @@ -1367,6 +1370,20 @@
>          if repositories:
>              GenericGitCollection.preloadDataForRepositories(repositories)
>  
> +        # if we need to include a vote summary, we should precache
> +        # that data too
> +        if include_votes:
> +            vote_list = list(load_referencing(
> +                CodeReviewVoteReference, branch_merge_proposals,
> +                ['branch_merge_proposalID']))
> +            for mp in branch_merge_proposals:
> +                get_property_cache(mp).votes = vote_list
> +
> +            # we also provide a summary of diffs, so load them
> +            load_related(LibraryFileAlias, diffs, ['diff_textID'])
> +            load_referencing(CodeReviewVoteReference, branch_merge_proposals,
> +            ['branch_merge_proposalID'])

The last two lines are confusingly-indented, but also apparently redundant.

> +
>  
>  @implementer(IBranchMergeProposalGetter)
>  class BranchMergeProposalGetter:


-- 
https://code.launchpad.net/~twom/launchpad/precache-gitrepository-branch-queries/+merge/354347
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.