launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-816833 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #816833 in Launchpad itself: "DistroSeries:+queue expander section for sync uploads"
  https://bugs.launchpad.net/launchpad/+bug/816833

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-816833/+merge/69597

= Summary =

The DistroSeries:+queue page shows package uploads, which can have different types of items attached.  (Some of those item types can be repeated or combined).  Each upload on the page has a foldout section that shows them, except we don't have one for the new copy-job ("sync") uploads yet.

What we want to show there is at least the source of the sync and the person who requested it.


== Proposed fix ==

A bit of TAL.  To be sure there's too much detail in the TAL already, but that really merits a separate cleanup round.  Moving one bit into the view when lots of similar bits are still in TAL just spreads knowledge of the layout details (how many columns in that table?) through too many places.


== Pre-implementation notes ==

I didn't do much in the way of pre-imp.  Perhaps I should have.  But this gives us a starting point for improvement that I'd rather land than keep hidden away in a branch; it's not a UI that your average user will look at so there will be no disappointment among the masses.


== Implementation details ==

The model objects that are now displayed are all batch-loaded from the database.


== Tests ==

{{{
./bin/test -vvc lp.soyuz.browser.tests.test_queue -t renders_copy_upload
}}}


== Demo and Q/A ==

Look up a sync upload on a DistroSeries:+queue page.  Its expander icon now works, and reveals a line stating where the sync comes from and who requested it.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/configure.zcml
  lib/lp/soyuz/browser/queue.py
  lib/lp/soyuz/browser/tests/test_queue.py
  lib/lp/soyuz/templates/distroseries-queue.pt
-- 
https://code.launchpad.net/~jtv/launchpad/bug-816833/+merge/69597
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-816833 into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2011-07-26 08:51:39 +0000
+++ lib/lp/soyuz/browser/queue.py	2011-07-28 07:59:32 +0000
@@ -27,6 +27,7 @@
     load_referencing,
     load_related,
     )
+from lp.services.job.model.job import Job
 from lp.soyuz.enums import (
     PackagePublishingPriority,
     PackageUploadStatus,
@@ -190,6 +191,19 @@
         return getUtility(IPackagesetSet).getForPackages(
             self.context, set(spr.sourcepackagenameID for spr in sprs))
 
+    def loadPackageCopyJobs(self, uploads):
+        """Batch-load `PackageCopyJob`s and related information."""
+        # Avoid circular imports.
+        from lp.registry.model.person import Person
+        from lp.soyuz.model.archive import Archive
+        from lp.soyuz.model.packagecopyjob import PackageCopyJob
+
+        package_copy_jobs = load_related(
+            PackageCopyJob, uploads, ['package_copy_job_id'])
+        load_related(Archive, package_copy_jobs, ['source_archive_id'])
+        jobs = load_related(Job, package_copy_jobs, ['job_id'])
+        load_related(Person, jobs, ['requester_id'])
+
     def decoratedQueueBatch(self):
         """Return the current batch, converted to decorated objects.
 
@@ -245,6 +259,8 @@
 
         package_sets = self.getPackagesetsFor(source_sprs)
 
+        self.loadPackageCopyJobs(uploads)
+
         return [
             CompletePackageUpload(
                 item, build_upload_files, source_upload_files, package_sets)

=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py	2011-06-23 13:05:52 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py	2011-07-28 07:59:32 +0000
@@ -236,6 +236,11 @@
             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.assertIn(
+            upload.package_copy_job.source_archive.displayname, html_text)
+        self.assertIn(
+            upload.package_copy_job.job.requester.displayname, html_text)
 
 
 class TestCompletePackageUpload(TestCaseWithFactory):

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2011-07-14 15:23:28 +0000
+++ lib/lp/soyuz/configure.zcml	2011-07-28 07:59:32 +0000
@@ -181,6 +181,7 @@
                 is_delayed_copy
                 isPPA
                 package_copy_job
+                package_copy_job_id
                 package_name
                 package_version
                 section_name

=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2011-07-18 09:23:10 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2011-07-28 07:59:32 +0000
@@ -239,6 +239,23 @@
       </tr>
     </tal:copy>
 
+    <tal:sync condition="packageupload/package_copy_job">
+      <tr>
+        <td />
+        <td tal:condition="view/availableActions" />
+        <td colspan="7">
+          Sync from
+          <tal:archive
+            content="packageupload/package_copy_job/source_archive/displayname">
+            Primary Archive for Ubuntu
+          </tal:archive>,
+          requested by
+          <tal:requester
+            content="structure packageupload/package_copy_job/job/requester/fmt:link" />
+        </td>
+      </tr>
+    </tal:sync>
+
     <tal:upload condition="not: packageupload/pending_delayed_copy">
       <tr tal:repeat="file packageupload/source_files">
         <td/>