Merge lp:~rvb/launchpad/dds-fix-localpackagediffs-745776 into lp:launchpad

[Raphaël Badin]

[r=allenap,julian-edwards][bug=745776] Fix the display of the versions on the localpackagediffs page.

This branch fixes the display of the versions in the localpackagediffs page and the link to the sourcepackagerelease.

= Test =

{{{
 ./bin/test -cvv test_series_views test_diff_row_shows_version_attached
}}}

= QA =
Check that the versions displayed in the localpackagediff page's table are consistent with the version displayed in the details of the difference.
(https://dogfood.launchpad.net/deribuntu/dangerous/+localpackagediffs)

[Gavin Panella (allenap)]

Lovely. A few minor comments.

[1]

+ @property
+ def parent_source_package_url(self):
+ return self._package_url(parent=True)
+
+ @property
+ def source_package_url(self):
+ return self._package_url()
+
+ def _package_url(self, parent=False):
+ if parent:
+ distro_series = self.context.derived_series.parent_series
+ version = self.context.parent_source_version
+ else:
+ distro_series = self.context.derived_series
+ version = self.context.source_version

You could get one less level of indirection if you pass distro_series
and version into _package_url():

    @property
    def parent_source_package_url(self):
        return self._package_url(
            self.context.derived_series.parent_series,
            self.context.parent_source_version)

    @property
    def source_package_url(self):
        return self._package_url(
            self.context.derived_series,
            self.context.source_version)

    def _package_url(self, distro_series, version):
        ...

But this is entirely up to you; it's fine as it is.

[2]

+ try:
+ url = canonical_url(
+ DistroSeriesSourcePackageRelease(
+ distro_series, pubs[0].sourcepackagerelease))
+ return url
+ except IndexError:
+ return ''

I assume you're attempting to catch any potential IndexErrors raised
by pubs[0], but it will also catch IndexError in any of the called
code.

Now pubs will be an SQLObjectResultSet, so you could do the following:

        from storm.zope.interfaces import IResultSet

        pub = IResultSet(pubs).first()
        if pub is None:
            return ''
        else:
            return canonical_url(
                DistroSeriesSourcePackageRelease(
                    distro_series, pub.sourcepackagerelease))

Out of interest, why do you only consider the first publication here?

[3]

+ row = diff_table.tbody.findAll('tr')[0]

This could simplified to:

        row = diff_table.tbody.tr

[4]

+ self.assertTrue(link.endswith(new_version))
...
+ self.assertTrue(
+ links[0].get('href').endswith(difference.source_version))

In the event of a failure, you could make the report more informative
by using the EndsWith matcher:

    from testtools.matchers import EndsWith
    self.assertTrue(link, EndsWith(new_version))

review: Approve

[Julian Edwards (julian-edwards)]

6 + def _package_url(self, distro_series, version):
57 + pubs = distro_series.getPublishedSources(
58 + self.context.source_package_name, include_pending=True,
59 + version=version)

This is going to break for 2 reasons:

 1. include_pending=True will make it potentially return more than one version
 2. the archive is not specified so it'll pick up publications from PPAs

I want to deprecate IDistroSeries.getPublishedSources(), it's a little evil. Let's use IArchive.getPublishedSources() instead, you can do the exact same thing by copying this code:

pubs = distro_series.main_archive.getPublishedSources(
    name=self.context.source_package_name,
    version=version,
    status=PackagePublishingStatus.PUBLISHED,
    exact_match=True)

and profit!

review: Needs Fixing

[Julian Edwards (julian-edwards)]

When you make the fix, can you put a comment in the interface and the model code for the IDistroSeries.getPublishedSources() call to say that it's deprecated in favour of the IArchive version. It's far too easy to do the wrong thing with it, plus it's not Storm-ified.

[Gavin Panella (allenap)]

> pubs = distro_series.main_archive.getPublishedSources(
> name=self.context.source_package_name,
> version=version,
> status=PackagePublishingStatus.PUBLISHED,
> exact_match=True)

How many records can pubs contain? Will it always be 1, 0 or 1, 0 or
more, or 1 or more?

[Julian Edwards (julian-edwards)]

I missed distroseries= in that example, oops :)

With that, it'll always be none or one.

[Raphaël Badin (rvb)]

[1], [2], [3], [4] Fixed.

[2] Only one or zero publication should be there. I've reworked the code to better take this fact into account.

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2011-03-31 16:21:15 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2011-04-01 10:04:32 +0000
@@ -10,6 +10,7 @@
     ]
 
 from lazr.restful.interfaces import IWebServiceClientRequest
