Merge lp:~michael.nelson/launchpad/627957-differences-schema-update into lp:launchpad/db-devel

Proposed by Michael Nelson
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: 9780
Proposed branch: lp:~michael.nelson/launchpad/627957-differences-schema-update
Merge into: lp:launchpad/db-devel
Diff against target: 514 lines (+210/-53)
7 files modified
database/schema/comments.sql (+4/-1)
database/schema/patch-2208-12-0.sql (+17/-0)
lib/lp/registry/enum.py (+4/-4)
lib/lp/registry/interfaces/distroseriesdifference.py (+9/-2)
lib/lp/registry/model/distroseriesdifference.py (+77/-25)
lib/lp/registry/tests/test_distroseriesdifference.py (+92/-20)
lib/lp/testing/factory.py (+7/-1)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/627957-differences-schema-update
Reviewer Review Type Date Requested Status
Stuart Bishop (community) db Approve
Abel Deuring (community) code Approve
Robert Collins db Pending
Review via email: mp+35073@code.launchpad.net

Commit message

Update schema for DistroSeriesDifferences based on further UI testing (two package diffs, remember versions).

Description of the change

Overview
========
This branch addresses bug 627957 by adding three new columns to the DistroSeriesDifference table: source_version, parent_source_version, and parent_package_diff.

Details
=======

The two version fields allow the object to remember the respective versions last time the record was updated (and change status where appropriate etc.). The tests ensure that the blacklisting setting changes appropriately when new versions are uploaded.

The additional package_diff column allows us to reference one package diff for the base-version -> derived series version, and one package diff for the base-version -> parent series version (as requested during UI testing).

The branch adds (and tests) a missing constraint on (source_package_name, derived_series).

This branch also renames the IGNORED difference type to BLACKLISTED (also discussed on the mailing list thread at:

https://lists.launchpad.net/launchpad-dev/msg04525.html

It also makes the source_pub and parent_source_pub cached properties as they are used numerous times when updating a difference.

Test
===
bin/test -vvm test_distroseriesdifference -m test_series_view

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

(13:23:56) adeuring: noodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;)
(13:49:05) noodles775: adeuring: thanks, yeah, it should be returning whether it updated the model.
(14:03:54) adeuring: noodles775: that's worth a test, I think
(14:04:33) noodles775: Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method.
(14:10:40) adeuring: noodles775: the IPropertyCacheManager(ds_diff).clear() in a test look a bit odd: If this is necessary in a test, wouldn't it be necessary in "real life"?
(14:16:38) noodles775: adeuring: It's necessary there because the test creates a new difference and then publishes a new version of the package, before then updating the difference.
(14:16:54) noodles775: so no, it's not something that would happen in the duration of a request.
(14:17:57) adeuring: noodles775: could you add comments with this explanation to the cache.clear() calls?
(14:18:30) noodles775: adeuring: to each one? (I think there are 8... how about at the top of the test case?)
(14:18:49) adeuring: noodles775: yes, that should be enough ;)

review: Approve (code)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Fri, Sep 10, 2010 at 2:48 PM, Abel Deuring
<email address hidden> wrote:
> Review: Approve code
> (13:23:56) adeuring: noodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;)
> (13:49:05) noodles775: adeuring: thanks, yeah, it should be returning whether it updated the model.
> (14:03:54) adeuring: noodles775: that's worth a test, I think

As it turned out, the private helper _updateType() does not need to
return anything, as the type will be updated if-and-only-if the
versions also update. I removed the expectation of a return value, and
renamed _updateStatus() to _updateVersionsAndStatus to reflect what it
is really doing.

> (14:04:33) noodles775: Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method.
> (14:10:40) adeuring: noodles775: the IPropertyCacheManager(ds_diff).clear() in a test look a bit odd: If this is necessary in a test, wouldn't it be necessary in "real life"?
> (14:16:38) noodles775: adeuring: It's necessary there because the test creates a new difference and then publishes a new version of the package, before then updating the difference.
> (14:16:54) noodles775: so no, it's not something that would happen in the duration of a request.
> (14:17:57) adeuring: noodles775: could you add comments with this explanation to the cache.clear() calls?
> (14:18:30) noodles775: adeuring: to each one? (I think there are 8... how about at the top of the test case?)
> (14:18:49) adeuring: noodles775: yes, that should be enough ;)

