Merge into db-devel : schema-distro-parents : Code : Launchpad itself

Status:

Merged

Approved by:

Julian Edwards on 2011-04-18

Approved revision:

no longer in the source branch.

Merged at revision:

10447

Proposed branch:

lp://staging/~julian-edwards/launchpad/schema-distro-parents

Merge into:

lp://staging/launchpad/db-devel

Diff against target:

424 lines (+336/-0)

9 files modified

database/schema/comments.sql (+6/-0)
database/schema/patch-2208-61-0.sql (+20/-0)
database/schema/security.cfg (+1/-0)
lib/canonical/launchpad/security.py (+13/-0)
lib/lp/registry/configure.zcml (+16/-0)
lib/lp/registry/interfaces/distroseriesparent.py (+62/-0)
lib/lp/registry/model/distroseriesparent.py (+74/-0)
lib/lp/registry/tests/test_distroseriesparent.py (+132/-0)
lib/lp/testing/factory.py (+12/-0)

To merge this branch:

bzr merge lp://staging/~julian-edwards/launchpad/schema-distro-parents

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Henning Eggers (community)	code	2011-04-15	Approve on 2011-04-18
Stuart Bishop (community)	db	2011-04-15	Approve on 2011-04-18
Review via email: mp+57898@code.staging.launchpad.net

Commit message

[r=henninge,stub][bug=754750] [incr] Add DistroSeriesParent table to track multiple parents for a derived distroseries.

Description of the change

Add a new database table, DistroSeriesParent, that will track all of a distroseries' derived distroseries and whether they've been initialized or not (initialized is when the packages get copied from the parent, this needs to be tracked so it can't happen twice).

This will eventually replace DistroSeries.parent_series as we need to track more than one parent for a derived series.

Most of the changes are boilerplate. The security declarations are set up so the person changing the data must have edit permission on the derived series.

Other than that, a routine and dull branch.

Revision history for this message

Henning Eggers (henninge) wrote on 2011-04-18:

#

Download full text (8.8 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Julian,
thanks for adding this linking table. You indeed almost had me bored but I did
find a few of my pet issues in your branch ... ;-)

I'd really like to see the tests cleaned up before I approve this.

review needs-fixing

Cheers,
Henning

Am 15.04.2011 18:17, schrieb Julian Edwards:
> === modified file 'database/schema/comments.sql'
> === added file 'database/schema/patch-2208-99-0.sql'
> === modified file 'database/schema/security.cfg'

Stub will comment on these, if needed. Look ok to me. ;)

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2011-04-14 15:26:55 +0000
> +++ lib/canonical/launchpad/security.py 2011-04-15 16:31:45 +0000
> @@ -113,6 +113,7 @@
> IDistributionSourcePackage,
> )
> from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.registry.interfaces.distroseriesparent import IDistroSeriesParent
> from lp.registry.interfaces.distroseriesdifference import (
> IDistroSeriesDifferenceEdit,
> )
> @@ -1014,6 +1015,16 @@
> usedfor = IDistroSeries
>
>
> +class EditDistroSeriesParent(AuthorizationBase):
> + """DistroSeriesParent can be edited by the same people who can edit
> + the derived_distroseries."""
> + permission = "launchpad.Edit"
> + usedfor = IDistroSeriesParent
> +
> + def checkAuthenticated(self, user):
> + return check_permission(self.permission, self.obj.derived_series)

Using check_permission in security adapters is flawed and needs to be stopped.
Please see bug 764406 as to why. You can fix this by referencing
EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins directly or leave it in
to be fixed when that bug is fixed. ;)

> +
> +
> class ViewCountry(AnonymousAuthorization):
> """Anyone can view a Country."""
> usedfor = ICountry
>
> === modified file 'lib/lp/registry/configure.zcml'
> === added file 'lib/lp/registry/interfaces/distroseriesparent.py'
> === added file 'lib/lp/registry/model/distroseriesparent.py'

Yeah, this is the boring bit ... ;) Thanks for a solid implementation.

