Merge lp:~jtv/launchpad/bug-827347 into lp:launchpad

[Jeroen T. Vermeulen]

[r=allenap][bug=827347] Link to +localpackagediffs from DistroSeries if there are any differences at all in it.

= Summary =

On the page for a derived distroseries, there's a link to the +localpackagediffs page. The latter page shows only DistroSeriesDifferences that need attention by default, but can also show them all.

Unfortunately the link only shows up if there are DSDs that need attention. If you want to have a look at resolved differences, say, well you could but there's not going to be any link to the page.

== Proposed fix ==

Link to +localpackagediffs (but one that shows all of the DSDs) when the series has any DSDs at all. Show the overall count of DSDs with that link. Add a parenthesized count of DSDs that need attention, if there are any.

The links and counts for the other two types of DSD (present in the parent but not in the derived series, or the other way around) stay essentially the same. A small difference is that the link no longer comprises the full phrase describing the differences; that was altogether too blue and made the separation between the adjacent +localpackagediffs links a bit hard to notice.

So now, the link goes "1234 packages <with differences> (60 _needing attention_)".

== Pre-implementation notes ==

Went through a bit of UI prototyping with Julian and Raphaël. Also discussed desired functionality.

== Implementation details ==

I moved some logic, including the construction of links and the formulation of plurals, from the TAL into the browser code. That also lets me test the details at a lower level, without needing to render the view and search the HTML or DOM.

Hopefully you won't find my method names too lofty: wordVersionDifferences, alludeToParent. I'm not doing that for the fun of it. These verbs really seemed to describe what I wanted to do more precisely.

You'll also notice a bit of unrelated cleanup where I replace DistroSeriesDifferenceStatus.items with None. That's because it's being passed to a method that now accepts None to mean "any status."

== Tests ==

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

== Demo and Q/A ==

Go over the Derived section of a bunch of derived DistroSeries. There should be no differences links where there are no differences. The two links for different versions should distinguish differences that need attention from differences overall, and either should appear only when they have something to display.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/distroseries-portlet-derivation.pt
  lib/lp/registry/browser/tests/test_distroseries.py
  lib/lp/registry/browser/distroseries.py

[Gavin Panella (allenap)]

review: Approve

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-08-16 20:35:27 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-08-18 12:34:26 +0000
@@ -101,6 +101,7 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.browser_helpers import get_plural_text
 from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
 from lp.services.worlddata.interfaces.country import ICountry
@@ -127,6 +128,11 @@
     ]
 
 
+def get_dsd_source():
+    """For convenience: the `IDistroSeriesDifferenceSource` utility."""
+    return getUtility(IDistroSeriesDifferenceSource)
+
+
 class DistroSeriesNavigation(GetitemNavigation, BugTargetTraversalMixin,
     StructuralSubscriptionTargetTraversalMixin):
 
@@ -400,6 +406,14 @@
                 return 'a parent series'
 
 
+def word_differences_count(count):
+    """Express "`count` differences" in words.
+
+    For example, "1 package", or "2 packages" and so on.
+    """
+    return get_plural_text(count, "%d package", "%d packages") % count
+
+
 class DistroSeriesView(LaunchpadView, MilestoneOverlayMixin,
                        DerivedDistroSeriesMixin):
 
@@ -463,29 +477,86 @@
     def milestone_batch_navigator(self):
         return BatchNavigator(self.context.all_milestones, self.request)
 
-    def _num_differences(self, difference_type):
-        differences = getUtility(
-            IDistroSeriesDifferenceSource).getForDistroSeries(
-                self.context,
-                difference_type=difference_type,
-                status=(DistroSeriesDifferenceStatus.NEEDS_ATTENTION,))
+    def countDifferences(self, difference_type, needing_attention_only=True):
+        """Count the number of differences of a given kind.
+
+        :param difference_type: Type of `DistroSeriesDifference` to look for.
+        :param needing_attention_only: Restrict count to differences that need
+            attention?  If not, count all that can be viewed.
+        """
+        if needing_attention_only:
+            status = (DistroSeriesDifferenceStatus.NEEDS_ATTENTION, )
+        else:
+            status = None
+
+        differences = get_dsd_source().getForDistroSeries(
+            self.context, difference_type=difference_type, status=status)
         return differences.count()
 
     @cachedproperty
