Merge lp:~cjwatson/launchpad/publishinghistory-show-copier into lp:launchpad

[Colin Watson]

Show the person who copied a source package, if available, in the publishing history.

== 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.

[Steve Kowalik (stevenk)]

review: Approve (code)

[William Grant (wgrant)]

As discussed on IRC, this requires preloading of the people to avoid timeouts.

review: Needs Fixing (code)

[Colin Watson (cjwatson)]

The IRC discussion in question was:

  http://irclogs.ubuntu.com/2012/08/04/%23ubuntu-release.html

[Colin Watson (cjwatson)]

I think I've addressed almost all of the comments William made on IRC. The exception is bug 851047, where I've left a comment on the bug explaining why that's hard to fix at the moment.

[William Grant (wgrant)]

review: Approve (code)

[William Grant (wgrant)]

I'm a bit wary about that high per-SPPH query count, but otherwise good. What sort of queries are they? I know there's a few for DistroSeries, but can't think what the other 9 would be.

[Colin Watson (cjwatson)]

I agree the high count is unpleasant, but I checked that it was the same
before this branch (or else I'd be worried), so it probably just
reflects +publishinghistory being dreadful already. Remember that those
10 queries are for two SPPHs.

Both SPPHs involve queries for Component and Section, and as you say
DistroSeries. Then there's Archive and Distribution in the case of the
copied SPPH, and an extra Person query for the archive owner there as
well. My count seems to be off by one somewhere but that's most of it.

I had a brief go at improving this, but I think it needs rather more
care than I have time for right now and should get independent review.
Given that bug 739066 exists, and this branch isn't a regression in
terms of query count as far as I can see, I think it would be best to
address the high per-SPPH count separately.

=== 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