> === added file 'lib/lp/registry/tests/test_distroseriesparent.py'
> --- lib/lp/registry/tests/test_distroseriesparent.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/tests/test_distroseriesparent.py 2011-04-15 16:31:45 +0000
> @@ -0,0 +1,145 @@
> +# Copyright 2011 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for DistroSeriesParent model class."""
> +
> +__metaclass__ = type
> +
> +from testtools.matchers import (
> + Equals,
> + MatchesStructure,
> + )
> +
> +from zope.component import getUtility
> +from zope.interface.verify import verifyObject
> +from zope.security.interfaces import Unauthorized
> +
> +from canonical.launchpad.ftests import login
> +from canonical.testing.layers import (
> + DatabaseFunctionalLayer,
> + ZopelessDatabaseLayer,
> + )
> +from lp.registry.interfaces.distroseriesparent import (
> + IDistroSeriesParent,
> + IDistroSeriesParentSet,
> + )
> +from lp.testing import (
> + person_logged_in,
> +...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Julian,
thanks for adding this linking table. You indeed almost had me bored but I did
find a few of my pet issues in your branch ... ;-)

I'd really like to see the tests cleaned up before I approve this.

review needs-fixing

Cheers,
Henning

Am 15.04.2011 18:17, schrieb Julian Edwards:
> === modified file 'database/schema/comments.sql'
> === added file 'database/schema/patch-2208-99-0.sql'
> === modified file 'database/schema/security.cfg'

Stub will comment on these, if needed. Look ok to me. ;)

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py	2011-04-14 15:26:55 +0000
> +++ lib/canonical/launchpad/security.py	2011-04-15 16:31:45 +0000
> @@ -113,6 +113,7 @@
>      IDistributionSourcePackage,
>      )
>  from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.registry.interfaces.distroseriesparent import IDistroSeriesParent
>  from lp.registry.interfaces.distroseriesdifference import (
>      IDistroSeriesDifferenceEdit,
>      )
> @@ -1014,6 +1015,16 @@
>      usedfor = IDistroSeries
>  
>  
> +class EditDistroSeriesParent(AuthorizationBase):
> +    """DistroSeriesParent can be edited by the same people who can edit
> +    the derived_distroseries."""
> +    permission = "launchpad.Edit"
> +    usedfor = IDistroSeriesParent
> +
> +    def checkAuthenticated(self, user):
> +        return check_permission(self.permission, self.obj.derived_series)

Using check_permission in security adapters is flawed and needs to be stopped.
Please see bug 764406 as to why. You can fix this  by referencing
EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins directly or leave it in
to be fixed when that bug is fixed. ;)

> +
> +
>  class ViewCountry(AnonymousAuthorization):
>      """Anyone can view a Country."""
>      usedfor = ICountry
> 
> === modified file 'lib/lp/registry/configure.zcml'
> === added file 'lib/lp/registry/interfaces/distroseriesparent.py'
> === added file 'lib/lp/registry/model/distroseriesparent.py'

Yeah, this is the boring bit ... ;) Thanks for a solid implementation.

> === added file 'lib/lp/registry/tests/test_distroseriesparent.py'
> --- lib/lp/registry/tests/test_distroseriesparent.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/tests/test_distroseriesparent.py	2011-04-15 16:31:45 +0000
> @@ -0,0 +1,145 @@
> +# Copyright 2011 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for DistroSeriesParent model class."""
> +
> +__metaclass__ = type
> +
> +from testtools.matchers import (
> +    Equals,
> +    MatchesStructure,
> +    )
> +
> +from zope.component import getUtility
> +from zope.interface.verify import verifyObject
> +from zope.security.interfaces import Unauthorized
> +
> +from canonical.launchpad.ftests import login
> +from canonical.testing.layers import (
> +    DatabaseFunctionalLayer,
> +    ZopelessDatabaseLayer,
> +    )
> +from lp.registry.interfaces.distroseriesparent import (
> +    IDistroSeriesParent,
> +    IDistroSeriesParentSet,
> +    )
> +from lp.testing import (
> +    person_logged_in,
> +    TestCaseWithFactory,
> +    )
> +from lp.testing.sampledata import LAUNCHPAD_ADMIN
> +
> +
> +class TestDistroSeriesParent(TestCaseWithFactory):
> +    """Test the `DistroSeriesParent` model."""
> +    layer = ZopelessDatabaseLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        #self.distribution = self.factory.makeDistribution(name='conftest')

