Code review comment for lp:~sinzui/launchpad/non-existent-packages-bug-204119

On Tue, 2010-01-12 at 18:45 +0000, Brad Crittenden wrote:
> > === modified file 'lib/lp/registry/browser/productseries.py'
> > --- lib/lp/registry/browser/productseries.py 2009-12-05 18:37:28 +0000
> > +++ lib/lp/registry/browser/productseries.py 2010-01-11 23:10:26 +0000
...

> > @@ -374,8 +377,8 @@
> > """Set the static packaging information for this series."""
> > super(ProductSeriesUbuntuPackagingView, self).__init__(
> > context, request)
> > - ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> > - self._ubuntu_series = ubuntu.currentseries
> > + self._ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> > + self._ubuntu_series = self._ubuntu.currentseries
> > try:
> > package = self.context.getPackage(self._ubuntu_series)
> > self.default_sourcepackagename = package.sourcepackagename
> > @@ -383,6 +386,28 @@
> > # The package has never been set.
> > self.default_sourcepackagename = None
> >
> > + def setUpFields(self):
> > + """See `LaunchpadFormView`.
> > +
> > + The packaging is restricted to ubuntu series and the default value
> > + is the current development series.
> > + """
> > + super(PackagingAddView, self).setUpFields()
>
> As we discussed on IRC the class name is wrong.

Fixed

> > + series_vocabulary = SimpleVocabulary(
> > + [SimpleTerm(series, series.name, series.named_version)
> > + for series in self._ubuntu.series])
> > + choice = Choice(__name__='distroseries',
> > + title=_('Series'),
> > + default=self._ubuntu_series,
> > + vocabulary=series_vocabulary,
> > + description=_(
> > + "Select the Ubuntu series that this package is published "
> > + "in. The current series is most important to the Ubuntu "
> > + "community."),
>
> I think "series where this package is published" sounds better. What
> do you think?

Much better. Accepted.

> > === modified file 'lib/lp/registry/browser/tests/packaging-views.txt'
> > --- lib/lp/registry/browser/tests/packaging-views.txt 2009-12-10 20:03:11 +0000
> > +++ lib/lp/registry/browser/tests/packaging-views.txt 2010-01-11 23:10:26 +0000
> > @@ -18,6 +18,8 @@
> > >>> sourcepackagename = factory.makeSourcePackageName('hot')
> > >>> sourcepackage = factory.makeSourcePackage(
> > ... sourcepackagename=sourcepackagename, distroseries=hoary)
> > + >>> spph = factory.makeSourcePackagePublishingHistory(
> > + ... sourcepackagename=sourcepackagename, distroseries=hoary)
> > >>> product = factory.makeProduct(name="hot", displayname='Hot')
> > >>> productseries = factory.makeProductSeries(
> > ... product=product, name='hotter')
> > @@ -103,6 +105,22 @@
> > ('sourcepackagename', u'Source Package Name', RequiredMissing())
> > You must choose the source package name.
> >
> > +In the case of full functionality distributions like Ubuntu, the source
> > +package must be published in the distro series.
> > +
> > + >>> vapour_spn = factory.makeSourcePackageName('vapour')
>
> Would you mind en_US'ing this to 'vapor'?

Done.

> > === modified file 'lib/lp/registry/stories/product/xx-product-package-pages.txt'
> > --- lib/lp/registry/stories/product/xx-product-package-pages.txt 2009-11-12 23:24:53 +0000
> > +++ lib/lp/registry/stories/product/xx-product-package-pages.txt 2010-01-11 23:10:26 +0000
> > @@ -35,14 +35,15 @@
> >
> > >>> evo_owner.getLink(url='/ubuntu/hoary/+source/evolution') is not None
> > True
> > - >>> evo_owner.getLink(url='/evolution/trunk/+addpackage') is not None
> > - True
> > -
> > -If you're not the owner or an admin, you still see the packages, but without
> > -the link to add packages yourself.
> > -
> > - >>> browser.open('http://launchpad.dev/evolution/+packages')
> > - >>> browser.getLink(url='/evolution/trunk/+addpackage')
> > +
> > +Any login users can still see the links to create a packaging link.
>
> s/login/logged in/

Fixed

{{{
=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py 2010-01-12 17:23:27 +0000
+++ lib/lp/registry/browser/productseries.py 2010-01-12 22:27:01 +0000
@@ -392,7 +392,7 @@
         The packaging is restricted to ubuntu series and the default value
         is the current development series.
         """
- super(PackagingAddView, self).setUpFields()
+ super(ProductSeriesUbuntuPackagingView, self).setUpFields()
         series_vocabulary = SimpleVocabulary(
             [SimpleTerm(series, series.name, series.named_version)
              for series in self._ubuntu.series])
@@ -401,9 +401,8 @@
             default=self._ubuntu_series,
             vocabulary=series_vocabulary,
             description=_(
- "Select the Ubuntu series that this package is published "
- "in. The current series is most important to the Ubuntu "
- "community."),
+ "Series where this package is published. The current series "
+ "is most important to the Ubuntu community."),
             required=True)
         field = form.Fields(choice, render_context=self.render_context)
         self.form_fields = self.form_fields.omit(choice.__name__) + field

=== modified file 'lib/lp/registry/browser/tests/packaging-views.txt'
--- lib/lp/registry/browser/tests/packaging-views.txt 2010-01-11 20:31:17 +0000
+++ lib/lp/registry/browser/tests/packaging-views.txt 2010-01-12 22:27:34 +0000
@@ -108,10 +108,10 @@
 In the case of full functionality distributions like Ubuntu, the source
 package must be published in the distro series.

- >>> vapour_spn = factory.makeSourcePackageName('vapour')
+ >>> vapor_spn = factory.makeSourcePackageName('vapor')
     >>> form = {
     ... 'field.distroseries': 'ubuntu/hoary',
- ... 'field.sourcepackagename': 'vapour',
+ ... 'field.sourcepackagename': 'vapor',
     ... 'field.packaging': 'Primary Project',
     ... 'field.actions.continue': 'Continue',
     ... }

=== modified file 'lib/lp/registry/stories/product/xx-product-package-pages.txt'
--- lib/lp/registry/stories/product/xx-product-package-pages.txt 2010-01-11 22:26:49 +0000
+++ lib/lp/registry/stories/product/xx-product-package-pages.txt 2010-01-12 22:28:01 +0000
@@ -36,7 +36,7 @@
     >>> evo_owner.getLink(url='/ubuntu/hoary/+source/evolution') is not None
     True

-Any login users can still see the links to create a packaging link.
+Any logged in users can still see the links to create a packaging link.

     >>> user_browser.open('http://launchpad.dev/evolution/+packages')
     >>> print user_browser.getLink(url='/evolution/trunk/+ubuntupkg').url

}}}

--
__Curtis C. Hovey_________
http://launchpad.net/