Merge lp:~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865 into lp:launchpad

[Gavin Panella]

[r=henninge][bug=798865] Show the package changer in DistroSeries:+localpackagediffs and co. as well as the uploader.

This changes the "Last changed by" column to "Last changed" because it shows a date too (it already did that before this branch). It also shows the in that column the person who made the package change instead of the person who uploaded it. In many cases these people are different. It does, however, show the uploader in smaller text.

Screenshot:
  http://people.canonical.com/~gavin/ui/localpackagediffs-show-changed-by-bug-798865/last-changed.png

It also merges the tests in TestDistroSeriesLocalDifferences and TestDistroSeriesLocalDifferencesZopeless and calls the resulting class TestDistroSeriesLocalDifferences.

[Henning Eggers (henninge)]

Thank you for this improvement.
As discussed on IRC, please make the tests in the two implementations of test_diff_row_shows_spr_creator work in the same way.

review: Approve

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2011-07-04 13:24:48 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2011-07-25 11:57:37 +0000
@@ -306,6 +306,9 @@
     min-width: 1em;
     outline: 1px #666;
     }
+.nowrap {
+    white-space: nowrap;
+    }
 
 /* =========================
    Universal presentation

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-07-21 09:27:45 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-07-25 11:57:37 +0000
@@ -91,6 +91,7 @@
     login,
     login_celebrity,
     login_person,
+    normalize_whitespace,
     person_logged_in,
     StormStatementRecorder,
     TestCaseWithFactory,
@@ -857,93 +858,6 @@
         return (derived_series, parent_series)
 
 
-class TestDistroSeriesLocalDifferences(
-    DistroSeriesDifferenceMixin, TestCaseWithFactory):
-    """Test the distroseries +localpackagediffs page."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        super(TestDistroSeriesLocalDifferences,
-              self).setUp('[e-mail]')
-        set_derived_series_ui_feature_flag(self)
-        self.simple_user = self.factory.makePerson()
-
-    def test_filter_form_if_differences(self):
-        # Test that the page includes the filter form if differences
-        # are present
-        login_person(self.simple_user)
-        derived_series, parent_series = self._createChildAndParent()
-        self.factory.makeDistroSeriesDifference(
-            derived_series=derived_series)
-
-        view = create_initialized_view(
-            derived_series, '+localpackagediffs', principal=self.simple_user)
-
-        self.assertIsNot(
-            None,
-            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
-        login_person(self.simple_user)
-        derived_series, parent_series = self._createChildAndParent()
-
-        view = create_initialized_view(
-            derived_series, '+localpackagediffs', principal=self.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()
-        with celebrity_logged_in('admin'):
-            ps = self.factory.makePackageset(
-                packages=[ds_diff.source_package_name],
-                distroseries=ds_diff.derived_series)
-
-        with person_logged_in(self.simple_user):
-            view = create_initialized_view(
-                ds_diff.derived_series,
-                '+localpackagediffs',
-                principal=self.simple_user)
-            html_content = view()
-
-        packageset_text = re.compile('\s*' + ps.name)
-        self._test_packagesets(
-            html_content, packageset_text, 'packagesets',
-            'Packagesets')
-
-    def test_parent_packagesets_localpackagediffs_sorts(self):
-        # Multiple packagesets are sorted in a comma separated list.
-        ds_diff = self.factory.makeDistroSeriesDifference()
-        unsorted_names = [u"zzz", u"aaa"]
-        with celebrity_logged_in('admin'):
-            for name in unsorted_names:
-                self.factory.makePackageset(
-                    name=name,
-                    packages=[ds_diff.source_package_name],
-                    distroseries=ds_diff.derived_series)
-
-        with person_logged_in(self.simple_user):
-            view = create_initialized_view(
-                ds_diff.derived_series,
-                '+localpackagediffs',
-                principal=self.simple_user)
-            html_content = view()
-
-        packageset_text = re.compile(
-            '\s*' + ', '.join(sorted(unsorted_names)))
-        self._test_packagesets(
-            html_content, packageset_text, 'packagesets',
-            'Packagesets')
-
-
 class TestDistroSeriesLocalDiffPerformance(TestCaseWithFactory,
                                            DistroSeriesDifferenceMixin):
     """Test the distroseries +localpackagediffs page's performance."""
@@ -1076,8 +990,8 @@
         self._assertQueryCount(derived_series)
 
 
-class TestDistroSeriesLocalDifferencesZopeless(TestCaseWithFactory,
-                                               DistroSeriesDifferenceMixin):
+class TestDistroSeriesLocalDifferences(TestCaseWithFactory,
+                                       DistroSeriesDifferenceMixin):
     """Test the distroseries +localpackagediffs view."""
 
     layer = LaunchpadFunctionalLayer
@@ -1107,6 +1021,88 @@
             principal=get_current_principal(),
             current_request=True)
 
+    def test_filter_form_if_differences(self):
+        # Test that the page includes the filter form if differences
+        # are present
+        simple_user = self.factory.makePerson()
+        login_person(simple_user)
+        derived_series, parent_series = self._createChildAndParent()
+        self.factory.makeDistroSeriesDifference(
+            derived_series=derived_series)
+
+        set_derived_series_ui_feature_flag(self)
+        view = create_initialized_view(
+            derived_series, '+localpackagediffs', principal=simple_user)
+
+        self.assertIsNot(
+            None,
+            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()
+        with celebrity_logged_in('admin'):
+            ps = self.factory.makePackageset(
+                packages=[ds_diff.source_package_name],
+                distroseries=ds_diff.derived_series)
+
+        set_derived_series_ui_feature_flag(self)
+        simple_user = self.factory.makePerson()
+        with person_logged_in(simple_user):
+            view = create_initialized_view(
+                ds_diff.derived_series,
+                '+localpackagediffs',
+                principal=simple_user)
+            html_content = view()
+
+        packageset_text = re.compile('\s*' + ps.name)
+        self._test_packagesets(
+            html_content, packageset_text, 'packagesets',
+            'Packagesets')
+
+    def test_parent_packagesets_localpackagediffs_sorts(self):
+        # Multiple packagesets are sorted in a comma separated list.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        unsorted_names = [u"zzz", u"aaa"]
+        with celebrity_logged_in('admin'):
+            for name in unsorted_names:
+                self.factory.makePackageset(
+                    name=name,
+                    packages=[ds_diff.source_package_name],
+                    distroseries=ds_diff.derived_series)
+
+        set_derived_series_ui_feature_flag(self)
+        simple_user = self.factory.makePerson()
+        with person_logged_in(simple_user):
+            view = create_initialized_view(
+                ds_diff.derived_series,
+                '+localpackagediffs',
+                principal=simple_user)
+            html_content = view()
+
+        packageset_text = re.compile(
+            '\s*' + ', '.join(sorted(unsorted_names)))
+        self._test_packagesets(
+            html_content, packageset_text, 'packagesets',
+            'Packagesets')
+
     def test_view_redirects_without_feature_flag(self):
         # If the feature flag soyuz.derived_series_ui.enabled is not set the
         # view simply redirects to the derived series.
@@ -1349,6 +1345,38 @@
         self.assertEqual(versions['derived'], derived_span[0].string.strip())
         self.assertEqual(versions['parent'], parent_span[0].string.strip())
 
+    def test_diff_row_last_changed(self):
+        # The SPR creator (i.e. who make the package change, rather than the
+        # uploader) is shown on each difference row.
+        set_derived_series_ui_feature_flag(self)
+        dsd = self.makePackageUpgrade()
+        view = self.makeView(dsd.derived_series)
+        root = html.fromstring(view())
+        [creator_cell] = root.cssselect(
+            "table.listing tbody td.last-changed")
+        self.assertEqual(
+            "a moment ago by %s" % (
+                dsd.source_package_release.creator.displayname,),
+            normalize_whitespace(creator_cell.text_content()))
+
+    def test_diff_row_last_changed_also_shows_uploader_if_different(self):
+        # When the SPR creator and uploader are different both are named on
+        # each difference row.
+        set_derived_series_ui_feature_flag(self)
+        dsd = self.makePackageUpgrade()
+        uploader = self.factory.makePerson()
+        removeSecurityProxy(dsd.source_package_release).dscsigningkey = (
+            self.factory.makeGPGKey(uploader))
+        view = self.makeView(dsd.derived_series)
+        root = html.fromstring(view())
+        [creator_cell] = root.cssselect(
+            "table.listing tbody td.last-changed")
+        self.assertEqual(
+            "a moment ago by %s (uploaded by %s)" % (
+                dsd.source_package_release.creator.displayname,
+                dsd.source_package_release.dscsigningkey.owner.displayname),
+            normalize_whitespace(creator_cell.text_content()))
+
     def test_getUpgrades_shows_updates_in_parent(self):
         # The view's getUpgrades methods lists packages that can be
         # trivially upgraded: changed in the parent, not changed in the
@@ -1477,12 +1505,6 @@
             recorder2,
             HasQueryCount(Equals(recorder1.count)))
 
-
-class TestDistroSeriesLocalDifferencesFunctional(TestCaseWithFactory,
-                                                 DistroSeriesDifferenceMixin):
-
-    layer = LaunchpadFunctionalLayer
-
     def makeDSDJob(self, dsd):
         """Create a `DistroSeriesDifferenceJob` to update `dsd`."""
         job_source = getUtility(IDistroSeriesDifferenceJobSource)
@@ -2213,7 +2235,7 @@
                                               DistroSeriesDifferenceMixin):
     """Test the distroseries +missingpackages page."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         super(DistroSeriesMissingPackagesPageTestCase,
@@ -2244,6 +2266,47 @@
             html_content, packageset_text, 'parent-packagesets',
             'Parent packagesets')
 
+    def test_diff_row_last_changed(self):
+        # The parent SPR creator (i.e. who make the package change, rather
+        # than the uploader) is shown on each difference row.
+        missing_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        dsd = self.factory.makeDistroSeriesDifference(
+            difference_type=missing_type)
+        with person_logged_in(self.simple_user):
+            view = create_initialized_view(
+                dsd.derived_series, '+missingpackages',
+                principal=self.simple_user)
+            root = html.fromstring(view())
+        [creator_cell] = root.cssselect(
+            "table.listing tbody td.last-changed")
+        self.assertEqual(
+            "a moment ago by %s" % (
+                dsd.parent_source_package_release.creator.displayname,),
+            normalize_whitespace(creator_cell.text_content()))
+
+    def test_diff_row_last_changed_also_shows_uploader_if_different(self):
+        # When the SPR creator and uploader are different both are named on
+        # each difference row.
+        missing_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+        dsd = self.factory.makeDistroSeriesDifference(
+            difference_type=missing_type)
+        uploader = self.factory.makePerson()
+        naked_spr = removeSecurityProxy(dsd.parent_source_package_release)
+        naked_spr.dscsigningkey = self.factory.makeGPGKey(uploader)
+        with person_logged_in(self.simple_user):
+            view = create_initialized_view(
+                dsd.derived_series, '+missingpackages',
+                principal=self.simple_user)
+            root = html.fromstring(view())
+        [creator_cell] = root.cssselect(
+            "table.listing tbody td.last-changed")
+        parent_spr = dsd.parent_source_package_release
+        self.assertEqual(
+            "a moment ago by %s (uploaded by %s)" % (
+                parent_spr.creator.displayname,
+                parent_spr.dscsigningkey.owner.displayname),
+            normalize_whitespace(creator_cell.text_content()))
+
 
 class DistroSerieUniquePackageDiffsTestCase(TestCaseWithFactory,
                                             DistroSeriesDifferenceMixin):

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-07-08 09:06:24 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-07-25 11:57:37 +0000
@@ -481,11 +481,13 @@
             gpgkeys = bulk.load_related(GPGKey, sprs, ("dscsigningkeyID",))
 
             # Load DistroSeriesDifferenceComment owners,
-            # SourcePackageRecipeBuild requesters and GPGKey owners.
+            # SourcePackageRecipeBuild requesters and GPGKey owners, and
+            # SourcePackageRelease creators.
             person_ids = set().union(
                 (dsdc.message.ownerID for dsdc in latest_comments),
                 (sprb.requester_id for sprb in sprbs),
-                (gpgkey.ownerID for gpgkey in gpgkeys))
+                (gpgkey.ownerID for gpgkey in gpgkeys),
+                (spr.creatorID for spr in sprs))
             uploaders = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
                 person_ids, need_validity=True)
             list(uploaders)

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-07-20 16:59:53 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-07-25 11:57:37 +0000
@@ -69,7 +69,7 @@
                   class="package-sets">
                 Package-sets
               </th>
-               <th>Last uploaded</th>
+              <th>Last changed</th>
               <th>Latest comment</th>
             </tr>
           </thead>
@@ -145,26 +145,42 @@
                 <tal:replace replace="difference/@@/packagesets_names" />
               </td>
 
-              <td>
+              <td class="last-changed">
                 <tal:parent condition="not: view/show_derived_version">
                   <tal:published condition="parent_source_pub">
                     <span tal:attributes="title parent_source_pub/datepublished/fmt:datetime"
                           tal:content="parent_source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
-                    <tal:signer condition="parent_source_pub/sourcepackagerelease/uploader">
-                      by <a tal:replace="structure parent_source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
-                    </tal:signer>
+                    <tal:creator define="spr parent_source_pub/sourcepackagerelease">
+                      <span class="nowrap">by <a tal:replace="structure spr/creator/fmt:link" /></span>
+                    </tal:creator>
+                    <tal:uploader
+                        define="spr parent_source_pub/sourcepackagerelease"
+                        condition="python: spr.uploader not in (None, spr.creator)">
+                      <br />
+                      <span class="discreet nowrap">
+                        (uploaded by <a tal:replace="structure spr/uploader/fmt:link" />)
+                      </span>
+                    </tal:uploader>
                   </tal:published>
                   <tal:not-published condition="not:parent_source_pub">
                     <span tal:content="difference/parent_source_version" />
                   </tal:not-published>
                 </tal:parent>
                 <tal:derived condition="view/show_derived_version">
-                  <tal:published condition="parent_source_pub">
+                  <tal:published condition="source_pub">
                     <span tal:attributes="title source_pub/datepublished/fmt:datetime"
                           tal:content="source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
-                    <tal:signer condition="source_pub/sourcepackagerelease/uploader">
-                      by <a tal:replace="structure source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
-                    </tal:signer>
+                    <tal:creator define="spr source_pub/sourcepackagerelease">
+                      <span class="nowrap">by <a tal:replace="structure spr/creator/fmt:link" /></span>
+                    </tal:creator>
+                    <tal:uploader
+                        define="spr source_pub/sourcepackagerelease"
+                        condition="python: spr.uploader not in (None, spr.creator)">
+                      <br />
+                      <span class="discreet nowrap">
+                        (uploaded by <a tal:replace="structure spr/uploader/fmt:link" />)
+                      </span>
+                    </tal:uploader>
                   </tal:published>
                   <tal:not-published condition="not:source_pub">
                     <span tal:content="difference/source_version" />

=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py	2011-06-16 11:31:24 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py	2011-07-25 11:57:37 +0000
@@ -47,7 +47,6 @@
     LibraryFileContent,
     )
 from canonical.launchpad.helpers import shortlist
-from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archiveuploader.utils import determine_source_file_type
 from lp.buildmaster.enums import BuildStatus
@@ -61,10 +60,7 @@
     PackageDiffStatus,
     PackagePublishingStatus,
     )
-from lp.soyuz.interfaces.archive import (
-    IArchiveSet,
-    MAIN_ARCHIVE_PURPOSES,
-    )
+from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.packagediff import PackageDiffAlreadyRequested
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
@@ -574,7 +570,7 @@
 
         queue = getUtility(ITranslationImportQueue)
 
-        only_templates=self.sourcepackage.has_sharing_translation_templates
+        only_templates = self.sourcepackage.has_sharing_translation_templates
         queue.addOrUpdateEntriesFromTarball(
             tarball, by_maintainer, importer,
             sourcepackagename=self.sourcepackagename,