Merge lp:~allenap/launchpad/dd-initseries-bug-727105-derivation-vocab into lp:launchpad

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 12705
Proposed branch: lp:~allenap/launchpad/dd-initseries-bug-727105-derivation-vocab
Merge into: lp:launchpad
Diff against target: 516 lines (+369/-12)
7 files modified
lib/lp/registry/adapters.py (+1/-2)
lib/lp/registry/tests/test_adapters.py (+36/-0)
lib/lp/registry/tests/test_distroseries_vocabularies.py (+183/-0)
lib/lp/registry/vocabularies.py (+120/-5)
lib/lp/registry/vocabularies.zcml (+12/-0)
lib/lp/testing/factory.py (+9/-5)
lib/lp/testing/tests/test_factory.py (+8/-0)
To merge this branch: bzr merge lp:~allenap/launchpad/dd-initseries-bug-727105-derivation-vocab
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+52471@code.launchpad.net

Description of the change

This branch adds a named vocabulary ("DistroSeriesDerivation") that
contains the set of distroseries that the context distroseries or
distribution can derive from.

It also fixes adapters for IProductSeries -> IProduct and
IDistroSeries -> IDistribution. The code already exists in the tree
but was not registered or tested.

Support for dir() has also been added to LaunchpadObjectFactory to
help out interactive shenanigans.

This branch will not be committed; it is part of a larger change.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

I don't know why you want to adapt a ProductSeries to a Product or a DistroSeries to a Distribution. It seems weird to me. I don't find any explanation of that in your cover letter. It would be helpful if you used our standard review template-- I'd expect to find this in the "implementation details" section.

With the adapters you've registered, wouldn't IDistribution.providedBy(distro_series) return True? If so, I think the tests are not testing whether the distroseries has been converted to a distribution. Instead of IDistribution.providedBy(distribution), you could do assertEqual(distribution, distro_series.distribution).

I think the comment on LaunchpadObjectFactory.__dir__ is too narrow. While it is helpful to interactive users, it can be used in other ways.

I thought distroseries were generally ordered by version or codename. Why is DistroSeriesDerivationVocabularyFactory using creation date instead?

Getting a list of distroseries by doing set(distribution) is also pretty wacky. I would have expected set(distribution) to fail, because distributions don't seem like they should be iterable.

review: Needs Information
Revision history for this message
Gavin Panella (allenap) wrote :

Hi Aaron,

Thanks for looking at this. In answering your questions below I
discovered a situation I had not tested. I've remedied that, and also
managed to make the code a bit tighter, requiring fewer queries. It
seems I wasn't thinking very well while coding this branch.

> I don't know why you want to adapt a ProductSeries to a Product or a
> DistroSeries to a Distribution. It seems weird to me. I don't find any
> explanation of that in your cover letter. It would be helpful if you used our
> standard review template-- I'd expect to find this in the "implementation
> details" section.

It's a convenient way of getting a IDistribution context from anything
that wants to register for it. Having said that, I think this
vocabulary is only ever needed in an IDistroSeries context, so I
/could/ remove the adaption in the vocabulary and thus the adapters.

Also, note I didn't actually write the adapters; the code already
existed and I just registered it. Not sure what it was doing there to
be honest, but it suited my needs (and I did productseries_to_product
for completeness).

Something that factors into my reasoning is my (mild) dislike of:

    if IOneInterface.providedBy(an_argument):
        do_something()
    elif IOtherInterface.providedBy(an_argument):
        do_something_else()
    ...

when we have a component system all ready and running, although I do
admit that it can be expedient to just do things this way.

> With the adapters you've registered, wouldn't
> IDistribution.providedBy(distro_series) return True? If so, I think the tests
> are not testing whether the distroseries has been converted to a distribution.
> Instead of IDistribution.providedBy(distribution), you could do
> assertEqual(distribution, distro_series.distribution).

IDistribution.providedBy(distro_series) is not true. At least, it
shouldn't be if I understand correctly!

I've added the tests you suggest, but maybe I'll end up killing the
adapters altogether.

> I think the comment on LaunchpadObjectFactory.__dir__ is too narrow. While it
> is helpful to interactive users, it can be used in other ways.

I've changed it to:

        """Enumerate the attributes and methods of the wrapped object factory.

        This is especially useful for interactive users."""

Is that the kind of thing you were thinking of?

> I thought distroseries were generally ordered by version or codename. Why is
> DistroSeriesDerivationVocabularyFactory using creation date instead?

I've ordered it the same as DistroSeriesVocabulary, which seems to be
used quite widely.

> Getting a list of distroseries by doing set(distribution) is also pretty
> wacky. I would have expected set(distribution) to fail, because distributions
> don't seem like they should be iterable.

Yeah, it does read a bit oddly. I'm not the author (I think...) of
Distribution.__iter__() so I can't say why that was decided. I've
spent far too long on this branch as it is to revisit that decision,
so do you mind if I leave it as it is?

Revision history for this message
Aaron Bentley (abentley) wrote :

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

On 11-03-11 12:57 PM, Gavin Panella wrote:
>> I don't know why you want to adapt a ProductSeries to a Product or a
>> DistroSeries to a Distribution. It seems weird to me. I don't find any
>> explanation of that in your cover letter. It would be helpful if you used our
>> standard review template-- I'd expect to find this in the "implementation
>> details" section.
>
> It's a convenient way of getting a IDistribution context from anything
> that wants to register for it.

That's a definition of an adapter, but it doesn't explain the motivation
for providing this adaptation.

> Having said that, I think this
> vocabulary is only ever needed in an IDistroSeries context, so I
> /could/ remove the adaption in the vocabulary and thus the adapters.

That would seem like a more straightforward way of doing things.

> Also, note I didn't actually write the adapters; the code already
> existed and I just registered it.

