launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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.