launchpad-reviewers team mailing list archive

[Michael Nelson <michael.nelson@xxxxxxxxxxxxx>]

Michael Nelson has proposed merging lp:~michael.nelson/launchpad/include-binary-size into lp:launchpad.

Commit message:
Add optional include_sizes kwarg to BinaryPackagePublishingHistory.binaryFileUrls() and expose on API.

Requested reviews:
  William Grant (wgrant)
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1088527 in Launchpad itself: "Needlessly asks for exact package name, file size, uploaded version"
  https://bugs.launchpad.net/launchpad/+bug/1088527

For more details, see:
https://code.launchpad.net/~michael.nelson/launchpad/include-binary-size/+merge/150831

Add an optional include_sizes kwarg option to BPPH.binaryFileUrls(), defaulting to False to preserve current behaviour.

Brief pre-imp with wgrant discussed on bug 1088527.

./bin/test -vvct TestBinaryPackagePublishingHistory -t BinaryPackagePublishingHistoryWebserviceTests

I'll start the full test run on my instance.
-- 
https://code.launchpad.net/~michael.nelson/launchpad/include-binary-size/+merge/150831
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/include-binary-size into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/tests/test_publishing_webservice.py'
--- lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/browser/tests/test_publishing_webservice.py	2013-02-27 17:18:32 +0000
@@ -17,18 +17,41 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def test_binaryFileUrls(self):
+    def make_bpph_url(self, person):
         bpr = self.factory.makeBinaryPackageRelease()
         self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
         bpph = self.factory.makeBinaryPackagePublishingHistory(
             binarypackagerelease=bpr)
-        person = self.factory.makePerson()
-        webservice = webservice_for_person(
-            person, permission=OAuthPermission.READ_PUBLIC)
         with person_logged_in(person):
             bpph_url = api_url(bpph)
+
+        return bpph_url
+
+    def test_binaryFileUrls(self):
+        person = self.factory.makePerson()
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.READ_PUBLIC)
+        bpph_url = self.make_bpph_url_for(person)
+
         response = webservice.named_get(
             bpph_url, 'binaryFileUrls', api_version='devel')
-        self.assertEqual(200, response.status)
-        urls = response.jsonBody()
-        self.assertEqual(1, len(urls))
+
+        self.assertEqual(200, response.status)
+        urls = response.jsonBody()
+        self.assertEqual(1, len(urls))
+        self.assertTrue(type(urls[0]) == unicode)
+
+    def test_binaryFileUrls_include_sizes(self):
+        person = self.factory.makePerson()
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.READ_PUBLIC)
+        bpph_url = self.make_bpph_url_for(person)
+
+        response = webservice.named_get(
+            bpph_url, 'binaryFileUrls', include_sizes=True,
+            api_version='devel')
+
+        self.assertEqual(200, response.status)
+        urls = response.jsonBody()
+        self.assertEqual(1, len(urls))
+        self.assertTrue(type(urls[0]) == dict)

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2013-02-20 12:28:38 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2013-02-27 17:18:32 +0000
@@ -946,10 +946,14 @@
         """
 
     @export_read_operation()
+    @operation_parameters(
+        include_sizes=Bool(title=_("Include Sizes"), required=False))
     @operation_for_version("devel")
-    def binaryFileUrls():
+    def binaryFileUrls(include_sizes=False):
         """URLs for this binary publication's binary files.
 