{{{
14:51 < noodles775> Thanks adeuring. I was also thinking, it may makes
sense to clear the cache at the start of the update() method too,
which would remove the need for them in the tests.
14:51 < adeuring> noodles775: yes, that would be even better
}}}

So I did this, and added a comment as to why it is cleared there even
though there is currently no need. I also cleared the cache before
returning the factory object so that the factory returns an object as
if it had been freshly obtained from the store.

Incremental here: http://pastebin.ubuntu.com/491627/

Thanks!

Revision history for this message
Abel Deuring (adeuring) wrote :

looks good

Revision history for this message
Michael Nelson (michael.nelson) wrote :

I'm also wondering if I should go back to using the normal cachedproperty rather than the new one, based on Gavin's recent emails (or removing the dependence on adaptions):

https://lists.launchpad.net/launchpad-dev/msg04583.html

Revision history for this message
Robert Collins (lifeless) wrote :

There's only one cachedproperty implementation.

Revision history for this message
Stuart Bishop (stub) wrote :

(14:43:29) stub: noodles775: versien isn't spelled that way
(14:44:26) noodles775: hrm... "base version to derived versien." I'll update it.
14:45
(14:45:22) Abel Deuring [~<email address hidden>] entered the room.
(14:45:56) stub: noodles775: So source_version and parent_source_version are debian version strings? If so we might want a CHECK constraint to ensure that.
(14:46:40) stub: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK (valid_debian_version(source_version));
(14:47:02) noodles775: stub: yes they are. Thanks.
(14:49:37) stub: noodles775: With the new UNIQUE constraint, you can drop the distroseriesdifference__derived_series__idx index.
(14:50:25) stub: noodles775: The name of the new UNIQUE constraint should be distroseriesdifference__derived_series__source_package_name__key
(14:51:16) noodles775: stub: Great - I didn't realised the two-column index makes the single-column foreign-key index redundant. And yep, I'll rename the constraint.
(14:51:16) stub: So a NULL diff means it hasn't been calculated yet I assume. What do NULL versions mean? Probaly worth clarifying this in comments.sql
(14:51:58) noodles775: stub: yes, different types of differences... (unique to the derived series, missing from the derived series etc.). Should there be a constraint to ensure that both are not null together?
(14:52:40) noodles775: stub: er, ignore that (I was thinking about the versions fields).
(14:52:46) stub: If that is what our code expects, then yes. ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid...
(14:52:47) stub: ok
(14:53:49) stub: noodles775: patch-2208-12-0.sql
(14:54:11) noodles775: *sigh*. I need a tea. So yes, you had asked about NULL versions, so we should have a constraint to ensure both versions are not null.
(14:54:15) noodles775: Thanks.
(14:55:27) stub: If only one or the other can be NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (source_version IS NULL != parent_source_version IS NULL)
(14:55:42) noodles775: Great, thanks.
(14:56:54) stub: If we just want to check at most one is NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (coalesce(source_version, parent_source_version) IS NOT NULL)
(14:58:39) stub: or just 'source_version IS NOT NULL OR parent_source_version IS NOT NULL' is better
(14:58:53) noodles775: Yep.
(14:58:55) stub: I'm not sure which of those two rules is appropriate
15:00
(15:00:22) stub: Can they both be NULL? Can they both be NOT NULL?
(15:00:48) noodles775: They cannot both be null, but they can both be NOT NULL.
(15:02:49) stub: ok... so CHECK (source_version IS NOT NULL OR parent_source_version IS NOT NULL)

review: Approve (db)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Mon, Sep 13, 2010 at 10:04 AM, Stuart Bishop
<email address hidden> wrote:
> Review: Approve db
> (14:43:29) stub: noodles775: versien isn't spelled that way
> (14:44:26) noodles775: hrm... "base version to derived versien." I'll update it.

Done.

> (14:45:56) stub: noodles775: So source_version and parent_source_version are debian version strings? If so we might want a CHECK constraint to ensure that.
> (14:46:40) stub: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK (valid_debian_version(source_version));
> (14:47:02) noodles775: stub: yes they are. Thanks.

Added.

> (14:49:37) stub: noodles775: With the new UNIQUE constraint, you can drop the distroseriesdifference__derived_series__idx index.
> (14:50:25) stub: noodles775: The name of the new UNIQUE constraint should be distroseriesdifference__derived_series__source_package_name__key
> (14:51:16) noodles775: stub: Great - I didn't realised the two-column index makes the single-column foreign-key index redundant. And yep, I'll rename the constraint.