Instead of going "We haven't used it" and deleting it? :-)

> Not sure what it was doing there to
> be honest, but it suited my needs (and I did productseries_to_product
> for completeness).
>
> Something that factors into my reasoning is my (mild) dislike of:
>
> if IOneInterface.providedBy(an_argument):
> do_something()
> elif IOtherInterface.providedBy(an_argument):
> do_something_else()

I share that dislike, but I don't see how that applies to this context.

This is a situation where you're passing in objects of the wrong type
and adapting them to objects of the correct type. Why not just pass in
objects of the correct type?

> IDistribution.providedBy(distro_series) is not true. At least, it
> shouldn't be if I understand correctly!

I have checked, by downloading your branch and tweaking the test, and
you're right, it doesn't.

> I've changed it to:
>
> """Enumerate the attributes and methods of the wrapped object factory.
>
> This is especially useful for interactive users."""
>
> Is that the kind of thing you were thinking of?

Sure, that's good.

>> Getting a list of distroseries by doing set(distribution) is also pretty
>> wacky. I would have expected set(distribution) to fail, because distributions
>> don't seem like they should be iterable.
>
> Yeah, it does read a bit oddly. I'm not the author (I think...) of
> Distribution.__iter__() so I can't say why that was decided.

Distribution.__iter__ is implemented in terms of Distribution.series, so
I recommend using that instead.

> I've
> spent far too long on this branch as it is to revisit that decision,
> so do you mind if I leave it as it is?

I think switching your client code to use Distribution.series is a very
small hardship, so I'd prefer if you changed it. I didn't mean that you
should remove Distribution.__iter__.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk16d5IACgkQ0F+nu1YWqI2rXACfX367xjl2QOscPS0Iv+8DKaFU
sBkAnAzeL6esmFNZd/FxI1VqO3rAp9c6
=PCpH
-----END PGP SIGNATURE-----

Revision history for this message
Gavin Panella (allenap) wrote :

> > It's a convenient way of getting a IDistribution context from anything
> > that wants to register for it.
>
> That's a definition of an adapter, but it doesn't explain the motivation
> for providing this adaptation.

I don't see the harm in doing it, but that's perhaps because I've not
really experienced a situation where this kind of thing has led to
problems later on.

>
> > Having said that, I think this vocabulary is only ever needed in
> > an IDistroSeries context, so I /could/ remove the adaption in the
> > vocabulary and thus the adapters.
>
> That would seem like a more straightforward way of doing things.

All the vocabulary needs is a distribution to function, yet it will
currently only be used in a distroseries context. It seems strange to
require a distroseries when I could just say "give me anything that
can be adapted to a distribution".

Anyway, I've made it work only for distroseries now.

>
> > Also, note I didn't actually write the adapters; the code already
> > existed and I just registered it.
>
> Instead of going "We haven't used it" and deleting it? :-)

I was wrong; that code is actually used as the basis for other
adapters.

>
> > Not sure what it was doing there to
> > be honest, but it suited my needs (and I did productseries_to_product
> > for completeness).
> >
> > Something that factors into my reasoning is my (mild) dislike of:
> >
> > if IOneInterface.providedBy(an_argument):
> > do_something()
> > elif IOtherInterface.providedBy(an_argument):
> > do_something_else()
>
> I share that dislike, but I don't see how that applies to this context.

If I don't use adapters but want to keep the ability to work in a
distribution or distroseries context I can't really see a way around
the IFoo.providedBy(...) stuff.

Well, I could getattr(context, "distribution", context) but I suspect that
might an order of magnitude worse again :)

>
> This is a situation where you're passing in objects of the wrong type
> and adapting them to objects of the correct type. Why not just pass in
> objects of the correct type?

Because it's a named vocabulary. You get the context in which it is
used. I don't think there is an opportunity to select what gets
passed.

> Distribution.__iter__ is implemented in terms of Distribution.series, so
> I recommend using that instead.

Done.

Thanks again!

