Merge lp:~bac/launchpad/bug-499351-batching-dls into lp:launchpad

[Brad Crittenden]

Add batching to +download page.

[Brad Crittenden (bac)]

= Summary =

For products with a large number of series or releases the +download page becomes too
big, loads slowly, and sometimes times out. There is too much information to be useful.

== Proposed fix ==

Batch the releases shown.

== Pre-implementation notes ==

Talked with Curtis.

== Implementation details ==

Create a class with a tuple of (series, release) and batch those.

For product administrators the links to all series and releases are maintained at the
bottom of each page.

== Tests ==

bin/test -vv -t product-files-views.txt -t xx-product-files.txt

== Demo and Q/A ==

Look at a big project like https://edge.launchpad.net/bzr/+download and see that the
batching works right.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/config/schema-lazr.conf
  lib/lp/registry/browser/tests/product-files-views.txt
  lib/lp/registry/templates/product-files.pt
  lib/lp/registry/templates/productreleasefile-macros.pt
  lib/lp/registry/browser/product.py
  lib/lp/testing/factory.py
  lib/lp/registry/stories/product/xx-product-files.txt
  lib/lp/registry/browser/tests/test_views.py
  configs/development/launchpad-lazr.conf

== Pylint notices ==

lib/lp/registry/browser/product.py
    54: [F0401] Unable to import 'lazr.delegates' (No module named delegates)

lib/lp/testing/factory.py
    17: [F0401] Unable to import 'email.Encoders' (No module named Encoders)
    18: [F0401] Unable to import 'email.Utils' (No module named Utils)
    19: [F0401] Unable to import 'email.Message' (No module named Message)
    20: [F0401] Unable to import 'email.MIMEText' (No module named MIMEText)
    21: [F0401] Unable to import 'email.MIMEMultipart' (No module named MIMEMultipart)

[Curtis Hovey (sinzui)]

Thanks for doing this. As I noted on IRC, the casting of a list to a list on line 87 of the diff looks redundant. You can fix the code before landing this if it is redundant.

review: Approve

[Brad Crittenden (bac)]

Requesting a UI review. Screenshots are attached to the bug.

[Brad Crittenden (bac)]

Preparing for the UI review I discovered I neglected to put the batch navigator links into the page template. I've done that and updated one of the factories to allow a release to be re-used.

Diff at http://pastebin.ubuntu.com/359047/

[Curtis Hovey (sinzui)]

Hi brad.

Your diff exposes an issue in the markup. The template is using:
    <h2 style="font-size: 1.2em;">
Which is inconsistent in browsers. We should either remove the style attribute or change the unit to percentage (116, 123.1):
    <h2 style="font-size: 116%;">

[Curtis Hovey (sinzui)]

Hi Brad.

I am looking at the UI, (lp does not let me vote twice). There are some improvement we can make.
  1 - 4 of 12 results => 1 - 4 of 12 releases
You can doo this subclassing the batch navigator or by setting after __init__:
    batch = BatchNavigator(results)
    batch = batch.setHeadings('release', 'releases')

My eye gets lost scanning from the batch summary to the navigation. This is not a problem on other pages because the table border creates horizontal line for my eye to follow. I am not sure what to do in this case. This is also a reason I was aprehensive about using a batch navigator on with this layout. +search solved the problem by setting the background-color of the table and a lower/upper border to create a bar (see style.css: upper-batch-nav, lower-batch-nav). I think we could modify batchnavigator-navigation-links.pt to have a class on the table, but it does not distinguish between upper and lower. I see that you are calling upper and lower batches in the downloads template, but both those calls are mapped to the same template. Since they are using different views, the views could define the view/css_class that the template uses.

