Merge lp:~michael.nelson/launchpad/distro-series-difference-model into lp:launchpad/db-devel

[Michael Nelson]

Restrict DistroSeriesDifference.addComment() to lp.Edit and add DSD.updateStatusAndType().

Overview
========

This branch ensures that comments can only be added to a DistroSeriesDifference by people with launchpad.Edit (currently people in the owning team of the derived series).

It also adds a getComments() method, and a method to update the status and/or type of difference based on any new publishings.

This branch follows on from:
https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-message/+merge/33929

Test:
bin/test -vv -m test_distroseriesdifference -m test_distroseriesdifferencecomment

No lint.

[Aaron Bentley (abentley)]

Why are source_version and parent_source_version returning ''? I would have expected them to return None if there was no such value.

Why not implement getComments directly instead of indirecting through IDistroSeriesDifferenceCommentSource?

Your test comments say None, but test for the empty string. Please fix either the test or the comment.

Why assertIs(True, was_updated) instead of assertTrue(was_updated)? We should only care about whether the returned value evaluates to True. We should definitely not care about its identity.

Why assertIs(True, was_updated) instead of assertTrue(ds_diff.updateStatusAndType())? It is shorter, so easier to read.

[Michael Nelson (michael.nelson)]

On Mon, Aug 30, 2010 at 7:18 PM, Aaron Bentley <email address hidden> wrote:
> Why are source_version and parent_source_version returning ''?  I would have expected them to return None if there was no such value.

They were originally returning None (as you saw by the test comment
below), but I'd thought for comparisons etc., it would be more
consistent if these properties always resulted in a string - an empty
one if there is no version. Anyway, I've switched them back to return
None.

>
> Why not implement getComments directly instead of indirecting through IDistroSeriesDifferenceCommentSource?

Because there's no reason why DistroSeriesDifference needs to know
about the implementation of DistroSeriesDifferenceComment ("Program to
interfaces, not implementations", "Depend on abstractions, do not
depend on concrete classes", etc.)

Currently DistroSeriesDifference doesn't import any model code, nor
does it do any storm queries directly. I thought that was a good
thing, but can change it if you want.

>
> Your test comments say None, but test for the empty string.  Please fix either the test or the comment.

Tests updated to expect None.

>
> Why assertIs(True, was_updated) instead of assertTrue(was_updated)?  We should only care about whether the returned value evaluates to True.  We should definitely not care about its identity.

My bad.

>
> Why assertIs(True, was_updated) instead of assertTrue(ds_diff.updateStatusAndType())?  It is shorter, so easier to read.

Ditto.

Thanks Aaron.

[Robert Collins (lifeless)]

@Michael regarding interfaces and model code; if our implementations
were substitutable *with* high performance, I'd totally support what
you're saying.

However, the reality is that object traversal is slow (because of lazy
loading); our model objects conflate database access with domain
knowledge, and these two things combined mean that:
 - you cannot replace Person in calls wanting IPerson with something
derived from Person and stored in another table: the whole structure
of model->storm relationships means that that will simply not work.
 - working solely via I* guarantees terrible performance and the
inability to drive the lower layers efficiently.

I would like to change this, as has been discussed on the list, but we
don't have a better pattern || code for it yet.

You should, of course, delegate, DRY and so forth, but Interface vs
Implementation as far as Launchpad Interfaces go is a weak story.

-Rob

[Aaron Bentley (abentley)]

Approved, assuming the changes in http://pastebin.ubuntu.com/486363/

review: Approve

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2010-08-31 15:59:47 +0000
+++ lib/lp/registry/configure.zcml	2010-08-31 15:59:49 +0000
@@ -111,7 +111,10 @@
     </securedutility>
     <class
         class="lp.registry.model.distroseriesdifference.DistroSeriesDifference">
-        <allow interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"/>
+        <allow interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifferencePublic"/>
+        <require
+            permission="launchpad.Edit"
+            interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifferenceEdit"/>
     </class>
 
     <!-- DistroSeriesDifferenceComment -->

=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py	2010-08-31 15:59:47 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py	2010-08-31 15:59:49 +0000
@@ -8,6 +8,8 @@
 
 __all__ = [
     'IDistroSeriesDifference',
+    'IDistroSeriesDifferencePublic',
+    'IDistroSeriesDifferenceEdit',
     'IDistroSeriesDifferenceSource',
     ]
 
@@ -16,7 +18,6 @@
 from zope.schema import (
     Choice,
     Int,
-    Text,
     TextLine,
     )
 
@@ -26,13 +27,15 @@
     DistroSeriesDifferenceType,
     )
 from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.sourcepackagename import ISourcePackageName
