Merge lp:~cjwatson/launchpad/queue-copy-archive-links into lp:launchpad

[Colin Watson]

Show link to source archive for copies from PPAs in DistroSeries:+queue; add PackageUpload.copy_source_archive property.

== Summary ==

When reviewing copies in DistroSeries:+queue, it's a pain to figure out where they came from in order to accurately review the change.

== Proposed fix ==

Linkify the archive displayname, at least for PPAs (archive URLs aren't very useful for primary archives). Add an exported copy_source_archive property to PackageUpload. Between them, these changes will make it easier to track things down by hand and should make it possible to teach our "queue" script to follow the breadcrumb trail automatically.

== LOC Rationale ==

+78. I think this is worth it to eliminate a barrier to review for ~ubuntu-release, particularly in the run-up to 13.04 release; and I should have lots of credit from past changes.

== Tests ==

bin/test -vvct lp.soyuz.browser.tests.test_queue -t lp.soyuz.tests.test_packageupload -t lp.testing.tests.test_factory

== Demo and Q/A ==

Copy a package to a frozen series from (a) Debian and (b) a PPA on dogfood, if there isn't one of each available already. Run the PCJ processor. Check that both render reasonably in the DistroSeries:+queue UI, and that the copy_source_archive property in the API is correct.

[William Grant (wgrant)]

Did you consider just using fmt:link for the archive? It's probably not worth special casing PPAs, as non-primary archives have informative URLs that at worst just redirect to the distribution, still making the origin clearer.

review: Approve (code)

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2013-04-03 03:09:04 +0000
+++ lib/lp/_schema_circular_imports.py	2013-04-17 11:10:37 +0000
@@ -591,6 +591,7 @@
 IPackageUpload['pocket'].vocabulary = PackagePublishingPocket
 patch_reference_property(IPackageUpload, 'distroseries', IDistroSeries)
 patch_reference_property(IPackageUpload, 'archive', IArchive)
+patch_reference_property(IPackageUpload, 'copy_source_archive', IArchive)
 
 # IStructuralSubscription
 patch_collection_property(
 
=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2013-04-10 07:42:31 +0000
+++ lib/lp/app/browser/configure.zcml	2013-04-17 11:10:37 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -596,13 +596,10 @@
       />
   <!-- TALES fmt: namespace -->
 
-  <!-- The next directive is registered for all dicts, but we really only
-       want it to apply to the page template's CONTEXTS dict.
-    -->
   <adapter
-      for="lp.soyuz.interfaces.archive.IPPA"
+      for="lp.soyuz.interfaces.archive.IArchive"
       provides="zope.traversing.interfaces.IPathAdapter"
-      factory="lp.app.browser.tales.PPAFormatterAPI"
+      factory="lp.app.browser.tales.ArchiveFormatterAPI"
       name="fmt"
       />
 
 
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2013-04-09 08:29:04 +0000
+++ lib/lp/app/browser/tales.py	2013-04-17 11:10:37 +0000
@@ -98,7 +98,10 @@
 from lp.services.webapp.session import get_cookie_domain
 from lp.services.webapp.url import urlappend
 from lp.soyuz.enums import ArchivePurpose
-from lp.soyuz.interfaces.archive import IPPA
+from lp.soyuz.interfaces.archive import (
+    IArchive,
+    IPPA,
+    )
 from lp.soyuz.interfaces.binarypackagename import IBinaryAndSourcePackageName
 
 
@@ -760,6 +763,8 @@
                 sprite_string = 'ppa-icon'
             else:
                 sprite_string = 'ppa-icon-inactive'
+        elif IArchive.providedBy(context):
+            sprite_string = 'distribution'
         elif IBranch.providedBy(context):
             sprite_string = 'branch'
         elif ISpecification.providedBy(context):
@@ -1847,8 +1852,8 @@
         return {'author': self._context.message.owner.displayname}
 
 
-class PPAFormatterAPI(CustomizableFormatter):
-    """Adapter providing fmt support for `IPPA` objects."""
+class ArchiveFormatterAPI(CustomizableFormatter):
+    """Adapter providing fmt support for `IArchive` objects."""
 
     _link_summary_template = '%(display_name)s'
     _link_permission = 'launchpad.View'
@@ -1859,19 +1864,32 @@
     final_traversable_names.update(
         CustomizableFormatter.final_traversable_names)
 
+    def url(self, view_name=None, rootsite='mainsite'):
+        """See `ObjectFormatterAPI`.
+
+        The default URL for a distribution main archive is the URL of the
+        distribution.  Other archive URLs are constructed as normal.
+        """
+        if self._context.is_main:
+            return queryAdapter(
+                self._context.distribution, IPathAdapter, 'fmt').url(
+                    view_name, rootsite)
+        else:
+            return super(ArchiveFormatterAPI, self).url(view_name, rootsite)
+
     def _link_summary_values(self):
         """See CustomizableFormatter._link_summary_values."""
         return {'display_name': self._context.displayname}
 
     def link(self, view_name):
-        """Return html including a link for the context PPA.
+        """Return html including a link for the context archive.
 
         Render a link using CSS sprites for users with permission to view
-        the PPA.
+        the archive.
 
         Disabled PPAs are listed with sprites but not linkified.
 
-        Unaccessible private PPA are not rendered at all (empty string
+        Inaccessible private PPAs are not rendered at all (empty string
         is returned).
         """
         summary = self._make_link_summary()
@@ -1887,6 +1905,10 @@
 
     def reference(self, view_name=None, rootsite=None):
         """Return the text PPA reference for a PPA."""
+        if not IPPA.providedBy(self._context):
+            raise NotImplementedError(
+                "No reference implementation for non-PPA archive %r." %
+                self._context)
         if not check_permission(self._reference_permission, self._context):
             return ''
         return self._reference_template % {
 
=== modified file 'lib/lp/app/doc/tales.txt'
--- lib/lp/app/doc/tales.txt	2013-01-24 05:50:23 +0000
+++ lib/lp/app/doc/tales.txt	2013-04-17 11:10:37 +0000
@@ -160,8 +160,8 @@
     ...     name='pppa', private=True, owner=owner)
 
     >>> print test_tales("ppa/fmt:link", ppa=private_ppa)
-    <a href="/~joe/+archive/pppa" class="sprite ppa-icon private">PPA named pppa
-    for Joe Smith</a>
+    <a href="/~joe/+archive/pppa" class="sprite ppa-icon private">PPA named
+    pppa for Joe Smith</a>
 
     >>> login(ANONYMOUS)
 
@@ -180,6 +180,33 @@
     >>> print test_tales("ppa/fmt:reference", ppa=private_ppa)
     ppa:joe/pppa
 
+The same 'link' formatter works for distribution archives, with a different
+sprite.  The link target for main archives (primary, partner, and debug) is
+the distribution rather than the archive, as the archives would just
+redirect anyway.
+
+    >>> print test_tales("archive/fmt:link", archive=primary)
+    <a href="/ubuntu" class="sprite distribution">Primary Archive for Ubuntu
+    Linux</a>
+
+    >>> print test_tales("archive/fmt:link", archive=partner)
+    <a href="/ubuntu" class="sprite distribution">Partner Archive for Ubuntu
+    Linux</a>
+
+    >>> print test_tales("archive/fmt:link", archive=debug)
+    <a href="/ubuntu" class="sprite distribution">Ubuntu DEBUG archive</a>
+
+    >>> print test_tales("archive/fmt:link", archive=copy)
+    <a href="/ubuntu/+archive/rebuild" class="sprite distribution">Copy
+    archive rebuild for Mark Shuttleworth</a>
+
+The 'reference' formatter is meaningless for non-PPA archives.
+
+    >>> test_tales("archive/fmt:reference", archive=primary)
+    Traceback (most recent call last):
+    ...
+    NotImplementedError: No reference implementation for non-PPA archive ...
+
 We also have icons for builds which may have different dimensions.
 
     >>> login('[e-mail]')
 
=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2013-01-08 05:45:26 +0000
+++ lib/lp/soyuz/browser/queue.py	2013-04-17 11:10:37 +0000
@@ -21,6 +21,7 @@
     UnexpectedFormData,
     )
 from lp.registry.interfaces.person import IPersonSet
+from lp.registry.model.distribution import Distribution
 from lp.services.database.bulk import (
     load_referencing,
     load_related,
@@ -196,11 +197,14 @@
         """Batch-load `PackageCopyJob`s and related information."""
         package_copy_jobs = load_related(
             PackageCopyJob, uploads, ['package_copy_job_id'])
-        load_related(Archive, package_copy_jobs, ['source_archive_id'])
+        archives = load_related(
+            Archive, package_copy_jobs, ['source_archive_id'])
+        load_related(Distribution, archives, ['distributionID'])
+        person_ids = map(attrgetter('ownerID'), archives)
         jobs = load_related(Job, package_copy_jobs, ['job_id'])
-        person_ids = map(attrgetter('requester_id'), jobs)
+        person_ids.extend(map(attrgetter('requester_id'), jobs))
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-            person_ids, need_validity=True))
+            person_ids, need_validity=True, need_icon=True))
 
     def decoratedQueueBatch(self):
         """Return the current batch, converted to decorated objects.
 
=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py	2013-01-31 02:02:37 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py	2013-04-17 11:10:37 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from lxml import html
+import soupmatchers
 from storm.store import Store
 from testtools.matchers import Equals
 import transaction
@@ -17,6 +18,7 @@
 from lp.archiveuploader.tests import datadir
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.webapp.escaping import html_escape
+from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.soyuz.browser.queue import CompletePackageUpload
 from lp.soyuz.enums import PackageUploadStatus
@@ -360,8 +362,28 @@
             html_text = view()
         self.assertIn(upload.package_name, html_text)
         # The details section states the sync's origin and requester.
+        archive = upload.package_copy_job.source_archive
+        url = canonical_url(archive.distribution, path_only_if_possible=True)
+        self.assertThat(html_text, soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                "link", "a", text=archive.displayname, attrs={"href": url}),
+            ))
         self.assertIn(
-            upload.package_copy_job.source_archive.displayname, html_text)
+            upload.package_copy_job.job.requester.displayname, html_text)
+
+    def test_view_renders_copy_upload_from_private_archive(self):
+        login(ADMIN_EMAIL)
+        p3a = self.factory.makeArchive(private=True)
+        upload = self.factory.makeCopyJobPackageUpload(source_archive=p3a)
+        queue_admin = self.factory.makeArchiveAdmin(
+            upload.distroseries.main_archive)
+        with person_logged_in(queue_admin):
+            view = self.makeView(upload.distroseries, queue_admin)
+            html_text = view()
+        self.assertIn(upload.package_name, html_text)
+        # The details section states the sync's origin and requester.
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            "Sync from <span>private archive</span>,", html_text)
         self.assertIn(
             upload.package_copy_job.job.requester.displayname, html_text)
 