/me sees there is extra white space between the files table and the (+) Add download file when the page is viewed in webkit :(

review: Needs Fixing

[Brad Crittenden (bac)]

Comments from Martin on IRC:

bueno bac, I think sinzui is right on the mark with the problem 16:45
bac beuno: ok 16:45
beuno bac, one way of fixing this could be 16:46
beuno to isntead of presenting it as a navigation 16:46
beuno have an "See older releases >" link

[Brad Crittenden (bac)]

Thanks for the suggestions Curtis and the help with the css.

The diff is at http://pastebin.ubuntu.com/359769/

[Curtis Hovey (sinzui)]

Thanks for adding rules for consistent batch navigator presenations.

review: Approve (ui)

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2010-01-13 18:36:41 +0000
+++ configs/development/launchpad-lazr.conf	2010-01-20 22:01:13 +0000
@@ -130,6 +130,7 @@
 branchlisting_batch_size: 6
 mugshot_batch_size: 8
 announcement_batch_size: 4
+download_batch_size: 4
 openid_preauthorization_acl:
     localhost http://launchpad.dev/
 max_bug_feed_cache_minutes: 30

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2010-01-12 18:21:29 +0000
+++ lib/canonical/config/schema-lazr.conf	2010-01-20 22:01:13 +0000
@@ -935,6 +935,10 @@
 # The default size used in a batched display of announcements.
 announcement_batch_size: 5
 
+# The default size used in a batched display of product download
+# files.  The releases are batched, not the individual files.
+download_batch_size: 10
+
 # If restrict_to_team is set (such as on the beta
 # website), then this indicates the hostname suffix for
 # the non-restricted version of Launchpad.  Replacing

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2010-01-14 22:07:30 +0000
+++ lib/canonical/launchpad/icing/style.css	2010-01-20 22:01:13 +0000
@@ -1494,19 +1494,18 @@
   max-width: 55em;
 }
 
-#search-results div.upper-batch-nav {
+#search-results .upper-batch-nav, .freeform-batch .upper-batch-nav {
   border-top: 1px solid #d2d2d2;
   margin-top: 2.5em;
-  margin-bottom: 0.5em;
-}
-#search-results .search-batch {
+  margin-bottom: 10px;
   background: #e7effc;
-  margin-bottom: 10px;
 }
 
-#search-results div.lower-batch-nav {
+#search-results .lower-batch-nav, .freeform-batch .lower-batch-nav  {
   margin-top: 1.5em;
   border-top: 1px solid #d2d2d2;
+  margin-bottom: 10px;
+  background: #e7effc;
 }
 
 #search-results .search-result-link {

=== modified file 'lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt'
--- lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt	2009-12-21 11:36:04 +0000
+++ lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt	2010-01-20 22:01:13 +0000
@@ -8,10 +8,11 @@
     total context/batch/total;
     size context/batch/size"
   style="width: 100%;"
+  tal:attributes="class view/css_class"
 >
   <tbody>
     <tr>
-      <td style="white-space: nowrap" 
+      <td style="white-space: nowrap"
           class="batch-navigation-index">
         <strong tal:content="context/batch/startNumber">1</strong>
         <tal:block condition="python: size > 1">
@@ -22,7 +23,7 @@
         <tal:total replace="total">42</tal:total>
         <tal:heading content="context/heading">results</tal:heading>
       </td>
-      <td style="text-align: right; white-space: nowrap" 
+      <td style="text-align: right; white-space: nowrap"
           class="batch-navigation-links">
         <a
           tal:condition="first_page_url"

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2009-08-18 22:26:02 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2010-01-20 22:01:13 +0000
@@ -35,6 +35,8 @@
 class UpperBatchNavigationView(LaunchpadView):
     """Only render navigation links if there is a batch."""
 
+    css_class = "upper-batch-nav"
+
     def render(self):
         if self.context.currentBatch():
             return LaunchpadView.render(self)
@@ -44,6 +46,8 @@
 class LowerBatchNavigationView(UpperBatchNavigationView):
     """Only render bottom navigation links if there are multiple batches."""
 