Done.

> (14:51:16) stub: So a NULL diff means it hasn't been calculated yet I assume. What do NULL versions mean? Probaly worth clarifying this in comments.sql
> (14:51:58) noodles775: stub: yes, different types of differences... (unique to the derived series, missing from the derived series etc.). Should there be a constraint to ensure that both are not null together?
> (14:52:40) noodles775: stub: er, ignore that (I was thinking about the versions fields).
> (14:52:46) stub: If that is what our code expects, then yes. ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid...
> (14:52:47) stub: ok
> (14:53:49) stub: noodles775: patch-2208-12-0.sql
> (14:54:11) noodles775: *sigh*. I need a tea. So yes, you had asked about NULL versions, so we should have a constraint to ensure both versions are not null.
> (14:54:15) noodles775: Thanks.
> (14:55:27) stub: If only one or the other can be NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (source_version IS NULL != parent_source_version IS NULL)
> (14:55:42) noodles775: Great, thanks.
> (14:56:54) stub: If we just want to check at most one is NULL: ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_versions CHECK (coalesce(source_version, parent_source_version) IS NOT NULL)
> (14:58:39) stub: or just 'source_version IS NOT NULL OR parent_source_version IS NOT NULL' is better
> (14:58:53) noodles775: Yep.
> (14:58:55) stub: I'm not sure which of those two rules is appropriate
> 15:00
> (15:00:22) stub: Can they both be NULL? Can they both be NOT NULL?
> (15:00:48) noodles775: They cannot both be null, but they can both be NOT NULL.
> (15:02:49) stub: ok... so CHECK (source_version IS NOT NULL OR parent_source_version IS NOT NULL)

After thinking a bit more about this, I didn't add the constraint to
stop both the versions being NULL, after realising that it would be a
valid way to resolve a difference that was unique to the derived
series. (ie. if resolving a difference for a package that is unique to
the derived series means deleting that version from the derived
series).