Oh, an unused setUp!

> +
> +    def test_verify_interface(self):
> +        # Test the interface for the model.
> +        dsp = self.factory.makeDistroSeriesParent()
> +        verified = verifyObject(IDistroSeriesParent, dsp)
> +        self.assertTrue(verified)
> +
> +    def test_properties(self):
> +        # Test the model properties.
> +        parent_series = self.factory.makeDistroSeries()
> +        derived_series = self.factory.makeDistroSeries()
> +        dsp = self.factory.makeDistroSeriesParent(
> +            derived_series=derived_series,
> +            parent_series=parent_series,
> +            initialized=True
> +            )
> +
> +        self.assertThat(
> +            dsp,
> +            MatchesStructure(
> +                derived_series=Equals(derived_series),
> +                parent_series=Equals(parent_series),
> +                initialized=Equals(True)
> +                ))
> +
> +    def test_getByDerivedSeries(self):
> +        parent_series = self.factory.makeDistroSeries()
> +        derived_series = self.factory.makeDistroSeries()
> +        self.factory.makeDistroSeriesParent(
> +            derived_series, parent_series)

Hm, why not let makeDistroSeriesParent create the series? Just pick them from
the dsp object you get from the factory.

> +        results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> +            derived_series)
> +        self.assertEqual(1, results.count())
> +        self.assertEqual(parent_series, results[0].parent_series)
> +
> +        # Making a second parent should add it to the results.

Uh-oh, two-stage test. I would prefer this in two tests.

> +        self.factory.makeDistroSeriesParent(
> +            derived_series, self.factory.makeDistroSeries())
> +        results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> +            derived_series)
> +        self.assertEqual(2, results.count())
> +
> +    def test_getByParentSeries(self):
> +        parent_series = self.factory.makeDistroSeries()
> +        derived_series = self.factory.makeDistroSeries()
> +        dsp = self.factory.makeDistroSeriesParent(
> +            derived_series, parent_series)

Same comment about the factory method here.

> +        results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> +            parent_series)
> +        self.assertEqual(1, results.count())
> +        self.assertEqual(derived_series, results[0].derived_series)
> +
> +        # Making a second child should add it to the results.

And here about splitting the test. ;-)

> +        self.factory.makeDistroSeriesParent(
> +            self.factory.makeDistroSeries(), parent_series)
> +        results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> +            parent_series)
> +        self.assertEqual(2, results.count())
> +
> +
> +class TestDistroSeriesParentSecurity(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestDistroSeriesParentSecurity, self).setUp()
> +        self.parent_series = self.factory.makeDistroSeries()
> +        self.derived_series = self.factory.makeDistroSeries()
> +        self.person = self.factory.makePerson()

In case you didn't notice, I don't think using setUp for sample data creation
is a good idea. In this case, self.person is only used in two tests, why not
just create them there? And the series could be created by
makeDistroSeriesParent, just like mentioned in the previous test case. Please
remove setUp from this test case.