@@ -379,6 +401,8 @@
             sprs[-1].addFile(dsc)
             uploads.append(self.factory.makeCustomPackageUpload(distroseries))
             uploads.append(self.factory.makeCopyJobPackageUpload(distroseries))
+            uploads.append(self.factory.makeCopyJobPackageUpload(
+                distroseries, source_archive=self.factory.makeArchive()))
         self.factory.makePackageset(
             packages=(sprs[0].sourcepackagename, sprs[2].sourcepackagename,
                 sprs[4].sourcepackagename),
@@ -398,7 +422,7 @@
             with StormStatementRecorder() as recorder:
                 view = self.makeView(distroseries, queue_admin)
                 view()
-        self.assertThat(recorder, HasQueryCount(Equals(54)))
+        self.assertThat(recorder, HasQueryCount(Equals(56)))
 
 
 class TestCompletePackageUpload(TestCaseWithFactory):
 
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2013-02-18 03:47:21 +0000
+++ lib/lp/soyuz/configure.zcml	2013-04-17 11:10:37 +0000
@@ -168,6 +168,7 @@
                 custom_file_urls
                 customFileUrls
                 getBinaryProperties
+                copy_source_archive
                 getFileByName
                 date_created
                 sourcepackagerelease
 
=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py	2013-01-08 01:10:35 +0000
+++ lib/lp/soyuz/interfaces/queue.py	2013-04-17 11:10:37 +0000
@@ -193,6 +193,14 @@
             readonly=True),
         ("devel", dict(exported=False)), exported=True)
 