+from storm.zope.interfaces import IResultSet
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.form.browser.itemswidgets import RadioWidget
 from zope.component import (
@@ -27,15 +28,14 @@
     )
 
 from canonical.launchpad.webapp import (
+    canonical_url,
     LaunchpadView,
     Navigation,
     stepthrough,
     )
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.launchpadform import custom_widget
-from lp.app.browser.launchpadform import (
-    LaunchpadFormView,
-    )
+from lp.app.browser.launchpadform import LaunchpadFormView
 from lp.registry.enum import DistroSeriesDifferenceStatus
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifference,
@@ -51,6 +51,10 @@
     IComment,
     IConversation,
     )
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.model.distroseriessourcepackagerelease import (
+    DistroSeriesSourcePackageRelease,
+    )
 
 
 class DistroSeriesDifferenceNavigation(Navigation):
@@ -67,6 +71,35 @@
             IDistroSeriesDifferenceCommentSource).getForDifference(
                 self.context, id)
 
+    @property
+    def parent_source_package_url(self):
+        return self._package_url(
+            self.context.derived_series.parent_series,
+            self.context.parent_source_version)
+
+    @property
+    def source_package_url(self):
+        return self._package_url(
+            self.context.derived_series,
+            self.context.source_version)
+
+    def _package_url(self, distro_series, version):
+        pubs = distro_series.main_archive.getPublishedSources(
+            name=self.context.source_package_name.name,
+            version=version,
+            status=PackagePublishingStatus.PUBLISHED,
+            distroseries=distro_series,
+            exact_match=True)
+
+        # There is only one or zero published package.
+        pub = IResultSet(pubs).one()
+        if pub is None:
+            return None
+        else:
+            return canonical_url(
+                DistroSeriesSourcePackageRelease(
+                    distro_series, pub.sourcepackagerelease))
+
 
 class IDistroSeriesDifferenceForm(Interface):
     """An interface used in the browser only for displaying form elements."""

=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2011-03-29 18:09:43 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2011-04-01 10:04:32 +0000
@@ -5,11 +5,10 @@
 
 from BeautifulSoup import BeautifulSoup
 from storm.zope.interfaces import IResultSet
+from testtools.matchers import EndsWith
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-import unittest
-
 from canonical.config import config
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.testing.pages import find_tag_by_id
@@ -17,32 +16,33 @@
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
-    LaunchpadFunctionalLayer,
     )
 from lp.registry.enum import (
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
     )
+from lp.services.features import (
+    getFeatureFlag,
+    install_feature_controller,
+    )
 from lp.services.features.flags import FeatureController
 from lp.services.features.model import (
     FeatureFlag,
     getFeatureStore,
     )
-from lp.services.features import (
-    getFeatureFlag,
-    install_feature_controller,
+from lp.soyuz.enums import (
+    PackagePublishingStatus,
+    SourcePackageFormat,
     )
 from lp.soyuz.interfaces.sourcepackageformat import (
     ISourcePackageFormatSelectionSet,
     )
-from lp.soyuz.enums import (
-    SourcePackageFormat,
-    )
 from lp.testing import (
-    TestCaseWithFactory,
     login_person,
     person_logged_in,
+    TestCaseWithFactory,
     )
 from lp.testing.views import create_initialized_view
 
@@ -268,6 +268,95 @@
         self.assertEqual(1, len(links))
         self.assertEqual(difference.source_package_name.name, links[0].string)
 
+    def test_diff_row_shows_version_attached(self):
+        # The +localpackagediffs page shows the version attached to the
+        # DSD and not the last published version (bug=745776).
+        package_name = 'package-1'
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
+        versions = {
+            'base': u'1.0',
+            'derived': u'1.0derived1',
+            'parent': u'1.0-1',
+        }
+        new_version = u'1.2'
+
+        difference = self.factory.makeDistroSeriesDifference(
+            versions=versions,
+            source_package_name_str=package_name,
+            derived_series=derived_series)
+
+        # Create a more recent source package publishing history.
+        sourcepackagename = self.factory.getOrMakeSourcePackageName(
+            package_name)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=sourcepackagename,
+            distroseries=derived_series,
+            version=new_version)
+
+        set_derived_series_ui_feature_flag(self)
+        view = create_initialized_view(
+            derived_series, '+localpackagediffs')
+        soup = BeautifulSoup(view())
+        diff_table = soup.find('table', {'class': 'listing'})
+        row = diff_table.tbody.tr
+        links = row.findAll('a', {'class': 'derived-version'})
+
+        # The version displayed is the version attached to the
+        # difference.
+        self.assertEqual(1, len(links))
+        self.assertEqual(versions['derived'], links[0].string.strip())
+
+        link = canonical_url(difference.source_pub.sourcepackagerelease)
+        self.assertTrue(link, EndsWith(new_version))
+        # The link points to the sourcepackagerelease referenced in the
+        # difference.
+        self.assertTrue(
+            links[0].get('href'), EndsWith(difference.source_version))
+
+    def test_diff_row_no_published_version(self):
+        # The +localpackagediffs page shows only the version (no link)
+        # if we fail to fetch the published version.
+        package_name = 'package-1'
+        derived_series = self.factory.makeDistroSeries(
+            name='derilucid', parent_series=self.factory.makeDistroSeries(
+                name='lucid'))
+        versions = {
+            'base': u'1.0',
+            'derived': u'1.0derived1',
+            'parent': u'1.0-1',
+        }
+        new_version = u'1.2'
+
+        difference = self.factory.makeDistroSeriesDifference(
+            versions=versions,
+            source_package_name_str=package_name,
+            derived_series=derived_series)
+
+        # Delete the publications.
+        difference.source_pub.status = PackagePublishingStatus.DELETED
+        difference.parent_source_pub.status = PackagePublishingStatus.DELETED
+
+        set_derived_series_ui_feature_flag(self)
+        view = create_initialized_view(
+            derived_series, '+localpackagediffs')
+        soup = BeautifulSoup(view())
+        diff_table = soup.find('table', {'class': 'listing'})
+        row = diff_table.tbody.tr
+
+        # The table feature a simple span since we were unable to fetch a
+        # published sourcepackage.
+        derived_span = row.findAll('span', {'class': 'derived-version'})
+        parent_span = row.findAll('span', {'class': 'parent-version'})
+        self.assertEqual(1, len(derived_span))
+        self.assertEqual(1, len(parent_span))
+
+        # The versions displayed are the versions attached to the
+        # difference.
+        self.assertEqual(versions['derived'], derived_span[0].string.strip())
+        self.assertEqual(versions['parent'], parent_span[0].string.strip())
+
 
 class DistroSeriesLocalPackageDiffsFunctionalTestCase(TestCaseWithFactory):
 