+    css_class = "lower-batch-nav"
+
     def render(self):
         if (self.context.currentBatch() and
             (self.context.nextBatchURL() or

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2010-01-11 13:46:49 +0000
+++ lib/lp/registry/browser/product.py	2010-01-20 22:01:13 +0000
@@ -712,10 +712,11 @@
     def processDeleteFiles(self):
         """If the 'delete_files' button was pressed, process the deletions."""
         del_count = None
-        self.delete_ids = [int(value) for key, value in self.form.items()
-                           if key.startswith('checkbox')]
         if 'delete_files' in self.form:
             if self.request.method == 'POST':
+                self.delete_ids = [
+                    int(value) for key, value in self.form.items()
+                    if key.startswith('checkbox')]
                 del(self.form['delete_files'])
                 releases = self.getReleases()
                 del_count = self.deleteFiles(releases)
@@ -979,12 +980,25 @@
         return results
 
 
+class SeriesReleasePair:
+    """Class for holding a series and release.
+
+    Replaces the use of a (series, release) tuple so that it can be more
+    clearly addressed in the view class.
+    """
+    def __init__(self, series, release):
+        self.series = series
+        self.release = release
+
+
 class ProductDownloadFilesView(LaunchpadView,
                                SortSeriesMixin,
                                ProductDownloadFileMixin):
     """View class for the product's file downloads page."""
     __used_for__ = IProduct
 
+    batch_size = config.launchpad.download_batch_size
+
     @property
     def page_title(self):
         return "%s project files" % self.context.displayname
@@ -1003,6 +1017,24 @@
         return releases
 
     @cachedproperty
+    def series_and_releases_batch(self):
+        """Get a batch of series and release
+
+        Each entry returned is a tuple of (series, release).
+        """
+        series_and_releases = []
+        for series in self.sorted_series_list:
+            for release in series.releases:
+                if len(release.files) > 0:
+                    pair = SeriesReleasePair(series, release)
+                    if pair not in series_and_releases:
+                        series_and_releases.append(pair)
+        batch = BatchNavigator(series_and_releases, self.request,
+                               size=self.batch_size)
+        batch.setHeadings("release", "releases")
+        return batch
+
+    @cachedproperty
     def has_download_files(self):
         """Across series and releases do any download files exist?"""
         for series in self.product.series:

=== added file 'lib/lp/registry/browser/tests/product-files-views.txt'
--- lib/lp/registry/browser/tests/product-files-views.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/product-files-views.txt	2010-01-20 22:01:13 +0000
@@ -0,0 +1,70 @@
+ Product Download Files Page
+=============================
+
+Test for the product/+download page.
+
+    >>> product = factory.makeProduct(name='alfajore')
+    >>> productseries = factory.makeSeries(product=product, name="sammy")
+    >>> milestone = factory.makeMilestone(productseries=productseries,
+    ...                                   name="apple")
+    >>> release_file = factory.makeProductReleaseFile(
+    ...     product=product, productseries=productseries, milestone=milestone)
+    >>> view = create_initialized_view(product, '+download')
+
+    >>> view.batch_size
+    4
+
+    >>> batch = view.series_and_releases_batch.currentBatch()
+    >>> print len(list(batch))
+    1
+
+    >>> def print_series_release(sr):
+    ...     print "%s from the %s series" % (sr.release.name_with_codename,
+    ...                                      sr.series.series.name)
+
+    >>> for sr in batch:
+    ...     print_series_release(sr)
+    apple from the sammy series
+
+    >>> product = factory.makeProduct(name='bombilla')
+    >>> for i in range(1,5):
+    ...     productseries = factory.makeSeries(product=product, name="s%d"%i)
+    ...     for j in range(1,4):
+    ...         milestone = factory.makeMilestone(productseries=productseries,
+    ...                                           name="%d.%d"%(i,j))
+    ...         release_file = factory.makeProductReleaseFile(
+    ...             product=product, productseries=productseries, milestone=milestone)
+    >>> view = create_initialized_view(product, '+download')
+    >>> batch = view.series_and_releases_batch.currentBatch()
+    >>> print len(batch)
+    4
+    >>> for sr in batch:
+    ...     print_series_release(sr)
+    4.3 from the s4 series
+    4.2 from the s4 series
+    4.1 from the s4 series
+    3.3 from the s3 series
+
+    >>> batch = batch.nextBatch()
+    >>> for sr in batch:
+    ...     print_series_release(sr)
+    3.2 from the s3 series
+    3.1 from the s3 series
+    2.3 from the s2 series
+    2.2 from the s2 series
+
+For an administrator of the project, at the bottom of each batched
+page will be links to add new files for each series and release.
+
+    >>> from canonical.launchpad.testing.pages import (
+    ...     extract_text, find_tag_by_id)
+    >>> login_person(product.owner)
+    >>> view = create_initialized_view(product, '+download',
+    ...                                principal=product.owner)
+    >>> admin_links = find_tag_by_id(view.render(), 'admin-links')
+    >>> content = extract_text(admin_links)
+    >>> print content
+    Add download file to the s4 series for release: 4.3, 4.2, 4.1
+    Add download file to the s3 series for release: 3.3, 3.2, 3.1
+    Add download file to the s2 series for release: 2.3, 2.2, 2.1
+    Add download file to the s1 series for release: 1.3, 1.2, 1.1

=== modified file 'lib/lp/registry/browser/tests/test_views.py'
--- lib/lp/registry/browser/tests/test_views.py	2010-01-09 16:48:38 +0000
+++ lib/lp/registry/browser/tests/test_views.py	2010-01-20 22:01:13 +0000
@@ -27,6 +27,7 @@
     'user-to-user-views.txt': LaunchpadFunctionalLayer,
     'distributionsourcepackage-views.txt': LaunchpadFunctionalLayer,
     'product-edit-people-view.txt': LaunchpadFunctionalLayer,
+    'product-files-views.txt': LaunchpadFunctionalLayer,
 }
 
 

=== modified file 'lib/lp/registry/stories/product/xx-product-files.txt'
--- lib/lp/registry/stories/product/xx-product-files.txt	2009-11-10 01:00:23 +0000
+++ lib/lp/registry/stories/product/xx-product-files.txt	2010-01-20 22:01:13 +0000
@@ -407,8 +407,9 @@
     >>> firefox_owner.open('http://launchpad.dev/firefox/+download')
     >>> firefox_owner.url
     'http://launchpad.dev/firefox/+download'
+
     >>> checkbox = firefox_owner.getControl(
-    ...     name="checkbox_2_0_1")
+    ...     name="checkbox_2_1")
     >>> checkbox.value = checkbox.options
     >>> firefox_owner.getControl("Delete Files").click()
     >>> get_feedback_messages(firefox_owner.contents)