+    copy_source_archive = exported(
+        Reference(
+            # Really IArchive, patched in _schema_circular_imports.py
+            schema=Interface,
+            description=_("The archive from which this package was copied, if "
+                          "any."),
+            title=_("Copy source archive"), required=False, readonly=True))
+
     displayname = exported(
         TextLine(
             title=_("Generic displayname for a queue item"), readonly=True),
 
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2013-02-06 08:20:13 +0000
+++ lib/lp/soyuz/model/queue.py	2013-04-17 11:10:37 +0000
@@ -295,6 +295,14 @@
                 })
         return properties
 
+    @property
+    def copy_source_archive(self):
+        """See `IPackageUpload`."""
+        if self.package_copy_job_id is not None:
+            return self.package_copy_job.source_archive
+        else:
+            return None
+
     def getFileByName(self, filename):
         """See `IPackageUpload`."""
         if (self.changesfile is not None and
 
=== modified file 'lib/lp/soyuz/templates/build-index.pt'
--- lib/lp/soyuz/templates/build-index.pt	2012-03-01 18:17:56 +0000
+++ lib/lp/soyuz/templates/build-index.pt	2013-04-17 11:10:37 +0000
@@ -86,14 +86,7 @@
       <dl>
         <dt>Archive:</dt>
           <dd>
-            <span tal:condition="view/is_ppa"
-                  tal:replace="structure context/archive/fmt:link"
-             >Celso PPA</span>
-            <a class="sprite distribution"
-               tal:condition="not: view/is_ppa"
-      	     tal:attributes="href context/archive/fmt:url"
-               tal:content="context/archive/displayname"
-             >Ubuntu Primary Archive</a>
+            <a tal:replace="structure context/archive/fmt:link" />
           </dd>
       </dl>
       <dl>
 
=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2012-11-15 01:41:14 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2013-04-17 11:10:37 +0000
@@ -224,15 +224,15 @@
       <tr>
         <td />
         <td tal:condition="view/availableActions" />
-        <td colspan="7">
+        <td colspan="7"
+            tal:define="pcj packageupload/package_copy_job;
+                        visible pcj/source_archive/required:launchpad.View">
           Sync from
-          <tal:archive
-            content="packageupload/package_copy_job/source_archive/displayname">
-            Primary Archive for Ubuntu
-          </tal:archive>,
+          <a tal:condition="visible"
+             tal:replace="structure pcj/source_archive/fmt:link" />
+          <span tal:condition="not:visible">private archive</span>,
           requested by
-          <tal:requester
-            content="structure packageupload/package_copy_job/job/requester/fmt:link" />
+          <tal:requester content="structure pcj/job/requester/fmt:link" />
         </td>
       </tr>
     </tal:sync>
 
=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2013-01-31 02:02:37 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2013-04-17 11:10:37 +0000
@@ -902,6 +902,13 @@
         transaction.commit()
         return upload, self.load(upload, person)
 
+    def makeCopyJobPackageUpload(self, person, **kwargs):
+        with person_logged_in(person):
+            upload = self.factory.makeCopyJobPackageUpload(
+                distroseries=self.distroseries, **kwargs)
+        transaction.commit()
+        return upload, self.load(upload, person)
+
     def makeBinaryPackageUpload(self, person, binarypackagename=None,
                                 component=None):
         with person_logged_in(person):
@@ -1293,6 +1300,24 @@
             }
         self.assertEqual(expected_custom, ws_binaries[-1])
 