@@ -504,7 +593,3 @@
             for item in view.milestone_batch_navigator.currentBatch()]
         self.assertEqual(expected, milestone_names)
         config.pop('default-batch-size')
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2011-03-30 11:28:04 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2011-04-01 10:04:32 +0000
@@ -559,6 +559,8 @@
                             archive=None):
         """Return the SourcePackagePublishingHistory(s)
 
+        Deprecated.  Use IArchive.getPublishedSources instead.
+
         Given a ISourcePackageName or name.
 
         If pocket is not specified, we look in all pockets.

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2011-03-30 11:32:49 +0000
+++ lib/lp/registry/model/distroseries.py	2011-04-01 10:04:32 +0000
@@ -1069,6 +1069,8 @@
                              pocket=None, include_pending=False,
                              exclude_pocket=None, archive=None):
         """See `IDistroSeries`."""
+        # Deprecated.  Use IArchive.getPublishedSources instead.
+
         # XXX cprov 2006-02-13 bug 31317:
         # We need a standard and easy API, no need
         # to support multiple type arguments, only string name should be

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-03-23 16:28:51 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-04-01 10:04:32 +0000
@@ -64,17 +64,30 @@
                     id string:field.selected_differences.${src_name}"/>
 
                 <a tal:attributes="href difference/fmt:url" class="toggle-extra"
-                   tal:content="parent_source_pub/source_package_name">Foo</a>
-              </td>
-              <td><a tal:attributes="href parent_source_pub/sourcepackagerelease/fmt:url">
-                    <tal:replace
-                    replace="parent_source_pub/sourcepackagerelease/version"/></a>
-              </td>
-              <td><a tal:attributes="href source_pub/sourcepackagerelease/fmt:url"
-                     class="derived-version">
-                    <tal:replace
-                    replace="source_pub/sourcepackagerelease/version"/></a>
-              </td>
+                    tal:content="parent_source_pub/source_package_name">Foo</a>
+              </td>
+              <td tal:define="parent_source_pck_url difference/@@/parent_source_package_url">
+                <a tal:condition="parent_source_pck_url"
+                    tal:attributes="href difference/@@/parent_source_package_url"
+                    class="parent-version">
+                    <tal:replace
+                        replace="difference/parent_source_version"/></a>
+                <span tal:condition="not: parent_source_pck_url"
+                    class="parent-version"
+                    tal:content="difference/parent_source_version">
+                </span>
+              </td>
+              <td tal:define="source_pck_url difference/@@/source_package_url">
+                <a tal:condition="source_pck_url"
+                    tal:attributes="href difference/@@/source_package_url"
+                    class="derived-version">
+                    <tal:replace
+                        replace="difference/source_version"/></a>
+                <span tal:condition="not: source_pck_url"
+                    class="derived-version"
+                    tal:content="difference/source_version">
+                </span>
+               </td>
               <td>
                   <span tal:attributes="title difference/source_pub/datepublished/fmt:datetime"
                         tal:content="difference/source_pub/datepublished/fmt:approximatedate">2005-09-16</span>