launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/prf-unique-files into lp:launchpad.

Commit message:
Do not find duplicate release files.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #556751 in Launchpad itself: "downloadable file repeated many times on project page"
  https://bugs.launchpad.net/launchpad/+bug/556751

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/prf-unique-files/+merge/134939

Pages timeout when they attempt to load all the release files for
a milestone or for the latest release (+index and +milestone). There
are several pathological project like glib and rhythmbox that have
hundreds of duplicate release files. The timeout may also manifest
as a 504 Gateway error instead of a oops.

--------------------------------------------------------------------

RULES

    Pre-implementation: wgrant and stevenk
    * Two series can have overlapping release file globs.
      * The rules to check if the file exists uses the expect path from
        the series to the release.
      * But the rules to create the file know that milestones must be
        unique to a project, so it takes a short cut to find the release,
        and find it on a different series!
    * Ensure the product-release-finder does not upload duplicate
      files to a released milestone.
    * Prepare a SQL/API script that will delete the duplicate files.


QA

    * Visit http://qastaging.launchpad.net/glib
    * Verify the page loads and shows only one file
      (because the download portlet want to show everything)
    * Visit https://qastaging.launchpad.net/glib/2.30
    * Verify that the page lists only one file.
    * Visit https://qastaging.launchpad.net/glib/+milestone/2.29.4
    * Verify only one file is listed.


LINT

    lib/lp/registry/scripts/productreleasefinder/finder.py
    lib/lp/registry/tests/test_prf_finder.py


TEST

    ./bin/test -vv -t HandleReleaseTestCase lp.registry.tests.test_prf_finder


IMPLEMENTATION

I changed the hasReleaseFile() to call product.getRelease() as is done
by addReleaseTarball().
    lib/lp/registry/scripts/productreleasefinder/finder.py
    lib/lp/registry/tests/test_prf_finder.py
-- 
https://code.launchpad.net/~sinzui/launchpad/prf-unique-files/+merge/134939
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/prf-unique-files into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/productreleasefinder/finder.py'
--- lib/lp/registry/scripts/productreleasefinder/finder.py	2012-11-01 16:14:00 +0000
+++ lib/lp/registry/scripts/productreleasefinder/finder.py	2012-11-19 15:07:25 +0000
@@ -151,14 +151,12 @@
         try:
             product = getUtility(IProductSet).getByName(product_name)
             if product is not None:
-                series = product.getSeries(series_name)
-                if series is not None:
-                    release = series.getRelease(release_name)
-                    if release is not None:
-                        for fileinfo in release.files:
-                            if filename == fileinfo.libraryfile.filename:
-                                has_file = True
-                                break
+                release = product.getRelease(release_name)
+                if release is not None:
+                    for fileinfo in release.files:
+                        if filename == fileinfo.libraryfile.filename:
+                            has_file = True
+                            break
         finally:
             self.ztm.abort()
         return has_file

=== modified file 'lib/lp/registry/tests/test_prf_finder.py'
--- lib/lp/registry/tests/test_prf_finder.py	2012-11-01 16:14:00 +0000
+++ lib/lp/registry/tests/test_prf_finder.py	2012-11-19 15:07:25 +0000
@@ -301,6 +301,21 @@
         release = trunk.getRelease('42.0')
         self.assertEqual(release.files.count(), 1)
 
+    def test_handleReleaseTwice_multiple_series(self):
+        # Series can have overlaping release file globs, but versions
+        # are unique to a project. A file is uploaded to a release only
+        # once, regardless of which series wants the upload.
+        ztm = self.layer.txn
+        logging.basicConfig(level=logging.CRITICAL)
+        prf = ProductReleaseFinder(ztm, logging.getLogger())
+        file_path, file_name = self.create_tarball('evolution-1.2.3.tar.gz')
+        prf.handleRelease('evolution', 'trunk', file_path)
+        file_path, file_name = self.create_tarball('evolution-1.2.3.tar.gz')
+        prf.handleRelease('evolution', '1.0', file_path)
+        product = getUtility(IProductSet).getByName('evolution')
+        release = product.getMilestone('1.2.3').product_release
+        self.assertEqual(release.files.count(), 1)
+
     def test_handleRelease_alternate_verstion(self):
         """Verify that tar.gz and tar.bz2 versions are both uploaded."""
         ztm = self.layer.txn