> +
> +    def test_random_person_is_unauthorized(self):
> +        dsp_set = getUtility(IDistroSeriesParentSet)
> +        dsp = dsp_set.new(self.derived_series, self.parent_series, False)
> +        with person_logged_in(self.person):
> +            self.assertRaises(
> +                Unauthorized,
> +                setattr, dsp, "derived_series", self.parent_series)
> +
> +    def assertCanEdit(self):
> +        dsp_set = getUtility(IDistroSeriesParentSet)
> +        dsp = dsp_set.new(
> +            self.derived_series, self.parent_series, False)
> +        self.assertThat(
> +            dsp,
> +            MatchesStructure(
> +                derived_series=Equals(self.derived_series),
> +                parent_series=Equals(self.parent_series),
> +                initialized=Equals(False)
> +                ))
> +
> +    def test_distro_drivers_can_edit(self):
> +        # Test that distro drivers can edit the data.
> +        login(LAUNCHPAD_ADMIN)
> +        self.derived_series.distribution.driver = self.person
> +        with person_logged_in(self.person):
> +            self.assertCanEdit()
> +
> +    def test_admins_can_edit(self):
> +        login(LAUNCHPAD_ADMIN)
> +        self.assertCanEdit()
> +
> +    def test_distro_owners_can_edit(self):
> +        login(LAUNCHPAD_ADMIN)
> +        self.derived_series.distribution.owner = self.person
> +        with person_logged_in(self.person):
> +            self.assertCanEdit()
> 
> === modified file 'lib/lp/testing/factory.py'

Thanks for adding the factory to go with this new class.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2sEG8ACgkQBT3oW1L17ijtCACeIwR8ef+KxGpz/UWHJ5e7+x5W
gdYAnjY6RX5u2HyxcrbmTfLtVASIv5YJ
=+P/s
-----END PGP SIGNATURE-----

review: Needs Fixing

Revision history for this message

Stuart Bishop (stub) wrote on 2011-04-18:

#

Discussed on IRC.

The proposed DB patch is fine from DBA pov.

Consider if an enum would be more use than a boolean field.

No need to add checks that the child != parent - the Python code already needs to maintain more than this to avoid loops.

patch-2208-61-0.sql

review: Approve (db)

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2011-04-18:

#

Download full text (7.4 KiB)

Thanks for the review Henning!

On Monday 18 April 2011 11:22:26 you wrote:
> Review: Needs Fixing
> Hi Julian,
> thanks for adding this linking table. You indeed almost had me bored but I
> did find a few of my pet issues in your branch ... ;-)
>
> I'd really like to see the tests cleaned up before I approve this.

Not sure I agree with all of your comments there :-) But read on ...

> >
> > +class EditDistroSeriesParent(AuthorizationBase):
> > + """DistroSeriesParent can be edited by the same people who can edit
> > + the derived_distroseries."""
> > + permission = "launchpad.Edit"
> > + usedfor = IDistroSeriesParent
> > +
> > + def checkAuthenticated(self, user):
> > + return check_permission(self.permission,
> > self.obj.derived_series)
>
> Using check_permission in security adapters is flawed and needs to be
> stopped. Please see bug 764406 as to why. You can fix this by referencing
> EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins directly or leave it
> in to be fixed when that bug is fixed. ;)

Woops, fixed :-)

> > === modified file 'lib/lp/registry/configure.zcml'
> > === added file 'lib/lp/registry/interfaces/distroseriesparent.py'
> > === added file 'lib/lp/registry/model/distroseriesparent.py'
>
> Yeah, this is the boring bit ... ;) Thanks for a solid implementation.

I've done it too many times now!

> > + def setUp(self):
> > + TestCaseWithFactory.setUp(self)
> > + #self.distribution =
> > self.factory.makeDistribution(name='conftest')
>
> Oh, an unused setUp!

Bugger, forgot to remove it :-)

> > +
> > + def test_verify_interface(self):
> > + # Test the interface for the model.
> > + dsp = self.factory.makeDistroSeriesParent()
> > + verified = verifyObject(IDistroSeriesParent, dsp)
> > + self.assertTrue(verified)
> > +
> > + def test_properties(self):
> > + # Test the model properties.
> > + parent_series = self.factory.makeDistroSeries()
> > + derived_series = self.factory.makeDistroSeries()
> > + dsp = self.factory.makeDistroSeriesParent(
> > + derived_series=derived_series,
> > + parent_series=parent_series,
> > + initialized=True
> > + )
> > +
> > + self.assertThat(
> > + dsp,
> > + MatchesStructure(
> > + derived_series=Equals(derived_series),
> > + parent_series=Equals(parent_series),
> > + initialized=Equals(True)
> > + ))
> > +
> > + def test_getByDerivedSeries(self):
> > + parent_series = self.factory.makeDistroSeries()
> > + derived_series = self.factory.makeDistroSeries()
> > + self.factory.makeDistroSeriesParent(
> > + derived_series, parent_series)
>
> Hm, why not let makeDistroSeriesParent create the series? Just pick them
> from the dsp object you get from the factory.