Incremental http://pastebin.ubuntu.com/492997/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/comments.sql'
2--- database/schema/comments.sql 2010-09-10 12:59:49 +0000
3+++ database/schema/comments.sql 2010-09-13 09:17:47 +0000
4@@ -532,9 +532,12 @@
5 COMMENT ON TABLE DistroSeriesDifference IS 'A difference of versions for a package in a derived distroseries and its parent distroseries.';
6 COMMENT ON COLUMN DistroSeriesDifference.derived_series IS 'The derived distroseries with the difference from its parent.';
7 COMMENT ON COLUMN DistroSeriesDifference.source_package_name IS 'The name of the source package which is different in the two series.';
8-COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for this difference.';
9+COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for the base version to derived version.';
10+COMMENT ON COLUMN DistroSeriesDifference.parent_package_diff IS 'The most recent package diff that was created for the base version to the parent version.';
11 COMMENT ON COLUMN DistroSeriesDifference.status IS 'A distroseries difference can be needing attention, ignored or resolved.';
12 COMMENT ON COLUMN DistroSeriesDifference.difference_type IS 'The type of difference that this record represents - a package unique to the derived series, or missing, or in both.';
13+COMMENT ON COLUMN DistroSeriesDifference.source_version IS 'The version of the package in the derived series.';
14+COMMENT ON COLUMN DistroSeriesDifference.parent_source_version IS 'The version of the package in the parent series.';
15
16 -- DistroSeriesDifferenceMessage
17 COMMENT ON TABLE DistroSeriesDifferenceMessage IS 'A message/comment on a distro series difference.';
18
19=== added file 'database/schema/patch-2208-12-0.sql'
20--- database/schema/patch-2208-12-0.sql 1970-01-01 00:00:00 +0000
21+++ database/schema/patch-2208-12-0.sql 2010-09-13 09:17:47 +0000
22@@ -0,0 +1,17 @@
23+SET client_min_messages=ERROR;
24+
25+-- Allow for package diffs against both derived and parent versions.
26+ALTER TABLE DistroSeriesDifference ADD COLUMN parent_package_diff integer CONSTRAINT distroseriesdifference__parent_package_diff__fk REFERENCES packagediff;
27+CREATE INDEX distroseriesdifference__parent_package_diff__idx ON distroseriesdifference(parent_package_diff);
28+
29+-- Add columns for source_version and parent_source_version
30+ALTER TABLE DistroSeriesDifference ADD COLUMN source_version text;
31+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK(valid_debian_version(source_version));
32+ALTER TABLE DistroSeriesDifference ADD COLUMN parent_source_version text;
33+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_parent_source_version CHECK(valid_debian_version(parent_source_version));
34+
35+-- Add a unique constraint/index for the source_package_name/derived series combo and drop the previous index.
36+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT distroseriesdifference__derived_series__source_package_name__key UNIQUE (derived_series, source_package_name);
37+DROP INDEX distroseriesdifference__derived_series__idx;
38+
39+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 12, 0);
40
41=== modified file 'lib/lp/registry/enum.py'
42--- lib/lp/registry/enum.py 2010-08-27 08:17:00 +0000
43+++ lib/lp/registry/enum.py 2010-09-13 09:17:47 +0000
44@@ -63,15 +63,15 @@
45 This difference is current and needs attention.
46 """)
47
48- IGNORED = DBItem(2, """
49- Ignored
50+ BLACKLISTED_CURRENT = DBItem(2, """
51+ Blacklisted current version
52
53 This difference is being ignored until a new package is uploaded
54 or the status is manually updated.
55 """)
56
57- IGNORED_ALWAYS = DBItem(3, """
58- Ignored always
59+ BLACKLISTED_ALWAYS = DBItem(3, """
60+ Blacklisted always
61
62 This difference should always be ignored.
63 """)
64
65=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
66--- lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-01 12:12:13 +0000
67+++ lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-13 09:17:47 +0000
68@@ -55,7 +55,14 @@
69 package_diff = Reference(
70 IPackageDiff, title=_("Package diff"), required=False,
71 readonly=True, description=_(
72- "The most recently generated package diff for this difference."))
73+ "The most recently generated package diff from the base to the "
74+ "derived version."))
75+
76+ parent_package_diff = Reference(
77+ IPackageDiff, title=_("Parent package diff"), required=False,
78+ readonly=True, description=_(
79+ "The most recently generated package diff from the base to the "
80+ "parent version."))
81
82 status = Choice(
83 title=_('Distro series difference status.'),
84@@ -102,7 +109,7 @@
85 title=_("Title"), readonly=True, required=False, description=_(
86 "A human-readable name describing this difference."))
87
88- def updateStatusAndType():
89+ def update():
90 """Checks that difference type and status matches current publishings.
91
92 If the record is updated, a relevant comment is added.
93
94=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
95--- lib/lp/registry/model/distroseriesdifference.py 2010-09-06 16:04:55 +0000
96+++ lib/lp/registry/model/distroseriesdifference.py 2010-09-13 09:17:47 +0000
97@@ -15,6 +15,7 @@
98 Int,
99 Reference,
100 Storm,
101+ Unicode,
102 )
103 from zope.component import getUtility
104 from zope.interface import (
105@@ -41,6 +42,10 @@
106 )
107 from lp.registry.model.distroseriesdifferencecomment import (
108 DistroSeriesDifferenceComment)
109+from lp.services.propertycache import (
110+ cachedproperty,
111+ IPropertyCacheManager,
112+ )
113
114
115 class DistroSeriesDifference(Storm):
116@@ -65,10 +70,18 @@
117 package_diff = Reference(
118 package_diff_id, 'PackageDiff.id')
119
120+ parent_package_diff_id = Int(
121+ name='parent_package_diff', allow_none=True)
122+ parent_package_diff = Reference(
123+ parent_package_diff_id, 'PackageDiff.id')
124+
125 status = DBEnum(name='status', allow_none=False,
126 enum=DistroSeriesDifferenceStatus)
127 difference_type = DBEnum(name='difference_type', allow_none=False,
128 enum=DistroSeriesDifferenceType)
129+ source_version = Unicode(name='source_version', allow_none=True)
130+ parent_source_version = Unicode(name='parent_source_version',
131+ allow_none=True)
132
133 @staticmethod
134 def new(derived_series, source_package_name, difference_type,
135@@ -84,6 +97,14 @@
136 diff.status = status
137 diff.difference_type = difference_type
138
139+ source_pub = diff.source_pub
140+ if source_pub is not None:
141+ diff.source_version = source_pub.source_package_version
142+ parent_source_pub = diff.parent_source_pub
143+ if parent_source_pub is not None:
144+ diff.parent_source_version = (
145+ parent_source_pub.source_package_version)
146+
147 return store.add(diff)
148
149 @staticmethod
150@@ -105,12 +126,12 @@
151 DistroSeriesDifference.difference_type == difference_type,
152 DistroSeriesDifference.status.is_in(status))
153
154- @property
155+ @cachedproperty
156 def source_pub(self):
157 """See `IDistroSeriesDifference`."""
158 return self._getLatestSourcePub()
159
160- @property
161+ @cachedproperty
162 def parent_source_pub(self):
163 """See `IDistroSeriesDifference`."""
164 return self._getLatestSourcePub(for_parent=True)
165@@ -149,22 +170,25 @@
166 else:
167 return None
168
169- @property
170- def source_version(self):
171- """See `IDistroSeriesDifference`."""
172- if self.source_pub:
173- return self.source_pub.source_package_version
174- return None
175-
176- @property
177- def parent_source_version(self):
178- """See `IDistroSeriesDifference`."""
179- if self.parent_source_pub:
180- return self.parent_source_pub.source_package_version
181- return None
182-
183- def updateStatusAndType(self):
184- """See `IDistroSeriesDifference`."""
185+ def update(self):
186+ """See `IDistroSeriesDifference`."""
187+ # Updating is expected to be a heavy operation (not called during
188+ # requests). We clear the cache beforehand - even though
189+ # it is not currently be necessary so that in the future it
190+ # won't cause a hard-to find bug if a script ever creates a difference,
191+ # copies/publishes a new version and then calls update() (like the
192+ # tests for this method do).
193+ IPropertyCacheManager(self).clear()
194+ self._updateType()
195+ updated = self._updateVersionsAndStatus()
196+ return updated
197+
198+ def _updateType(self):
199+ """Helper for update() interface method.
200+
201+ Check whether the presence of a source in the derived or parent
202+ series has changed (which changes the type of difference).
203+ """
204 if self.source_pub is None:
205 new_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
206 elif self.parent_source_pub is None:
207@@ -172,19 +196,47 @@
208 else:
209 new_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
210
211- updated = False
212 if new_type != self.difference_type:
213- updated = True
214 self.difference_type = new_type
215
216- version = self.source_version
217- parent_version = self.parent_source_version
218+ def _updateVersionsAndStatus(self):
219+ """Helper for the update() interface method.
220+
221+ Check whether the status of this difference should be updated.
222+ """
223+ updated = False
224+ new_source_version = new_parent_source_version = None
225+ if self.source_pub:
226+ new_source_version = self.source_pub.source_package_version
227+ if self.source_version != new_source_version:
228+ self.source_version = new_source_version
229+ updated = True
230+ # If the derived version has change and the previous version
231+ # was blacklisted, then we remove the blacklist now.
232+ if self.status == (
233+ DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
234+ self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
235+ if self.parent_source_pub:
236+ new_parent_source_version = (
237+ self.parent_source_pub.source_package_version)
238+ if self.parent_source_version != new_parent_source_version:
239+ self.parent_source_version = new_parent_source_version
240+ updated = True
241+
242+ # If this difference was resolved but now the versions don't match
243+ # then we re-open the difference.
244 if self.status == DistroSeriesDifferenceStatus.RESOLVED:
245- if version != parent_version:
246+ if self.source_version != self.parent_source_version:
247 updated = True
248 self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
249- else:
250- if version == parent_version:
251+ # If this difference was needing attention, or the current version
252+ # was blacklisted and the versions now match we resolve it. Note:
253+ # we don't resolve it if this difference was blacklisted for all
254+ # versions.
255+ elif self.status in (
256+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
257+ DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
258+ if self.source_version == self.parent_source_version:
259 updated = True
260 self.status = DistroSeriesDifferenceStatus.RESOLVED
261
262
263=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
264--- lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-06 16:04:55 +0000
265+++ lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-13 09:17:47 +0000
266@@ -9,6 +9,7 @@
267
268 import unittest
269
270+from storm.exceptions import IntegrityError
271 from storm.store import Store
272 from zope.component import getUtility
273 from zope.security.interfaces import Unauthorized
274@@ -16,10 +17,6 @@
275 from canonical.launchpad.webapp.authorization import check_permission
276 from canonical.launchpad.webapp.testing import verifyObject
277 from canonical.testing import DatabaseFunctionalLayer
278-from lp.testing import (
279- person_logged_in,
280- TestCaseWithFactory,
281- )
282 from lp.registry.enum import (
283 DistroSeriesDifferenceStatus,
284 DistroSeriesDifferenceType,
285@@ -29,7 +26,12 @@
286 IDistroSeriesDifference,
287 IDistroSeriesDifferenceSource,
288 )
289+from lp.services.propertycache import IPropertyCacheManager
290 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
291+from lp.testing import (
292+ person_logged_in,
293+ TestCaseWithFactory,
294+ )
295
296
297 class DistroSeriesDifferenceTestCase(TestCaseWithFactory):
298@@ -132,7 +134,7 @@
299
300 self.assertEqual(None, ds_diff.source_version)
301
302- def test_updateStatusAndType_resolves_difference(self):
303+ def test_update_resolves_difference(self):
304 # Status is set to resolved when versions match.
305 ds_diff = self.factory.makeDistroSeriesDifference(
306 source_package_name_str="foonew",
307@@ -146,14 +148,14 @@
308 status=PackagePublishingStatus.PENDING,
309 version='1.0')
310
311- was_updated = ds_diff.updateStatusAndType()
312+ was_updated = ds_diff.update()
313
314 self.assertTrue(was_updated)
315 self.assertEqual(
316 DistroSeriesDifferenceStatus.RESOLVED,
317 ds_diff.status)
318
319- def test_updateStatusAndType_re_opens_difference(self):
320+ def test_update_re_opens_difference(self):
321 # The status of a resolved difference will updated with new
322 # uploads.
323 ds_diff = self.factory.makeDistroSeriesDifference(
324@@ -169,18 +171,16 @@
325 status=PackagePublishingStatus.PENDING,
326 version='1.1')
327
328- was_updated = ds_diff.updateStatusAndType()
329+ was_updated = ds_diff.update()
330
331 self.assertTrue(was_updated)
332 self.assertEqual(
333 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
334 ds_diff.status)
335
336- def test_updateStatusAndType_new_version_no_change(self):
337- # Uploading a new (different) version does not necessarily
338- # update the record.
339- # In this case, a new version is uploaded, but there is still a
340- # difference needing attention.
341+ def test_update_new_version_doesnt_change_status(self):
342+ # Uploading a new (different) version does not update the
343+ # status of the record, but the version is updated.
344 ds_diff = self.factory.makeDistroSeriesDifference(
345 source_package_name_str="foonew",
346 versions={
347@@ -193,14 +193,15 @@
348 status=PackagePublishingStatus.PENDING,
349 version='1.1')
350
351- was_updated = ds_diff.updateStatusAndType()
352+ was_updated = ds_diff.update()
353
354- self.assertFalse(was_updated)
355+ self.assertTrue(was_updated)
356 self.assertEqual(
357 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
358 ds_diff.status)
359+ self.assertEqual('1.1', ds_diff.source_version)
360
361- def test_updateStatusAndType_changes_type(self):
362+ def test_update_changes_type(self):
363 # The type of difference is updated when appropriate.
364 # In this case, a package that was previously only in the
365 # derived series (UNIQUE_TO_DERIVED_SERIES), is uploaded
366@@ -218,13 +219,62 @@
367 status=PackagePublishingStatus.PENDING,
368 version='1.1')
369
370- was_updated = ds_diff.updateStatusAndType()
371+ was_updated = ds_diff.update()
372
373 self.assertTrue(was_updated)
374 self.assertEqual(
375 DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
376 ds_diff.difference_type)
377
378+ def test_update_removes_version_blacklist(self):
379+ # A blacklist on a version of a package is removed when a new
380+ # version is uploaded to the derived series.
381+ ds_diff = self.factory.makeDistroSeriesDifference(
382+ source_package_name_str="foonew",
383+ versions={
384+ 'derived': '0.9',
385+ },
386+ difference_type=(
387+ DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES),
388+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
389+ new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
390+ sourcepackagename=ds_diff.source_package_name,
391+ distroseries=ds_diff.derived_series,
392+ status=PackagePublishingStatus.PENDING,
393+ version='1.1')
394+
395+ was_updated = ds_diff.update()
396+
397+ self.assertTrue(was_updated)
398+ self.assertEqual(
399+ DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
400+ ds_diff.status)
401+
402+ def test_update_does_not_remove_permanent_blacklist(self):
403+ # A permanent blacklist is not removed when a new version
404+ # is uploaded, even if it resolves the difference (as later
405+ # uploads could re-create a difference, and we want to keep
406+ # the blacklist).
407+ ds_diff = self.factory.makeDistroSeriesDifference(
408+ source_package_name_str="foonew",
409+ versions={
410+ 'derived': '0.9',
411+ 'parent': '1.0',
412+ },
413+ status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
414+ new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
415+ sourcepackagename=ds_diff.source_package_name,
416+ distroseries=ds_diff.derived_series,
417+ status=PackagePublishingStatus.PENDING,
418+ version='1.0')
419+
420+ was_updated = ds_diff.update()
421+
422+ self.assertTrue(was_updated)
423+ self.assertEqual(
424+ DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
425+ ds_diff.status)
426+
427 def test_title(self):
428 # The title is a friendly description of the difference.
429 parent_series = self.factory.makeDistroSeries(name="lucid")
430@@ -287,6 +337,28 @@
431 diff_comment = ds_diff.addComment(
432 ds_diff.derived_series.owner, "Boo")
433
434+ def test_source_package_name_unique_for_derived_series(self):
435+ # We cannot create two differences for the same derived series
436+ # for the same package.
437+ ds_diff = self.factory.makeDistroSeriesDifference(
438+ source_package_name_str="foo")
439+ self.assertRaises(
440+ IntegrityError, self.factory.makeDistroSeriesDifference,
441+ derived_series=ds_diff.derived_series,
442+ source_package_name_str="foo")
443+
444+ def test_cached_properties(self):
445+ # The source and parent publication properties are cached on the
446+ # model.
447+ ds_diff = self.factory.makeDistroSeriesDifference()
448+ ds_diff.source_pub
449+ ds_diff.parent_source_pub
450+
451+ cache = IPropertyCacheManager(ds_diff).cache
452+
453+ self.assertContentEqual(
454+ ['source_pub', 'parent_source_pub'], cache)
455+
456
457 class DistroSeriesDifferenceSourceTestCase(TestCaseWithFactory):
458
459@@ -317,7 +389,7 @@
460 diffs['ignored'].append(
461 self.factory.makeDistroSeriesDifference(
462 derived_series=derived_series,
463- status=DistroSeriesDifferenceStatus.IGNORED))
464+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
465 return diffs
466
467 def makeDerivedSeries(self):
468@@ -364,7 +436,7 @@
469
470 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
471 derived_series,
472- status=DistroSeriesDifferenceStatus.IGNORED)
473+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
474
475 self.assertContentEqual(diffs['ignored'], result)
476
477@@ -376,7 +448,7 @@
478 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
479 derived_series,
480 status=(
481- DistroSeriesDifferenceStatus.IGNORED,
482+ DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
483 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
484 ))
485
486
487=== modified file 'lib/lp/testing/factory.py'
488--- lib/lp/testing/factory.py 2010-09-09 20:12:02 +0000
489+++ lib/lp/testing/factory.py 2010-09-13 09:17:47 +0000
490@@ -212,6 +212,7 @@
491 from lp.registry.model.suitesourcepackage import SuiteSourcePackage
492 from lp.services.mail.signedmessage import SignedMessage
493 from lp.services.openid.model.openididentifier import OpenIdIdentifier
494+from lp.services.propertycache import IPropertyCacheManager
495 from lp.services.worlddata.interfaces.country import ICountrySet
496 from lp.services.worlddata.interfaces.language import ILanguageSet
497 from lp.soyuz.adapters.packagelocation import PackageLocation
498@@ -1887,10 +1888,15 @@
499 sourcepackagename=source_package_name,
500 status = PackagePublishingStatus.PUBLISHED)
501
502- return getUtility(IDistroSeriesDifferenceSource).new(
503+ diff = getUtility(IDistroSeriesDifferenceSource).new(
504 derived_series, source_package_name, difference_type,
505 status=status)
506
507+ # We clear the cache on the diff, returning the object as if it
508+ # was just loaded from the store.
509+ IPropertyCacheManager(diff).clear()
510+ return diff
511+
512 def makeDistroSeriesDifferenceComment(
513 self, distro_series_difference=None, owner=None, comment=None):
514 """Create a new distro series difference comment."""

Subscribers

People subscribed via source and target branches

to status/vote changes: