Review: Needs Fixing code
Diff comments:
>
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2015-09-17 11:21:28 +0000
> +++ lib/lp/code/model/branch.py 2015-09-21 10:22:52 +0000
> @@ -474,10 +475,22 @@
> @property
> def landing_candidates(self):
> """See `IBranch`."""
> - return BranchMergeProposal.select("""
> - BranchMergeProposal.target_branch = %s AND
> - BranchMergeProposal.queue_status NOT IN %s
> - """ % sqlvalues(self, BRANCH_MERGE_PROPOSAL_FINAL_STATES))
> + # Circular import.
> + from lp.code.model.branchcollection import GenericBranchCollection
> +
> + result = Store.of(self).find(
> + BranchMergeProposal, BranchMergeProposal.target_branch == self,
> + Not(BranchMergeProposal.queue_status.is_in(
> + BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
> +
> + def eager_load(rows):
> + user = getUtility(ILaunchBag).user
> + branches = load_related(
> + Branch, rows, ['source_branchID', 'prerequisite_branchID'])
> + GenericBranchCollection.preloadVisibleStackedOnBranches(
> + branches, user)
LaunchBag in model code is dodgy, and using it to precache permissions is
treacherous at best. The user must be either passed in explicitly from the
view, or the view needs to do the permission bits itself somehow.
> +
> + return DecoratedResultSet(result, pre_iter_hook=eager_load)
>
> @property
> def dependent_branches(self):
>
> === modified file 'lib/lp/soyuz/model/archivesubscriber.py'
> --- lib/lp/soyuz/model/archivesubscriber.py 2015-07-08 16:05:11 +0000
> +++ lib/lp/soyuz/model/archivesubscriber.py 2015-09-21 10:22:52 +0000
> @@ -204,7 +212,20 @@
>
> def getBySubscriberWithActiveToken(self, subscriber, archive=None):
> """See `IArchiveSubscriberSet`."""
> - return self._getBySubscriber(subscriber, archive, True, True)
> + result = self._getBySubscriber(subscriber, archive, True, True)
> +
> + def eager_load(rows):
> + user = getUtility(ILaunchBag).user
> + subscriptions = map(itemgetter(0), rows)
> + precache_permission_for_objects(
> + None, 'launchpad.View', subscriptions)
> + archives = load_related(Archive, subscriptions, ['archive_id'])
> + list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> + [archive.ownerID for archive in archives],
> need_validity=True))
> + for archive in archives:
> + get_property_cache(archive)._known_subscribers = [user]
More excessive LaunchBaggery. Plus we've no guarantee that the user is a known
subscriber, just that the subscriber is.
You could move the permission precaching back to the view; it's not required
before accessing archive_id here, since the objects aren't proxied yet.
> +
> + return DecoratedResultSet(result, pre_iter_hook=eager_load)
>
> def getByArchive(self, archive, current_only=True):
> """See `IArchiveSubscriberSet`."""
--
https://code.launchpad.net/~cjwatson/launchpad/import-warnings/+merge/271790
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
