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

[Michael Nelson]

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

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

[Abel Deuring (adeuring)]

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

[Michael Nelson (michael.nelson)]

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!

[Abel Deuring (adeuring)]

looks good

[Michael Nelson (michael.nelson)]

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

[Robert Collins (lifeless)]

There's only one cachedproperty implementation.

[Stuart Bishop (stub)]

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

[Michael Nelson (michael.nelson)]

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/

=== 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 @@
 COMMENT ON TABLE DistroSeriesDifference IS 'A difference of versions for a package in a derived distroseries and its parent distroseries.';
 COMMENT ON COLUMN DistroSeriesDifference.derived_series IS 'The derived distroseries with the difference from its parent.';
 COMMENT ON COLUMN DistroSeriesDifference.source_package_name IS 'The name of the source package which is different in the two series.';
-COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for this difference.';
+COMMENT ON COLUMN DistroSeriesDifference.package_diff IS 'The most recent package diff that was created for the base version to derived version.';
+COMMENT ON COLUMN DistroSeriesDifference.parent_package_diff IS 'The most recent package diff that was created for the base version to the parent version.';
 COMMENT ON COLUMN DistroSeriesDifference.status IS 'A distroseries difference can be needing attention, ignored or resolved.';
 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.';
+COMMENT ON COLUMN DistroSeriesDifference.source_version IS 'The version of the package in the derived series.';
+COMMENT ON COLUMN DistroSeriesDifference.parent_source_version IS 'The version of the package in the parent series.';
 
 -- DistroSeriesDifferenceMessage
 COMMENT ON TABLE DistroSeriesDifferenceMessage IS 'A message/comment on a distro series difference.';

=== 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 @@
+SET client_min_messages=ERROR;
+
+-- Allow for package diffs against both derived and parent versions.
+ALTER TABLE DistroSeriesDifference ADD COLUMN parent_package_diff integer CONSTRAINT distroseriesdifference__parent_package_diff__fk REFERENCES packagediff;
+CREATE INDEX distroseriesdifference__parent_package_diff__idx ON distroseriesdifference(parent_package_diff);
+
+-- Add columns for source_version and parent_source_version
+ALTER TABLE DistroSeriesDifference ADD COLUMN source_version text;
+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_source_version CHECK(valid_debian_version(source_version));
+ALTER TABLE DistroSeriesDifference ADD COLUMN parent_source_version text;
+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT valid_parent_source_version CHECK(valid_debian_version(parent_source_version));
+
+-- Add a unique constraint/index for the source_package_name/derived series combo and drop the previous index.
+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT distroseriesdifference__derived_series__source_package_name__key UNIQUE (derived_series, source_package_name);
+DROP INDEX distroseriesdifference__derived_series__idx;
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 12, 0);

=== 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 @@
         This difference is current and needs attention.
         """)
 
-    IGNORED = DBItem(2, """
-        Ignored
+    BLACKLISTED_CURRENT = DBItem(2, """
+        Blacklisted current version
 
         This difference is being ignored until a new package is uploaded
         or the status is manually updated.
         """)
 
-    IGNORED_ALWAYS = DBItem(3, """
-        Ignored always
+    BLACKLISTED_ALWAYS = DBItem(3, """
+        Blacklisted always
 
         This difference should always be ignored.
         """)

=== 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 @@
     package_diff = Reference(
         IPackageDiff, title=_("Package diff"), required=False,
         readonly=True, description=_(
-            "The most recently generated package diff for this difference."))
+            "The most recently generated package diff from the base to the "
+            "derived version."))
+
+    parent_package_diff = Reference(
+        IPackageDiff, title=_("Parent package diff"), required=False,
+        readonly=True, description=_(
+            "The most recently generated package diff from the base to the "
+            "parent version."))
 
     status = Choice(
         title=_('Distro series difference status.'),
@@ -102,7 +109,7 @@
         title=_("Title"), readonly=True, required=False, description=_(
             "A human-readable name describing this difference."))
 
-    def updateStatusAndType():
+    def update():
         """Checks that difference type and status matches current publishings.
 
         If the record is updated, a relevant comment is added.

=== 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 @@
     Int,
     Reference,
     Storm,
+    Unicode,
     )
 from zope.component import getUtility
 from zope.interface import (
@@ -41,6 +42,10 @@
     )
 from lp.registry.model.distroseriesdifferencecomment import (
     DistroSeriesDifferenceComment)