I've re-worked these tests entirely so it's actually testing *editing*
(previously it was doing nothing :/) and they are much simpler now.

>
> > + results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> > + derived_series)
> > + s...

Thanks for the review Henning!

On Monday 18 April 2011 11:22:26 you wrote:
> Review: Needs Fixing
> Hi Julian,
> thanks for adding this linking table. You indeed almost had me bored but I
> did find a few of my pet issues in your branch ... ;-)
> 
> I'd really like to see the tests cleaned up before I approve this.

Not sure I agree with all of your comments there :-)  But read on ...

> > 
> > +class EditDistroSeriesParent(AuthorizationBase):
> > +    """DistroSeriesParent can be edited by the same people who can edit
> > +    the derived_distroseries."""
> > +    permission = "launchpad.Edit"
> > +    usedfor = IDistroSeriesParent
> > +
> > +    def checkAuthenticated(self, user):
> > +        return check_permission(self.permission,
> > self.obj.derived_series)
> 
> Using check_permission in security adapters is flawed and needs to be
> stopped. Please see bug 764406 as to why. You can fix this  by referencing
> EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins directly or leave it
> in to be fixed when that bug is fixed. ;)

Woops, fixed :-)

> > === modified file 'lib/lp/registry/configure.zcml'
> > === added file 'lib/lp/registry/interfaces/distroseriesparent.py'
> > === added file 'lib/lp/registry/model/distroseriesparent.py'
> 
> Yeah, this is the boring bit ... ;) Thanks for a solid implementation.

I've done it too many times now!

> > +    def setUp(self):
> > +        TestCaseWithFactory.setUp(self)
> > +        #self.distribution =
> > self.factory.makeDistribution(name='conftest')
> 
> Oh, an unused setUp!

Bugger, forgot to remove it :-)

> > +
> > +    def test_verify_interface(self):
> > +        # Test the interface for the model.
> > +        dsp = self.factory.makeDistroSeriesParent()
> > +        verified = verifyObject(IDistroSeriesParent, dsp)
> > +        self.assertTrue(verified)
> > +
> > +    def test_properties(self):
> > +        # Test the model properties.
> > +        parent_series = self.factory.makeDistroSeries()
> > +        derived_series = self.factory.makeDistroSeries()
> > +        dsp = self.factory.makeDistroSeriesParent(
> > +            derived_series=derived_series,
> > +            parent_series=parent_series,
> > +            initialized=True
> > +            )
> > +
> > +        self.assertThat(
> > +            dsp,
> > +            MatchesStructure(
> > +                derived_series=Equals(derived_series),
> > +                parent_series=Equals(parent_series),
> > +                initialized=Equals(True)
> > +                ))
> > +
> > +    def test_getByDerivedSeries(self):
> > +        parent_series = self.factory.makeDistroSeries()
> > +        derived_series = self.factory.makeDistroSeries()
> > +        self.factory.makeDistroSeriesParent(
> > +            derived_series, parent_series)
> 
> Hm, why not let makeDistroSeriesParent create the series? Just pick them
> from the dsp object you get from the factory.

I've re-worked these tests entirely so it's actually testing *editing* 
(previously it was doing nothing :/) and they are much simpler now.

> 
> > +        results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> > +            derived_series)
> > +        self.assertEqual(1, results.count())
> > +        self.assertEqual(parent_series, results[0].parent_series)
> > +
> > +        # Making a second parent should add it to the results.
> 
> Uh-oh, two-stage test. I would prefer this in two tests.

I know a few people don't like this pattern, but IMHO that would be bad here 
where the second part depends on the first.  Splitting tests up makes our test 
suite slower and that pisses me off more than multi-stage tests.