=== modified file 'lib/lp/registry/templates/product-files.pt'
--- lib/lp/registry/templates/product-files.pt	2009-10-26 16:57:55 +0000
+++ lib/lp/registry/templates/product-files.pt	2010-01-20 22:01:13 +0000
@@ -7,9 +7,7 @@
   i18n:domain="launchpad"
 >
 
-<h1 metal:fill-slot="heading">
-  Download project files
-</h1>
+<h1 metal:fill-slot="heading">Download project files</h1>
 
 <body>
 
@@ -37,117 +35,135 @@
       </p>
     </div>
 
-    <div
-      tal:repeat="series view/sorted_series_list"
-      tal:attributes="id series/name/fmt:css-id/series-">
-
-      <tal:seriesfilesexist condition="series/has_release_files">
-        <div tal:condition="not: repeat/series/start" class="portlet-border" />
-        <tal:releases repeat="release series/releases">
-          <tal:releasefilesexist condition="release/files">
-            <div tal:condition="not: repeat/release/start" class="portlet-border" />
-            <div class="top-portlet">
-              <h2 style="font-size: 1.2em;">
-                <a tal:attributes="href release/fmt:url">
-                  <span tal:replace="release/name_with_codename" /> release</a>
-                from the
-                <a tal:attributes="href series/fmt:url"
-                   tal:content="series/name">name</a> series released
-                <span
-                   tal:attributes="title release/datereleased/fmt:datetime"
-                   tal:content="release/datereleased/fmt:approximatedate" />
-              </h2>
-
-              <div tal:attributes="id release/version/fmt:css-id/release-information-"
-                   tal:condition="python: release.release_notes or release.changelog">
-
-                <fieldset class="collapsible collapsed" style="padding: 0px;">
-                  <legend>Release information</legend>
-
-                  <div tal:condition="release/release_notes">
-                    <strong>Release notes:</strong>
-                    <div style="margin-bottom: 0px;"
-                         tal:attributes="id release/version/fmt:css-id/release-notes-"
-                         tal:define="notes release/release_notes/fmt:shorten/800"
-                         tal:content="structure notes/fmt:text-to-html">
-                      ProductRelease.release_notes
-                    </div>
-                  </div>
-
-                  <div tal:condition="release/changelog">
-                    <strong>Changelog:</strong>
-                    <div style="margin-bottom: 0px;"
-                         tal:attributes="id release/version/fmt:css-id/changelog-"
-                         tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
-                      ProductRelease.changelog.
-                    </div>
-                  </div>
-                </fieldset>
-              </div>
-              <div>
-                <table class="listing" style="margin-top: 1em;">
-                  <thead>
-                    <tr>
-                      <th width="35%">File</th>
-                      <th width="45%">Description</th>
-                      <th>Downloads</th>
-                      <th tal:condition="has_edit">Delete</th>
-                    </tr>
-                  </thead>
-                  <tbody>
-                    <tr tal:repeat="file release/files">
-                      <tal:define-vars
-                         tal:define="checkbox_index string:${repeat/series/index}_${repeat/release/index}_${repeat/file/index}">
-                        <tal:release-file
-                           metal:use-macro="file/@@+macros/detailed_display" />
-                  </tal:define-vars>
-                    </tr>
-                  </tbody>
-                  <tfoot tal:condition="release/total_downloads">
-                    <tr>
-                      <th colspan="2" style="padding-top: 1em; text-align: right;">
-                        Total downloads:
-                      </th>
-                      <td style="border: none; text-align: center;">
-                        <span tal:replace="release/total_downloads/fmt:intcomma" />
-                      </td>
-                      <td style="border: none;"
-                          tal:condition="has_edit" />
-                    </tr>
-                  </tfoot>
-                </table>
-              </div>
-              <div tal:condition="not: release/total_downloads"
-                   style="margin-top: 2em;" />
-
-              <tal:linkexists
-                 define="link release/menu:context/add_file"
-                 condition="link/enabled">
-                <ul style="float: right; margin-top: 1em;">
-                  <li>
-                    <a tal:replace="structure link/fmt:link" />
+    <div class="freeform-batch">
+    <tal:batch condition="view/has_download_files"
+               define="batch view/series_and_releases_batch">
+      <tal:multipage tal:condition="batch/has_multiple_pages">
+        <tal:navigation
+           replace="structure batch/@@+navigation-links-upper"/>
+      </tal:multipage>
+      <tal:repeat_current_batch repeat="batch_item batch/currentBatch">
+        <div tal:define="series batch_item/series;
+                         release batch_item/release"
+             tal:attributes="id series/name/fmt:css-id/series-">
+
+          <tal:seriesfilesexist condition="series/has_release_files">
+            <div tal:condition="not: repeat/batch_item/start" class="portlet-border" />
+
+            <tal:releasefilesexist condition="release/files">
+
+              <div class="top-portlet">
+                <h2 style="font-size: 1.2em;">
+                  <a tal:attributes="href release/fmt:url">
+                    <span tal:replace="release/name_with_codename" /> release</a>
+                  from the
+                  <a tal:attributes="href series/fmt:url"
+                     tal:content="series/name">name</a> series released
+                  <span
+                     tal:attributes="title release/datereleased/fmt:datetime"
+                     tal:content="release/datereleased/fmt:approximatedate" />
+                </h2>
+
+                <div tal:attributes="id release/version/fmt:css-id/release-information-"
+                     tal:condition="python: release.release_notes or release.changelog">
+
+                  <fieldset class="collapsible collapsed" style="padding: 0px;">
+                    <legend>Release information</legend>
+
+                    <div tal:condition="release/release_notes">
+                      <strong>Release notes:</strong>
+                      <div style="margin-bottom: 0px;"
+                           tal:attributes="id release/version/fmt:css-id/release-notes-"
+                           tal:define="notes release/release_notes/fmt:shorten/800"
+                           tal:content="structure notes/fmt:text-to-html">
+                        ProductRelease.release_notes
+                      </div>
+                    </div>
+
+                    <div tal:condition="release/changelog">
+                      <strong>Changelog:</strong>
+                      <div style="margin-bottom: 0px;"
+                           tal:attributes="id release/version/fmt:css-id/changelog-"
+                           tal:content="structure release/changelog/fmt:obfuscate-email/fmt:text-to-html">
+                        ProductRelease.changelog.
+                      </div>
+                  </div>
+                  </fieldset>
+                </div>
+                <div>
+                  <table class="listing" style="margin-top: 1em;">
+                    <thead>
+                      <tr>
+                        <th width="35%">File</th>
+                        <th width="45%">Description</th>
+                        <th>Downloads</th>
+                        <th tal:condition="has_edit">Delete</th>
+                      </tr>
+                    </thead>
+                    <tbody>
+                      <tr tal:repeat="file release/files">
+                        <tal:define-vars
+                           tal:define="checkbox_index string:${repeat/batch_item/index}_${repeat/file/index}">
+                          <tal:release-file
+                             metal:use-macro="file/@@+macros/detailed_display" />
+                        </tal:define-vars>
+                      </tr>
+                    </tbody>
+                    <tfoot tal:condition="release/total_downloads">
+                      <tr>
+                        <th colspan="2" style="padding-top: 1em; text-align: right;">
+                          Total downloads:
+                        </th>
+                        <td style="border: none; text-align: center;">
+                          <span tal:replace="release/total_downloads/fmt:intcomma" />
+                        </td>
+                        <td style="border: none;"
+                            tal:condition="has_edit" />
+                      </tr>
+                    </tfoot>
+                  </table>
+                </div>
+                <div tal:condition="not: release/total_downloads"
+                     style="margin-bottom: 1em;" />
+
+                <tal:linkexists
+                   define="link release/menu:context/add_file"
+                   condition="link/enabled">
+                  <ul style="float: right; margin-top: 1em;">
+                    <li>
+                      <a tal:replace="structure link/fmt:link" />
                   </li>
