launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'lib/lp/code/browser/configure.zcml'
> --- lib/lp/code/browser/configure.zcml	2018-08-20 23:33:01 +0000
> +++ lib/lp/code/browser/configure.zcml	2018-08-28 15:42:29 +0000
> @@ -819,6 +819,9 @@
>              name="++repository-recipes"
>              template="../templates/gitrepository-recipes.pt"/>
>          <browser:page
> +            name="++ref-pending-merges"

This should be ++repository-pending-merges (and update links and tal: element names in templates to match), or we'll get somewhat confused later.

> +            template="../templates/gitrepository-pending-merges.pt"/>
> +        <browser:page
>              name="++repository-import-details"
>              template="../templates/import-details.pt"/>
>      </browser:pages>
> 
> === modified file 'lib/lp/code/browser/gitrepository.py'
> --- lib/lp/code/browser/gitrepository.py	2018-08-24 16:52:27 +0000
> +++ lib/lp/code/browser/gitrepository.py	2018-08-28 15:42:29 +0000
> @@ -369,6 +372,44 @@
>          """Is this an imported repository?"""
>          return self.context.repository_type == GitRepositoryType.IMPORTED
>  
> +    @property
> +    def show_merge_links(self):
> +        return True

Maybe just "show_merge_links = True"?

> +
> +    @cachedproperty
> +    def landing_candidates(self):
> +        candidates = self.context.getPrecachedLandingCandidates(self.user)
> +        return [proposal for proposal in candidates
> +                if check_permission("launchpad.View", proposal)]
> +
> +    def _getBranchCountText(self, count):
> +        """Help to show user friendly text."""
> +        if count == 0:
> +            return 'No branches'
> +        elif count == 1:
> +            return '1 branch'
> +        else:
> +            return '%s branches' % coun

Typo for "count", and this implies missing tests.

> +
> +    @cachedproperty
> +    def landing_candidate_count_text(self):
> +        return self._getBranchCountText(len(self.landing_candidates))
> +
> +    @cachedproperty
> +    def dependent_landings(self):
> +        return [proposal for proposal in self.context.dependent_landings
> +                if check_permission("launchpad.View", proposal)]
> +
> +    @cachedproperty
> +    def dependent_landing_count_text(self):
> +        return self._getBranchCountText(len(self.dependent_landings))

Maybe we should omit these two properties since we decided not to use them for repositories?

> +
> +    @property

Any reason this isn't a cachedproperty?  It's returning a materialised list rather than a ResultSet or similar, so I think it could be.

> +    def landing_targets(self):
> +        """Return a filtered list of landing targets."""
> +        targets = self.context.getPrecachedLandingTargets(self.user)
> +        return latest_proposals_for_each_branch(targets)
> +
>  
>  class GitRepositoryEditFormView(LaunchpadEditFormView):
>      """Base class for forms that edit a Git repository."""
> 
> === modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
> --- lib/lp/code/browser/tests/test_gitrepository.py	2018-08-24 16:52:27 +0000
> +++ lib/lp/code/browser/tests/test_gitrepository.py	2018-08-28 15:42:29 +0000
> @@ -24,10 +24,10 @@
>  from lp.app.enums import InformationType
>  from lp.app.interfaces.launchpad import ILaunchpadCelebrities
>  from lp.app.interfaces.services import IService
> -from lp.code.enums import GitRepositoryType
> +from lp.code.enums import BranchMergeProposalStatus, GitRepositoryType

We always wrap these; use utilities/format-new-and-modified-imports (though do check the resulting diff, as it occasionally gets slightly confused, particularly by try/except in import blocks).

>  from lp.code.interfaces.revision import IRevisionSet
>  from lp.code.tests.helpers import GitHostingFixture
> -from lp.registry.enums import BranchSharingPolicy
> +from lp.registry.enums import BranchSharingPolicy, VCSType
>  from lp.registry.interfaces.accesspolicy import IAccessPolicySource
>  from lp.registry.interfaces.person import PersonVisibility
>  from lp.services.beautifulsoup import BeautifulSoup
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2018-07-24 11:57:33 +0000
> +++ lib/lp/code/model/gitrepository.py	2018-08-28 15:42:29 +0000
> @@ -942,6 +942,21 @@
>              Not(BranchMergeProposal.queue_status.is_in(
>                  BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
>  
> +    def getMergeProposals(self, status=None, visible_by_user=None,
> +                          merged_revision_ids=None, eager_load=False):
> +        """See `IGitRef`."""

IGitRepository?

> +        if not status:
> +            status = (
> +                BranchMergeProposalStatus.CODE_APPROVED,
> +                BranchMergeProposalStatus.NEEDS_REVIEW,
> +                BranchMergeProposalStatus.WORK_IN_PROGRESS)
> +
> +        collection = getUtility(IAllGitRepositories).visibleByUser(
> +            visible_by_user)
> +        return collection.getMergeProposals(
> +            status, target_repository=self,
> +            merged_revision_ids=merged_revision_ids, eager_load=eager_load)
> +
>      def getMergeProposalByID(self, id):
>          """See `IGitRepository`."""
>          return self.landing_targets.find(BranchMergeProposal.id == id).one()


-- 
https://code.launchpad.net/~twom/launchpad/git-active-review-on-more-git-repo/+merge/353884
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.