Merge lp:~jcsackett/launchpad/bugfix-612408 into lp:launchpad

[Curtis Hovey]

Handles situations where summary is requested in sp and dsp with no publication history. Previously if summary was called for a package without history, an oops was the result.

During the preimplementation discussion, the cause of the error was discovered in the model and that it is possible for there to be a sourcepackagerelease that had no binaries, which is why an empty list was returned instead of None.

The fix was to convert the empty list to None in the return. Additional test coverage was added to verify the 3 conditions that SourcePackage.summary must handle.

[Curtis Hovey (sinzui)]

Launcpad wraps code at 78 characters so that it can be quoted in email. Run `make lint` to verify that files are okay.

review: Needs Information (code)

[Curtis Hovey (sinzui)]

Hi John.

Thanks for making this change. Creating the soyuz publication and cache data for binary package is truly difficult. I think we want to make that easier in the future, but for now, using sample data to close an oops is the best thing.

You can land your branch after you address my style concern below.

> === modified file 'lib/lp/registry/tests/test_sourcepackage.py'
> --- lib/lp/registry/tests/test_sourcepackage.py 2010-08-03 15:28:48 +0000
> +++ lib/lp/registry/tests/test_sourcepackage.py 2010-08-04 14:23:49 +0000
> @@ -226,6 +226,25 @@
> self.assertRaises(
> NoPartnerArchive, sourcepackage.get_default_archive)
>
> + def test_source_package_summary_no_releases_returns_None(self):
> + sourcepackage = self.factory.makeSourcePackage()
> + self.assertEqual(sourcepackage.summary, None)
> +
> + def test_source_package_summary_with_releases_returns_None(self):
> + sourcepackage = self.factory.makeSourcePackage()
> + self.factory.makeSourcePackageRelease(
> + sourcepackagename=sourcepackage.sourcepackagename)
> + self.assertEqual(sourcepackage.summary, None)
> +
> + def test_source_package_summary_with_binaries_returns_list(self):
> + sp = getUtility(
> + ILaunchpadCelebrities).ubuntu['warty'].getSourcePackage(
> + 'mozilla-firefox')
> + expected_summary = u'mozilla-firefox: Mozilla Firefox \
> +Web Browser\nmozilla-firefox-data: No summary available for \
> +mozilla-firefox-data in ubuntu warty.'
> + self.assertEqual(expected_summary, sp.summary)
> +

This form of wrapping is hard to read we use () to work with long strings
and we take advantage of string concatenation rules. We want to see preserve
indentation so that the code is easy to scan. In general, we avoid
the use of \ because you have to read the ending of a line to know what is
happening on another. We avoid end comments for the same reason. I think
format works:
        expected_summary = (
            u'mozilla-firefox: Mozilla Firefox Web Browser\n'
            u'mozilla-firefox-data: No summary available for '
            u'mozilla-firefox-data in ubuntu warty.')

review: Approve (code)

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py	2010-07-15 15:01:18 +0000
+++ lib/lp/registry/model/sourcepackage.py	2010-08-05 17:33:00 +0000
@@ -307,6 +307,8 @@
         name_summaries = [
             '%s: %s' % (binary.name, binary.summary)
             for binary in current.sample_binary_packages]
+        if name_summaries == []:
+            return None
         return '\n'.join(name_summaries)
 
     @property

=== modified file 'lib/lp/registry/tests/test_sourcepackage.py'
--- lib/lp/registry/tests/test_sourcepackage.py	2010-08-03 15:28:48 +0000
+++ lib/lp/registry/tests/test_sourcepackage.py	2010-08-05 17:33:00 +0000
@@ -226,6 +226,27 @@
         self.assertRaises(
             NoPartnerArchive, sourcepackage.get_default_archive)
 
+    def test_source_package_summary_no_releases_returns_None(self):
+        sourcepackage = self.factory.makeSourcePackage()
+        self.assertEqual(sourcepackage.summary, None)
+
+    def test_source_package_summary_with_releases_returns_None(self):
+        sourcepackage = self.factory.makeSourcePackage()
+        self.factory.makeSourcePackageRelease(
+            sourcepackagename=sourcepackage.sourcepackagename)
+        self.assertEqual(sourcepackage.summary, None)
+
+    def test_source_package_summary_with_binaries_returns_list(self):
+        sp = getUtility(
+            ILaunchpadCelebrities).ubuntu['warty'].getSourcePackage(
+            'mozilla-firefox')
+
+        expected_summary = (
+            u'mozilla-firefox: Mozilla Firefox Web Browser\n'
+            u'mozilla-firefox-data: No summary available for '
+            u'mozilla-firefox-data in ubuntu warty.')
+        self.assertEqual(''.join(expected_summary), sp.summary)
+
 
 class TestSourcePackageSecurity(TestCaseWithFactory):
     """Tests for source package branch linking security."""

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2010-08-02 02:13:52 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2010-08-05 17:33:00 +0000
@@ -146,8 +146,7 @@
             PackageUploadStatus.DONE: 'setDone',
             PackageUploadStatus.ACCEPTED: 'setAccepted',
             }
-        naked_package_upload = remove_security_proxy_and_shout_at_engineer(
-            package_upload)
+        naked_package_upload = removeSecurityProxy(package_upload)
         method = getattr(
             naked_package_upload, status_to_method[upload_status])
         method()
@@ -225,7 +224,7 @@
             changes_file_name=changes_file_name,
             changes_file_content=changes_file_content,
             upload_status=upload_status)
-        naked_package_upload = remove_security_proxy_and_shout_at_engineer(
+        naked_package_upload = removeSecurityProxy(
             package_upload)
         naked_package_upload.addSource(spr)