-    def num_differences(self):
-        return self._num_differences(
+    def num_version_differences_needing_attention(self):
+        return self.countDifferences(
             DistroSeriesDifferenceType.DIFFERENT_VERSIONS)
 
     @cachedproperty
+    def num_version_differences(self):
+        return self.countDifferences(
+            DistroSeriesDifferenceType.DIFFERENT_VERSIONS,
+            needing_attention_only=False)
+
+    def wordVersionDifferences(self):
+        return word_differences_count(self.num_version_differences)
+
+    @cachedproperty
     def num_differences_in_parent(self):
-        return self._num_differences(
+        return self.countDifferences(
             DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES)
 
+    def wordDifferencesInParent(self):
+        return word_differences_count(self.num_differences_in_parent)
+
     @cachedproperty
     def num_differences_in_child(self):
-        return self._num_differences(
+        return self.countDifferences(
             DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES)
 
+    def wordDifferencesInChild(self):
+        return word_differences_count(self.num_differences_in_child)
+
+    def alludeToParent(self):
+        """Wording to refer to the series' parent(s).
+
+        If there is a single parent, returns its display name.  Otherwise
+        says "a parent series" (more vague, but we could also name parents
+        if there are very few).
+        """
+        if self.has_unique_parent:
+            return self.unique_parent.displayname
+        else:
+            return "a parent series"
+
+    @cachedproperty
+    def link_to_version_diffs_needing_attention(self):
+        """Return URL for +localpackagediffs page."""
+        return canonical_url(self.context, view_name='+localpackagediffs')
+
+    @property
+    def link_to_all_version_diffs(self):
+        """Return URL for +localdiffs page for all statuses."""
+        return (
+            "%s?field.package_type=all"
+            % self.link_to_version_diffs_needing_attention)
+
+    @property
+    def link_to_differences_in_parent(self):
+        """Return URL for +missingpackages page."""
+        return canonical_url(self.context, view_name='+missingpackages')
+
+    @property
+    def link_to_differences_in_child(self):
+        """Return URL for +uniquepackages page."""
+        return canonical_url(self.context, view_name='+uniquepackages')
+
 
 class DistroSeriesEditView(LaunchpadEditFormView, SeriesStatusMixin):
     """View class that lets you edit a DistroSeries object.
@@ -1090,12 +1161,11 @@
     def cached_differences(self):
         """Return a batch navigator of filtered results."""
         package_type_dsd_status = {
-            NON_IGNORED: (
-                DistroSeriesDifferenceStatus.NEEDS_ATTENTION,),
+            NON_IGNORED: DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             HIGHER_VERSION_THAN_PARENT: (
                 DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT),
             RESOLVED: DistroSeriesDifferenceStatus.RESOLVED,
-            ALL: DistroSeriesDifferenceStatus.items,
+            ALL: None,
         }
 
         # If the package_type option is not supported, add an error to
@@ -1107,13 +1177,12 @@
             status = package_type_dsd_status[self.specified_package_type]
             child_version_higher = (
                 self.specified_package_type == HIGHER_VERSION_THAN_PARENT)
-            differences = getUtility(
-                IDistroSeriesDifferenceSource).getForDistroSeries(
-                    self.context, difference_type=self.differences_type,
-                    name_filter=self.specified_name_filter,
-                    status=status, child_version_higher=child_version_higher,
-                    packagesets=self.specified_packagesets_filter,
-                    changed_by=self.specified_changed_by_filter)
+            differences = get_dsd_source().getForDistroSeries(
+                self.context, difference_type=self.differences_type,
+                name_filter=self.specified_name_filter, status=status,
+                child_version_higher=child_version_higher,
+                packagesets=self.specified_packagesets_filter,
+                changed_by=self.specified_changed_by_filter)
 
         return BatchNavigator(differences, self.request)
 
@@ -1185,8 +1254,7 @@
 
         :return: A result set of `DistroSeriesDifference`s.
         """
-        return getUtility(IDistroSeriesDifferenceSource).getSimpleUpgrades(
-            self.context)
+        return get_dsd_source().getSimpleUpgrades(self.context)
 
     @action(_("Upgrade Packages"), name="upgrade", condition='canUpgrade')
     def upgrade(self, action, data):

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-08-18 10:05:38 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-08-18 12:34:26 +0000
@@ -141,18 +141,55 @@
         view = create_initialized_view(distroseries, '+index')
         self.assertEqual(view.needs_linking, None)
 
-    def _createDifferenceAndGetView(self, difference_type):
+    def _createDifferenceAndGetView(self, difference_type, status=None):
+        if status is None:
+            status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
         # Helper function to create a valid DSD.
         dsp = self.factory.makeDistroSeriesParent()
         self.factory.makeDistroSeriesDifference(
             derived_series=dsp.derived_series,
-            difference_type=difference_type)
+            difference_type=difference_type, status=status)
         return create_initialized_view(dsp.derived_series, '+index')
 
-    def test_num_differences(self):
-        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
-        view = self._createDifferenceAndGetView(diff_type)
-        self.assertEqual(1, view.num_differences)
+    def test_num_version_differences_needing_attention(self):
+        # num_version_differences_needing_attention counts
+        # different-versions-type differences in needs-attention state.
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual(1, view.num_version_differences_needing_attention)
+
+    def test_num_version_differences_needing_attention_limits_type(self):
+        # num_version_differences_needing_attention ignores other types
+        # of difference.
+        diff_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual(0, view.num_version_differences_needing_attention)
+
+    def test_num_version_differences_needing_attention_limits_status(self):
+        # num_version_differences_needing_attention ignores differences
+        # that do not need attention.
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        view = self._createDifferenceAndGetView(
+            diff_type, status=DistroSeriesDifferenceStatus.RESOLVED)
+        self.assertEqual(0, view.num_version_differences_needing_attention)
+
+    def test_num_version_differences_counts_all_statuses(self):
+        # num_version_differences counts DIFFERENT_VERSIONS differences
+        # of all statuses.
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        series = self.factory.makeDistroSeriesParent().derived_series
+        dsds = [
+            self.factory.makeDistroSeriesDifference(
+                series, difference_type=diff_type, status=status)
+            for status in DistroSeriesDifferenceStatus.items]
+        view = create_initialized_view(series, '+index')
+        self.assertEqual(len(dsds), view.num_version_differences)
+
+    def test_num_version_differences_ignores_limits_type(self):
+        # num_version_differences ignores other types of difference.
+        diff_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual(0, view.num_version_differences)
 
     def test_num_differences_in_parent(self):
         diff_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
@@ -164,6 +201,56 @@
         view = self._createDifferenceAndGetView(diff_type)
         self.assertEqual(1, view.num_differences_in_child)
 
+    def test_wordVersionDifferences(self):
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual("1 package", view.wordVersionDifferences())
+
+    def test_wordDifferencesInParent(self):
+        diff_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual("1 package", view.wordDifferencesInParent())
+
+    def test_wordDifferencesInChild(self):
+        diff_type = DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        self.assertEqual("1 package", view.wordDifferencesInChild())
+
+    def test_alludeToParent_names_single_parent(self):
+        dsp = self.factory.makeDistroSeriesParent()
+        view = create_initialized_view(dsp.derived_series, '+index')
+        self.assertEqual(dsp.parent_series.displayname, view.alludeToParent())
+
+    def test_alludeToParent_refers_to_multiple_parents_collectively(self):
+        dsp = self.factory.makeDistroSeriesParent()
+        self.factory.makeDistroSeriesParent(derived_series=dsp.derived_series)
+        view = create_initialized_view(dsp.derived_series, '+index')
+        self.assertEqual("a parent series", view.alludeToParent())
+
+    def test_link_to_version_diffs_needing_attention(self):
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        view = self._createDifferenceAndGetView(diff_type)
+        link = view.link_to_version_diffs_needing_attention
+        self.assertThat(link, EndsWith('/+localpackagediffs'))
+
+    def test_link_to_all_version_diffs(self):
+        diff_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+        view = self._createDifferenceAndGetView(diff_type)
+        link = view.link_to_all_version_diffs
+        self.assertIn('/+localpackagediffs?', link)
+
+    def test_link_to_differences_in_parent(self):
+        diff_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        link = view.link_to_differences_in_parent
+        self.assertThat(link, EndsWith('/+missingpackages'))
+
+    def test_link_to_differences_in_child(self):
+        diff_type = DistroSeriesDifferenceType.UNIQUE_TO_DERIVED_SERIES
+        view = self._createDifferenceAndGetView(diff_type)
+        link = view.link_to_differences_in_child
+        self.assertThat(link, EndsWith('/+uniquepackages'))
+
 
 class DistroSeriesIndexFunctionalTestCase(TestCaseWithFactory):
     """Test the distroseries +index page."""
@@ -236,19 +323,7 @@
         portlet_display = soupmatchers.HTMLContains(
             soupmatchers.Tag(
                 'Derivation portlet header', 'h2',
-                text='Derived from Sid'),
-            soupmatchers.Tag(
-                'Differences link', 'a',
-                text=re.compile('\s*1 package with differences\s*'),
-                attrs={'href': re.compile('.*/\+localpackagediffs')}),
-            soupmatchers.Tag(
-                'Parent diffs link', 'a',
-                text=re.compile('\s*2 packages only in Sid\s*'),
-                attrs={'href': re.compile('.*/\+missingpackages')}),
-            soupmatchers.Tag(
-                'Child diffs link', 'a',
-                text=re.compile('\s*3 packages only in Deri\s*'),
-                attrs={'href': re.compile('.*/\+uniquepackages')}))
+                text='Derived from Sid'))
 
         with person_logged_in(self.simple_user):
             view = create_initialized_view(
@@ -268,14 +343,9 @@
         set_derived_series_ui_feature_flag(self)
         derived_series = self._setupDifferences(
             'deri', ['sid1', 'sid2'], 0, 1, 0)
-        portlet_display = soupmatchers.HTMLContains(
-            soupmatchers.Tag(
-                'Derivation portlet header', 'h2',
-                text='Derived from 2 parents'),
-            soupmatchers.Tag(
-                'Parent diffs link', 'a',
-                text=re.compile('\s*1 package only in a parent series\s*'),
-                attrs={'href': re.compile('.*/\+missingpackages')}))
+        portlet_display = soupmatchers.HTMLContains(soupmatchers.Tag(
+            'Derivation portlet header', 'h2',
+            text='Derived from 2 parents'))
 
         with person_logged_in(self.simple_user):
             view = create_initialized_view(

=== modified file 'lib/lp/registry/templates/distroseries-portlet-derivation.pt'
--- lib/lp/registry/templates/distroseries-portlet-derivation.pt	2011-07-06 16:19:18 +0000
+++ lib/lp/registry/templates/distroseries-portlet-derivation.pt	2011-08-18 12:34:26 +0000
@@ -13,44 +13,46 @@
       <h2>Derived from <tal:name replace="view/number_of_parents"/> parents</h2>
     </tal:multiple_parents>
 
-      <tal:diffs define="nb_diffs view/num_differences;
+      <tal:diffs define="nb_diffs view/num_version_differences;
                          nb_diffs_in_parent view/num_differences_in_parent;
                          nb_diffs_in_child view/num_differences_in_child;">
         <ul id="derivation_stats">
-          <li tal:condition="nb_diffs">
-            <a tal:attributes="href string:${context/fmt:url}/+localpackagediffs"
-               class="sprite info"
-               title="Source package differences between this series and his parent(s)">
-              <tal:nb_diffs replace="nb_diffs"/> package<tal:plural
-                content="string:s"
-                condition="python:nb_diffs!=1"/> with differences
-            </a>
-          </li>
-         <li tal:condition="nb_diffs_in_parent">
-            <a tal:attributes="href string:${context/fmt:url}/+missingpackages"
-               class="sprite info"
-               title="Source packages only in parent series">
-              <tal:one_parent condition="view/has_unique_parent">
-                <tal:nb_diffs replace="nb_diffs_in_parent"/> package<tal:plural
-                  content="string:s"
-                  condition="python:nb_diffs_in_parent!=1"/> only in <tal:replace
-                    replace="view/unique_parent/displayname">Sid</tal:replace>
-              </tal:one_parent>
-              <tal:multiple_parents condition="not: view/has_unique_parent">
-                <tal:nb_diffs replace="nb_diffs_in_parent"/> package<tal:plural
-                  content="string:s"
-                  condition="python:nb_diffs_in_parent!=1"/> only in a parent series
-               </tal:multiple_parents>
-            </a>
-          </li>
-          <li tal:condition="nb_diffs_in_child">
-            <a tal:attributes="href string:${context/fmt:url}/+uniquepackages"
-               class="sprite info"
-               title="Source packages only in derived series">
-              <tal:nb_diffs replace="nb_diffs_in_child"/> package<tal:plural
-                content="string:s"
-                condition="python:nb_diffs_in_child!=1"/> only in <tal:replace
-                  replace="context/displayname">Natty</tal:replace>
+          <li class="sprite info" tal:condition="nb_diffs">
+            <tal:differences_count replace="view/wordVersionDifferences">
+              123 packages
+            </tal:differences_count>
+            <a tal:attributes="href view/link_to_all_version_diffs">
+              with differences
+            </a>
+            <tal:needing_attention
+                condition="view/num_version_differences_needing_attention">
+              (<tal:differences_count
+                  replace="view/num_version_differences_needing_attention">9
+               </tal:differences_count>
+               <a tal:attributes="href view/link_to_version_diffs_needing_attention">
+                 needing attention</a>)
+            </tal:needing_attention>
+          </li>
+         <li class="sprite info" tal:condition="nb_diffs_in_parent">
+            <tal:differences_count replace="view/wordDifferencesInParent">
+              234 packages
+            </tal:differences_count>
+            <a tal:attributes="href view/link_to_differences_in_parent">
+              only in
+              <tal:parent replace="view/alludeToParent">
+                a parent series
+              </tal:parent>
+            </a>
+          </li>
+          <li class="sprite info" tal:condition="nb_diffs_in_child">
+            <a tal:attributes="href view/link_to_differences_in_child">
+              <tal:differences_count replace="view/wordDifferencesInChild">
+                345 packages
+              </tal:differences_count>
+              only in
+              <tal:child replace="context/displayname">
+                Natty
+              </tal:child>
             </a>
           </li>
          </ul>