launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798179 in Launchpad itself: "Support PackageUploads w/o changes files on +queue"
  https://bugs.launchpad.net/launchpad/+bug/798179

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

= Summary =

The Distroseries:+queue page breaks when displaying a PackageUpload without changes file attached, as might be the case with a PackageCopyJob upload.


== Proposed fix ==

I don't have time to clean up the TAL and move the logic into view code.  Instead, I just add some safety checks around the breaking tags.


== Pre-implementation notes ==

Julian would love me to move logic from TAL into view code, but I find there's no view for the individual PackageUpload lines.  I'd have to create one, and I've bitten off too much of this work to chew already.  Maybe later!


== Implementation details ==

There was an import missing.  No idea how that got through; I'm juggling a number of branches at the moment and I'll search them for the origin of this problem and if possible, fix it at the source.


== Tests ==

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


== Demo and Q/A ==

Create a PackageUpload with a PackageCopyJob but nothing else attached, and view it in the UI.  It should display something sensible: package name as the name, no link to any changes file, no oops.


= Launchpad lint =

Checking for conflicts and issues in changed files.

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

=== modified file 'lib/lp/soyuz/browser/tests/test_queue.py'
--- lib/lp/soyuz/browser/tests/test_queue.py	2011-06-14 10:08:40 +0000
+++ lib/lp/soyuz/browser/tests/test_queue.py	2011-06-16 12:35:42 +0000
@@ -10,12 +10,10 @@
     getUtility,
     queryMultiAdapter,
     )
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.archiveuploader.tests import datadir
-from lp.soyuz.adapters.overrides import SourceOverride
 from lp.soyuz.enums import PackageUploadStatus
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.queue import IPackageUploadSet
@@ -26,6 +24,7 @@
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.sampledata import ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
 
@@ -197,38 +196,40 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def makeView(self, distroseries, user, form):
+    def makeView(self, distroseries, user):
         """Create a queue view."""
         return create_initialized_view(
             distroseries, name='+queue', principal=user)
 
-    def makeCopyJobUpload(self, distroseries, component):
-        """Create a `PackageUpload` with a `PackageCopyJob`."""
-        job = self.factory.makePlainPackageCopyJob(
-            target_distroseries=distroseries)
-        job.addSourceOverride(SourceOverride(
-            self.factory.makeSourcePackageName().name,
-            component=component, section=self.factory.makeSection()))
-        naked_job = removeSecurityProxy(job).context
-        return self.factory.makePackageUpload(
-            distroseries=distroseries, package_copy_job=naked_job)
-
-    def test_copy_upload_does_not_break_rendering(self):
-        # The presence of a PackageUpload with a PackageCopyJob does not
-        # break rendering of the page.
-        # XXX JeroenVermeulen 2011-06-13 bug=394645: the current reason
-        # why this doesn't break is that the view uses getQueueItems,
-        # which won't return PackageUploads with copy jobs.  This test
-        # may start breaking once the view uses getPackageUploads.
-        distroseries = self.factory.makeDistroSeries()
-        component = self.factory.makeComponent()
-        upload = self.makeCopyJobUpload(distroseries, component=component)
-        queue_admin = self.factory.makeArchiveAdmin(distroseries.main_archive)
-        form = {
-            'queue_state': PackageUploadStatus.NEW.value,
-            'Accept': 'Accept',
-            'QUEUE_ID': [upload.id],
-            }
-        with person_logged_in(queue_admin):
-            view = self.makeView(distroseries, queue_admin, form)
-            view()
+    def test_view_renders_source_upload(self):
+        login(ADMIN_EMAIL)
+        upload = self.factory.makeSourcePackageUpload()
+        queue_admin = self.factory.makeArchiveAdmin(
+            upload.distroseries.main_archive)
+        with person_logged_in(queue_admin):
+            view = self.makeView(upload.distroseries, queue_admin)
+            view.setupQueueList()
+            html = view()
+        self.assertIn(upload.package_name, html)
+
+    def test_view_renders_build_upload(self):
+        login(ADMIN_EMAIL)
+        upload = self.factory.makeBuildPackageUpload()
+        queue_admin = self.factory.makeArchiveAdmin(
+            upload.distroseries.main_archive)
+        with person_logged_in(queue_admin):
+            view = self.makeView(upload.distroseries, queue_admin)
+            view.setupQueueList()
+            html = view()
+        self.assertIn(upload.package_name, html)
+
+    def test_view_renders_copy_upload(self):
+        login(ADMIN_EMAIL)
+        upload = self.factory.makeCopyJobPackageUpload()
+        queue_admin = self.factory.makeArchiveAdmin(
+            upload.distroseries.main_archive)
+        with person_logged_in(queue_admin):
+            view = self.makeView(upload.distroseries, queue_admin)
+            view.setupQueueList()
+            html = view()
+        self.assertIn(upload.package_name, html)

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-06-16 09:57:51 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-06-16 12:35:42 +0000
@@ -17,7 +17,6 @@
     Unicode,
     )
 import transaction