-                </ul>
-              </tal:linkexists>
-            </div>
-          </tal:releasefilesexist>
+                  </ul>
+                </tal:linkexists>
+              </div>
+            </tal:releasefilesexist>
+
+            <br />
+          </tal:seriesfilesexist>
+
+        </div>
+      </tal:repeat_current_batch>
+      <tal:multipage condition="batch/has_multiple_pages">
+        <tal:navigation
+           replace="structure batch/@@+navigation-links-lower"/>
+      </tal:multipage>
+    </tal:batch>
+    </div>
+    <div id="admin-links">
+      <tal:series repeat="series view/sorted_series_list">
+        <tal:releases condition="series/releases">
+          <p tal:condition="has_edit" class="add-files">
+            Add download file to the
+            <a tal:attributes="href series/fmt:url"
+               tal:content="series/name">name</a> series for release:
+            <tal:release repeat="release series/releases">
+              <a tal:attributes="href string:${series/name}/${release/version}/+adddownloadfile"
+                 tal:content="release/version"
+                 >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
+            </tal:release>
+          </p>
         </tal:releases>
-        <br />
-      </tal:seriesfilesexist>
-
-      <tal:releases condition="series/releases">
-        <p tal:condition="has_edit" class="add-files">
-          Add download file to the
-          <a tal:attributes="href series/fmt:url"
-             tal:content="series/name">name</a> series for release:
-          <tal:release repeat="release series/releases">
-            <a tal:attributes="href string:${series/name}/${release/version}/+adddownloadfile"
-               tal:content="release/version"
-               >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
-          </tal:release>
-        </p>
-      </tal:releases>
-
+      </tal:series>
     </div>
     <input tal:condition="python: has_edit and view.has_download_files"
         type="submit"
