launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === added file 'lib/lp/code/templates/branch-snaps.pt'
> --- lib/lp/code/templates/branch-snaps.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/branch-snaps.pt	2015-09-17 11:24:27 +0000
> @@ -0,0 +1,24 @@
> +<div
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  tal:define="context_menu view/context/menu:context"
> +  id="related-snaps">
> +
> +  <h3>Related snap packages</h3>
> +
> +  <div id="snap-links" class="actions">
> +    <div id="snap-summary" tal:condition="view/show_snap_information">
> +      <tal:no-snaps condition="not: view/snap_count">
> +      No snap packages
> +      </tal:no-snaps>
> +      <tal:snaps condition="view/snap_count">
> +        <a href="+snaps" tal:content="structure view/snap_count_text">

Unnecessary structure.

> +          1 snap package
> +        </a>
> +      </tal:snaps>
> +      using this branch.
> +    </div>
> +  </div>
> +
> +</div>
> 
> === added file 'lib/lp/code/templates/gitref-snaps.pt'
> --- lib/lp/code/templates/gitref-snaps.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/gitref-snaps.pt	2015-09-17 11:24:27 +0000
> @@ -0,0 +1,25 @@
> +<div
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  tal:condition="view/show_snap_information"
> +  tal:define="context_menu view/context/menu:context"
> +  id="related-snaps">
> +
> +  <h3>Related snap packages</h3>
> +
> +  <div id="snap-links" class="actions">
> +    <div id="snap-summary">
> +      <tal:no-snaps condition="not: view/snap_count">
> +      No snap packages
> +      </tal:no-snaps>
> +      <tal:snaps condition="view/snap_count">
> +        <a href="+snaps" tal:content="structure view/snap_count_text">

Unnecessary structure.

> +          1 snap package
> +        </a>
> +      </tal:snaps>
> +      using this branch.
> +    </div>
> +  </div>
> +
> +</div>
> 
> === added file 'lib/lp/code/templates/gitrepository-snaps.pt'
> --- lib/lp/code/templates/gitrepository-snaps.pt	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/templates/gitrepository-snaps.pt	2015-09-17 11:24:27 +0000
> @@ -0,0 +1,25 @@
> +<div
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  tal:condition="view/show_snap_information"
> +  tal:define="context_menu context/menu:context"
> +  id="related-snaps">
> +
> +  <h3>Related snap packages</h3>
> +
> +  <div id="snap-links" class="actions">
> +    <div id="snap-summary">
> +      <tal:no-snaps condition="not: view/snap_count">
> +      No snap packages
> +      </tal:no-snaps>
> +      <tal:snaps condition="view/snap_count">
> +        <a href="+snaps" tal:content="structure view/snap_count_text">

Unnecessary structure.

> +          1 snap package
> +        </a>
> +      </tal:snaps>
> +      using this repository.
> +    </div>
> +  </div>
> +
> +</div>
> 
> === modified file 'lib/lp/snappy/browser/configure.zcml'
> --- lib/lp/snappy/browser/configure.zcml	2015-09-04 16:20:26 +0000
> +++ lib/lp/snappy/browser/configure.zcml	2015-09-17 11:24:27 +0000
> @@ -91,5 +91,36 @@
>              for="lp.snappy.interfaces.snapbuild.ISnapBuild"
>              factory="lp.services.webapp.breadcrumb.TitleBreadcrumb"
>              permission="zope.Public" />
> +
> +        <browser:page
> +            for="lp.code.interfaces.branch.IBranch"
> +            class="lp.snappy.browser.snaplisting.BranchSnapListingView"
> +            permission="zope.Public"
> +            name="+snaps"
> +            template="../templates/snap-listing.pt" />
> +        <browser:page
> +            for="lp.code.interfaces.gitrepository.IGitRepository"
> +            class="lp.snappy.browser.snaplisting.GitSnapListingView"
> +            permission="zope.Public"
> +            name="+snaps"
> +            template="../templates/snap-listing.pt" />
> +        <browser:page
> +            for="lp.code.interfaces.gitref.IGitRef"
> +            class="lp.snappy.browser.snaplisting.GitSnapListingView"
> +            permission="zope.Public"
> +            name="+snaps"
> +            template="../templates/snap-listing.pt" />
> +        <browser:page
> +            for="lp.registry.interfaces.person.IPerson"
> +            class="lp.snappy.browser.snaplisting.PersonSnapListingView"
> +            permission="zope.Public"
> +            name="+snaps"
> +            template="../templates/snap-listing.pt" />
> +        <browser:page
> +            for="lp.registry.interfaces.product.IProduct"
> +            class="lp.snappy.browser.snaplisting.ProjectSnapListingView"
> +            permission="zope.Public"
> +            name="+snaps"
> +            template="../templates/snap-listing.pt" />

These should all probably require launchpad.View rather than zope.Public.

