launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/publishinghistory-show-copier into lp:launchpad.

Requested reviews:
  William Grant (wgrant): code
  Steve Kowalik (stevenk): code
Related bugs:
  Bug #1032857 in Launchpad itself: "DistributionSourcePackage:+publishinghistory should show the audit trail of copies"
  https://bugs.launchpad.net/launchpad/+bug/1032857

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/publishinghistory-show-copier/+merge/118223

== Summary ==

There's no way to see who copied a package in the +publishinghistory UI; you have to wheel out the API and look at SPPH.creator.  This is more cumbersome than it should be.

== Proposed fix ==

Add "by [creator]" to the template if we know who that is.

== LOC Rationale ==

+5.  I have 3911 lines of credit.

== Tests ==

bin/test -vvct soyuz/stories/ppa/xx-copy-packages.txt

== Demo and Q/A ==

Find a package that's been copied using a PCJ - the auto-syncer is a good source of these on old DB snapshots such as qastaging, so try https://qastaging.launchpad.net/ubuntu/+source/man-db/+publishinghistory - and look for the new text.
-- 
https://code.launchpad.net/~cjwatson/launchpad/publishinghistory-show-copier/+merge/118223
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py	2012-08-07 02:31:56 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py	2012-08-09 00:49:24 +0000
@@ -598,6 +598,16 @@
 
     page_title = 'Publishing history'
 
+    def initialize(self):
+        """Preload relevant `IPerson` objects."""
+        ids = set()
+        for spph in self.context.publishing_history:
+            ids.update((spph.removed_byID, spph.creatorID, spph.sponsorID))
+        ids.discard(None)
+        if ids:
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                ids, need_validity=True))
+
     @property
     def label(self):
         return 'Publishing history of %s' % self.context.title