=== modified file 'lib/lp/registry/tests/test_distroseries_vocabularies.py'
--- lib/lp/registry/tests/test_distroseries_vocabularies.py 2011-03-11 17:29:36 +0000
+++ lib/lp/registry/tests/test_distroseries_vocabularies.py 2011-03-14 14:58:34 +0000
@@ -51,28 +51,20 @@
51 def test_interfaces(self):51 def test_interfaces(self):
52 # DistroSeriesDerivationVocabularyFactory instances provide52 # DistroSeriesDerivationVocabularyFactory instances provide
53 # IVocabulary and IVocabularyTokenized.53 # IVocabulary and IVocabularyTokenized.
54 distribution = self.factory.makeDistribution()54 distroseries = self.factory.makeDistroSeries()
55 vocabulary = self.vocabulary_factory(distribution)55 vocabulary = self.vocabulary_factory(distroseries)
56 self.assertProvides(vocabulary, IVocabulary)56 self.assertProvides(vocabulary, IVocabulary)
57 self.assertProvides(vocabulary, IVocabularyTokenized)57 self.assertProvides(vocabulary, IVocabularyTokenized)
5858
59 def test_distribution_without_series(self):
60 # Given a distribution without any series, the vocabulary factory
61 # returns a vocabulary for all distroseries in all distributions.
62 distribution = self.factory.makeDistribution()
63 vocabulary = self.vocabulary_factory(distribution)
64 expected_distroseries = set(self.all_distroseries)
65 observed_distroseries = set(term.value for term in vocabulary)
66 self.assertEqual(expected_distroseries, observed_distroseries)
67
68 def test_distribution_with_non_derived_series(self):59 def test_distribution_with_non_derived_series(self):
69 # Given a distribution with series, none of which are derived, the60 # Given a distribution with series, none of which are derived, the
70 # vocabulary factory returns a vocabulary for all distroseries in all61 # vocabulary factory returns a vocabulary for all distroseries in all
71 # distributions *except* the given distribution.62 # distributions *except* the given distribution.
72 distroseries = self.factory.makeDistroSeries()63 distroseries = self.factory.makeDistroSeries()
73 vocabulary = self.vocabulary_factory(distroseries.distribution)64 vocabulary = self.vocabulary_factory(distroseries)
74 expected_distroseries = (65 expected_distroseries = (
75 set(self.all_distroseries) - set(distroseries.distribution))66 set(self.all_distroseries).difference(
67 distroseries.distribution.series))
76 observed_distroseries = set(term.value for term in vocabulary)68 observed_distroseries = set(term.value for term in vocabulary)
77 self.assertEqual(expected_distroseries, observed_distroseries)69 self.assertEqual(expected_distroseries, observed_distroseries)
7870
@@ -86,8 +78,8 @@
86 distribution=parent_distroseries.distribution)78 distribution=parent_distroseries.distribution)
87 distroseries = self.factory.makeDistroSeries(79 distroseries = self.factory.makeDistroSeries(
88 parent_series=parent_distroseries)80 parent_series=parent_distroseries)
89 vocabulary = self.vocabulary_factory(distroseries.distribution)81 vocabulary = self.vocabulary_factory(distroseries)
90 expected_distroseries = set(parent_distroseries.distribution)82 expected_distroseries = set(parent_distroseries.distribution.series)
91 observed_distroseries = set(term.value for term in vocabulary)83 observed_distroseries = set(term.value for term in vocabulary)
92 self.assertEqual(expected_distroseries, observed_distroseries)84 self.assertEqual(expected_distroseries, observed_distroseries)
9385
@@ -100,9 +92,10 @@
100 distroseries = self.factory.makeDistroSeries(92 distroseries = self.factory.makeDistroSeries(
101 distribution=parent_distroseries.distribution,93 distribution=parent_distroseries.distribution,
102 parent_series=parent_distroseries)94 parent_series=parent_distroseries)
103 vocabulary = self.vocabulary_factory(distroseries.distribution)95 vocabulary = self.vocabulary_factory(distroseries)
104 expected_distroseries = (96 expected_distroseries = (
105 set(self.all_distroseries) - set(distroseries.distribution))97 set(self.all_distroseries).difference(
98 distroseries.distribution.series))
106 observed_distroseries = set(term.value for term in vocabulary)99 observed_distroseries = set(term.value for term in vocabulary)
107 self.assertEqual(expected_distroseries, observed_distroseries)100 self.assertEqual(expected_distroseries, observed_distroseries)
108101
@@ -112,7 +105,8 @@
112 distroseries = self.factory.makeDistroSeries()105 distroseries = self.factory.makeDistroSeries()
113 vocabulary = self.vocabulary_factory(distroseries)106 vocabulary = self.vocabulary_factory(distroseries)
114 expected_distroseries = (107 expected_distroseries = (
115 set(self.all_distroseries) - set(distroseries.distribution))108 set(self.all_distroseries).difference(
109 distroseries.distribution.series))
116 observed_distroseries = set(term.value for term in vocabulary)110 observed_distroseries = set(term.value for term in vocabulary)
117 self.assertEqual(expected_distroseries, observed_distroseries)111 self.assertEqual(expected_distroseries, observed_distroseries)
118112
@@ -140,8 +134,9 @@
140 removeSecurityProxy(bbb_series_newer).date_created = two_days_ago134 removeSecurityProxy(bbb_series_newer).date_created = two_days_ago
141135
142 ccc = self.factory.makeDistribution(displayname="ccc")136 ccc = self.factory.makeDistribution(displayname="ccc")
137 ccc_series = self.factory.makeDistroSeries(distribution=ccc)
143138
144 vocabulary = self.vocabulary_factory(ccc)139 vocabulary = self.vocabulary_factory(ccc_series)
145 expected_distroseries = [140 expected_distroseries = [
146 aaa_series_newer, aaa_series_older,141 aaa_series_newer, aaa_series_older,
147 bbb_series_newer, bbb_series_older]142 bbb_series_newer, bbb_series_older]
@@ -154,27 +149,20 @@
154 if series in expected_distroseries]149 if series in expected_distroseries]
155 self.assertEqual(expected_distroseries, observed_distroseries)150 self.assertEqual(expected_distroseries, observed_distroseries)
156151
157 def test_queries_for_distribution_without_series(self):
158 for index in range(10):
159 self.factory.makeDistroSeries()
160 distribution = self.factory.makeDistribution()
161 flush_database_caches()
162 # Getting terms issues two queries: one for the distribution's
163 # serieses and a second for all serieses.
164 with StormStatementRecorder() as recorder:
165 self.vocabulary_factory(distribution).terms
166 self.assertThat(recorder, HasQueryCount(Equals(2)))
167
168 def test_queries_for_distribution_with_non_derived_series(self):152 def test_queries_for_distribution_with_non_derived_series(self):
169 for index in range(10):153 for index in range(10):
170 self.factory.makeDistroSeries()154 self.factory.makeDistroSeries()
171 distribution = self.factory.makeDistribution()155 distribution = self.factory.makeDistribution()
172 self.factory.makeDistroSeries(distribution=distribution)156 distroseries = self.factory.makeDistroSeries(
157 distribution=distribution)
173 flush_database_caches()158 flush_database_caches()
159 # Reload distroseries and distribution; these will reasonably already
160 # be loaded before using the vocabulary.
161 distroseries.distribution
174 # Getting terms issues two queries: one to search for parent serieses162 # Getting terms issues two queries: one to search for parent serieses
175 # (of which there are none) and a second for all serieses.163 # (of which there are none) and a second for all serieses.
176 with StormStatementRecorder() as recorder:164 with StormStatementRecorder() as recorder:
177 self.vocabulary_factory(distribution).terms165 self.vocabulary_factory(distroseries).terms
178 self.assertThat(recorder, HasQueryCount(Equals(2)))166 self.assertThat(recorder, HasQueryCount(Equals(2)))
179167
180 def test_queries_for_distribution_with_derived_series(self):168 def test_queries_for_distribution_with_derived_series(self):
@@ -182,11 +170,14 @@
182 self.factory.makeDistroSeries()170 self.factory.makeDistroSeries()
183 distribution = self.factory.makeDistribution()171 distribution = self.factory.makeDistribution()
184 parent_distroseries = self.factory.makeDistroSeries()172 parent_distroseries = self.factory.makeDistroSeries()
185 self.factory.makeDistroSeries(173 distroseries = self.factory.makeDistroSeries(
186 parent_series=parent_distroseries,174 parent_series=parent_distroseries,
187 distribution=distribution)175 distribution=distribution)
188 flush_database_caches()176 flush_database_caches()
177 # Reload distroseries and distribution; these will reasonably already
178 # be loaded before using the vocabulary.
179 distroseries.distribution
189 # Getting terms issues one query to find parent serieses.180 # Getting terms issues one query to find parent serieses.
190 with StormStatementRecorder() as recorder:181 with StormStatementRecorder() as recorder:
191 self.vocabulary_factory(distribution).terms182 self.vocabulary_factory(distroseries).terms
192 self.assertThat(recorder, HasQueryCount(Equals(1)))183 self.assertThat(recorder, HasQueryCount(Equals(1)))
193184
=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py 2011-03-11 17:29:36 +0000
+++ lib/lp/registry/vocabularies.py 2011-03-14 14:36:36 +0000
@@ -1500,7 +1500,8 @@
15001500
1501 def __init__(self, context):1501 def __init__(self, context):
1502 """See `IVocabularyFactory.__call__`."""1502 """See `IVocabularyFactory.__call__`."""
1503 self.context = context1503 assert IDistroSeries.providedBy(context)
1504 self.distribution = context.distribution
15041505
1505 def find_terms(self, *where):1506 def find_terms(self, *where):
1506 """Return a `tuple` of terms matching the given criteria.1507 """Return a `tuple` of terms matching the given criteria.
@@ -1525,19 +1526,18 @@
15251526
1526 The order is the same as for `DistroSeriesVocabulary`.1527 The order is the same as for `DistroSeriesVocabulary`.
1527 """1528 """
1528 distribution = IDistribution(self.context)
1529 parent = ClassAlias(DistroSeries, "parent")1529 parent = ClassAlias(DistroSeries, "parent")
1530 child = ClassAlias(DistroSeries, "child")1530 child = ClassAlias(DistroSeries, "child")
1531 parent_distributions = Select(1531 parent_distributions = Select(
1532 parent.distributionID, And(1532 parent.distributionID, And(
1533 parent.distributionID != distribution.id,1533 parent.distributionID != self.distribution.id,
1534 child.distributionID == distribution.id,1534 child.distributionID == self.distribution.id,
1535 child.parent_seriesID == parent.id))1535 child.parent_seriesID == parent.id))
1536 terms = self.find_terms(1536 terms = self.find_terms(
1537 DistroSeries.distributionID.is_in(parent_distributions))1537 DistroSeries.distributionID.is_in(parent_distributions))
1538 if len(terms) == 0:1538 if len(terms) == 0:
1539 terms = self.find_terms(1539 terms = self.find_terms(
1540 DistroSeries.distribution != distribution)1540 DistroSeries.distribution != self.distribution)
1541 return terms1541 return terms
15421542
1543 @cachedproperty1543 @cachedproperty
Revision history for this message
Robert Collins (lifeless) wrote :

