launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-685401-expired-queue-files into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #685401 LocationError in DistroSeries:+queue
  https://bugs.launchpad.net/bugs/685401
  #691844 canonical.launchpad.scripts.logger broken in Python 2.7
  https://bugs.launchpad.net/bugs/691844


Files related to a PackageUpload can be expired once the upload is processed (translations tarballs are occasionally manually expired, and binaries are expired after a series' obsolescence). DistroSeries:+queue doesn't handle this case, crashing when it attempts to display the file size.

This branch fixes +queue to not link to nor attempt to show the size of expired files. I've slightly augmented an existing doctest to verify this behaviour.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-685401-expired-queue-files/+merge/44190
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-685401-expired-queue-files into lp:launchpad.

=== modified file 'lib/canonical/launchpad/scripts/logger.py'
--- lib/canonical/launchpad/scripts/logger.py	2010-10-22 10:23:17 +0000
+++ lib/canonical/launchpad/scripts/logger.py	2010-12-20 04:50:19 +0000
@@ -428,7 +428,7 @@
     # Make it print output in a standard format, suitable for
     # both command line tools and cron jobs (command line tools often end
     # up being run from inside cron, so this is a good thing).
-    hdlr = logging.StreamHandler(strm=out_stream)
+    hdlr = logging.StreamHandler(out_stream)
     # We set the level on the handler rather than the logger, so other
     # handlers with different levels can be added for things like debug
     # logs.

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt'
--- lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-queue-pages.txt	2010-12-20 04:50:19 +0000
@@ -170,7 +170,8 @@
 
 == Queue item filelist ==
 
-First set up some additional data: a package diff to show in the listing.
+First set up some additional data to show in the listing: a package diff
+and an extra, expired, source file.
 
     >>> from zope.security.proxy import removeSecurityProxy
     >>> from StringIO import StringIO
@@ -190,6 +191,9 @@
     >>> diff.diff_content = getUtility(ILibrarianClient).addFile(
     ...     'alsa-utils.diff.gz', 11, StringIO('i am a diff'),
     ...     'application/gzipped-patch')
+    >>> sprf = new.addFile(factory.makeLibraryFileAlias(
+    ...     filename='alsa-utils_1.0.9a-4ubuntu1.diff.gz'))
+    >>> removeSecurityProxy(sprf.libraryfile).content = None
     >>> logout()
 
 Each queue item has a hidden 'filelist' section which is
@@ -212,20 +216,23 @@
   ...     anon_browser.contents, 'queue-4')
 
 It contains a list of files related to the queue item clicked, followed
-by its size, one file per line:
+by its size, one file per line. Expired files have no size.
 
   >>> for row in filelist:
   ...     print extract_text(row)
   alsa-utils_1.0.9a-4ubuntu1.dsc (3 bytes)
+  alsa-utils_1.0.9a-4ubuntu1.diff.gz
   diff from 1.0.9a-4 to 1.0.9a-4ubuntu1 (11 bytes)
 
-Each filename links to its respective librarian URL:
+Each unexpired filename links to its respective librarian URL. Expired
+files have no link, so we just get None.
 
   >>> for row in filelist:
   ...     print row.find('a')
   <a href="http://localhost:58000/.../alsa-utils_1.0.9a-4ubuntu1.dsc";>
     alsa-utils_1.0.9a-4ubuntu1.dsc
   </a>
+  None
   <a href="http://localhost:58000/.../alsa-utils.diff.gz";>diff from 1.0.9a-4 to 1.0.9a-4ubuntu1</a>
 
 On binary queue items we also present the stamp 'NEW' for files never

=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt	2010-05-13 12:04:56 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt	2010-12-20 04:50:19 +0000
@@ -232,11 +232,8 @@
           style="display:none">
         <td/>
         <td tal:condition="view/availableActions"/>
-        <td>
-          <a tal:attributes="href file/libraryfile/http_url">
-            <tal:filename content="file/libraryfile/filename"/>
-          </a>
-          (<span tal:replace="file/libraryfile/content/filesize/fmt:bytes" />)
+        <td tal:define="libraryfilealias file/libraryfile">
+          <metal:file use-macro="template/macros/package-file"/>
         </td>
         <td colspan="6"/>
       </tr>
@@ -252,11 +249,8 @@
             style="display:none">
           <td/>
           <td tal:condition="view/availableActions"/>
-          <td>
-            <a tal:attributes="href file/libraryfile/http_url">
-              <tal:filename replace="file/libraryfile/filename"/>
-            </a>
-            (<tal:size replace="file/libraryfile/content/filesize/fmt:bytes" />)
+          <td tal:define="libraryfilealias file/libraryfile">
+            <metal:file use-macro="template/macros/package-file"/>
             <span style="color: red" tal:condition="is_new">NEW</span>
           </td>
           <td tal:content="version"/>
@@ -274,12 +268,8 @@
           style="display:none">
         <td/>
         <td tal:condition="view/availableActions"></td>
-        <td>
-          <a tal:attributes="href custom/libraryfilealias/http_url">
-            <tal:filename content="custom/libraryfilealias/filename"/>
-          </a>
-          (<tal:size replace="
-                custom/libraryfilealias/content/filesize/fmt:bytes" />)
+        <td tal:define="libraryfilealias custom/libraryfilealias">
+          <metal:file use-macro="template/macros/package-file"/>
         </td>
         <td colspan="6"/>
       </tr>
@@ -302,6 +292,23 @@
 
 </metal:macro>
 
+<metal:macro define-macro="package-file">
+  <tal:comment replace="nothing">
+    This macro expects the following variables defined:
+    :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>
+</metal:macro>
+
 <metal:macro define-macro="package-iconlist">
   <tal:comment replace="nothing">
     This macro expects the following variables defined: