launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-783435 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #783435 in Launchpad itself: "+localpackagediffs shows a sync checkbox for packages that can't be synced"
  https://bugs.launchpad.net/launchpad/+bug/783435

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-783435/+merge/61703

= Summary =

The DistroSeries:+localpackagediffs page shows version differences between the packages of a derived DistroSeries and its parents.  Individual differences can be selected using a checkbox, and then collectively "synchronized."  This involves copying the parent's version into the derived series.

In some cases, however, the derived series will have a newer version of the package.  In those cases it makes no sense to present the checkbox.


== Proposed fix ==

The view code already has a method for deciding whether the checkbox should be offered on a given entry.  This branch splices the version comparison into that method.


== Pre-implementation notes ==

I asked William and Steve about the comparison properties of the Debian-version type.  They don't complicate the code as such, but it would have been easy to write the wrong simple code.


== Implementation details ==

The tests are surprisingly slow because of the time it takes to produce a DistroSeriesDifference in the factory.  It'd be really, really nice if we could speed that up.  But doing that individually for each test is a bottomless pit; we need systemic optimizations.


== Tests ==

{{{
./bin/test -vvc lp.registry.browser.tests.test_distroseries -t canRequest -t isNewer
}}}


== Demo and Q/A ==

Look at the +localpackagediffs page for a derived distroseries (at the moment Oneiric would be good).  Make sure you are privileged to synchronize packages; most differences will have checkboxes that allow you to do so, but the ones where the derived series has a newer version than the parent won't.

Work is also in progress to hide these differences from the default view, so you may need to reveal them.  (We currently call this hiding "blacklisting," but the term is also being changed and the new term escapes me for the moment).


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_distroseries.py
  lib/lp/registry/browser/distroseries.py
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-783435/+merge/61703
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-783435 into lp:launchpad/db-devel.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-05-19 16:05:17 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-05-20 06:43:26 +0000
@@ -866,10 +866,24 @@
         """Is there a package-copying job pending to resolve `dsd`?"""
         return self.pending_syncs.get(specify_dsd_package(dsd)) is not None
 
+    def isNewerThanParent(self, dsd):
+        """Is the child's version of this package newer than the parent's?
+
+        If it is, there's no point in offering to sync it.
+        """
+        # This is trickier than it looks: versions are not totally
+        # ordered.  Two non-identical versions may compare as equal.
+        # Only consider cases where the child's version is conclusively
+        # newer, not where the relationship is in any way unclear.
+        return dsd.parent_source_version < dsd.source_version
+
     def canRequestSync(self, dsd):
         """Does it make sense to request a sync for this difference?"""
-        # XXX JeroenVermeulen bug=783435: Also compare versions.
-        return not self.hasPendingSync(dsd)
+        # There are two conditions for this: it doesn't make sense to
+        # sync if the child's version of the package is newer than the
+        # parent's version, or if there is already a sync pending.
+        return (
+            not self.isNewerThanParent(dsd) and not self.hasPendingSync(dsd))
 
     @property
     def specified_name_filter(self):

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-19 16:05:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-20 06:43:26 +0000
@@ -1362,6 +1362,30 @@
         view.pending_syncs = {specify_dsd_package(dsd): object()}
         self.assertTrue(view.hasPendingSync(dsd))
 
+    def test_isNewerThanParent_is_False_for_parent_update(self):
+        dsd = self.factory.makeDistroSeriesDifference(
+            versions=dict(base='1.0', parent='1.1', derived='1.0'))
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertFalse(view.isNewerThanParent(dsd))
+
+    def test_isNewerThanParent_is_False_for_equivalent_updates(self):
+        # Some non-identical version numbers compare as "equal."  If the
+        # child and parent versions compare as equal, the child version
+        # is not considered newer.
+        dsd = self.factory.makeDistroSeriesDifference(
+            versions=dict(base='1.0', parent='1.1', derived='1.1'))
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertFalse(view.isNewerThanParent(dsd))
+
+    def test_isNewerThanParent_is_True_for_child_update(self):
+        dsd = self.factory.makeDistroSeriesDifference(
+            versions=dict(base='1.0', parent='1.0', derived='1.1'))
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertTrue(view.isNewerThanParent(dsd))
+
     def test_canRequestSync_returns_False_if_pending_sync(self):
         dsd = self.factory.makeDistroSeriesDifference()
         view = create_initialized_view(
@@ -1369,6 +1393,13 @@
         view.pending_syncs = {specify_dsd_package(dsd): object()}
         self.assertFalse(view.canRequestSync(dsd))
 
+    def test_canRequestSync_returns_False_if_child_is_newer(self):
+        dsd = self.factory.makeDistroSeriesDifference(
+            versions=dict(base='1.0', parent='1.0', derived='1.1'))
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertFalse(view.canRequestSync(dsd))
+
     def test_canRequestSync_returns_True_if_sync_makes_sense(self):
         dsd = self.factory.makeDistroSeriesDifference()
         view = create_initialized_view(