+from lp.services.propertycache import (
+    cachedproperty,
+    IPropertyCacheManager,
+    )
 
 
 class DistroSeriesDifference(Storm):
@@ -65,10 +70,18 @@
     package_diff = Reference(
         package_diff_id, 'PackageDiff.id')
 
+    parent_package_diff_id = Int(
+        name='parent_package_diff', allow_none=True)
+    parent_package_diff = Reference(
+        parent_package_diff_id, 'PackageDiff.id')
+
     status = DBEnum(name='status', allow_none=False,
                     enum=DistroSeriesDifferenceStatus)
     difference_type = DBEnum(name='difference_type', allow_none=False,
                              enum=DistroSeriesDifferenceType)
+    source_version = Unicode(name='source_version', allow_none=True)
+    parent_source_version = Unicode(name='parent_source_version',
+                                    allow_none=True)
 
     @staticmethod
     def new(derived_series, source_package_name, difference_type,
@@ -84,6 +97,14 @@
         diff.status = status
         diff.difference_type = difference_type
 
+        source_pub = diff.source_pub
+        if source_pub is not None:
+            diff.source_version = source_pub.source_package_version
+        parent_source_pub = diff.parent_source_pub
+        if parent_source_pub is not None:
+            diff.parent_source_version = (
+                parent_source_pub.source_package_version)
+
         return store.add(diff)
 
     @staticmethod
@@ -105,12 +126,12 @@
             DistroSeriesDifference.difference_type == difference_type,
             DistroSeriesDifference.status.is_in(status))
 
-    @property
+    @cachedproperty
     def source_pub(self):
         """See `IDistroSeriesDifference`."""
         return self._getLatestSourcePub()
 
-    @property
+    @cachedproperty
     def parent_source_pub(self):
         """See `IDistroSeriesDifference`."""
         return self._getLatestSourcePub(for_parent=True)
@@ -149,22 +170,25 @@
         else:
             return None
 
-    @property
-    def source_version(self):
-        """See `IDistroSeriesDifference`."""
-        if self.source_pub:
-            return self.source_pub.source_package_version
-        return None
-
-    @property
-    def parent_source_version(self):
-        """See `IDistroSeriesDifference`."""
-        if self.parent_source_pub:
-            return self.parent_source_pub.source_package_version
-        return None
-
-    def updateStatusAndType(self):
-        """See `IDistroSeriesDifference`."""
+    def update(self):
+        """See `IDistroSeriesDifference`."""
+        # Updating is expected to be a heavy operation (not called during
+        # requests). We clear the cache beforehand - even though
+        # it is not currently be necessary so that in the future it
+        # won't cause a hard-to find bug if a script ever creates a difference,
+        # copies/publishes a new version and then calls update() (like the
+        # tests for this method do).
+        IPropertyCacheManager(self).clear()
+        self._updateType()
+        updated = self._updateVersionsAndStatus()
+        return updated
+
+    def _updateType(self):
+        """Helper for update() interface method.
+
+        Check whether the presence of a source in the derived or parent
+        series has changed (which changes the type of difference).
+        """
         if self.source_pub is None:
             new_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
         elif self.parent_source_pub is None:
@@ -172,19 +196,47 @@
         else:
             new_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
 
-        updated = False
         if new_type != self.difference_type:
-            updated = True
             self.difference_type = new_type
 
