Merge lp:~rvb/launchpad/dds-missing-base-version-747202 into lp:launchpad

[Raphaël Badin]

[r=jcsackett][bug=747202] Fix the UI on the localpackagediffs page when dsd's base_version is None.

This branch fixes the UI on the localpackagediffs page when dsd's base_version is None. It displays 'unknown' instead of an empty line for "Last common version:" and prevents the package diff slot to be displayed.

== Tests ==
./bin/test -cvv test_distroseriesdifference_views test_package_diff_no_base_version

== QA ==
- Fire up a local instance.
- Turn on the feature flag :
    'soyuz.derived-series-ui.enabled default 1 on'
- Update the dsd:
    update distroseriesdifference set base_version = NULL ;
- Access the page
    https://launchpad.dev/deribuntu/deriwarty/+localpackagediffs
- Open up the first row.
- Make sure:
    - "Last common version:" is Unknown.
    - the package diff request link is not there.

[j.c.sackett (jcsackett)]

As discussed in IRC, I think this needs to be investigated, as I'm not sure the boolean logic ever evaulates to True

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-03-31 20:40:32 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-04-05 10:03:19 +0000
@@ -48,10 +48,13 @@
         </span>
     </dt>
     <dt
- tal:condition="not: view/show_package_diffs_request_link">
+ tal:condition="python: not(view.show_package_diffs_request_link) and context.base_version">
       Differences from last common version:
     </dt>
- <dd>
+ <dd class="greyed-out" tal:condition="not: context/base_version">
+ Unknown
+ </dd>
+ <dd tal:condition="context/base_version">
       <ul class="package-diff-status">
         <tal:source-diff-option condition="view/display_child_diff">
           <li tal:condition="context/package_diff">

As you said, it could probably use a test to make sure things *are* displayed under the right conditions and not just *not* displayed as appropriate.

review: Needs Information

[j.c.sackett (jcsackett)]

This looks good to land. Thanks for adding the test.