@@ -157,5 +173,4 @@
   </form>
 </div>
 </body>
-</html
->
+</html>

=== modified file 'lib/lp/registry/templates/productreleasefile-macros.pt'
--- lib/lp/registry/templates/productreleasefile-macros.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/productreleasefile-macros.pt	2010-01-20 22:01:13 +0000
@@ -10,7 +10,7 @@
       - 'file': The ProductReleaseFile
       - 'has_edit': A boolean saying whether the user has edit rights on this
                     ProductReleaseFile.
-      - checkbox_index: A unique integer to be appended to the name of the
+      - checkbox_index: A unique string to be appended to the name of the
                         checkbox included in this macro.
   </tal:comment>
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-01-15 04:09:45 +0000
+++ lib/lp/testing/factory.py	2010-01-20 22:01:13 +0000
@@ -76,7 +76,7 @@
 from lp.translations.interfaces.translationgroup import (
     ITranslationGroupSet)
 from lp.translations.interfaces.translator import ITranslatorSet
-from lp.translations.interfaces.translationsperson import ITranslationsPerson 
+from lp.translations.interfaces.translationsperson import ITranslationsPerson
 from canonical.launchpad.ftests._sqlobject import syncUpdate
 from lp.services.mail.signedmessage import SignedMessage
 from lp.services.worlddata.interfaces.country import ICountrySet
