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-05 16:58:13 +0000
> @@ -890,7 +890,12 @@
>      @cachedproperty
>      def reviews(self):
>          """Return the decorated votes for the proposal."""
> -        users_vote = self.context.getUsersVoteReference(self.user)
> +
> +        # this would use getUsersVoteReference, but we need to
> +        # be able to cache the property. We don't need to normalize
> +        # the review types.

Super-minor nit: it would be good if your sentence-style comments were always in sentence case, i.e. beginning with a capital letter.

I agree with your argument that we don't need to normalise (or indeed consider) the review type, since getUsersVoteReference only considers that at all if the user is a team, and that can't be the case here.

> +        users_vote = [uv for uv in self.context.votes
> +                      if uv.reviewer == self.user]

Hmm.  Really?  This produces a list, while getUsersVoteReference returns a single element.  Something smells fishy here.

DecoratedCodeReviewVoteReference actually makes relatively little use of this, so it won't cause a crash; but it does have behaviour that depends on whether it's None, and with this code it will never be None.  You can't tell the difference from tests based on branchmergeproposal-vote-summary.pt, but it affects the rendering of branchmergeproposal-votes.pt.  At least lib/lp/code/stories/branches/xx-branchmergeproposals.txt, lib/lp/code/stories/branches/xx-claiming-team-code-reviews.txt, and lib/lp/code/stories/branches/xx-reviewing.txt fail on your branch due to this (though you'll need to fix `BranchMergeProposalMenuMixin.request_review` first in order to see these failures without extra confusion; see below).

>          return [DecoratedCodeReviewVoteReference(vote, self.user, users_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-05 16:58:13 +0000
> @@ -275,6 +278,32 @@
>          self.assertEqual('1 branch', view._getBranchCountText(1))
>          self.assertEqual('2 branches', view._getBranchCountText(2))
>  
> +    def test_landing_candidate_query_count(self):

Could you make this be "landing_candidates" rather than "landing_candidate", and similar with test_landing_target_query_count below?  Seems minor, but I prefer this because it makes it easier to grep for things later if they're spelled the same way.

> +        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)
> +
> +        create_merge_proposal()

Is this extra creation necessary?  Usually it's enough to rely on record_two_runs calling item_creator before the first call to tested_method.  (Same in test_landing_target_query_count.)

> +        recorder1, recorder2 = record_two_runs(
> +            login_and_view,
> +            create_merge_proposal,
> +            10)

10 is probably a bit slower than necessary - I wouldn't have thought you got much more interesting information after say 2.  (Same in test_landing_target_query_count.)

> +        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)
> @@ -296,6 +325,38 @@
>                  self.assertIsNotNone(
>                      find_tag_by_id(browser.contents, 'landing-targets'))
>  
> +    def test_landing_target_query_count(self):
> +        product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
> +        target_repository = self.factory.makeGitRepository(target=product)
> +        source_repository = self.factory.makeGitRepository(target=product)
> +
> +        def create_merge_proposal():
> +            target_git_refs = self.factory.makeGitRefs(
> +                target_repository)

I usually prefer the style `[target_git_ref] = self.factory.makeGitRefs(target_repository)` when just creating a single one.  (Maybe some day we should add a `makeGitRef` helper, but I'd prefer to do that separately and all in one go to see if it feels globally worthwhile.)

> +            source_git_refs = self.factory.makeGitRefs(
> +                source_repository)
> +            bmp = self.factory.makeBranchMergeProposalForGit(
> +                target_ref=target_git_refs[0],
> +                source_ref=source_git_refs[0],
> +                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
> +            self.factory.makePreviewDiff(merge_proposal=bmp)
> +
> +        def login_and_view():
> +            with FeatureFixture({"code.git.show_repository_mps": "on"}):
> +                with person_logged_in(target_repository.owner):
> +                    browser = self.getViewBrowser(
> +                        source_repository, user=source_repository.owner)
> +                    self.assertIsNotNone(
> +                        find_tag_by_id(browser.contents, 'landing-targets'))
> +
> +        create_merge_proposal()
> +
> +        recorder1, recorder2 = record_two_runs(
> +            login_and_view,
> +            create_merge_proposal,
> +            10)
> +        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
> +
>      def test_view_with_inactive_landing_targets(self):
>          product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
>          target_repository = self.factory.makeGitRepository(target=product)
> 
> === 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-05 16:58:13 +0000
> @@ -610,8 +610,11 @@
>      def preview_diff(self):
>          return self._preview_diffs.last()
>  
> -    votes = SQLMultipleJoin(
> -        'CodeReviewVoteReference', joinColumn='branch_merge_proposal')
> +    @cachedproperty
> +    def votes(self):
> +        return list(Store.of(self).find(
> +            CodeReviewVoteReference,
> +            CodeReviewVoteReference.branch_merge_proposal == self))

You'll need to change `BranchMergeProposalMenuMixin.request_review` to account for this, because `self.context.votes` is now a list rather than a ResultSet and so doesn't have `.count`.

>  
>      def getNotificationRecipients(self, min_level):
>          """See IBranchMergeProposal.getNotificationRecipients"""
> @@ -1275,7 +1278,7 @@
>          return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
>  
>      @staticmethod
> -    def preloadDataForBMPs(branch_merge_proposals, user):
> +    def preloadDataForBMPs(branch_merge_proposals, user, include_summary=True):

First reaction is "summary of what?".  Maybe "include_votes"?

>          # Utility to load the data related to a list of bmps.
>          # Circular imports.
>          from lp.code.model.branch import Branch
> @@ -1367,6 +1370,19 @@
>          if repositories:
>              GenericGitCollection.preloadDataForRepositories(repositories)
>  
> +        # if we need to include a vote summary, we should precache
> +        # that data too
> +        if include_summary:
> +            vote_list = list(load_referencing(
> +                                CodeReviewVoteReference,
> +                                branch_merge_proposals,
> +                                ['branch_merge_proposalID']))

Typical indentation style would be more like:

  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'])
> +
>  
>  @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.