launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/more-820456 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #820456 in Launchpad itself: "Initializing a new distroseries from parents doesn't always show its "resolved differences""
  https://bugs.launchpad.net/launchpad/+bug/820456

For more details, see:
https://code.launchpad.net/~jtv/launchpad/more-820456/+merge/71689

= Summary =

If a distroseries shares packages with its parents, and any of those packages has a version difference between the series and its parent that needs attention, the series page will link to DistroSeries:+localpackagediffs.  That page shows a search box that lets you, for instance, show resolved differences, show deliberately hidden differences, and so on.  The default (naturally) is to show differences that need attention.

This search box tries to stay hidden if there are no interesting differences.  But that happens when there are no differences except ones that are resolved or permanently-blacklisted: circumstances under which the link to the page isn't shown in the first place!  Except in the case of resolved differences, you could see the differences if only the search box would appear.  You _can_ see them as long as there's just one non-resolved difference to make the search box come out.


== Proposed fix ==

Don't hide the search box.  There's no point.


== Pre-implementation notes ==

There are other problems: this still won't make the link to the +localpackagediffs appear in cases where there are no differences that need attention.  We filed that as bug 827347; it can be done separately and requires a bit of UI design.


== Implementation details ==

Of all my Launchpad branches, this one's been the most fun ever.


== Tests ==

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


== Demo and Q/A ==

Hack your way into a DistroSeries:+localpackagediffs page when there are no differences.  The search box will appear.

Or: initialize a distroseries from a parent, run ISDJs/PCJs/DSDJs, and hack your way to the +localpackagediffs page.  You'll be able to show resolved differences.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/distroseries-localdifferences.pt
  lib/lp/registry/browser/tests/test_distroseries.py
  lib/lp/registry/browser/distroseries.py
-- 
https://code.launchpad.net/~jtv/launchpad/more-820456/+merge/71689
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/more-820456 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-08-10 17:00:16 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-08-16 13:38:27 +0000
@@ -1117,27 +1117,6 @@
 
         return BatchNavigator(differences, self.request)
 
-    @cachedproperty
-    def has_differences(self):
-        """Whether or not differences between this derived series and
-        its parent exist.
-        """
-        # Performance optimisation: save a query if we have differences
-        # to show in the batch.
-        if self.cached_differences.batch.total() > 0:
-            return True
-        else:
-            # Here we check the whole dataset since the empty batch
-            # might be filtered.
-            differences = getUtility(
-                IDistroSeriesDifferenceSource).getForDistroSeries(
-                    self.context,
-                    difference_type=self.differences_type,
-                    status=(
-                        DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
-                        DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT))
-            return not differences.is_empty()
-
     def parent_changelog_url(self, distroseriesdifference):
         """The URL to the /parent/series/+source/package/+changelog """
         distro = distroseriesdifference.parent_series.distribution

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-08-10 15:40:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-08-16 13:38:27 +0000
@@ -1042,22 +1042,6 @@
             find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
             "Form filter should be shown when there are differences.")
 
-    def test_filter_noform_if_nodifferences(self):
-        # Test that the page doesn't includes the filter form if no
-        # differences are present
-        simple_user = self.factory.makePerson()
-        login_person(simple_user)
-        derived_series, parent_series = self._createChildAndParent()
-
-        set_derived_series_ui_feature_flag(self)
-        view = create_initialized_view(
-            derived_series, '+localpackagediffs', principal=simple_user)
-
-        self.assertIs(
-            None,
-            find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
-            "Form filter should not be shown when there are no differences.")
-
     def test_parent_packagesets_localpackagediffs(self):
         # +localpackagediffs displays the packagesets.
         ds_diff = self.factory.makeDistroSeriesDifference()
@@ -1938,7 +1922,7 @@
         pu = self.factory.makePackageUpload(distroseries=dsd.derived_series)
         # A copy job with an attached packageupload means the job is
         # waiting in the queues.
-        removeSecurityProxy(pu).package_copy_job=pcj.id
+        removeSecurityProxy(pu).package_copy_job = pcj.id
         view.pending_syncs = {dsd.source_package_name.name: pcj}
         expected = (
             'waiting in <a href="%s/+queue?queue_state=%s">%s</a>&hellip;'

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-08-11 11:11:54 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-08-16 13:38:27 +0000
@@ -29,11 +29,8 @@
                   can_perform_sync view/canPerformSync;">
       <p><tal:replace replace="structure view/explanation/escapedtext" /></p>
 
-      <tal:distroseries_localdiff_search_form
-        tal:condition="view/has_differences">
-        <metal:package_filter_form
-          use-macro="context/@@+macros/distroseries-localdiff-search-form" />
-      </tal:distroseries_localdiff_search_form>
+      <metal:package_filter_form
+	use-macro="context/@@+macros/distroseries-localdiff-search-form" />
 
 
       <div metal:use-macro="context/@@launchpad_form/form">