On Tue, Mar 15, 2011 at 4:12 AM, Gavin Panella
<email address hidden> wrote:
>> >     if IOneInterface.providedBy(an_argument):
>> >         do_something()
>> >     elif IOtherInterface.providedBy(an_argument):
>> >         do_something_else()
>>
>> I share that dislike, but I don't see how that applies to this context.
>
> If I don't use adapters but want to keep the ability to work in a
> distribution or distroseries context I can't really see a way around
> the IFoo.providedBy(...) stuff.
>
> Well, I could getattr(context, "distribution", context) but I suspect that
> might an order of magnitude worse again :)

There are a few general patterns that can be used here:
 - delegation:
   an_argument.do_something() -> trust it will dtrt.
   an_argument can have a range of techniques to have do_something
implemented appropropriately.
 - multiple dispatch: register (in advance) how you want to dispatch
things, and then dispatch off a tuple of types rather than just the
type of an_argument:
   do_something(an_argument, self) -> will handle the type consideration for you
 - adaption - a primitive form of multiple dispatch - cast or acquire
into a known type and then call a method on it:
 IPillar(an_argument).do_something
 - case statements: the thing we all dislike :)

adaption makes me nervous unless there is /clearly/ and /forevermore/
One Sensible Answer : and distroseries -> distribution violates the
second condition: for a derived distribution, there are multiple
reasonable answers to 'I need a distribution for this distroseries' -
there is .distribution, and there is .parent_series.distribution.

Generally speaking, I reach for delegation as the first tool to hand
in these sorts of situations, though it may not be the right tool for
this specific situation.

HTH,
Rob

Revision history for this message
Gavin Panella (allenap) wrote :

> adaption makes me nervous unless there is /clearly/ and
> /forevermore/ One Sensible Answer

I like this rule; I'll try and keep it in mind. Thanks.

Revision history for this message
Aaron Bentley (abentley) wrote :

Sorry for the delay. I thought Graham had approved it, but that was something else.