@@ -512,7 +512,8 @@
             group = self.makeTranslationGroup()
         if person is None:
             person = self.makePerson()
-        ITranslationsPerson(person).translations_relicensing_agreement = license
+        ITranslationsPerson(person).translations_relicensing_agreement = (
+            license)
         return getUtility(ITranslatorSet).new(group, language, person)
 
     def makeMilestone(
@@ -527,25 +528,34 @@
                          productseries=productseries,
                          name=name)
 
-    def makeProductRelease(self, milestone=None, product=None):
+    def makeProductRelease(self, milestone=None, product=None,
+                           productseries=None):
         if milestone is None:
-            milestone = self.makeMilestone(product=product)
+            milestone = self.makeMilestone(product=product,
+                                           productseries=productseries)
         return milestone.createProductRelease(
             milestone.product.owner, datetime.now(pytz.UTC))
 
-    def makeProductReleaseFile(self, signed=True):
+    def makeProductReleaseFile(self, signed=True,
+                               product=None, productseries=None,
+                               milestone=None,
+                               release=None,
+                               description="test file"):
         signature_filename = None
         signature_content = None
         if signed:
             signature_filename = 'test.txt.asc'
             signature_content = '123'
-        release = self.makeProductRelease()
+        if release is None:
+            release = self.makeProductRelease(product=product,
+                                              productseries=productseries,
+                                              milestone=milestone)
         return release.addReleaseFile(
             'test.txt', 'test', 'text/plain',
             uploader=release.milestone.product.owner,
             signature_filename=signature_filename,
             signature_content=signature_content,
-            description="test file")
+            description=description)
 
     def makeProduct(self, *args, **kwargs):
         """As makeProductNoCommit with an explicit transaction commit.