Merge into devel : bugfix-612408 : Code : Launchpad itself

Status:

Merged

Approved by:

Curtis Hovey on 2010-08-04

Approved revision:

no longer in the source branch.

Merged at revision:

11310

Proposed branch:

lp://qastaging/~jcsackett/launchpad/bugfix-612408

Merge into:

lp://qastaging/launchpad

Diff against target:

66 lines (+25/-3)

3 files modified

lib/lp/registry/model/sourcepackage.py (+2/-0)
lib/lp/registry/tests/test_sourcepackage.py (+21/-0)
lib/lp/soyuz/tests/test_publishing.py (+2/-3)

To merge this branch:

bzr merge lp://qastaging/~jcsackett/launchpad/bugfix-612408

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	code		Approve on 2010-08-04
Review via email: mp+31599@code.qastaging.launchpad.net

Commit message

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.

Description of the change

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.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2010-08-03:

#

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)

Revision history for this message

Curtis Hovey (sinzui) wrote on 2010-08-04:

#

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.')

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)

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers