Code review comment for lp://staging/~julian-edwards/launchpad/schema-distro-parents

-----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-----