=== modified file 'lib/lp/registry/browser/tests/test_distributionsourcepackage.py'
--- lib/lp/registry/browser/tests/test_distributionsourcepackage.py	2012-08-07 00:01:04 +0000
+++ lib/lp/registry/browser/tests/test_distributionsourcepackage.py	2012-08-09 00:49:24 +0000
@@ -1,10 +1,13 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test distributionsourcepackage views."""
 
 __metaclass__ = type
 
+import re
+
+import soupmatchers
 from zope.component import getUtility
 
 from lp.app.enums import ServiceUsage
@@ -12,6 +15,7 @@
 from lp.soyuz.interfaces.archive import ArchivePurpose
 from lp.testing import (
     celebrity_logged_in,
+    person_logged_in,
     test_tales,
     TestCaseWithFactory,
     )
@@ -20,7 +24,10 @@
     LaunchpadFunctionalLayer,
     )
 from lp.testing.matchers import BrowsesWithQueryLimit
-from lp.testing.views import create_view
+from lp.testing.views import (
+    create_initialized_view,
+    create_view,
+    )
 
 
 class TestDistributionSourcePackageFormatterAPI(TestCaseWithFactory):
@@ -57,6 +64,65 @@
         self.assertThat(dsp, changelog_browses_under_limit)
 
 
+class TestDistributionSourcePackagePublishingHistoryView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_publishinghistory_query_count(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            archive=archive)
+        spn = spph.sourcepackagerelease.sourcepackagename
+        dsp = archive.distribution.getSourcePackage(spn)
+        publishinghistory_browses_under_limit = BrowsesWithQueryLimit(
+            27, self.factory.makePerson(), "+publishinghistory")
+        self.assertThat(dsp, publishinghistory_browses_under_limit)
+        with person_logged_in(archive.owner):
+            copy_source_archive = self.factory.makeArchive()
+            copy_spph = self.factory.makeSourcePackagePublishingHistory(
+                archive=copy_source_archive, sourcepackagename=spn)
+            copy_spph.copyTo(
+                spph.distroseries, spph.pocket, archive,
+                creator=self.factory.makePerson(),
+                sponsor=self.factory.makePerson())
+            delete_spph = self.factory.makeSourcePackagePublishingHistory(
+                archive=archive, sourcepackagename=spn)
+            delete_spph.requestDeletion(self.factory.makePerson())
+        # This is a lot of extra queries per publication, and should be
+        # ratcheted down over time; but it at least ensures that we don't
+        # make matters any worse.
+        publishinghistory_browses_under_limit.query_limit += 10
+        self.assertThat(dsp, publishinghistory_browses_under_limit)
+
+    def test_show_sponsor(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        ppa = self.factory.makeArchive()
+        spph = self.factory.makeSourcePackagePublishingHistory(archive=ppa)
+        creator = self.factory.makePerson()
+        sponsor = self.factory.makePerson()
+        copied_spph = spph.copyTo(
+            spph.distroseries, spph.pocket, archive, creator=creator,
+            sponsor=sponsor)
+        html = create_initialized_view(copied_spph, "+record-details").render()
+        record_matches = soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                "copy summary", "li", text=re.compile("sponsored by")),
+            soupmatchers.Tag(
+                "copy creator", "a", text=creator.displayname,
+                attrs={
+                    "href": "/~%s" % creator.name,
+                    "class": "sprite person",
+                    }),
+            soupmatchers.Tag(
+                "copy sponsor", "a", text=sponsor.displayname,
+                attrs={
+                    "href": "/~%s" % sponsor.name,
+                    "class": "sprite person",
+                    }),
+            )
+        self.assertThat(html, record_matches)
+
+
 class TestDistributionSourceView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2012-06-19 17:10:12 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2012-08-09 00:49:24 +0000
@@ -429,6 +429,7 @@
             required=False, readonly=False,
             ),
         exported_as="date_removed")
+    removed_byID = Attribute("DB ID for removed_by.")
     removed_by = exported(
         Reference(
             IPerson,
@@ -500,6 +501,7 @@
         description=_('The previous release of this source package.'),
         required=False, readonly=True)
 
+    creatorID = Attribute("DB ID for creator.")
     creator = exported(
         Reference(
             IPerson,
@@ -508,6 +510,7 @@
             required=False, readonly=True
         ))
 
+    sponsorID = Attribute("DB ID for sponsor.")
     sponsor = exported(
         Reference(
             IPerson,

=== modified file 'lib/lp/soyuz/stories/ppa/xx-copy-packages.txt'
--- lib/lp/soyuz/stories/ppa/xx-copy-packages.txt	2012-08-01 11:02:13 +0000
+++ lib/lp/soyuz/stories/ppa/xx-copy-packages.txt	2012-08-09 00:49:24 +0000
@@ -272,7 +272,8 @@
     >>> jblack_extra_browser.open(expander_url)
     >>> print extract_text(jblack_extra_browser.contents)
     Publishing details
-      Copied from ubuntu hoary in Primary Archive for Ubuntu Linux
+      Copied from ubuntu hoary in Primary Archive for Ubuntu Linux by James
+      Blackwell
     Changelog
       pmount (0.1-1) hoary; urgency=low
       * Fix description (Malone #1)

=== modified file 'lib/lp/soyuz/templates/packagepublishing-details.pt'
--- lib/lp/soyuz/templates/packagepublishing-details.pt	2012-02-10 10:14:20 +0000
+++ lib/lp/soyuz/templates/packagepublishing-details.pt	2012-08-09 00:49:24 +0000
@@ -41,8 +41,7 @@
           define="linkify_archive view/linkify_source_archive;
                   source context/sourcepackagerelease">
           <tal:message
-            define="archive source/upload_archive;
-                    series source/upload_distroseries;
+            define="series source/upload_distroseries;
                     distro series/distribution;
                     message string:${distro/name} ${series/name} in "
             replace="message" />
@@ -55,6 +54,12 @@
                     message string:${archive/displayname}"
             replace="message" />
         </tal:define>
+        <tal:source_creator condition="context/creator">
+          by <a tal:replace="structure context/creator/fmt:link"/>
+        </tal:source_creator>
+        <tal:source_sponsor condition="context/sponsor">
+          (sponsored by <a tal:replace="structure context/sponsor/fmt:link"/>)
+        </tal:source_sponsor>
       </tal:source_original_location>
       <tal:binary_build_location condition="view/is_binary">
         <tal:message