> 
> > +        self.factory.makeDistroSeriesParent(
> > +            derived_series, self.factory.makeDistroSeries())
> > +        results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> > +            derived_series)
> > +        self.assertEqual(2, results.count())
> > +
> > +    def test_getByParentSeries(self):
> > +        parent_series = self.factory.makeDistroSeries()
> > +        derived_series = self.factory.makeDistroSeries()
> > +        dsp = self.factory.makeDistroSeriesParent(
> > +            derived_series, parent_series)
> 
> Same comment about the factory method here.

Same reply :)

> 
> > +        results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> > +            parent_series)
> > +        self.assertEqual(1, results.count())
> > +        self.assertEqual(derived_series, results[0].derived_series)
> > +
> > +        # Making a second child should add it to the results.
> 
> And here about splitting the test. ;-)

And .... same :)

> 
> > +        self.factory.makeDistroSeriesParent(
> > +            self.factory.makeDistroSeries(), parent_series)
> > +        results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> > +            parent_series)
> > +        self.assertEqual(2, results.count())
> > +
> > +
> > +class TestDistroSeriesParentSecurity(TestCaseWithFactory):
> > +
> > +    layer = DatabaseFunctionalLayer
> > +
> > +    def setUp(self):
> > +        super(TestDistroSeriesParentSecurity, self).setUp()
> > +        self.parent_series = self.factory.makeDistroSeries()
> > +        self.derived_series = self.factory.makeDistroSeries()
> > +        self.person = self.factory.makePerson()
> 
> In case you didn't notice, I don't think using setUp for sample data
> creation is a good idea. In this case, self.person is only used in two
> tests, why not just create them there? And the series could be created by
> makeDistroSeriesParent, just like mentioned in the previous test case.
> Please remove setUp from this test case.

I was someone who disagreed with your email about this pattern as I take each 
case on its own merits ;)

I've re-written the testst a bit anyway and negated the need for a setup at 
all.  See what you think.

> > +
> > +    def test_random_person_is_unauthorized(self):
> > +        dsp_set = getUtility(IDistroSeriesParentSet)
> > +        dsp = dsp_set.new(self.derived_series, self.parent_series,
> > False) +        with person_logged_in(self.person):
> > +            self.assertRaises(
> > +                Unauthorized,
> > +                setattr, dsp, "derived_series", self.parent_series)
> > +
> > +    def assertCanEdit(self):
> > +        dsp_set = getUtility(IDistroSeriesParentSet)
> > +        dsp = dsp_set.new(
> > +            self.derived_series, self.parent_series, False)
> > +        self.assertThat(
> > +            dsp,
> > +            MatchesStructure(
> > +                derived_series=Equals(self.derived_series),
> > +                parent_series=Equals(self.parent_series),
> > +                initialized=Equals(False)
> > +                ))
> > +
> > +    def test_distro_drivers_can_edit(self):
> > +        # Test that distro drivers can edit the data.
> > +        login(LAUNCHPAD_ADMIN)
> > +        self.derived_series.distribution.driver = self.person
> > +        with person_logged_in(self.person):
> > +            self.assertCanEdit()
> > +
> > +    def test_admins_can_edit(self):
> > +        login(LAUNCHPAD_ADMIN)
> > +        self.assertCanEdit()
> > +
> > +    def test_distro_owners_can_edit(self):
> > +        login(LAUNCHPAD_ADMIN)
> > +        self.derived_series.distribution.owner = self.person
> > +        with person_logged_in(self.person):
> > +            self.assertCanEdit()
> > 
> > === modified file 'lib/lp/testing/factory.py'
> 
> Thanks for adding the factory to go with this new class.

My pleasure.  Thanks for reviewing, let me know what you think.

Cheers
J

partial.diff

Revision history for this message

Henning Eggers (henninge) wrote on 2011-04-18:

#

Yes, much better, thank you. I had actually wondered about how you were testing "can edit" but forgot to ask. Thanks for fixing that.

review: Approve (code)

Launchpad itself