+from lp.registry.interfaces.role import IHasOwner
 from lp.soyuz.interfaces.packagediff import IPackageDiff
 from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
 
 
-class IDistroSeriesDifference(Interface):
-    """An interface for a package difference between two distroseries."""
+class IDistroSeriesDifferencePublic(IHasOwner, Interface):
+    """The public interface for distro series differences."""
 
     id = Int(title=_('ID'), required=True, readonly=True)
 
@@ -54,10 +57,6 @@
         readonly=True, description=_(
             "The most recently generated package diff for this difference."))
 
-    activity_log = Text(
-        title=_('A log of activity and comments for this difference.'),
-        required=False, readonly=True)
-
     status = Choice(
         title=_('Distro series difference status.'),
         description=_('The current status of this difference.'),
@@ -76,20 +75,60 @@
         description=_(
             "The most recent published version in the derived series."))
 
+    source_version = TextLine(
+        title=_("Source version"), readonly=True,
+        description=_(
+            "The version of the most recent source publishing in the "
+            "derived series."))
+
     parent_source_pub = Reference(
         ISourcePackagePublishingHistory,
         title=_("Parent source pub"), readonly=True,
         description=_(
             "The most recent published version in the parent series."))
 
+    parent_source_version = TextLine(
+        title=_("Parent source version"), readonly=True,
+        description=_(
+            "The version of the most recent source publishing in the "
+            "parent series."))
+
+    owner = Reference(
+        IPerson, title=_("Owning team of the derived series"), readonly=True,
+        description=_(
+            "This attribute mirrors the owner of the derived series."))
+
     title = TextLine(
         title=_("Title"), readonly=True, required=False, description=_(
             "A human-readable name describing this difference."))
 
+    def updateStatusAndType():
+        """Checks that difference type and status matches current publishings.
+
+        If the record is updated, a relevant comment is added.
+
+        If there is no longer a difference (ie. the versions are
+        the same) then the status is updated to RESOLVED.
+
+        :return: True if the record was updated, False otherwise.
+        """
+
+    def getComments():
+        """Return a result set of the comments for this difference."""
+
+
+class IDistroSeriesDifferenceEdit(Interface):
+    """Difference attributes requiring launchpad.Edit."""
+
     def addComment(owner, comment):
         """Add a comment on this difference."""
 
 
+class IDistroSeriesDifference(IDistroSeriesDifferencePublic,
+                              IDistroSeriesDifferenceEdit):
+    """An interface for a package difference between two distroseries."""
+
+
 class IDistroSeriesDifferenceSource(Interface):
     """A utility of this interface can be used to create differences."""
 

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2010-08-31 15:59:47 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2010-08-31 15:59:49 +0000
@@ -9,7 +9,6 @@
     'DistroSeriesDifference',
     ]
 
-
 from storm.locals import (
     Int,
     Reference,
@@ -22,7 +21,10 @@
     )
 
 from canonical.database.enumcol import DBEnum
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import (
+    IMasterStore,
+    IStore,
+    )
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
@@ -35,6 +37,8 @@
 from lp.registry.interfaces.distroseriesdifferencecomment import (
     IDistroSeriesDifferenceCommentSource,
     )
+from lp.registry.model.distroseriesdifferencecomment import (
+    DistroSeriesDifferenceComment)
 
 
 class DistroSeriesDifference(Storm):
@@ -91,23 +95,24 @@
         return self._getLatestSourcePub(for_parent=True)
 
     @property