I like the change as far as it goes, but it seems you've retained the adaptation even though now it's no longer needed by your vocabulary. If that was an oversight, consider this approved. Otherwise, I'd like to understand why.

Revision history for this message
Gavin Panella (allenap) wrote :

The adaption has gone, but I left in a couple of trivial-ish tests for distroseries_to_distribution() and productseries_to_product(). That code no longer has anything else to do with this branch, but they are still valid tests of code that isn't otherwise tested directly.

Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/adapters.py'
--- lib/lp/registry/adapters.py 2011-01-04 16:08:57 +0000
+++ lib/lp/registry/adapters.py 2011-03-30 16:10:58 +0000
@@ -6,8 +6,7 @@
6__metaclass__ = type6__metaclass__ = type
77
8__all__ = [8__all__ = [
9 'distroseries_to_launchpadusage',9 'distroseries_to_distribution',
10 'distroseries_to_serviceusage',
11 'PollSubset',10 'PollSubset',
12 'productseries_to_product',11 'productseries_to_product',
13 ]12 ]
1413
=== added file 'lib/lp/registry/tests/test_adapters.py'
--- lib/lp/registry/tests/test_adapters.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_adapters.py 2011-03-30 16:10:58 +0000
@@ -0,0 +1,36 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for adapters."""
5
6__metaclass__ = type
7
8from canonical.testing.layers import DatabaseFunctionalLayer
9from lp.registry.adapters import (
10 distroseries_to_distribution,
11 productseries_to_product,
12 )
13from lp.registry.interfaces.distribution import IDistribution
14from lp.registry.interfaces.product import IProduct
15from lp.testing import TestCaseWithFactory
16
17
18class TestAdapters(TestCaseWithFactory):
19
20 layer = DatabaseFunctionalLayer
21
22 def test_distroseries_to_distribution(self):
23 # distroseries_to_distribution() returns an IDistribution given an
24 # IDistroSeries.
25 distro_series = self.factory.makeDistroSeries()
26 distribution = distroseries_to_distribution(distro_series)
27 self.assertTrue(IDistribution.providedBy(distribution))
28 self.assertEqual(distribution, distro_series.distribution)
29
30 def test_productseries_to_product(self):
31 # productseries_to_product() returns an IProduct given an
32 # IProductSeries.
33 product_series = self.factory.makeProductSeries()
34 product = productseries_to_product(product_series)
35 self.assertTrue(IProduct.providedBy(product))
36 self.assertEqual(product, product_series.product)
037
=== added file 'lib/lp/registry/tests/test_distroseries_vocabularies.py'
--- lib/lp/registry/tests/test_distroseries_vocabularies.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_distroseries_vocabularies.py 2011-03-30 16:10:58 +0000
@@ -0,0 +1,183 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for distroseries vocabularies in `lp.registry.vocabularies`."""
5
6__metaclass__ = type
7
8from datetime import (
9 datetime,
10 timedelta,
11 )
12
13from pytz import utc
14from testtools.matchers import Equals
15from zope.component import getUtility
16from zope.schema.interfaces import (
17 IVocabulary,
18 IVocabularyFactory,
19 IVocabularyTokenized,
20 )
21from zope.security.proxy import removeSecurityProxy
22
23from canonical.database.sqlbase import flush_database_caches
24from canonical.testing.layers import DatabaseFunctionalLayer
25from lp.registry.interfaces.distroseries import IDistroSeriesSet
26from lp.registry.vocabularies import DistroSeriesDerivationVocabularyFactory
27from lp.testing import (
28 StormStatementRecorder,
29 TestCaseWithFactory,
30 )
31from lp.testing.matchers import HasQueryCount
32
33
34class TestDistroSeriesDerivationVocabularyFactory(TestCaseWithFactory):
35 """Tests for `DistroSeriesDerivationVocabularyFactory`."""
36
37 layer = DatabaseFunctionalLayer
38
39 def setUp(self):
40 super(TestDistroSeriesDerivationVocabularyFactory, self).setUp()
41 self.vocabulary_factory = DistroSeriesDerivationVocabularyFactory
42 self.all_distroseries = getUtility(IDistroSeriesSet).search()
43
44 def test_registration(self):
45 # DistroSeriesDerivationVocabularyFactory is registered as a named
46 # utility for IVocabularyFactory.
47 self.assertEqual(
48 getUtility(IVocabularyFactory, name="DistroSeriesDerivation"),
49 DistroSeriesDerivationVocabularyFactory)
50
51 def test_interfaces(self):
52 # DistroSeriesDerivationVocabularyFactory instances provide
53 # IVocabulary and IVocabularyTokenized.
54 distroseries = self.factory.makeDistroSeries()
55 vocabulary = self.vocabulary_factory(distroseries)
56 self.assertProvides(vocabulary, IVocabulary)
57 self.assertProvides(vocabulary, IVocabularyTokenized)
58
59 def test_distribution_with_non_derived_series(self):
60 # Given a distribution with series, none of which are derived, the
61 # vocabulary factory returns a vocabulary for all distroseries in all
62 # distributions *except* the given distribution.
63 distroseries = self.factory.makeDistroSeries()
64 vocabulary = self.vocabulary_factory(distroseries)
65 expected_distroseries = (
66 set(self.all_distroseries).difference(
67 distroseries.distribution.series))
68 observed_distroseries = set(term.value for term in vocabulary)
69 self.assertEqual(expected_distroseries, observed_distroseries)
70
71 def test_distribution_with_derived_series(self):
72 # Given a distribution with series, one or more of which are derived,
73 # the vocabulary factory returns a vocabulary for all distroseries of
74 # the distribution from which the derived series have been derived.
75 parent_distroseries = self.factory.makeDistroSeries()
76 # Create a sibling to the parent.
77 self.factory.makeDistroSeries(
78 distribution=parent_distroseries.distribution)
79 distroseries = self.factory.makeDistroSeries(
80 parent_series=parent_distroseries)
81 vocabulary = self.vocabulary_factory(distroseries)
82 expected_distroseries = set(parent_distroseries.distribution.series)
83 observed_distroseries = set(term.value for term in vocabulary)
84 self.assertEqual(expected_distroseries, observed_distroseries)
85
86 def test_distribution_with_derived_series_of_self(self):
87 # Given a distribution with series derived from other of its series
88 # (which shouldn't happen), the vocabulary factory returns a
89 # vocabulary for all distroseries in all distributions *except* the
90 # given distribution.
91 parent_distroseries = self.factory.makeDistroSeries()
92 distroseries = self.factory.makeDistroSeries(
93 distribution=parent_distroseries.distribution,
94 parent_series=parent_distroseries)
95 vocabulary = self.vocabulary_factory(distroseries)
96 expected_distroseries = (
97 set(self.all_distroseries).difference(
98 distroseries.distribution.series))
99 observed_distroseries = set(term.value for term in vocabulary)
100 self.assertEqual(expected_distroseries, observed_distroseries)
101
102 def test_distroseries(self):
103 # Given a distroseries, the vocabulary factory returns the vocabulary
104 # the same as for its distribution.
105 distroseries = self.factory.makeDistroSeries()
106 vocabulary = self.vocabulary_factory(distroseries)
107 expected_distroseries = (
108 set(self.all_distroseries).difference(
109 distroseries.distribution.series))
110 observed_distroseries = set(term.value for term in vocabulary)
111 self.assertEqual(expected_distroseries, observed_distroseries)
112
113 def test_ordering(self):
114 # The vocabulary is sorted by distribution display name then by the
115 # date the distroseries was created, newest first.
116 now = datetime.now(utc)
117 two_days_ago = now - timedelta(2)
118 six_days_ago = now - timedelta(7)
119
120 aaa = self.factory.makeDistribution(displayname="aaa")
121 aaa_series_older = self.factory.makeDistroSeries(
122 name="aaa-series-older", distribution=aaa)
123 removeSecurityProxy(aaa_series_older).date_created = six_days_ago
124 aaa_series_newer = self.factory.makeDistroSeries(
125 name="aaa-series-newer", distribution=aaa)
126 removeSecurityProxy(aaa_series_newer).date_created = two_days_ago
127
128 bbb = self.factory.makeDistribution(displayname="bbb")
129 bbb_series_older = self.factory.makeDistroSeries(
130 name="bbb-series-older", distribution=bbb)
131 removeSecurityProxy(bbb_series_older).date_created = six_days_ago
132 bbb_series_newer = self.factory.makeDistroSeries(
133 name="bbb-series-newer", distribution=bbb)
134 removeSecurityProxy(bbb_series_newer).date_created = two_days_ago
135
136 ccc = self.factory.makeDistribution(displayname="ccc")
137 ccc_series = self.factory.makeDistroSeries(distribution=ccc)
138
139 vocabulary = self.vocabulary_factory(ccc_series)
140 expected_distroseries = [
141 aaa_series_newer, aaa_series_older,
142 bbb_series_newer, bbb_series_older]
143 observed_distroseries = list(term.value for term in vocabulary)
144 # observed_distroseries will contain distroseries from the sample
145 # data, so we must only look at the set of distroseries we have
146 # created.
147 observed_distroseries = [
148 series for series in observed_distroseries
149 if series in expected_distroseries]
150 self.assertEqual(expected_distroseries, observed_distroseries)
151
152 def test_queries_for_distribution_with_non_derived_series(self):
153 for index in range(10):
154 self.factory.makeDistroSeries()
155 distribution = self.factory.makeDistribution()
156 distroseries = self.factory.makeDistroSeries(
157 distribution=distribution)
158 flush_database_caches()
159 # Reload distroseries and distribution; these will reasonably already
160 # be loaded before using the vocabulary.
161 distroseries.distribution
162 # Getting terms issues two queries: one to search for parent serieses
163 # (of which there are none) and a second for all serieses.
164 with StormStatementRecorder() as recorder:
165 self.vocabulary_factory(distroseries).terms
166 self.assertThat(recorder, HasQueryCount(Equals(2)))
167
168 def test_queries_for_distribution_with_derived_series(self):
169 for index in range(10):
170 self.factory.makeDistroSeries()
171 distribution = self.factory.makeDistribution()
172 parent_distroseries = self.factory.makeDistroSeries()
173 distroseries = self.factory.makeDistroSeries(
174 parent_series=parent_distroseries,
175 distribution=distribution)
176 flush_database_caches()
177 # Reload distroseries and distribution; these will reasonably already
178 # be loaded before using the vocabulary.
179 distroseries.distribution
180 # Getting terms issues one query to find parent serieses.
181 with StormStatementRecorder() as recorder:
182 self.vocabulary_factory(distroseries).terms
183 self.assertThat(recorder, HasQueryCount(Equals(1)))
0184
=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py 2011-03-07 02:32:57 +0000
+++ lib/lp/registry/vocabularies.py 2011-03-30 16:10:58 +0000
@@ -31,6 +31,7 @@
31 'DistributionOrProductOrProjectGroupVocabulary',31 'DistributionOrProductOrProjectGroupVocabulary',
32 'DistributionOrProductVocabulary',32 'DistributionOrProductVocabulary',
33 'DistributionVocabulary',33 'DistributionVocabulary',
34 'DistroSeriesDerivationVocabularyFactory',
34 'DistroSeriesVocabulary',35 'DistroSeriesVocabulary',
35 'FeaturedProjectVocabulary',36 'FeaturedProjectVocabulary',
36 'FilteredDistroSeriesVocabulary',37 'FilteredDistroSeriesVocabulary',
@@ -38,23 +39,23 @@
38 'KarmaCategoryVocabulary',39 'KarmaCategoryVocabulary',
39 'MilestoneVocabulary',40 'MilestoneVocabulary',
40 'NonMergedPeopleAndTeamsVocabulary',41 'NonMergedPeopleAndTeamsVocabulary',
42 'person_team_participations_vocabulary_factory',
41 'PersonAccountToMergeVocabulary',43 'PersonAccountToMergeVocabulary',
42 'PersonActiveMembershipVocabulary',44 'PersonActiveMembershipVocabulary',
43 'ProductReleaseVocabulary',45 'ProductReleaseVocabulary',
44 'ProductSeriesVocabulary',46 'ProductSeriesVocabulary',
45 'ProductVocabulary',47 'ProductVocabulary',
48 'project_products_vocabulary_factory',
46 'ProjectGroupVocabulary',49 'ProjectGroupVocabulary',
47 'SourcePackageNameVocabulary',50 'SourcePackageNameVocabulary',
51 'UserTeamsParticipationPlusSelfVocabulary',
48 'UserTeamsParticipationVocabulary',52 'UserTeamsParticipationVocabulary',
49 'UserTeamsParticipationPlusSelfVocabulary',
50 'ValidPersonOrClosedTeamVocabulary',53 'ValidPersonOrClosedTeamVocabulary',
51 'ValidPersonOrTeamVocabulary',54 'ValidPersonOrTeamVocabulary',
52 'ValidPersonVocabulary',55 'ValidPersonVocabulary',
53 'ValidTeamMemberVocabulary',56 'ValidTeamMemberVocabulary',
54 'ValidTeamOwnerVocabulary',57 'ValidTeamOwnerVocabulary',
55 'ValidTeamVocabulary',58 'ValidTeamVocabulary',
56 'person_team_participations_vocabulary_factory',
57 'project_products_vocabulary_factory',
58 ]59 ]
5960
6061
@@ -73,11 +74,16 @@
73 LeftJoin,74 LeftJoin,
74 Not,75 Not,
75 Or,76 Or,
77 Select,
76 SQL,78 SQL,
77 )79 )
80from storm.info import ClassAlias
78from zope.component import getUtility81from zope.component import getUtility
79from zope.interface import implements82from zope.interface import implements
80from zope.schema.interfaces import IVocabularyTokenized83from zope.schema.interfaces import (
84 IVocabulary,
85 IVocabularyTokenized,
86 )
81from zope.schema.vocabulary import (87from zope.schema.vocabulary import (
82 SimpleTerm,88 SimpleTerm,
83 SimpleVocabulary,89 SimpleVocabulary,
@@ -1432,7 +1438,8 @@
1432 for series in sorted(series, key=attrgetter('sortkey')):1438 for series in sorted(series, key=attrgetter('sortkey')):
1433 yield self.toTerm(series)1439 yield self.toTerm(series)
14341440
1435 def toTerm(self, obj):1441 @staticmethod
1442 def toTerm(obj):
1436 """See `IVocabulary`."""1443 """See `IVocabulary`."""
1437 # NB: We use '/' as the separator because '-' is valid in1444 # NB: We use '/' as the separator because '-' is valid in
1438 # a distribution.name1445 # a distribution.name
@@ -1474,6 +1481,114 @@
1474 return objs1481 return objs
14751482
14761483
1484class DistroSeriesDerivationVocabularyFactory:
1485 """A vocabulary source for series to derive from.
1486
1487 Once a distribution has a series that has derived from a series in another
1488 distribution, all other derived series must also derive from a series in
1489 the same distribution.
1490
1491 A distribution can have non-derived series. Any of these can be changed to
1492 derived at a later date, but as soon as this happens, the above rule
1493 applies.
1494
1495 It is permissible for a distribution to have both derived and non-derived
1496 series at the same time.
1497 """
1498
1499 implements(IVocabulary, IVocabularyTokenized)
1500
1501 def __init__(self, context):
1502 """See `IVocabularyFactory.__call__`."""
1503 assert IDistroSeries.providedBy(context)
1504 self.distribution = context.distribution
1505
1506 def find_terms(self, *where):
1507 """Return a `tuple` of terms matching the given criteria.
1508
1509 The terms are returned in order. The `Distribution`s related to those
1510 terms are preloaded at the same time.
1511 """
1512 query = IStore(DistroSeries).find(
1513 (DistroSeries, Distribution),
1514 DistroSeries.distribution == Distribution.id,
1515 *where)
1516 query = query.order_by(
1517 Distribution.displayname,
1518 Desc(DistroSeries.date_created))
1519 return tuple(
1520 DistroSeriesVocabulary.toTerm(series)
1521 for (series, distribution) in query)
1522
1523 @cachedproperty
1524 def terms(self):
1525 """Terms for the series the context can derive from, in order.
1526
1527 The order is the same as for `DistroSeriesVocabulary`.
1528 """
1529 parent = ClassAlias(DistroSeries, "parent")
1530 child = ClassAlias(DistroSeries, "child")
1531 parent_distributions = Select(
1532 parent.distributionID, And(
1533 parent.distributionID != self.distribution.id,
1534 child.distributionID == self.distribution.id,
1535 child.parent_seriesID == parent.id))
1536 terms = self.find_terms(
1537 DistroSeries.distributionID.is_in(parent_distributions))
1538 if len(terms) == 0:
1539 terms = self.find_terms(
1540 DistroSeries.distribution != self.distribution)
1541 return terms
1542
1543 @cachedproperty
1544 def terms_by_value(self):
1545 """Mapping of terms by value."""
1546 return dict((term.value, term) for term in self.terms)
1547
1548 @cachedproperty
1549 def terms_by_token(self):
1550 """Mapping of terms by token."""
1551 return dict((term.token, term) for term in self.terms)
1552
1553 def __iter__(self):
1554 """Returns an iterator over the terms in the vocabulary.
1555
1556 See `IIterableVocabulary`.
1557 """
1558 return iter(self.terms)
1559
1560 def __len__(self):
1561 """The number of terms.
1562
1563 See `IIterableVocabulary`.
1564 """
1565 return len(self.terms)
1566
1567 def getTerm(self, value):
1568 """Return the `ITerm` object for the term 'value'.
1569
1570 See `IBaseVocabulary`.
1571 """
1572 try:
1573 return self.terms_by_value[value]
1574 except KeyError:
1575 raise LookupError(value)
1576
1577 def __contains__(self, value):
1578 """Return whether the value is available in this source.
1579
1580 See `ISource`.
1581 """
1582 return (value in self.terms_by_value)
1583
1584 def getTermByToken(self, token):
1585 """See `IVocabularyTokenized`."""
1586 try:
1587 return self.terms_by_token[token]
1588 except KeyError:
1589 raise LookupError(token)
1590
1591
1477class PillarVocabularyBase(NamedSQLObjectHugeVocabulary):1592class PillarVocabularyBase(NamedSQLObjectHugeVocabulary):
1478 """Active `IPillar` objects vocabulary."""1593 """Active `IPillar` objects vocabulary."""
1479 displayname = 'Needs to be overridden'1594 displayname = 'Needs to be overridden'
14801595
=== modified file 'lib/lp/registry/vocabularies.zcml'
--- lib/lp/registry/vocabularies.zcml 2010-11-30 22:55:58 +0000
+++ lib/lp/registry/vocabularies.zcml 2011-03-30 16:10:58 +0000
@@ -30,6 +30,18 @@
3030
3131
32 <securedutility32 <securedutility
33 name="DistroSeriesDerivation"
34 component=".vocabularies.DistroSeriesDerivationVocabularyFactory"
35 provides="zope.schema.interfaces.IVocabularyFactory">
36 <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
37 </securedutility>
38
39 <class class=".vocabularies.DistroSeriesDerivationVocabularyFactory">
40 <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
41 </class>
42
43
44 <securedutility
33 name="FeaturedProject"45 name="FeaturedProject"
34 component="lp.registry.vocabularies.FeaturedProjectVocabulary"46 component="lp.registry.vocabularies.FeaturedProjectVocabulary"
35 provides="zope.schema.interfaces.IVocabularyFactory"47 provides="zope.schema.interfaces.IVocabularyFactory"
3648
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-03-28 20:54:50 +0000
+++ lib/lp/testing/factory.py 2011-03-30 16:10:58 +0000
@@ -36,19 +36,19 @@
36 isSequenceType,36 isSequenceType,
37 )37 )
38import os38import os
39from pytz import UTC
40from random import randint39from random import randint
41from StringIO import StringIO40from StringIO import StringIO
42from textwrap import dedent41from textwrap import dedent
43from threading import local42from threading import local
44import transaction
45from types import InstanceType43from types import InstanceType
46import warnings44import warnings
4745
48from bzrlib.merge_directive import MergeDirective246from bzrlib.merge_directive import MergeDirective2
49from bzrlib.plugins.builder.recipe import BaseRecipeBranch47from bzrlib.plugins.builder.recipe import BaseRecipeBranch
50import pytz48import pytz
49from pytz import UTC
51import simplejson50import simplejson
51import transaction
52from twisted.python.util import mergeFunctionMetadata52from twisted.python.util import mergeFunctionMetadata
53from zope.component import (53from zope.component import (
54 ComponentLookupError,54 ComponentLookupError,
@@ -279,10 +279,8 @@
279 time_counter,279 time_counter,
280 )280 )
281from lp.translations.enums import RosettaImportStatus281from lp.translations.enums import RosettaImportStatus
282from lp.translations.interfaces.side import (
283 TranslationSide,
284 )
285from lp.translations.interfaces.potemplate import IPOTemplateSet282from lp.translations.interfaces.potemplate import IPOTemplateSet
283from lp.translations.interfaces.side import TranslationSide
286from lp.translations.interfaces.translationfileformat import (284from lp.translations.interfaces.translationfileformat import (
287 TranslationFileFormat,285 TranslationFileFormat,
288 )286 )
@@ -3992,6 +3990,12 @@
3992 else:3990 else:
3993 return attr3991 return attr
39943992
3993 def __dir__(self):
3994 """Enumerate the attributes and methods of the wrapped object factory.
3995
3996 This is especially useful for interactive users."""
3997 return dir(self._factory)
3998
39953999
3996def remove_security_proxy_and_shout_at_engineer(obj):4000def remove_security_proxy_and_shout_at_engineer(obj):
3997 """Remove an object's security proxy and print a warning.4001 """Remove an object's security proxy and print a warning.
39984002
=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py 2010-12-02 16:13:51 +0000
+++ lib/lp/testing/tests/test_factory.py 2011-03-30 16:10:58 +0000
@@ -611,6 +611,14 @@
611 sequence='2000-1234', cvestate=CveStatus.DEPRECATED)611 sequence='2000-1234', cvestate=CveStatus.DEPRECATED)
612 self.assertEqual(CveStatus.DEPRECATED, cve.status)612 self.assertEqual(CveStatus.DEPRECATED, cve.status)
613613
614 # dir() support.
615 def test_dir(self):
616 # LaunchpadObjectFactory supports dir() even though all of its
617 # attributes are pseudo-attributes.
618 self.assertEqual(
619 dir(self.factory._factory),
620 dir(self.factory))
621
614622
615class TestFactoryWithLibrarian(TestCaseWithFactory):623class TestFactoryWithLibrarian(TestCaseWithFactory):
616624