Merge lp://staging/~julian-edwards/launchpad/schema-distro-parents into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lib/canonical/launchpad/security.py'
 --- lib/canonical/launchpad/security.py	2011-04-15 16:23:07 +0000
 +++ lib/canonical/launchpad/security.py	2011-04-18 11:40:43 +0000
@@ -1022,7 +1022,9 @@
      usedfor = IDistroSeriesParent
      def checkAuthenticated(self, user):
--        return check_permission(self.permission, self.obj.derived_series)
++        auth = EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins(
++            self.obj.derived_series)
++        return auth.checkAuthenticated(user)
  class ViewCountry(AnonymousAuthorization):
 === modified file 'lib/lp/registry/tests/test_distroseriesparent.py'
 --- lib/lp/registry/tests/test_distroseriesparent.py	2011-04-15 15:51:18 +0000
 +++ lib/lp/registry/tests/test_distroseriesparent.py	2011-04-18 11:40:55 +0000
@@ -34,10 +34,6 @@
      """Test the `DistroSeriesParent` model."""
      layer = ZopelessDatabaseLayer
--    def setUp(self):
--        TestCaseWithFactory.setUp(self)
--        #self.distribution = self.factory.makeDistribution(name='conftest')
--
      def test_verify_interface(self):
          # Test the interface for the model.
          dsp = self.factory.makeDistroSeriesParent()
@@ -101,45 +97,36 @@
      layer = DatabaseFunctionalLayer
--    def setUp(self):
--        super(TestDistroSeriesParentSecurity, self).setUp()
--        self.parent_series = self.factory.makeDistroSeries()
--        self.derived_series = self.factory.makeDistroSeries()
--        self.person = self.factory.makePerson()
--
      def test_random_person_is_unauthorized(self):
--        dsp_set = getUtility(IDistroSeriesParentSet)
--        dsp = dsp_set.new(self.derived_series, self.parent_series, False)
--        with person_logged_in(self.person):
++        dsp = self.factory.makeDistroSeriesParent()
++        person = self.factory.makePerson()
++        with person_logged_in(person):
              self.assertRaises(
                  Unauthorized,
--                setattr, dsp, "derived_series", self.parent_series)
--
--    def assertCanEdit(self):
--        dsp_set = getUtility(IDistroSeriesParentSet)
--        dsp = dsp_set.new(
--            self.derived_series, self.parent_series, False)
--        self.assertThat(
--            dsp,
--            MatchesStructure(
--                derived_series=Equals(self.derived_series),
--                parent_series=Equals(self.parent_series),
--                initialized=Equals(False)
--                ))
--
--    def test_distro_drivers_can_edit(self):
--        # Test that distro drivers can edit the data.
++                setattr, dsp, "derived_series", dsp.parent_series)
++
++    def assertCanEdit(self, dsp):
++        dsp.initialized = False
++        self.assertEquals(False, dsp.initialized)
++
++    def test_distroseries_drivers_can_edit(self):
++        # Test that distroseries drivers can edit the data.
++        dsp = self.factory.makeDistroSeriesParent()
++        person = self.factory.makePerson()
          login(LAUNCHPAD_ADMIN)
--        self.derived_series.distribution.driver = self.person
--        with person_logged_in(self.person):
--            self.assertCanEdit()
++        dsp.derived_series.driver = person
++        with person_logged_in(person):
++            self.assertCanEdit(dsp)
      def test_admins_can_edit(self):
++        dsp = self.factory.makeDistroSeriesParent()
          login(LAUNCHPAD_ADMIN)
--        self.assertCanEdit()
++        self.assertCanEdit(dsp)
      def test_distro_owners_can_edit(self):
++        dsp = self.factory.makeDistroSeriesParent()
++        person = self.factory.makePerson()
          login(LAUNCHPAD_ADMIN)
--        self.derived_series.distribution.owner = self.person
--        with person_logged_in(self.person):
--            self.assertCanEdit()
++        dsp.derived_series.distribution.owner = person
++        with person_logged_in(person):
++            self.assertCanEdit(dsp)