-        version = self.source_version
-        parent_version = self.parent_source_version
+    def _updateVersionsAndStatus(self):
+        """Helper for the update() interface method.
+
+        Check whether the status of this difference should be updated.
+        """
+        updated = False
+        new_source_version = new_parent_source_version = None
+        if self.source_pub:
+            new_source_version = self.source_pub.source_package_version
+            if self.source_version != new_source_version:
+                self.source_version = new_source_version
+                updated = True
+                # If the derived version has change and the previous version
+                # was blacklisted, then we remove the blacklist now.
+                if self.status == (
+                    DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
+                    self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+        if self.parent_source_pub:
+            new_parent_source_version = (
+                self.parent_source_pub.source_package_version)
+            if self.parent_source_version != new_parent_source_version:
+                self.parent_source_version = new_parent_source_version
+                updated = True
+
+        # If this difference was resolved but now the versions don't match
+        # then we re-open the difference.
         if self.status == DistroSeriesDifferenceStatus.RESOLVED:
-            if version != parent_version:
+            if self.source_version != self.parent_source_version:
                 updated = True
                 self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
-        else:
-            if version == parent_version:
+        # If this difference was needing attention, or the current version
+        # was blacklisted and the versions now match we resolve it. Note:
+        # we don't resolve it if this difference was blacklisted for all
+        # versions.
+        elif self.status in (
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT):
+            if self.source_version == self.parent_source_version:
                 updated = True
                 self.status = DistroSeriesDifferenceStatus.RESOLVED
 

=== 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 @@
 
 import unittest
 
+from storm.exceptions import IntegrityError
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
@@ -16,10 +17,6 @@
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import DatabaseFunctionalLayer
-from lp.testing import (
-    person_logged_in,
-    TestCaseWithFactory,
-    )
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
@@ -29,7 +26,12 @@
     IDistroSeriesDifference,
     IDistroSeriesDifferenceSource,
     )
+from lp.services.propertycache import IPropertyCacheManager
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 
 
 class DistroSeriesDifferenceTestCase(TestCaseWithFactory):
@@ -132,7 +134,7 @@
 
         self.assertEqual(None, ds_diff.source_version)
 
-    def test_updateStatusAndType_resolves_difference(self):
+    def test_update_resolves_difference(self):
         # Status is set to resolved when versions match.
         ds_diff = self.factory.makeDistroSeriesDifference(
             source_package_name_str="foonew",
@@ -146,14 +148,14 @@
             status=PackagePublishingStatus.PENDING,
             version='1.0')
 
-        was_updated = ds_diff.updateStatusAndType()
+        was_updated = ds_diff.update()
 
         self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.RESOLVED,
             ds_diff.status)
 
-    def test_updateStatusAndType_re_opens_difference(self):
+    def test_update_re_opens_difference(self):
         # The status of a resolved difference will updated with new
         # uploads.
         ds_diff = self.factory.makeDistroSeriesDifference(
@@ -169,18 +171,16 @@
             status=PackagePublishingStatus.PENDING,
             version='1.1')
 
-        was_updated = ds_diff.updateStatusAndType()
+        was_updated = ds_diff.update()
 
         self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             ds_diff.status)
 
-    def test_updateStatusAndType_new_version_no_change(self):
-        # Uploading a new (different) version does not necessarily
-        # update the record.
-        # In this case, a new version is uploaded, but there is still a
-        # difference needing attention.
+    def test_update_new_version_doesnt_change_status(self):
+        # Uploading a new (different) version does not update the
+        # status of the record, but the version is updated.
         ds_diff = self.factory.makeDistroSeriesDifference(
             source_package_name_str="foonew",
             versions={
@@ -193,14 +193,15 @@
             status=PackagePublishingStatus.PENDING,
             version='1.1')
 
-        was_updated = ds_diff.updateStatusAndType()
+        was_updated = ds_diff.update()
 
-        self.assertFalse(was_updated)
+        self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             ds_diff.status)
+        self.assertEqual('1.1', ds_diff.source_version)
 
-    def test_updateStatusAndType_changes_type(self):
+    def test_update_changes_type(self):
         # The type of difference is updated when appropriate.
         # In this case, a package that was previously only in the
         # derived series (UNIQUE_TO_DERIVED_SERIES), is uploaded
@@ -218,13 +219,62 @@
             status=PackagePublishingStatus.PENDING,
             version='1.1')
 