+        :param include_sizes: Return a dict with keys url and size
+            for each url instead of a list of urls.
         :return: A collection of URLs for this binary.
         """
 

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2013-02-20 12:30:47 +0000
+++ lib/lp/soyuz/model/publishing.py	2013-02-27 17:18:32 +0000
@@ -1375,10 +1375,14 @@
         """See `IPublishing`."""
         self.setDeleted(removed_by, removal_comment)
 
-    def binaryFileUrls(self):
+    def binaryFileUrls(self, include_sizes=False):
         """See `IBinaryPackagePublishingHistory`."""
         binary_urls = proxied_urls(
             [f.libraryfilealias for f in self.files], self.archive)
+        if include_sizes:
+            sizes = [f.libraryfilealias.content.filesize for f in self.files]
+            return [dict(url=url, size=size)
+                for url, size in zip(binary_urls, sizes)]
         return binary_urls
 
 

=== modified file 'lib/lp/soyuz/tests/test_publishing_models.py'
--- lib/lp/soyuz/tests/test_publishing_models.py	2013-01-24 01:09:04 +0000
+++ lib/lp/soyuz/tests/test_publishing_models.py	2013-02-27 17:18:32 +0000
@@ -4,11 +4,9 @@
 """Test model and set utilities used for publishing."""
 
 from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
-from lp.services.database.constants import UTC_NOW
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.webapp.publisher import canonical_url
 from lp.soyuz.enums import BinaryPackageFileType
@@ -154,34 +152,56 @@
 
     layer = LaunchpadFunctionalLayer
 
+    def get_urls_for_bpph(self, bpph, include_sizes=False):
+        bpr = bpph.binarypackagerelease
+        archive = bpph.archive
+        urls = [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
+            for f in bpr.files]
+
+        if include_sizes:
+            sizes = [f.libraryfile.content.filesize for f in bpr.files]
+            return [dict(url=url, size=size)
+                for url, size in zip(urls, sizes)]
+        return urls
+
+    def make_bpph(self, num_binaries=1):
+        archive = self.factory.makeArchive(private=False)
+        bpr = self.factory.makeBinaryPackageRelease()
+        filetypes = [BinaryPackageFileType.DEB, BinaryPackageFileType.DDEB]
+        for count in range(num_binaries):
+            self.factory.makeBinaryPackageFile(binarypackagerelease=bpr,
+                                               filetype=filetypes[count % 2])
+        return self.factory.makeBinaryPackagePublishingHistory(
+            binarypackagerelease=bpr, archive=archive)
+
     def test_binaryFileUrls_no_binaries(self):
-        bpr = self.factory.makeBinaryPackageRelease()
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr)
+        bpph = self.make_bpph(num_binaries=0)
         expected_urls = []
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
-
-    def get_urls_for_binarypackagerelease(self, bpr, archive):
-        return [ProxiedLibraryFileAlias(f.libraryfile, archive).http_url
-            for f in bpr.files]
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual(expected_urls, urls)
 
     def test_binaryFileUrls_one_binary(self):
-        archive = self.factory.makeArchive(private=False)
-        bpr = self.factory.makeBinaryPackageRelease()
-        self.factory.makeBinaryPackageFile(binarypackagerelease=bpr)
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr, archive=archive)
-        expected_urls = self.get_urls_for_binarypackagerelease(bpr, archive)
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
+        bpph = self.make_bpph(num_binaries=1)
+        expected_urls = self.get_urls_for_bpph(bpph)
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual(expected_urls, urls)
 
     def test_binaryFileUrls_two_binaries(self):
-        archive = self.factory.makeArchive(private=False)
-        bpr = self.factory.makeBinaryPackageRelease()
-        self.factory.makeBinaryPackageFile(
-            binarypackagerelease=bpr, filetype=BinaryPackageFileType.DEB)
-        self.factory.makeBinaryPackageFile(
-            binarypackagerelease=bpr, filetype=BinaryPackageFileType.DDEB)
-        bpph = self.factory.makeBinaryPackagePublishingHistory(
-            binarypackagerelease=bpr, archive=archive)
-        expected_urls = self.get_urls_for_binarypackagerelease(bpr, archive)
-        self.assertContentEqual(expected_urls, bpph.binaryFileUrls())
+        bpph = self.make_bpph(num_binaries=2)
+        expected_urls = self.get_urls_for_bpph(bpph)
+
+        urls = bpph.binaryFileUrls()
+
+        self.assertContentEqual(expected_urls, urls)
+
+    def test_binaryFileUrls_include_sizes(self):
+        bpph = self.make_bpph(num_binaries=2)
+        expected_urls = self.get_urls_for_bpph(bpph, include_sizes=True)
+
+        urls = bpph.binaryFileUrls(include_sizes=True)
+
+        self.assertContentEqual(expected_urls, urls)