review: Approve

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2011-04-06 07:34:18 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2011-04-08 06:51:07 +0000
@@ -178,6 +178,7 @@
         request link.
         """
         return (check_permission('launchpad.Edit', self.context) and
+                self.context.base_version and
                 (not self.context.package_diff or
                  not self.context.parent_package_diff))
 

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2011-04-05 13:18:48 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2011-04-08 06:51:07 +0000
@@ -9,7 +9,10 @@
 
 from BeautifulSoup import BeautifulSoup
 import soupmatchers
-from testtools.matchers import Not
+from testtools.matchers import (
+    MatchesAny,
+    Not,
+    )
 import transaction
 from zope.component import getUtility
 
@@ -39,6 +42,7 @@
     person_logged_in,
     TestCaseWithFactory,
     )
+from zope.security.proxy import removeSecurityProxy
 from lp.testing.views import create_initialized_view
 
 
@@ -261,7 +265,8 @@
     def test_both_request_diff_texts_rendered(self):
         # An unlinked description of a potential diff is displayed when
         # no diff is present.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
 
         view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
         # Both diffs present simple text repr. of proposed diff.
@@ -270,7 +275,8 @@
     def test_source_diff_rendering_diff(self):
         # A linked description of the diff is displayed when
         # it is present.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
 
         with person_logged_in(ds_diff.derived_series.owner):
             ds_diff.package_diff = self.factory.makePackageDiff()
@@ -286,7 +292,8 @@
     def test_source_diff_rendering_diff_no_link(self):
         # The status of the package is shown if the package diff is in a
         # PENDING or FAILED state.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
 
         statuses_and_classes = [
             (PackageDiffStatus.PENDING, 'PENDING'),
@@ -309,7 +316,8 @@
     def test_parent_source_diff_rendering_diff_no_link(self):
         # The status of the package is shown if the parent package diff is
         # in a PENDING or FAILED state.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
 
         statuses_and_classes = [
             (PackageDiffStatus.PENDING, 'PENDING'),
@@ -342,7 +350,8 @@
     def test_parent_source_diff_rendering_diff(self):
         # A linked description of the diff is displayed when
         # it is present.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
 
         with person_logged_in(ds_diff.derived_series.owner):
             ds_diff.parent_package_diff = self.factory.makePackageDiff()
@@ -425,7 +434,8 @@
     def test_package_diff_request_link(self):
         # The link to compute package diffs is only shown to
         # a user with lp.Edit persmission.
-        ds_diff = self.factory.makeDistroSeriesDifference()
+        ds_diff = self.factory.makeDistroSeriesDifference(
+            set_base_version=True)
         package_diff_request_matcher = soupmatchers.HTMLContains(
             soupmatchers.Tag(
                 'Request link', 'a',
@@ -443,3 +453,75 @@
                 ds_diff, '+listing-distroseries-extra')
             self.assertThat(view(), package_diff_request_matcher)
             self.assertTrue(view.show_package_diffs_request_link)
+
+    def test_package_diff_label(self):
+        # If base_version is not None the label for the section is
+        # there.
+        changelog_lfa = self.factory.makeChangelog('foo', ['0.30-1'])
+        parent_changelog_lfa = self.factory.makeChangelog(
+            'foo', ['0.32-1', '0.30-1'])
+        transaction.commit() # Yay, librarian.
+        ds_diff = self.factory.makeDistroSeriesDifference(versions={
+            'derived': '0.30-1',
+            'parent': '0.32-1',
+            }, changelogs={
+            'derived': changelog_lfa,
+            'parent': parent_changelog_lfa})
+        package_diff_header_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Package diffs header', 'dt',
+                text=re.compile(
+                    '\s*Differences from last common version:')))
+
+        with celebrity_logged_in('admin'):
+            ds_diff.parent_package_diff = self.factory.makePackageDiff()
+            ds_diff.package_diff = self.factory.makePackageDiff()
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra')
+            html = view()
+            self.assertThat(html, package_diff_header_matcher)
+
+    def test_package_diff_no_base_version(self):
+        # If diff's base_version is None packages diffs are not displayed
+        # and neither is the link to compute them.
+        versions={
+            'base': None, # No base version.
+            'derived': '0.1-1derived1',
+            'parent': '0.1-2'}
+        ds_diff = self.factory.makeDistroSeriesDifference(versions=versions)
+        package_diff_request_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Request link', 'a',
+                text=re.compile(
+                    '\s*Compute differences from last common version\s*')))
+
+        pending_package_diff_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Pending package diff', 'span',
+                attrs={'class': 'PENDING'}))
+
+        package_diff_header_matcher = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Package diffs header', 'dt',
+                text=re.compile(
+                    '\s*Differences from last common version:')))
+
+        unknown_base_version = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                'Unknown base version', 'dd',
+                text=re.compile(
+                    '\s*Unknown')))
+
+        with celebrity_logged_in('admin'):
+            view = create_initialized_view(
+                ds_diff, '+listing-distroseries-extra')
+            html = view()
+            self.assertFalse(view.show_package_diffs_request_link)
+            self.assertThat(html, unknown_base_version)
+            self.assertThat(
+                html,
+                Not(
+                    MatchesAny(
+                        package_diff_request_matcher,
+                        pending_package_diff_matcher,
+                        package_diff_header_matcher)))

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-04-01 15:18:38 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-04-08 06:51:07 +0000
@@ -39,7 +39,11 @@
       </ul></dd>
     <tal:package-diffs tal:condition="view/can_have_packages_diffs">
     <dt>Last common version:</dt>
-    <dd tal:content="context/base_version">1.2.1</dd>
+    <dd tal:condition="context/base_version"
+        tal:content="context/base_version">1.2.1</dd>
+    <dd tal:condition="not: context/base_version">
+      Unknown
+    </dd>
     <dt
       tal:condition="view/show_package_diffs_request_link"
       class="package-diff-placeholder">
@@ -49,10 +53,10 @@
         </span>
     </dt>
     <dt
-      tal:condition="not: view/show_package_diffs_request_link">
+      tal:condition="python: not(view.show_package_diffs_request_link) and context.base_version">
       Differences from last common version:
     </dt>
-    <dd>
+    <dd tal:condition="context/base_version">
       <ul class="package-diff-status">
         <tal:source-diff-option condition="view/display_child_diff">
           <li tal:condition="context/package_diff">

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-04-04 07:21:23 +0000
+++ lib/lp/testing/factory.py	2011-04-08 06:51:07 +0000
@@ -2304,7 +2304,8 @@
         versions=None,
         difference_type=DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
         status=DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
-        changelogs=None):
+        changelogs=None,
+        set_base_version=False):
         """Create a new distro series source package difference."""
         if derived_series is None:
             parent_series = self.makeDistroSeries()
@@ -2358,6 +2359,10 @@
 
         removeSecurityProxy(diff).status = status
 
+        if set_base_version:
+            version = versions.get('base', "%s.0" % self.getUniqueInteger())
+            removeSecurityProxy(diff).base_version = version
+
         # We clear the cache on the diff, returning the object as if it
         # was just loaded from the store.
         clear_property_cache(diff)