-        was_updated = ds_diff.updateStatusAndType()
+        was_updated = ds_diff.update()
 
         self.assertTrue(was_updated)
         self.assertEqual(
             DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
             ds_diff.difference_type)
 
+    def test_update_removes_version_blacklist(self):
+        # A blacklist on a version of a package is removed when a new
+        # version is uploaded to the derived series.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'derived': '0.9',
+                },
+            difference_type=(
+                DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES),
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+        new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=ds_diff.source_package_name,
+            distroseries=ds_diff.derived_series,
+            status=PackagePublishingStatus.PENDING,
+            version='1.1')
+
+        was_updated = ds_diff.update()
+
+        self.assertTrue(was_updated)
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            ds_diff.status)
+
+    def test_update_does_not_remove_permanent_blacklist(self):
+        # A permanent blacklist is not removed when a new version
+        # is uploaded, even if it resolves the difference (as later
+        # uploads could re-create a difference, and we want to keep
+        # the blacklist).
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'derived': '0.9',
+                'parent': '1.0',
+                },
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
+        new_derived_pub = self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=ds_diff.source_package_name,
+            distroseries=ds_diff.derived_series,
+            status=PackagePublishingStatus.PENDING,
+            version='1.0')
+
+        was_updated = ds_diff.update()
+
+        self.assertTrue(was_updated)
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
+            ds_diff.status)
+
     def test_title(self):
         # The title is a friendly description of the difference.
         parent_series = self.factory.makeDistroSeries(name="lucid")
@@ -287,6 +337,28 @@
             diff_comment = ds_diff.addComment(
                 ds_diff.derived_series.owner, "Boo")
 
+    def test_source_package_name_unique_for_derived_series(self):
+        # We cannot create two differences for the same derived series
+        # for the same package.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foo")
+        self.assertRaises(
+            IntegrityError, self.factory.makeDistroSeriesDifference,
+            derived_series=ds_diff.derived_series,
+            source_package_name_str="foo")
+
+    def test_cached_properties(self):
+        # The source and parent publication properties are cached on the
+        # model.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff.source_pub
+        ds_diff.parent_source_pub
+
+        cache = IPropertyCacheManager(ds_diff).cache
+
+        self.assertContentEqual(
+            ['source_pub', 'parent_source_pub'], cache)
+
 
 class DistroSeriesDifferenceSourceTestCase(TestCaseWithFactory):
 
@@ -317,7 +389,7 @@
         diffs['ignored'].append(
             self.factory.makeDistroSeriesDifference(
                 derived_series=derived_series,
-                status=DistroSeriesDifferenceStatus.IGNORED))
+                status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
         return diffs
 
     def makeDerivedSeries(self):
@@ -364,7 +436,7 @@
 
         result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
             derived_series,
-            status=DistroSeriesDifferenceStatus.IGNORED)
+            status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
 
         self.assertContentEqual(diffs['ignored'], result)
 
@@ -376,7 +448,7 @@
         result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
             derived_series,
             status=(
-                DistroSeriesDifferenceStatus.IGNORED,
+                DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
                 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
                 ))
 

=== 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 @@
 from lp.registry.model.suitesourcepackage import SuiteSourcePackage
 from lp.services.mail.signedmessage import SignedMessage
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
+from lp.services.propertycache import IPropertyCacheManager
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.soyuz.adapters.packagelocation import PackageLocation
@@ -1887,10 +1888,15 @@
                 sourcepackagename=source_package_name,
                 status = PackagePublishingStatus.PUBLISHED)
 
-        return getUtility(IDistroSeriesDifferenceSource).new(
+        diff = getUtility(IDistroSeriesDifferenceSource).new(
             derived_series, source_package_name, difference_type,
             status=status)
 
+        # We clear the cache on the diff, returning the object as if it
+        # was just loaded from the store.
+        IPropertyCacheManager(diff).clear()
+        return diff
+
     def makeDistroSeriesDifferenceComment(
         self, distro_series_difference=None, owner=None, comment=None):
         """Create a new distro series difference comment."""