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

Proposed by Colin Watson
Status: Merged
Approved by: William Grant
Approved revision: no longer in the source branch.
Merged at revision: 16570
Proposed branch: lp:~cjwatson/launchpad/queue-copy-archive-links
Merge into: lp:launchpad
Diff against target: 452 lines (+152/-38)
14 files modified
lib/lp/_schema_circular_imports.py (+1/-0)
lib/lp/app/browser/configure.zcml (+3/-6)
lib/lp/app/browser/tales.py (+28/-6)
lib/lp/app/doc/tales.txt (+29/-2)
lib/lp/soyuz/browser/queue.py (+7/-3)
lib/lp/soyuz/browser/tests/test_queue.py (+26/-2)
lib/lp/soyuz/configure.zcml (+1/-0)
lib/lp/soyuz/interfaces/queue.py (+8/-0)
lib/lp/soyuz/model/queue.py (+8/-0)
lib/lp/soyuz/templates/build-index.pt (+1/-8)
lib/lp/soyuz/templates/distroseries-queue.pt (+7/-7)
lib/lp/soyuz/tests/test_packageupload.py (+25/-0)
lib/lp/testing/factory.py (+3/-2)
lib/lp/testing/tests/test_factory.py (+5/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/queue-copy-archive-links
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+159250@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/_schema_circular_imports.py'
2--- lib/lp/_schema_circular_imports.py 2013-04-03 03:09:04 +0000
3+++ lib/lp/_schema_circular_imports.py 2013-04-17 11:10:37 +0000
4@@ -591,6 +591,7 @@
5 IPackageUpload['pocket'].vocabulary = PackagePublishingPocket
6 patch_reference_property(IPackageUpload, 'distroseries', IDistroSeries)
7 patch_reference_property(IPackageUpload, 'archive', IArchive)
8+patch_reference_property(IPackageUpload, 'copy_source_archive', IArchive)
9
10 # IStructuralSubscription
11 patch_collection_property(
12
13=== modified file 'lib/lp/app/browser/configure.zcml'
14--- lib/lp/app/browser/configure.zcml 2013-04-10 07:42:31 +0000
15+++ lib/lp/app/browser/configure.zcml 2013-04-17 11:10:37 +0000
16@@ -1,4 +1,4 @@
17-<!-- Copyright 2009-2011 Canonical Ltd. This software is licensed under the
18+<!-- Copyright 2009-2013 Canonical Ltd. This software is licensed under the
19 GNU Affero General Public License version 3 (see the file LICENSE).
20 -->
21
22@@ -596,13 +596,10 @@
23 />
24 <!-- TALES fmt: namespace -->
25
26- <!-- The next directive is registered for all dicts, but we really only
27- want it to apply to the page template's CONTEXTS dict.
28- -->
29 <adapter
30- for="lp.soyuz.interfaces.archive.IPPA"
31+ for="lp.soyuz.interfaces.archive.IArchive"
32 provides="zope.traversing.interfaces.IPathAdapter"
33- factory="lp.app.browser.tales.PPAFormatterAPI"
34+ factory="lp.app.browser.tales.ArchiveFormatterAPI"
35 name="fmt"
36 />
37
38
39=== modified file 'lib/lp/app/browser/tales.py'
40--- lib/lp/app/browser/tales.py 2013-04-09 08:29:04 +0000
41+++ lib/lp/app/browser/tales.py 2013-04-17 11:10:37 +0000
42@@ -98,7 +98,10 @@
43 from lp.services.webapp.session import get_cookie_domain
44 from lp.services.webapp.url import urlappend
45 from lp.soyuz.enums import ArchivePurpose
46-from lp.soyuz.interfaces.archive import IPPA
47+from lp.soyuz.interfaces.archive import (
48+ IArchive,
49+ IPPA,
50+ )
51 from lp.soyuz.interfaces.binarypackagename import IBinaryAndSourcePackageName
52
53
54@@ -760,6 +763,8 @@
55 sprite_string = 'ppa-icon'
56 else:
57 sprite_string = 'ppa-icon-inactive'
58+ elif IArchive.providedBy(context):
59+ sprite_string = 'distribution'
60 elif IBranch.providedBy(context):
61 sprite_string = 'branch'
62 elif ISpecification.providedBy(context):
63@@ -1847,8 +1852,8 @@
64 return {'author': self._context.message.owner.displayname}
65
66
67-class PPAFormatterAPI(CustomizableFormatter):
68- """Adapter providing fmt support for `IPPA` objects."""
69+class ArchiveFormatterAPI(CustomizableFormatter):
70+ """Adapter providing fmt support for `IArchive` objects."""
71
72 _link_summary_template = '%(display_name)s'
73 _link_permission = 'launchpad.View'
74@@ -1859,19 +1864,32 @@
75 final_traversable_names.update(
76 CustomizableFormatter.final_traversable_names)
77
78+ def url(self, view_name=None, rootsite='mainsite'):
79+ """See `ObjectFormatterAPI`.
80+
81+ The default URL for a distribution main archive is the URL of the
82+ distribution. Other archive URLs are constructed as normal.
83+ """
84+ if self._context.is_main:
85+ return queryAdapter(
86+ self._context.distribution, IPathAdapter, 'fmt').url(
87+ view_name, rootsite)
88+ else:
89+ return super(ArchiveFormatterAPI, self).url(view_name, rootsite)
90+
91 def _link_summary_values(self):
92 """See CustomizableFormatter._link_summary_values."""
93 return {'display_name': self._context.displayname}
94
95 def link(self, view_name):
96- """Return html including a link for the context PPA.
97+ """Return html including a link for the context archive.
98
99 Render a link using CSS sprites for users with permission to view
100- the PPA.
101+ the archive.
102
103 Disabled PPAs are listed with sprites but not linkified.
104
105- Unaccessible private PPA are not rendered at all (empty string
106+ Inaccessible private PPAs are not rendered at all (empty string
107 is returned).
108 """
109 summary = self._make_link_summary()
110@@ -1887,6 +1905,10 @@
111
112 def reference(self, view_name=None, rootsite=None):
113 """Return the text PPA reference for a PPA."""
114+ if not IPPA.providedBy(self._context):
115+ raise NotImplementedError(
116+ "No reference implementation for non-PPA archive %r." %
117+ self._context)
118 if not check_permission(self._reference_permission, self._context):
119 return ''
120 return self._reference_template % {
121
122=== modified file 'lib/lp/app/doc/tales.txt'
123--- lib/lp/app/doc/tales.txt 2013-01-24 05:50:23 +0000
124+++ lib/lp/app/doc/tales.txt 2013-04-17 11:10:37 +0000
125@@ -160,8 +160,8 @@
126 ... name='pppa', private=True, owner=owner)
127
128 >>> print test_tales("ppa/fmt:link", ppa=private_ppa)
129- <a href="/~joe/+archive/pppa" class="sprite ppa-icon private">PPA named pppa
130- for Joe Smith</a>
131+ <a href="/~joe/+archive/pppa" class="sprite ppa-icon private">PPA named
132+ pppa for Joe Smith</a>
133
134 >>> login(ANONYMOUS)
135
136@@ -180,6 +180,33 @@
137 >>> print test_tales("ppa/fmt:reference", ppa=private_ppa)
138 ppa:joe/pppa
139
140+The same 'link' formatter works for distribution archives, with a different
141+sprite. The link target for main archives (primary, partner, and debug) is
142+the distribution rather than the archive, as the archives would just
143+redirect anyway.
144+
145+ >>> print test_tales("archive/fmt:link", archive=primary)
146+ <a href="/ubuntu" class="sprite distribution">Primary Archive for Ubuntu
147+ Linux</a>
148+
149+ >>> print test_tales("archive/fmt:link", archive=partner)
150+ <a href="/ubuntu" class="sprite distribution">Partner Archive for Ubuntu
151+ Linux</a>
152+
153+ >>> print test_tales("archive/fmt:link", archive=debug)
154+ <a href="/ubuntu" class="sprite distribution">Ubuntu DEBUG archive</a>
155+
156+ >>> print test_tales("archive/fmt:link", archive=copy)
157+ <a href="/ubuntu/+archive/rebuild" class="sprite distribution">Copy
158+ archive rebuild for Mark Shuttleworth</a>
159+
160+The 'reference' formatter is meaningless for non-PPA archives.
161+
162+ >>> test_tales("archive/fmt:reference", archive=primary)
163+ Traceback (most recent call last):
164+ ...
165+ NotImplementedError: No reference implementation for non-PPA archive ...
166+
167 We also have icons for builds which may have different dimensions.
168
169 >>> login('admin@canonical.com')
170
171=== modified file 'lib/lp/soyuz/browser/queue.py'
172--- lib/lp/soyuz/browser/queue.py 2013-01-08 05:45:26 +0000
173+++ lib/lp/soyuz/browser/queue.py 2013-04-17 11:10:37 +0000
174@@ -21,6 +21,7 @@
175 UnexpectedFormData,
176 )
177 from lp.registry.interfaces.person import IPersonSet
178+from lp.registry.model.distribution import Distribution
179 from lp.services.database.bulk import (
180 load_referencing,
181 load_related,
182@@ -196,11 +197,14 @@
183 """Batch-load `PackageCopyJob`s and related information."""
184 package_copy_jobs = load_related(
185 PackageCopyJob, uploads, ['package_copy_job_id'])
186- load_related(Archive, package_copy_jobs, ['source_archive_id'])
187+ archives = load_related(
188+ Archive, package_copy_jobs, ['source_archive_id'])
189+ load_related(Distribution, archives, ['distributionID'])
190+ person_ids = map(attrgetter('ownerID'), archives)
191 jobs = load_related(Job, package_copy_jobs, ['job_id'])
192- person_ids = map(attrgetter('requester_id'), jobs)
193+ person_ids.extend(map(attrgetter('requester_id'), jobs))
194 list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
195- person_ids, need_validity=True))
196+ person_ids, need_validity=True, need_icon=True))
197
198 def decoratedQueueBatch(self):
199 """Return the current batch, converted to decorated objects.
200
201=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
202--- lib/lp/soyuz/browser/tests/test_queue.py 2013-01-31 02:02:37 +0000
203+++ lib/lp/soyuz/browser/tests/test_queue.py 2013-04-17 11:10:37 +0000
204@@ -6,6 +6,7 @@
205 __metaclass__ = type
206
207 from lxml import html
208+import soupmatchers
209 from storm.store import Store
210 from testtools.matchers import Equals
211 import transaction
212@@ -17,6 +18,7 @@
213 from lp.archiveuploader.tests import datadir
214 from lp.registry.interfaces.pocket import PackagePublishingPocket
215 from lp.services.webapp.escaping import html_escape
216+from lp.services.webapp.publisher import canonical_url
217 from lp.services.webapp.servers import LaunchpadTestRequest
218 from lp.soyuz.browser.queue import CompletePackageUpload
219 from lp.soyuz.enums import PackageUploadStatus
220@@ -360,8 +362,28 @@
221 html_text = view()
222 self.assertIn(upload.package_name, html_text)
223 # The details section states the sync's origin and requester.
224+ archive = upload.package_copy_job.source_archive
225+ url = canonical_url(archive.distribution, path_only_if_possible=True)
226+ self.assertThat(html_text, soupmatchers.HTMLContains(
227+ soupmatchers.Tag(
228+ "link", "a", text=archive.displayname, attrs={"href": url}),
229+ ))
230 self.assertIn(
231- upload.package_copy_job.source_archive.displayname, html_text)
232+ upload.package_copy_job.job.requester.displayname, html_text)
233+
234+ def test_view_renders_copy_upload_from_private_archive(self):
235+ login(ADMIN_EMAIL)
236+ p3a = self.factory.makeArchive(private=True)
237+ upload = self.factory.makeCopyJobPackageUpload(source_archive=p3a)
238+ queue_admin = self.factory.makeArchiveAdmin(
239+ upload.distroseries.main_archive)
240+ with person_logged_in(queue_admin):
241+ view = self.makeView(upload.distroseries, queue_admin)
242+ html_text = view()
243+ self.assertIn(upload.package_name, html_text)
244+ # The details section states the sync's origin and requester.
245+ self.assertTextMatchesExpressionIgnoreWhitespace(
246+ "Sync from <span>private archive</span>,", html_text)
247 self.assertIn(
248 upload.package_copy_job.job.requester.displayname, html_text)
249
250@@ -379,6 +401,8 @@
251 sprs[-1].addFile(dsc)
252 uploads.append(self.factory.makeCustomPackageUpload(distroseries))
253 uploads.append(self.factory.makeCopyJobPackageUpload(distroseries))
254+ uploads.append(self.factory.makeCopyJobPackageUpload(
255+ distroseries, source_archive=self.factory.makeArchive()))
256 self.factory.makePackageset(
257 packages=(sprs[0].sourcepackagename, sprs[2].sourcepackagename,
258 sprs[4].sourcepackagename),
259@@ -398,7 +422,7 @@
260 with StormStatementRecorder() as recorder:
261 view = self.makeView(distroseries, queue_admin)
262 view()
263- self.assertThat(recorder, HasQueryCount(Equals(54)))
264+ self.assertThat(recorder, HasQueryCount(Equals(56)))
265
266
267 class TestCompletePackageUpload(TestCaseWithFactory):
268
269=== modified file 'lib/lp/soyuz/configure.zcml'
270--- lib/lp/soyuz/configure.zcml 2013-02-18 03:47:21 +0000
271+++ lib/lp/soyuz/configure.zcml 2013-04-17 11:10:37 +0000
272@@ -168,6 +168,7 @@
273 custom_file_urls
274 customFileUrls
275 getBinaryProperties
276+ copy_source_archive
277 getFileByName
278 date_created
279 sourcepackagerelease
280
281=== modified file 'lib/lp/soyuz/interfaces/queue.py'
282--- lib/lp/soyuz/interfaces/queue.py 2013-01-08 01:10:35 +0000
283+++ lib/lp/soyuz/interfaces/queue.py 2013-04-17 11:10:37 +0000
284@@ -193,6 +193,14 @@
285 readonly=True),
286 ("devel", dict(exported=False)), exported=True)
287
288+ copy_source_archive = exported(
289+ Reference(
290+ # Really IArchive, patched in _schema_circular_imports.py
291+ schema=Interface,
292+ description=_("The archive from which this package was copied, if "
293+ "any."),
294+ title=_("Copy source archive"), required=False, readonly=True))
295+
296 displayname = exported(
297 TextLine(
298 title=_("Generic displayname for a queue item"), readonly=True),
299
300=== modified file 'lib/lp/soyuz/model/queue.py'
301--- lib/lp/soyuz/model/queue.py 2013-02-06 08:20:13 +0000
302+++ lib/lp/soyuz/model/queue.py 2013-04-17 11:10:37 +0000
303@@ -295,6 +295,14 @@
304 })
305 return properties
306
307+ @property
308+ def copy_source_archive(self):
309+ """See `IPackageUpload`."""
310+ if self.package_copy_job_id is not None:
311+ return self.package_copy_job.source_archive
312+ else:
313+ return None
314+
315 def getFileByName(self, filename):
316 """See `IPackageUpload`."""
317 if (self.changesfile is not None and
318
319=== modified file 'lib/lp/soyuz/templates/build-index.pt'
320--- lib/lp/soyuz/templates/build-index.pt 2012-03-01 18:17:56 +0000
321+++ lib/lp/soyuz/templates/build-index.pt 2013-04-17 11:10:37 +0000
322@@ -86,14 +86,7 @@
323 <dl>
324 <dt>Archive:</dt>
325 <dd>
326- <span tal:condition="view/is_ppa"
327- tal:replace="structure context/archive/fmt:link"
328- >Celso PPA</span>
329- <a class="sprite distribution"
330- tal:condition="not: view/is_ppa"
331- tal:attributes="href context/archive/fmt:url"
332- tal:content="context/archive/displayname"
333- >Ubuntu Primary Archive</a>
334+ <a tal:replace="structure context/archive/fmt:link" />
335 </dd>
336 </dl>
337 <dl>
338
339=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
340--- lib/lp/soyuz/templates/distroseries-queue.pt 2012-11-15 01:41:14 +0000
341+++ lib/lp/soyuz/templates/distroseries-queue.pt 2013-04-17 11:10:37 +0000
342@@ -224,15 +224,15 @@
343 <tr>
344 <td />
345 <td tal:condition="view/availableActions" />
346- <td colspan="7">
347+ <td colspan="7"
348+ tal:define="pcj packageupload/package_copy_job;
349+ visible pcj/source_archive/required:launchpad.View">
350 Sync from
351- <tal:archive
352- content="packageupload/package_copy_job/source_archive/displayname">
353- Primary Archive for Ubuntu
354- </tal:archive>,
355+ <a tal:condition="visible"
356+ tal:replace="structure pcj/source_archive/fmt:link" />
357+ <span tal:condition="not:visible">private archive</span>,
358 requested by
359- <tal:requester
360- content="structure packageupload/package_copy_job/job/requester/fmt:link" />
361+ <tal:requester content="structure pcj/job/requester/fmt:link" />
362 </td>
363 </tr>
364 </tal:sync>
365
366=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
367--- lib/lp/soyuz/tests/test_packageupload.py 2013-01-31 02:02:37 +0000
368+++ lib/lp/soyuz/tests/test_packageupload.py 2013-04-17 11:10:37 +0000
369@@ -902,6 +902,13 @@
370 transaction.commit()
371 return upload, self.load(upload, person)
372
373+ def makeCopyJobPackageUpload(self, person, **kwargs):
374+ with person_logged_in(person):
375+ upload = self.factory.makeCopyJobPackageUpload(
376+ distroseries=self.distroseries, **kwargs)
377+ transaction.commit()
378+ return upload, self.load(upload, person)
379+
380 def makeBinaryPackageUpload(self, person, binarypackagename=None,
381 component=None):
382 with person_logged_in(person):
383@@ -1293,6 +1300,24 @@
384 }
385 self.assertEqual(expected_custom, ws_binaries[-1])
386
387+ def test_copy_info(self):
388+ # API clients can inspect properties of copies, including the source
389+ # archive.
390+ person = self.makeQueueAdmin([self.universe])
391+ archive = self.factory.makeArchive()
392+ upload, ws_upload = self.makeCopyJobPackageUpload(
393+ person, sourcepackagename="hello", source_archive=archive)
394+
395+ self.assertFalse(ws_upload.contains_source)
396+ self.assertFalse(ws_upload.contains_build)
397+ self.assertTrue(ws_upload.contains_copy)
398+ self.assertEqual("hello", ws_upload.display_name)
399+ self.assertEqual("sync", ws_upload.display_arches)
400+ self.assertEqual("hello", ws_upload.package_name)
401+ with person_logged_in(person):
402+ archive_url = api_url(archive)
403+ self.assertEndsWith(ws_upload.copy_source_archive_link, archive_url)
404+
405 def test_getPackageUploads_query_count(self):
406 person = self.makeQueueAdmin([self.universe])
407 uploads = []
408
409=== modified file 'lib/lp/testing/factory.py'
410--- lib/lp/testing/factory.py 2013-03-08 22:17:31 +0000
411+++ lib/lp/testing/factory.py 2013-04-17 11:10:37 +0000
412@@ -3460,12 +3460,13 @@
413 return upload
414
415 def makeCopyJobPackageUpload(self, distroseries=None,
416- sourcepackagename=None, target_pocket=None):
417+ sourcepackagename=None, source_archive=None,
418+ target_pocket=None):
419 """Make a `PackageUpload` with a `PackageCopyJob` attached."""
420 if distroseries is None:
421 distroseries = self.makeDistroSeries()
422 spph = self.makeSourcePackagePublishingHistory(
423- sourcepackagename=sourcepackagename)
424+ archive=source_archive, sourcepackagename=sourcepackagename)
425 spr = spph.sourcepackagerelease
426 job = self.makePlainPackageCopyJob(
427 package_name=spr.sourcepackagename.name,
428
429=== modified file 'lib/lp/testing/tests/test_factory.py'
430--- lib/lp/testing/tests/test_factory.py 2012-09-22 23:58:54 +0000
431+++ lib/lp/testing/tests/test_factory.py 2013-04-17 11:10:37 +0000
432@@ -1,4 +1,4 @@
433-# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
434+# Copyright 2010-2013 Canonical Ltd. This software is licensed under the
435 # GNU Affero General Public License version 3 (see the file LICENSE).
436
437 """Tests for the Launchpad object factory."""
438@@ -833,11 +833,14 @@
439 def test_makeCopyJobPackageUpload_passes_on_args(self):
440 distroseries = self.factory.makeDistroSeries()
441 spn = self.factory.makeSourcePackageName()
442+ source_archive = self.factory.makeArchive()
443 pu = self.factory.makeCopyJobPackageUpload(
444- distroseries=distroseries, sourcepackagename=spn)
445+ distroseries=distroseries, sourcepackagename=spn,
446+ source_archive=source_archive)
447 job = removeSecurityProxy(pu.package_copy_job)
448 self.assertEqual(distroseries, pu.distroseries)
449 self.assertEqual(distroseries.distribution, pu.archive.distribution)
450+ self.assertEqual(source_archive, job.source_archive)
451 self.assertEqual(distroseries, job.target_distroseries)
452 self.assertEqual(spn.name, job.package_name)
453