-
 from zope.component import getUtility
 from zope.interface import (
     classProvides,
@@ -38,11 +37,12 @@
 from lp.registry.interfaces.distroseriesdifference import (
     IDistroSeriesDifferenceSource,
     )
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+    IDistroSeriesDifferenceCommentSource,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.distroseries import DistroSeries
-from lp.registry.interfaces.distroseriesdifferencecomment import (
-    IDistroSeriesDifferenceCommentSource,
-    )
 from lp.services.database.stormbase import StormBase
 from lp.services.job.interfaces.job import (
     JobStatus,

=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2010-12-20 04:43:45 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2011-06-16 12:35:42 +0000
@@ -298,15 +298,17 @@
     :libraryfilealias: A LibraryFileAlias to link to. If it is expired,
       no link will be created.
   </tal:comment>
-  <tal:unexpired tal:condition="libraryfilealias/content">
-    <a tal:attributes="href libraryfilealias/http_url">
-      <tal:filename replace="libraryfilealias/filename"/>
-    </a>
-    (<span tal:replace="libraryfilealias/content/filesize/fmt:bytes" />)
-  </tal:unexpired>
-  <tal:expired tal:condition="not:libraryfilealias/content">
-    <span tal:content="libraryfilealias/filename"/>
-  </tal:expired>
+  <tal:not-none condition="libraryfilealias">
+    <tal:unexpired tal:condition="libraryfilealias/content">
+      <a tal:attributes="href libraryfilealias/http_url">
+        <tal:filename replace="libraryfilealias/filename"/>
+      </a>
+      (<span tal:replace="libraryfilealias/content/filesize/fmt:bytes" />)
+    </tal:unexpired>
+    <tal:expired tal:condition="not:libraryfilealias/content">
+      <span tal:content="libraryfilealias/filename"/>
+    </tal:expired>
+  </tal:not-none>
 </metal:macro>
 
 <metal:macro define-macro="package-iconlist">
@@ -333,12 +335,17 @@
          alt="[Debian Description Translation Project Indexes]"
          src="/@@/ubuntu-icon"
          title="Debian Description Translation Project Indexes"/>
-    <a tal:condition="not: packageupload/pending_delayed_copy"
-       tal:content="packageupload/displayname"
-       tal:attributes="
-         href packageupload/changesfile/http_url;
-         title string:Changes file for ${packageupload/displayname};">
-    </a>
+    <tal:not-delayed condition="not: packageupload/pending_delayed_copy">
+      <a tal:condition="packageupload/changesfile"
+         tal:content="packageupload/displayname"
+         tal:attributes="
+           href packageupload/changesfile/http_url;
+           title string:Changes file for ${packageupload/displayname};">
+      </a>
+      <tal:no-changes-file
+        condition="not: packageupload/changesfile"
+        replace="packageupload/displayname"/>
+    </tal:not-delayed>
     <tal:pending_delayed_copy_title
       condition="packageupload/pending_delayed_copy"
       replace="packageupload/displayname" />