>      </facet>
>  </configure>
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2015-09-10 17:15:45 +0000
> +++ lib/lp/snappy/model/snap.py	2015-09-17 11:24:27 +0000
> @@ -360,6 +429,76 @@
>          """See `ISnapSet`."""
>          return IStore(Snap).find(Snap, Snap.git_repository == repository)
>  
> +    def findByGitRef(self, ref):
> +        """See `ISnapSet`."""
> +        return IStore(Snap).find(
> +            Snap,
> +            Snap.git_repository == ref.repository, Snap.git_path == ref.path)
> +
> +    def findByContext(self, context, visible_by_user=None, order_by_date=True):
> +        if IPerson.providedBy(context):
> +            snaps = self.findByPerson(context, visible_by_user=visible_by_user)
> +        elif IProduct.providedBy(context):
> +            snaps = self.findByProject(
> +                context, visible_by_user=visible_by_user)
> +        # XXX cjwatson 2015-09-15: At the moment we can assume that if you
> +        # can see the source context then you can see the snap packages
> +        # based on it.  This will cease to be true if snap packages gain
> +        # privacy of their own.
> +        elif IBranch.providedBy(context):
> +            snaps = self.findByBranch(context)
> +        elif IGitRepository.providedBy(context):
> +            snaps = self.findByGitRepository(context)
> +        elif IGitRef.providedBy(context):
> +            snaps = self.findByGitRef(context)
> +        else:
> +            raise BadSnapSearchContext(context)
> +        if order_by_date:
> +            snaps.order_by(Desc(Snap.date_last_modified))
> +        return snaps
> +
> +    def preloadDataForSnaps(self, snaps, user=None):
> +        """See `ISnapSet`."""
> +        snaps = [removeSecurityProxy(snap) for snap in snaps]
> +
> +        branch_ids = set()
> +        git_ref_keys = set()
> +        person_ids = set()
> +        for snap in snaps:
> +            if snap.branch_id is not None:
> +                branch_ids.add(snap.branch_id)
> +            if snap.git_repository_id is not None:
> +                git_ref_keys.add((snap.git_repository_id, snap.git_path))
> +            person_ids.add(snap.registrant_id)
> +            person_ids.add(snap.owner_id)
> +        git_repository_ids = set(
> +            repository_id for repository_id, _ in git_ref_keys)
> +
> +        branches = load_related(Branch, snaps, ["branch_id"])
> +        repositories = load_related(
> +            GitRepository, snaps, ["git_repository_id"])
> +        load(GitRef, git_ref_keys)

There won't always be matching GitRefs, so this preloading won't always find everything.

> +        # The stacked-on branches are used to check branch visibility.
> +        GenericBranchCollection.preloadVisibleStackedOnBranches(branches, user)
> +        GenericGitCollection.preloadVisibleRepositories(repositories, user)
> +
> +        # Add branch/repository owners to the list of pre-loaded persons.
> +        # We need the target repository owner as well; unlike branches,
> +        # repository unique names aren't trigger-maintained.
> +        person_ids.update(
> +            branch.ownerID for branch in branches if branch.id in branch_ids)
> +        person_ids.update(
> +            repository.owner_id for repository in repositories
> +            if repository.id in git_repository_ids)

When would an ID be in repositories but not git_repository_ids? git_repository_ids is generated from all snaps that have Git repositories.

> +
> +        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> +            person_ids, need_validity=True))
> +
> +        if branches:
> +            GenericBranchCollection.preloadDataForBranches(branches)
> +        if repositories:
> +            GenericGitCollection.preloadDataForRepositories(repositories)

Could these be next to the security preloading?

> +
>      def detachFromBranch(self, branch):
>          """See `ISnapSet`."""
>          self.findByBranch(branch).set(
> 
> === modified file 'lib/lp/soyuz/templates/person-portlet-ppas.pt'
> --- lib/lp/soyuz/templates/person-portlet-ppas.pt	2012-11-01 03:41:36 +0000
> +++ lib/lp/soyuz/templates/person-portlet-ppas.pt	2015-09-17 11:24:27 +0000
> @@ -31,10 +31,13 @@
>  
>      </ul>
>    </div>
> -  <div tal:define="link context/menu:overview/view_recipes"
> -       tal:condition="link/enabled">
> -    <a tal:replace="structure link/fmt:link" />
> -  </div>
> +  <ul class="horizontal" style="margin-top: 0;"
> +      tal:define="recipes_link context/menu:overview/view_recipes;
> +                  snaps_link context/menu:overview/view_snaps"
> +      tal:condition="python: recipes_link.enabled or snaps_link.enabled">
> +    <li><a tal:replace="structure recipes_link/fmt:link" /></li>
> +    <li><a tal:replace="structure snaps_link/fmt:link" /></li>

Do the <li>s need conditions, or are the empty elements visually benign?

> +  </ul>
>  
>  
>  </tal:root>


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-listings/+merge/271307
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.