Code review comment for lp:~james-w/launchpad/archive-collection

On Mon, Aug 2, 2010 at 1:41 AM, James Westby <email address hidden> wrote:
> James Westby has proposed merging lp:~james-w/launchpad/archive-collection into lp:launchpad/devel.
>
> Requested reviews:
>  Launchpad code reviewers (launchpad-reviewers)
>
>
> Hi,
>
> Here's a branch to add an ArchiveCollection based
> on the new generic Collection.
>
> It's fairly standalone right now, though I ported a
> few easy queries in ArchiveSet methods over to it.
> Hopefully once it is available it will see more use.
>
>

Thanks for doing this. It's important to have new code actually be used.

> === modified file 'lib/lp/services/database/collection.py'
> --- lib/lp/services/database/collection.py      2010-07-16 14:10:18 +0000
> +++ lib/lp/services/database/collection.py      2010-08-02 00:41:06 +0000
> @@ -68,9 +68,6 @@
>             base_tables = list(base.tables)
>
>         self.store = kwargs.get('store')
> -        if self.store is None:
> -            self.store = getUtility(IStoreSelector).get(
> -                MAIN_STORE, DEFAULT_FLAVOR)
>
>         self.tables = (
>             starting_tables + base_tables +
> @@ -118,6 +115,9 @@
>         If no values are requested, this selects the type of object that
>         the Collection is a collection of.
>         """
> +        if self.store is None:
> +            self.store = getUtility(IStoreSelector).get(
> +                MAIN_STORE, DEFAULT_FLAVOR)
>         if len(self.tables) == 0:
>             source = self.store
>         else:
>

What's the reason for this change? Where are the corresponding tests?

> === added file 'lib/lp/soyuz/interfaces/archivecollection.py'
> --- lib/lp/soyuz/interfaces/archivecollection.py        1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/interfaces/archivecollection.py        2010-08-02 00:41:06 +0000
> @@ -0,0 +1,70 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""A collection of archives.
> +
...
> +
> +    def withPurposes(purposes):
> +        """Restrict the archives to one of the given purposes."""

In IBranchCollection, we have a *args method for something similar. Do
you think that would be nicer here?

> === added file 'lib/lp/soyuz/model/archivecollection.py'
> --- lib/lp/soyuz/model/archivecollection.py     1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/model/archivecollection.py     2010-08-02 00:41:06 +0000
> @@ -0,0 +1,86 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
...
> +
> +    def visibleByUser(self, user):
> +        public_archive = And(Archive.private == False,
> +                             Archive._enabled == True)

Maybe it's my crappy email client, but the indentation on this looks messed up.

> +        if user is None:
> +            # Anonymous user; filter to include only public archives in
> +            # the results.
> +            return self.refine(public_archive)
> +        celebs = getUtility(ILaunchpadCelebrities)
> +        if user.inTeam(celebs.admin):
> +            # Admins can see everything
> +            return self
> +        # Enforce privacy-awareness for non-admin users,
> +        # so that they can only see the private archives that they're
> +        # allowed to see.
> +
> +        # Create a subselect to capture all the teams that are
> +        # owners of archives AND the user is a member of:
> +        user_teams_subselect = Select(
> +            TeamParticipation.teamID,
> +            where=And(
> +               TeamParticipation.personID == user.id,
> +               TeamParticipation.teamID == Archive.ownerID))
> +
> +        # Append the extra expression to capture either public
> +        # archives, or archives owned by the user, or archives
> +        # owned by a team of which the user is a member:
> +        # Note: 'Archive.ownerID == user.id'
> +        # is unnecessary below because there is a TeamParticipation
> +        # entry showing that each person is a member of the "team"
> +        # that consists of themselves.
> +
> +        # FIXME: Include private PPA's if user is an uploader
> +        return self.refine(
> +            Or(public_archive, Archive.ownerID.is_in(user_teams_subselect)))
>

Are you going to fix this in this branch? Won't this be buggy if
someone uses it?

> === added file 'lib/lp/soyuz/tests/test_archivecollection.py'
> --- lib/lp/soyuz/tests/test_archivecollection.py        1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/tests/test_archivecollection.py        2010-08-02 00:41:06 +0000
> @@ -0,0 +1,220 @@
...

> +class TestGetPublicationsInArchive(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +

These tests are good, thanks.

One thing I've done with similar tests is have a setUp() that removes
all of the data in question (e.g. removing all branches). That way you
can do actual equality testing rather than testing to if if objects
are members in tests.

You don't have to though.

...
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py   2010-08-01 04:34:26 +0000
> +++ lib/lp/testing/factory.py   2010-08-02 00:41:06 +0000
...
> @@ -1772,11 +1775,17 @@
>             enabled=enabled, require_virtualized=virtualized,
>             description=description)
>
> -        if private:
> +        if private or commercial:
>             naked_archive = removeSecurityProxy(archive)
>             naked_archive.private = True
>             naked_archive.buildd_secret = "sekrit"
>
> +        if commercial:
> +            removeSecurityProxy(archive).commercial = True
> +
> +        if status is not None:
> +            removeSecurityProxy(archive).status = status
> +
>         return archive

Could you please add unit tests for your changes in the factory?
lp/testing/tests/test_factory.py?

jml