+    def test_copy_info(self):
+        # API clients can inspect properties of copies, including the source
+        # archive.
+        person = self.makeQueueAdmin([self.universe])
+        archive = self.factory.makeArchive()
+        upload, ws_upload = self.makeCopyJobPackageUpload(
+            person, sourcepackagename="hello", source_archive=archive)
+
+        self.assertFalse(ws_upload.contains_source)
+        self.assertFalse(ws_upload.contains_build)
+        self.assertTrue(ws_upload.contains_copy)
+        self.assertEqual("hello", ws_upload.display_name)
+        self.assertEqual("sync", ws_upload.display_arches)
+        self.assertEqual("hello", ws_upload.package_name)
+        with person_logged_in(person):
+            archive_url = api_url(archive)
+        self.assertEndsWith(ws_upload.copy_source_archive_link, archive_url)
+
     def test_getPackageUploads_query_count(self):
         person = self.makeQueueAdmin([self.universe])
         uploads = []
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2013-03-08 22:17:31 +0000
+++ lib/lp/testing/factory.py	2013-04-17 11:10:37 +0000
@@ -3460,12 +3460,13 @@
         return upload
 
     def makeCopyJobPackageUpload(self, distroseries=None,
-                                 sourcepackagename=None, target_pocket=None):
+                                 sourcepackagename=None, source_archive=None,
+                                 target_pocket=None):
         """Make a `PackageUpload` with a `PackageCopyJob` attached."""
         if distroseries is None:
             distroseries = self.makeDistroSeries()
         spph = self.makeSourcePackagePublishingHistory(
-            sourcepackagename=sourcepackagename)
+            archive=source_archive, sourcepackagename=sourcepackagename)
         spr = spph.sourcepackagerelease
         job = self.makePlainPackageCopyJob(
             package_name=spr.sourcepackagename.name,
 
=== modified file 'lib/lp/testing/tests/test_factory.py'
--- lib/lp/testing/tests/test_factory.py	2012-09-22 23:58:54 +0000
+++ lib/lp/testing/tests/test_factory.py	2013-04-17 11:10:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the Launchpad object factory."""
@@ -833,11 +833,14 @@
     def test_makeCopyJobPackageUpload_passes_on_args(self):
         distroseries = self.factory.makeDistroSeries()
         spn = self.factory.makeSourcePackageName()
+        source_archive = self.factory.makeArchive()
         pu = self.factory.makeCopyJobPackageUpload(
-            distroseries=distroseries, sourcepackagename=spn)
+            distroseries=distroseries, sourcepackagename=spn,
+            source_archive=source_archive)
         job = removeSecurityProxy(pu.package_copy_job)
         self.assertEqual(distroseries, pu.distroseries)
         self.assertEqual(distroseries.distribution, pu.archive.distribution)
+        self.assertEqual(source_archive, job.source_archive)
         self.assertEqual(distroseries, job.target_distroseries)
         self.assertEqual(spn.name, job.package_name)