Launchpad itself

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

  1. 627957-differences-schema-update
  2. Merge into 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
=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql 2010-09-10 12:59:49 +0000
+++ database/schema/comments.sql 2010-09-13 09:17:47 +0000
@@ -532,9 +532,12 @@
532COMMENT ON TABLE DistroSeriesDifference IS 'A difference of versions for a package in a derived distroseries and its parent distroseries.';532COMMENT ON TABLE DistroSeriesDifference IS 'A difference of versions for a package in a derived distroseries and its parent distroseries.';
533COMMENT ON COLUMN DistroSeriesDifference.derived_series IS 'The derived distroseries with the difference from its parent.';533COMMENT ON COLUMN DistroSeriesDifference.derived_series IS 'The derived distroseries with the difference from its parent.';
534COMMENT ON COLUMN DistroSeriesDifference.source_package_name IS 'The name of the source package which is different in the two series.';534COMMENT ON COLUMN DistroSeriesDifference.source_package_name IS 'The name of the source package which is different in the two series.';
535COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for this difference.';535COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for the base version to derived version.';
536COMMENT ON COLUMN DistroSeriesDifference.parent_package_diff IS 'The most recent package diff that was created for the base version to the parent version.';
536COMMENT ON COLUMN DistroSeriesDifference.status IS 'A distroseries difference can be needing attention, ignored or resolved.';537COMMENT ON COLUMN DistroSeriesDifference.status IS 'A distroseries difference can be needing attention, ignored or resolved.';
537COMMENT 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.';538COMMENT 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.';
539COMMENT ON COLUMN DistroSeriesDifference.source_version IS 'The version of the package in the derived series.';
540COMMENT ON COLUMN DistroSeriesDifference.parent_source_version IS 'The version of the package in the parent series.';
538541
539-- DistroSeriesDifferenceMessage542-- DistroSeriesDifferenceMessage
540COMMENT ON TABLE DistroSeriesDifferenceMessage IS 'A message/comment on a distro series difference.';543COMMENT ON TABLE DistroSeriesDifferenceMessage IS 'A message/comment on a distro series difference.';
541544
=== added file 'database/schema/patch-2208-12-0.sql'
--- database/schema/patch-2208-12-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-12-0.sql 2010-09-13 09:17:47 +0000
@@ -0,0 +1,17 @@
1SET client_min_messages=ERROR;
2
3-- Allow for package diffs against both derived and parent versions.
4ALTER TABLE DistroSeriesDifference ADD COLUMN parent_package_diff integer CONSTRAINT distroseriesdifference__parent_package_diff__fk REFERENCES packagediff;
5CREATE INDEX distroseriesdifference__parent_package_diff__idx ON distroseriesdifference(parent_package_diff);
6
7-- Add columns for source_version and parent_source_version
8ALTER TABLE DistroSeriesDifference ADD COLUMN source_version text;
9ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK(valid_debian_version(source_version));
10ALTER TABLE DistroSeriesDifference ADD COLUMN parent_source_version text;
11ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_parent_source_version CHECK(valid_debian_version(parent_source_version));
12
13-- Add a unique constraint/index for the source_package_name/derived series combo and drop the previous index.
14ALTER TABLE DistroSeriesDifference ADD CONSTRAINT distroseriesdifference__derived_series__source_package_name__key UNIQUE (derived_series, source_package_name);
15DROP INDEX distroseriesdifference__derived_series__idx;
16
17INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 12, 0);
018
=== modified file 'lib/lp/registry/enum.py'
--- lib/lp/registry/enum.py 2010-08-27 08:17:00 +0000
+++ lib/lp/registry/enum.py 2010-09-13 09:17:47 +0000
@@ -63,15 +63,15 @@
63 This difference is current and needs attention.63 This difference is current and needs attention.
64 """)64 """)
6565
66 IGNORED = DBItem(2, """66 BLACKLISTED_CURRENT = DBItem(2, """
67 Ignored67 Blacklisted current version
6868
69 This difference is being ignored until a new package is uploaded69 This difference is being ignored until a new package is uploaded
70 or the status is manually updated.70 or the status is manually updated.
71 """)71 """)
7272
73 IGNORED_ALWAYS = DBItem(3, """73 BLACKLISTED_ALWAYS = DBItem(3, """
74 Ignored always74 Blacklisted always
7575
76 This difference should always be ignored.76 This difference should always be ignored.
77 """)77 """)
7878
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-01 12:12:13 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-13 09:17:47 +0000
@@ -55,7 +55,14 @@
55 package_diff = Reference(55 package_diff = Reference(
56 IPackageDiff, title=_("Package diff"), required=False,56 IPackageDiff, title=_("Package diff"), required=False,
57 readonly=True, description=_(57 readonly=True, description=_(
58 "The most recently generated package diff for this difference."))58 "The most recently generated package diff from the base to the "
59 "derived version."))
60
61 parent_package_diff = Reference(
62 IPackageDiff, title=_("Parent package diff"), required=False,
63 readonly=True, description=_(
64 "The most recently generated package diff from the base to the "
65 "parent version."))
5966
60 status = Choice(67 status = Choice(
61 title=_('Distro series difference status.'),68 title=_('Distro series difference status.'),
@@ -102,7 +109,7 @@
102 title=_("Title"), readonly=True, required=False, description=_(109 title=_("Title"), readonly=True, required=False, description=_(
103 "A human-readable name describing this difference."))110 "A human-readable name describing this difference."))
104111
105 def updateStatusAndType():112 def update():
106 """Checks that difference type and status matches current publishings.113 """Checks that difference type and status matches current publishings.
107114
108 If the record is updated, a relevant comment is added.115 If the record is updated, a relevant comment is added.
109116
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2010-09-06 16:04:55 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2010-09-13 09:17:47 +0000
@@ -15,6 +15,7 @@
15 Int,15 Int,
16 Reference,16 Reference,
17 Storm,17 Storm,
18 Unicode,
18 )19 )
19from zope.component import getUtility20from zope.component import getUtility
20from zope.interface import (21from zope.interface import (
@@ -41,6 +42,10 @@
41 )42 )
42from lp.registry.model.distroseriesdifferencecomment import (43from lp.registry.model.distroseriesdifferencecomment import (
43 DistroSeriesDifferenceComment)44 DistroSeriesDifferenceComment)
45from lp.services.propertycache import (
46 cachedproperty,
47 IPropertyCacheManager,
48 )
4449
4550
46class DistroSeriesDifference(Storm):51class DistroSeriesDifference(Storm):
@@ -65,10 +70,18 @@
65 package_diff = Reference(70 package_diff = Reference(
66 package_diff_id, 'PackageDiff.id')71 package_diff_id, 'PackageDiff.id')
6772
73 parent_package_diff_id = Int(
74 name='parent_package_diff', allow_none=True)
75 parent_package_diff = Reference(
76 parent_package_diff_id, 'PackageDiff.id')
77
68 status = DBEnum(name='status', allow_none=False,78 status = DBEnum(name='status', allow_none=False,
69 enum=DistroSeriesDifferenceStatus)79 enum=DistroSeriesDifferenceStatus)
70 difference_type = DBEnum(name='difference_type', allow_none=False,80 difference_type = DBEnum(name='difference_type', allow_none=False,
71 enum=DistroSeriesDifferenceType)81 enum=DistroSeriesDifferenceType)
82 source_version = Unicode(name='source_version', allow_none=True)
83 parent_source_version = Unicode(name='parent_source_version',
84 allow_none=True)
7285
73 @staticmethod86 @staticmethod
74 def new(derived_series, source_package_name, difference_type,87 def new(derived_series, source_package_name, difference_type,
@@ -84,6 +97,14 @@
84 diff.status = status97 diff.status = status
85 diff.difference_type = difference_type98 diff.difference_type = difference_type
8699
100 source_pub = diff.source_pub
101 if source_pub is not None:
102 diff.source_version = source_pub.source_package_version
103 parent_source_pub = diff.parent_source_pub
104 if parent_source_pub is not None:
105 diff.parent_source_version = (
106 parent_source_pub.source_package_version)
107
87 return store.add(diff)108 return store.add(diff)
88109
89 @staticmethod110 @staticmethod
@@ -105,12 +126,12 @@
105 DistroSeriesDifference.difference_type == difference_type,126 DistroSeriesDifference.difference_type == difference_type,
106 DistroSeriesDifference.status.is_in(status))127 DistroSeriesDifference.status.is_in(status))
107128
108 @property129 @cachedproperty
109 def source_pub(self):130 def source_pub(self):
110 """See `IDistroSeriesDifference`."""131 """See `IDistroSeriesDifference`."""
111 return self._getLatestSourcePub()132 return self._getLatestSourcePub()
112133
113 @property134 @cachedproperty
114 def parent_source_pub(self):135 def parent_source_pub(self):
115 """See `IDistroSeriesDifference`."""136 """See `IDistroSeriesDifference`."""
116 return self._getLatestSourcePub(for_parent=True)137 return self._getLatestSourcePub(for_parent=True)
@@ -149,22 +170,25 @@
149 else:170 else:
150 return None171 return None
151172
152 @property173 def update(self):
153 def source_version(self):174 """See `IDistroSeriesDifference`."""
154 """See `IDistroSeriesDifference`."""175 # Updating is expected to be a heavy operation (not called during
155 if self.source_pub:176 # requests). We clear the cache beforehand - even though
156 return self.source_pub.source_package_version177 # it is not currently be necessary so that in the future it
157 return None178 # won't cause a hard-to find bug if a script ever creates a difference,
158179 # copies/publishes a new version and then calls update() (like the
159 @property180 # tests for this method do).
160 def parent_source_version(self):181 IPropertyCacheManager(self).clear()
161 """See `IDistroSeriesDifference`."""182 self._updateType()
162 if self.parent_source_pub:183 updated = self._updateVersionsAndStatus()
163 return self.parent_source_pub.source_package_version184 return updated
164 return None185
165186 def _updateType(self):
166 def updateStatusAndType(self):187 """Helper for update() interface method.
167 """See `IDistroSeriesDifference`."""188
189 Check whether the presence of a source in the derived or parent
190 series has changed (which changes the type of difference).
191 """
168 if self.source_pub is None:192 if self.source_pub is None:
169 new_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES193 new_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
170 elif self.parent_source_pub is None:194 elif self.parent_source_pub is None:
@@ -172,19 +196,47 @@
172 else:196 else:
173 new_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS197 new_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
174198
175 updated = False
176 if new_type != self.difference_type:199 if new_type != self.difference_type:
177 updated = True
178 self.difference_type = new_type200 self.difference_type = new_type
179201
180 version = self.source_version202 def _updateVersionsAndStatus(self):
181 parent_version = self.parent_source_version203 """Helper for the update() interface method.
204
205 Check whether the status of this difference should be updated.
206 """
207 updated = False
208 new_source_version = new_parent_source_version = None
209 if self.source_pub:
210 new_source_version = self.source_pub.source_package_version
211 if self.source_version != new_source_version:
212 self.source_version = new_source_version
213 updated = True
214 # If the derived version has change and the previous version
215 # was blacklisted, then we remove the blacklist now.
216 if self.status == (
217 DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
218 self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
219 if self.parent_source_pub:
220 new_parent_source_version = (
221 self.parent_source_pub.source_package_version)
222 if self.parent_source_version != new_parent_source_version:
223 self.parent_source_version = new_parent_source_version
224 updated = True
225
226 # If this difference was resolved but now the versions don't match
227 # then we re-open the difference.
182 if self.status == DistroSeriesDifferenceStatus.RESOLVED:228 if self.status == DistroSeriesDifferenceStatus.RESOLVED:
183 if version != parent_version:229 if self.source_version != self.parent_source_version:
184 updated = True230 updated = True
185 self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION231 self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
186 else:232 # If this difference was needing attention, or the current version
187 if version == parent_version:233 # was blacklisted and the versions now match we resolve it. Note:
234 # we don't resolve it if this difference was blacklisted for all
235 # versions.
236 elif self.status in (
237 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
238 DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
239 if self.source_version == self.parent_source_version:
188 updated = True240 updated = True
189 self.status = DistroSeriesDifferenceStatus.RESOLVED241 self.status = DistroSeriesDifferenceStatus.RESOLVED
190242
191243
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-06 16:04:55 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2010-09-13 09:17:47 +0000
@@ -9,6 +9,7 @@
99
10import unittest10import unittest
1111
12from storm.exceptions import IntegrityError
12from storm.store import Store13from storm.store import Store
13from zope.component import getUtility14from zope.component import getUtility
14from zope.security.interfaces import Unauthorized15from zope.security.interfaces import Unauthorized
@@ -16,10 +17,6 @@
16from canonical.launchpad.webapp.authorization import check_permission17from canonical.launchpad.webapp.authorization import check_permission
17from canonical.launchpad.webapp.testing import verifyObject18from canonical.launchpad.webapp.testing import verifyObject
18from canonical.testing import DatabaseFunctionalLayer19from canonical.testing import DatabaseFunctionalLayer
19from lp.testing import (
20 person_logged_in,
21 TestCaseWithFactory,
22 )
23from lp.registry.enum import (20from lp.registry.enum import (
24 DistroSeriesDifferenceStatus,21 DistroSeriesDifferenceStatus,
25 DistroSeriesDifferenceType,22 DistroSeriesDifferenceType,
@@ -29,7 +26,12 @@
29 IDistroSeriesDifference,26 IDistroSeriesDifference,
30 IDistroSeriesDifferenceSource,27 IDistroSeriesDifferenceSource,
31 )28 )
29from lp.services.propertycache import IPropertyCacheManager
32from lp.soyuz.interfaces.publishing import PackagePublishingStatus30from lp.soyuz.interfaces.publishing import PackagePublishingStatus
31from lp.testing import (
32 person_logged_in,
33 TestCaseWithFactory,
34 )
3335
3436
35class DistroSeriesDifferenceTestCase(TestCaseWithFactory):37class DistroSeriesDifferenceTestCase(TestCaseWithFactory):
@@ -132,7 +134,7 @@
132134
133 self.assertEqual(None, ds_diff.source_version)135 self.assertEqual(None, ds_diff.source_version)
134136
135 def test_updateStatusAndType_resolves_difference(self):137 def test_update_resolves_difference(self):
136 # Status is set to resolved when versions match.138 # Status is set to resolved when versions match.
137 ds_diff = self.factory.makeDistroSeriesDifference(139 ds_diff = self.factory.makeDistroSeriesDifference(
138 source_package_name_str="foonew",140 source_package_name_str="foonew",
@@ -146,14 +148,14 @@
146 status=PackagePublishingStatus.PENDING,148 status=PackagePublishingStatus.PENDING,
147 version='1.0')149 version='1.0')
148150
149 was_updated = ds_diff.updateStatusAndType()151 was_updated = ds_diff.update()
150152
151 self.assertTrue(was_updated)153 self.assertTrue(was_updated)
152 self.assertEqual(154 self.assertEqual(
153 DistroSeriesDifferenceStatus.RESOLVED,155 DistroSeriesDifferenceStatus.RESOLVED,
154 ds_diff.status)156 ds_diff.status)
155157
156 def test_updateStatusAndType_re_opens_difference(self):158 def test_update_re_opens_difference(self):
157 # The status of a resolved difference will updated with new159 # The status of a resolved difference will updated with new
158 # uploads.160 # uploads.
159 ds_diff = self.factory.makeDistroSeriesDifference(161 ds_diff = self.factory.makeDistroSeriesDifference(
@@ -169,18 +171,16 @@
169 status=PackagePublishingStatus.PENDING,171 status=PackagePublishingStatus.PENDING,
170 version='1.1')172 version='1.1')
171173
172 was_updated = ds_diff.updateStatusAndType()174 was_updated = ds_diff.update()
173175
174 self.assertTrue(was_updated)176 self.assertTrue(was_updated)
175 self.assertEqual(177 self.assertEqual(
176 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,178 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
177 ds_diff.status)179 ds_diff.status)
178180
179 def test_updateStatusAndType_new_version_no_change(self):181 def test_update_new_version_doesnt_change_status(self):
180 # Uploading a new (different) version does not necessarily182 # Uploading a new (different) version does not update the
181 # update the record.183 # status of the record, but the version is updated.
182 # In this case, a new version is uploaded, but there is still a
183 # difference needing attention.
184 ds_diff = self.factory.makeDistroSeriesDifference(184 ds_diff = self.factory.makeDistroSeriesDifference(
185 source_package_name_str="foonew",185 source_package_name_str="foonew",
186 versions={186 versions={
@@ -193,14 +193,15 @@
193 status=PackagePublishingStatus.PENDING,193 status=PackagePublishingStatus.PENDING,
194 version='1.1')194 version='1.1')
195195
196 was_updated = ds_diff.updateStatusAndType()196 was_updated = ds_diff.update()
197197
198 self.assertFalse(was_updated)198 self.assertTrue(was_updated)
199 self.assertEqual(199 self.assertEqual(
200 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,200 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
201 ds_diff.status)201 ds_diff.status)
202 self.assertEqual('1.1', ds_diff.source_version)
202203
203 def test_updateStatusAndType_changes_type(self):204 def test_update_changes_type(self):
204 # The type of difference is updated when appropriate.205 # The type of difference is updated when appropriate.
205 # In this case, a package that was previously only in the206 # In this case, a package that was previously only in the
206 # derived series (UNIQUE_TO_DERIVED_SERIES), is uploaded207 # derived series (UNIQUE_TO_DERIVED_SERIES), is uploaded
@@ -218,13 +219,62 @@
218 status=PackagePublishingStatus.PENDING,219 status=PackagePublishingStatus.PENDING,
219 version='1.1')220 version='1.1')
220221
221 was_updated = ds_diff.updateStatusAndType()222 was_updated = ds_diff.update()
222223
223 self.assertTrue(was_updated)224 self.assertTrue(was_updated)
224 self.assertEqual(225 self.assertEqual(
225 DistroSeriesDifferenceType.DIFFERENT_VERSIONS,226 DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
226 ds_diff.difference_type)227 ds_diff.difference_type)
227228
229 def test_update_removes_version_blacklist(self):
230 # A blacklist on a version of a package is removed when a new
231 # version is uploaded to the derived series.
232 ds_diff = self.factory.makeDistroSeriesDifference(
233 source_package_name_str="foonew",
234 versions={
235 'derived': '0.9',
236 },
237 difference_type=(
238 DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES),
239 status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
240 new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
241 sourcepackagename=ds_diff.source_package_name,
242 distroseries=ds_diff.derived_series,
243 status=PackagePublishingStatus.PENDING,
244 version='1.1')
245
246 was_updated = ds_diff.update()
247
248 self.assertTrue(was_updated)
249 self.assertEqual(
250 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
251 ds_diff.status)
252
253 def test_update_does_not_remove_permanent_blacklist(self):
254 # A permanent blacklist is not removed when a new version
255 # is uploaded, even if it resolves the difference (as later
256 # uploads could re-create a difference, and we want to keep
257 # the blacklist).
258 ds_diff = self.factory.makeDistroSeriesDifference(
259 source_package_name_str="foonew",
260 versions={
261 'derived': '0.9',
262 'parent': '1.0',
263 },
264 status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
265 new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
266 sourcepackagename=ds_diff.source_package_name,
267 distroseries=ds_diff.derived_series,
268 status=PackagePublishingStatus.PENDING,
269 version='1.0')
270
271 was_updated = ds_diff.update()
272
273 self.assertTrue(was_updated)
274 self.assertEqual(
275 DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
276 ds_diff.status)
277
228 def test_title(self):278 def test_title(self):
229 # The title is a friendly description of the difference.279 # The title is a friendly description of the difference.
230 parent_series = self.factory.makeDistroSeries(name="lucid")280 parent_series = self.factory.makeDistroSeries(name="lucid")
@@ -287,6 +337,28 @@
287 diff_comment = ds_diff.addComment(337 diff_comment = ds_diff.addComment(
288 ds_diff.derived_series.owner, "Boo")338 ds_diff.derived_series.owner, "Boo")
289339
340 def test_source_package_name_unique_for_derived_series(self):
341 # We cannot create two differences for the same derived series
342 # for the same package.
343 ds_diff = self.factory.makeDistroSeriesDifference(
344 source_package_name_str="foo")
345 self.assertRaises(
346 IntegrityError, self.factory.makeDistroSeriesDifference,
347 derived_series=ds_diff.derived_series,
348 source_package_name_str="foo")
349
350 def test_cached_properties(self):
351 # The source and parent publication properties are cached on the
352 # model.
353 ds_diff = self.factory.makeDistroSeriesDifference()
354 ds_diff.source_pub
355 ds_diff.parent_source_pub
356
357 cache = IPropertyCacheManager(ds_diff).cache
358
359 self.assertContentEqual(
360 ['source_pub', 'parent_source_pub'], cache)
361
290362
291class DistroSeriesDifferenceSourceTestCase(TestCaseWithFactory):363class DistroSeriesDifferenceSourceTestCase(TestCaseWithFactory):
292364
@@ -317,7 +389,7 @@
317 diffs['ignored'].append(389 diffs['ignored'].append(
318 self.factory.makeDistroSeriesDifference(390 self.factory.makeDistroSeriesDifference(
319 derived_series=derived_series,391 derived_series=derived_series,
320 status=DistroSeriesDifferenceStatus.IGNORED))392 status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
321 return diffs393 return diffs
322394
323 def makeDerivedSeries(self):395 def makeDerivedSeries(self):
@@ -364,7 +436,7 @@
364436
365 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(437 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
366 derived_series,438 derived_series,
367 status=DistroSeriesDifferenceStatus.IGNORED)439 status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
368440
369 self.assertContentEqual(diffs['ignored'], result)441 self.assertContentEqual(diffs['ignored'], result)
370442
@@ -376,7 +448,7 @@
376 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(448 result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
377 derived_series,449 derived_series,
378 status=(450 status=(
379 DistroSeriesDifferenceStatus.IGNORED,451 DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
380 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,452 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
381 ))453 ))
382454
383455
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-09-09 20:12:02 +0000
+++ lib/lp/testing/factory.py 2010-09-13 09:17:47 +0000
@@ -212,6 +212,7 @@
212from lp.registry.model.suitesourcepackage import SuiteSourcePackage212from lp.registry.model.suitesourcepackage import SuiteSourcePackage
213from lp.services.mail.signedmessage import SignedMessage213from lp.services.mail.signedmessage import SignedMessage
214from lp.services.openid.model.openididentifier import OpenIdIdentifier214from lp.services.openid.model.openididentifier import OpenIdIdentifier
215from lp.services.propertycache import IPropertyCacheManager
215from lp.services.worlddata.interfaces.country import ICountrySet216from lp.services.worlddata.interfaces.country import ICountrySet
216from lp.services.worlddata.interfaces.language import ILanguageSet217from lp.services.worlddata.interfaces.language import ILanguageSet
217from lp.soyuz.adapters.packagelocation import PackageLocation218from lp.soyuz.adapters.packagelocation import PackageLocation
@@ -1887,10 +1888,15 @@
1887 sourcepackagename=source_package_name,1888 sourcepackagename=source_package_name,
1888 status = PackagePublishingStatus.PUBLISHED)1889 status = PackagePublishingStatus.PUBLISHED)
18891890
1890 return getUtility(IDistroSeriesDifferenceSource).new(1891 diff = getUtility(IDistroSeriesDifferenceSource).new(
1891 derived_series, source_package_name, difference_type,1892 derived_series, source_package_name, difference_type,
1892 status=status)1893 status=status)
18931894
1895 # We clear the cache on the diff, returning the object as if it
1896 # was just loaded from the store.
1897 IPropertyCacheManager(diff).clear()
1898 return diff
1899
1894 def makeDistroSeriesDifferenceComment(1900 def makeDistroSeriesDifferenceComment(
1895 self, distro_series_difference=None, owner=None, comment=None):1901 self, distro_series_difference=None, owner=None, comment=None):
1896 """Create a new distro series difference comment."""1902 """Create a new distro series difference comment."""

Subscribers

People subscribed via source and target branches

to status/vote changes: