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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
