launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00952
[Merge] lp:~michael.nelson/launchpad/627957-differences-schema-update into lp:launchpad
Michael Nelson has proposed merging lp:~michael.nelson/launchpad/627957-differences-schema-update into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#627957 DistroSeriesDifference can't blacklist current version
https://bugs.launchpad.net/bugs/627957
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
--
https://code.launchpad.net/~michael.nelson/launchpad/627957-differences-schema-update/+merge/35073
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/627957-differences-schema-update into lp:launchpad.
=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql 2010-09-09 16:53:16 +0000
+++ database/schema/comments.sql 2010-09-10 10:54:51 +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 versien.';
+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-99-0.sql'
--- database/schema/patch-2208-99-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-99-0.sql 2010-09-10 10:54:51 +0000
@@ -0,0 +1,14 @@
+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 COLUMN parent_source_version text;
+
+-- Add a unique constraint/index for the source_package_name/derived series combo.
+ALTER TABLE DistroSeriesDifference ADD CONSTRAINT distroseriesdifference_source_package_name__derived_series__uni UNIQUE (derived_series, source_package_name);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 99, 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-10 10:54:51 +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-10 10:54:51 +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-10 10:54:51 +0000
@@ -15,6 +15,7 @@
Int,
Reference,
Storm,
+ Unicode,
)
from zope.component import getUtility
from zope.interface import (
@@ -41,6 +42,7 @@
)
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment)
+from lp.services.propertycache import cachedproperty
class DistroSeriesDifference(Storm):
@@ -65,10 +67,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 +94,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 +123,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 +167,18 @@
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`."""
+ type_updated = self._updateType()
+ status_updated = self._updateStatus()
+ return type_updated or status_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:
@@ -177,14 +191,43 @@
updated = True
self.difference_type = new_type
- version = self.source_version
- parent_version = self.parent_source_version
+ def _updateStatus(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-10 10:54:51 +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):
@@ -82,6 +84,7 @@
sourcepackagename=ds_diff.source_package_name,
distroseries=ds_diff.derived_series,
status=PackagePublishingStatus.PENDING)
+ IPropertyCacheManager(ds_diff).clear()
self.assertEqual(pending_pub, ds_diff.source_pub)
@@ -112,6 +115,7 @@
sourcepackagename=ds_diff.source_package_name,
distroseries=ds_diff.derived_series.parent_series,
status=PackagePublishingStatus.PENDING)
+ IPropertyCacheManager(ds_diff).clear()
self.assertEqual(pending_pub, ds_diff.parent_source_pub)
@@ -132,7 +136,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",
@@ -145,15 +149,16 @@
distroseries=ds_diff.derived_series,
status=PackagePublishingStatus.PENDING,
version='1.0')
+ IPropertyCacheManager(ds_diff).clear()
- 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(
@@ -168,19 +173,18 @@
distroseries=ds_diff.derived_series,
status=PackagePublishingStatus.PENDING,
version='1.1')
+ IPropertyCacheManager(ds_diff).clear()
- 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={
@@ -192,15 +196,17 @@
distroseries=ds_diff.derived_series,
status=PackagePublishingStatus.PENDING,
version='1.1')
-
- was_updated = ds_diff.updateStatusAndType()
-
- self.assertFalse(was_updated)
+ IPropertyCacheManager(ds_diff).clear()
+
+ was_updated = ds_diff.update()
+
+ 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
@@ -217,14 +223,66 @@
distroseries=ds_diff.derived_series.parent_series,
status=PackagePublishingStatus.PENDING,
version='1.1')
+ IPropertyCacheManager(ds_diff).clear()
- 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')
+ IPropertyCacheManager(ds_diff).clear()
+
+ 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')
+ IPropertyCacheManager(ds_diff).clear()
+
+ 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 +345,26 @@
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()
+
+ cache = IPropertyCacheManager(ds_diff).cache
+
+ self.assertContentEqual(
+ ['source_pub', 'parent_source_pub'], cache)
+
class DistroSeriesDifferenceSourceTestCase(TestCaseWithFactory):
@@ -317,7 +395,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 +442,7 @@
result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
derived_series,
- status=DistroSeriesDifferenceStatus.IGNORED)
+ status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
self.assertContentEqual(diffs['ignored'], result)
@@ -376,7 +454,7 @@
result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
derived_series,
status=(
- DistroSeriesDifferenceStatus.IGNORED,
+ DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
))
Follow ups