+    def owner(self):
+        """See `IDistroSeriesDifference`."""
+        return self.derived_series.owner
+
+    @property
     def title(self):
         """See `IDistroSeriesDifference`."""
         parent_name = self.derived_series.parent_series.displayname
         return ("Difference between distroseries '%(parent_name)s' and "
                 "'%(derived_name)s' for package '%(pkg_name)s' "
-                "(%(versions)s)" % {
+                "(%(parent_version)s/%(source_version)s)" % {
                     'parent_name': parent_name,
                     'derived_name': self.derived_series.displayname,
                     'pkg_name': self.source_package_name.name,
-                    'versions': self._getVersions(),
+                    'parent_version': self.parent_source_version,
+                    'source_version': self.source_version,
                     })
 
-    @property
-    def activity_log(self):
-        """See `IDistroSeriesDifference`."""
-        return u""
-
     def _getLatestSourcePub(self, for_parent=False):
         """Helper to keep source_pub/parent_source_pub DRY."""
         distro_series = self.derived_series
@@ -123,16 +128,56 @@
         else:
             return None
 
-    def _getVersions(self):
-        """Helper method returning versions string."""
-        src_pub_ver = parent_src_pub_ver = "-"
+    @property
+    def source_version(self):
+        """See `IDistroSeriesDifference`."""
         if self.source_pub:
-            src_pub_ver = self.source_pub.source_package_version
-        if self.parent_source_pub is not None:
-            parent_src_pub_ver = self.parent_source_pub.source_package_version
-        return parent_src_pub_ver + "/" + src_pub_ver
+            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`."""
+        if self.source_pub is None:
+            new_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        elif self.parent_source_pub is None:
+            new_type = DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES
+        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
+        if self.status == DistroSeriesDifferenceStatus.RESOLVED:
+            if version != parent_version:
+                updated = True
+                self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+        else:
+            if version == parent_version:
+                updated = True
+                self.status = DistroSeriesDifferenceStatus.RESOLVED
+
+        return updated
 
     def addComment(self, owner, comment):
         """See `IDistroSeriesDifference`."""
         return getUtility(IDistroSeriesDifferenceCommentSource).new(
             self, owner, comment)
+
+    def getComments(self):
+        """See `IDistroSeriesDifference`."""
+        DSDComment = DistroSeriesDifferenceComment
+        comments = IStore(DSDComment).find(
+            DistroSeriesDifferenceComment,
+            DSDComment.distro_series_difference == self)
+        return comments.order_by(DSDComment.id)

=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py	2010-08-31 15:59:47 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py	2010-08-31 15:59:49 +0000
@@ -3,17 +3,27 @@
 
 """Model tests for the DistroSeriesDifference class."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 import unittest
 
 from storm.store import Store
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 
+from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import DatabaseFunctionalLayer
-from lp.testing import TestCaseWithFactory
-from lp.registry.enum import DistroSeriesDifferenceType
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.registry.enum import (
+    DistroSeriesDifferenceStatus,
+    DistroSeriesDifferenceType,
+    )
 from lp.registry.exceptions import NotADerivedSeriesError
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifference,
@@ -105,6 +115,116 @@
 
         self.assertEqual(pending_pub, ds_diff.parent_source_pub)
 
+    def test_source_version(self):
+        # The version of the source in the derived series is returned.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew")
+
+        self.assertEqual(
+            ds_diff.source_pub.source_package_version, ds_diff.source_version)
+
+    def test_source_version_none(self):
+        # None is returned for source_version when there is no source pub.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            difference_type=(
+                DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES))
+
+        self.assertEqual(None, ds_diff.source_version)
+
+    def test_updateStatusAndType_resolves_difference(self):
+        # Status is set to resolved when versions match.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'parent': '1.0',
+                'derived': '0.9',
+                })
+        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.updateStatusAndType()
+
+        self.assertTrue(was_updated)
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.RESOLVED,
+            ds_diff.status)
+
+    def test_updateStatusAndType_re_opens_difference(self):
+        # The status of a resolved difference will updated with new
+        # uploads.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'parent': '1.0',
+                'derived': '1.0',
+                },
+            status=DistroSeriesDifferenceStatus.RESOLVED)
+        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.updateStatusAndType()
+
+        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.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'parent': '1.0',
+                'derived': '0.9',
+                })
+        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.updateStatusAndType()
+
+        self.assertFalse(was_updated)
+        self.assertEqual(
+            DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
+            ds_diff.status)
+
+    def test_updateStatusAndType_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
+        # to the parent series with a different version.
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foonew",
+            versions={
+                'derived': '0.9',
+                },
+            difference_type=(
+                DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES))
+        new_parent_pub = self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=ds_diff.source_package_name,
+            distroseries=ds_diff.derived_series.parent_series,
+            status=PackagePublishingStatus.PENDING,
+            version='1.1')
+
+        was_updated = ds_diff.updateStatusAndType()
+
+        self.assertTrue(was_updated)
+        self.assertEqual(
+            DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
+            ds_diff.difference_type)
+
     def test_title(self):
         # The title is a friendly description of the difference.
         parent_series = self.factory.makeDistroSeries(name="lucid")
@@ -126,11 +246,45 @@
         # Adding a comment creates a new DistroSeriesDifferenceComment
         ds_diff = self.factory.makeDistroSeriesDifference(
             source_package_name_str="foonew")
+
+        with person_logged_in(ds_diff.owner):
+            dsd_comment = ds_diff.addComment(
+                ds_diff.owner, "Wait until version 2.1")
+
+        self.assertEqual(ds_diff, dsd_comment.distro_series_difference)
+
+    def test_getComments(self):
+        # All comments for this difference are returned.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        with person_logged_in(ds_diff.owner):
+            dsd_comment = ds_diff.addComment(
+                ds_diff.owner, "Wait until version 2.1")
+            dsd_comment_2 = ds_diff.addComment(
+                ds_diff.owner, "Wait until version 2.1")
+
+        self.assertEqual(
+            [dsd_comment, dsd_comment_2], list(ds_diff.getComments()))
+
+    def test_addComment_not_public(self):
+        # Comments cannot be added with launchpad.View.
+        ds_diff = self.factory.makeDistroSeriesDifference()
         person = self.factory.makePerson()
 
-        dsd_comment = ds_diff.addComment(person, "Wait until version 2.1")
-
-        self.assertEqual(ds_diff, dsd_comment.distro_series_difference)
+        with person_logged_in(person):
+            self.assertTrue(check_permission('launchpad.View', ds_diff))
+            self.assertFalse(check_permission('launchpad.Edit', ds_diff))
+            self.assertRaises(Unauthorized, getattr, ds_diff, 'addComment')
+
+    def test_addComment_for_owners(self):
+        # Comments can be added by any of the owners of the derived
+        # series.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        with person_logged_in(ds_diff.owner):
+            self.assertTrue(check_permission('launchpad.Edit', ds_diff))
+            diff_comment = ds_diff.addComment(
+                ds_diff.derived_series.owner, "Boo")
 
 
 def test_suite():

=== modified file 'lib/lp/registry/tests/test_distroseriesdifferencecomment.py'
--- lib/lp/registry/tests/test_distroseriesdifferencecomment.py	2010-08-31 15:59:47 +0000
+++ lib/lp/registry/tests/test_distroseriesdifferencecomment.py	2010-08-31 15:59:49 +0000
@@ -6,11 +6,13 @@
 __metaclass__ = type
 
 from storm.store import Store
+from zope.component import getUtility
 
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.interfaces.distroseriesdifferencecomment import (
     IDistroSeriesDifferenceComment,
+    IDistroSeriesDifferenceCommentSource,
     )
 from lp.testing import TestCaseWithFactory
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-08-31 15:59:47 +0000
+++ lib/lp/testing/factory.py	2010-08-31 15:59:49 +0000
@@ -154,7 +154,10 @@
     IHWSubmissionDeviceSet,
     IHWSubmissionSet,
     )
-from lp.registry.enum import DistroSeriesDifferenceType
+from lp.registry.enum import (
+    DistroSeriesDifferenceStatus,
+    DistroSeriesDifferenceType,
+    )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.distributionmirror import (
     MirrorContent,
@@ -1827,7 +1830,8 @@
     def makeDistroSeriesDifference(
         self, derived_series=None, source_package_name_str=None,
         versions=None,
-        difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS):
+        difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
+        status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION):
         """Create a new distro series source package difference."""
         if derived_series is None:
             parent_series = self.makeDistroSeries()
@@ -1860,7 +1864,8 @@
                 sourcepackagename=source_package_name)
 
         return getUtility(IDistroSeriesDifferenceSource).new(
-            derived_series, source_package_name, difference_type)
+            derived_series, source_package_name, difference_type,
+            status=status)
 
     def makeDistroSeriesDifferenceComment(
         self, distro_series_difference=None, owner=None, comment=None):