Merge lp:~sinzui/launchpad/non-existent-packages-bug-204119 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to prevent packaging links to unpublished Ubuntu packages.

    lp:~sinzui/launchpad/non-existent-packages-bug-204119
    Diff size: 305
    Launchpad bug: https://bugs.launchpad.net/bugs/204119
    Test command: ./bin/test -vv \
        -t packaging-views.txt \
        -t xx-product-package-pages.txt \
        -t xx-productseries-index.txt
    Pre-implementation: beuno, bigjools, flacoste
    Target release: 10.01

= Prevent packaging links to unpublished Ubuntu packages =

Launchpad allows users to create links to non-existent packages such as
https://edge.launchpad.net/ubuntu/breezy/+source/warzone2100.

The base view does not verify the that the package was published because
it is designed to accommodate any distro. But this feature is intended for
Ubuntu and it would not have been added if it Ubuntu did not want it, so
the rules must favour Ubuntu.

The work in this branch was started months ago. It was stalled by a debate
regarding the motivations for upstream projects linking to Ubuntu, and the
feature's design to accommodate all distros.

It was decided that the feature will never work well for other distros, and
that is why there are less then 100 links after 4 years. The feature must
favour Ubuntu. There is a problem with the Ubuntu-only form; some users
want to select the non-current series, and we should accommodate that before
we remove the generic form.

== Rules ==

    * Update the base view to verify the package was published in the series if
      the series is full functionality (Ubuntu)
    * Update the ubuntu view to show all the Ubuntu series, but retain the
      rule to use the current series as the default.
    * Update all the project and series pages to use +ubuntupkg instead of
      +addpackage.
    * ADDENDUM: I improved the instructions to use the ubuntu form.

There will be follow up branches to remove the +addpackage view and a query
to remove the packaging links to the non-existent series package.

== QA ==

On staging (or edge if you are willing to clean up)
    * https://edge.launchpad.net/gdp/+packages
    * Verify the action is `(+) Link to Ubuntu package`
    * Follow the link
    * Verify the form lists all Ubuntu series
    * Verify the default series is Lucid
    * Enter gedit-developer-plugins into the source package name field
      and submit
    * Verify that the error message is:
      The source package is not published in Lucid

== Lint ==

Linting changed files:
  lib/lp/registry/browser/packaging.py
  lib/lp/registry/browser/productseries.py
  lib/lp/registry/browser/tests/packaging-views.txt
  lib/lp/registry/stories/product/xx-product-package-pages.txt
  lib/lp/registry/stories/productseries/xx-productseries-index.txt
  lib/lp/registry/templates/product-packages.pt
  lib/lp/registry/templates/productseries-portlet-packages.pt
  lib/lp/registry/templates/productseries-ubuntupkg.pt
  lib/lp/testing/factory.py

== Test ==

    * lib/lp/registry/browser/tests/packaging-views.txt
    * lib/lp/registry/stories/product/xx-product-package-pages.txt
    * lib/lp/registry/stories/productseries/xx-productseries-index.txt

== Implementation ==

    * lib/lp/registry/browser/pack...

[Brad Crittenden (bac)]

Hi Curtis,

Thanks for the branch. It looks good save for a few small items below.

> === 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
> @@ -33,6 +33,9 @@
>
> from zope.component import getUtility
> from zope.app.form.browser import TextAreaWidget, TextWidget
> +from zope.formlib import form
> +from zope.schema import Choice
> +from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
>
> from z3c.ptcompat import ViewPageTemplateFile
>
> @@ -366,7 +369,7 @@
>
> class ProductSeriesUbuntuPackagingView(PackagingAddView):
>
> - field_names = ['sourcepackagename']
> + field_names = ['sourcepackagename', 'distroseries']
> page_title = 'Ubuntu source packaging'
> label = page_title
>
> @@ -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.

> + 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?

> + required=True)
> + field = form.Fields(choice, render_context=self.render_context)
> + self.form_fields = self.form_fields.omit(choice.__name__) + field
> +
> def setUpWidgets(self):
> """See `LaunchpadFormView`.
>

> === 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.make...

review: Approve (code)

[Curtis Hovey (sinzui)]

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.makeSourcePack...

=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-project-guided-filebug.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-project-guided-filebug.txt	2009-10-22 01:39:19 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-project-guided-filebug.txt	2010-01-13 15:11:15 +0000
@@ -205,6 +205,18 @@
 If we set up an external bug tracker for Testy Product, this will be reflected
 in the Test Group's filebug page.
 
+    >>> from zope.component import getUtility
+    >>> from canonical.launchpad.interfaces.launchpad import (
+    ...     ILaunchpadCelebrities)
+
+    >>> login('[e-mail]')
+    >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+    >>> hoary = ubuntu.getSeries('hoary')
+    >>> thunderbird_spn = factory.getOrMakeSourcePackageName('thunderbird')
+    >>> spph = factory.makeSourcePackagePublishingHistory(
+    ...     sourcepackagename=thunderbird_spn, distroseries=hoary)
+    >>> logout()
+
     >>> admin_browser.open('http://launchpad.dev/testy/+edit')
     >>> admin_browser.getControl(name='field.bugtracker').value = ['external']
     >>> admin_browser.getControl(

=== modified file 'lib/lp/registry/browser/packaging.py'
--- lib/lp/registry/browser/packaging.py	2009-10-23 14:23:18 +0000
+++ lib/lp/registry/browser/packaging.py	2010-01-13 15:11:15 +0000
@@ -47,6 +47,15 @@
         if sourcepackagename is None:
             message = "You must choose the source package name."
             self.setFieldError('sourcepackagename', message)
+        # Do not allow users it create links to unpublished Ubuntu packages.
+        elif distroseries.distribution.full_functionality:
+            source_package = distroseries.getSourcePackage(sourcepackagename)
+            if source_package.currentrelease is None:
+                message = ("The source package is not published in %s." %
+                    distroseries.displayname)
+                self.setFieldError('sourcepackagename', message)
+        else:
+            pass
         packaging_util = getUtility(IPackagingUtil)
         if packaging_util.packagingEntryExists(
             productseries=productseries,

=== 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-13 15:11:15 +0000
@@ -33,6 +33,9 @@
 
 from zope.component import getUtility
 from zope.app.form.browser import TextAreaWidget, TextWidget
+from zope.formlib import form
+from zope.schema import Choice
+from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
 
 from z3c.ptcompat import ViewPageTemplateFile
 
@@ -366,7 +369,7 @@
 
 class ProductSeriesUbuntuPackagingView(PackagingAddView):
 
-    field_names = ['sourcepackagename']
+    field_names = ['sourcepackagename', 'distroseries']
     page_title = 'Ubuntu source packaging'
     label = page_title
 
@@ -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,27 @@
             # 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(ProductSeriesUbuntuPackagingView, self).setUpFields()
+        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=_(
+                "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
+
     def setUpWidgets(self):
         """See `LaunchpadFormView`.
 

=== 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-13 15:11:15 +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. 
+
+    >>> vapor_spn = factory.makeSourcePackageName('vapor')
+    >>> form = {
+    ...     'field.distroseries': 'ubuntu/hoary',
+    ...     'field.sourcepackagename': 'vapor',
+    ...     'field.packaging': 'Primary Project',
+    ...     'field.actions.continue': 'Continue',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     productseries, '+addpackage', form=form)
+    >>> for error in view.errors:
+    ...     print error
+    The source package is not published in Hoary.
+
 The +addpackage view provides the default_distroseries property. It is None
 by default, but subclasses may change it.
 
@@ -129,16 +147,27 @@
     Ubuntu source packaging
 
     >>> print view.field_names
-    ['sourcepackagename']
+    ['sourcepackagename', 'distroseries']
 
     >>> print view.cancel_url
     http://launchpad.dev/hot/hotter
 
-The view restricts the packaging to the current Ubuntu series.
+The view restricts the packaging to Ubuntu series, and default is the current
+Ubuntu series.
 
     >>> print view.default_distroseries.name
     hoary
 
+    >>> print view.widgets['distroseries']._getDefault().name
+    hoary
+
+    >>> for term in view.widgets['distroseries'].vocabulary:
+    ...     print term.title
+    Breezy Badger Autotest (6.6.6)
+    Grumpy (5.10)
+    Hoary (5.04)
+    Warty (4.10)
+
 The sourcepackagename is None if the package link was never set. The view's
 packaging history is empty, and the sourcepackagename widget is empty.
 
@@ -172,6 +201,10 @@
 
 The package in the current Ubuntu series can be updated.
 
+    >>> thunderbird_spn = factory.getOrMakeSourcePackageName('thunderbird')
+    >>> spph = factory.makeSourcePackagePublishingHistory(
+    ...     sourcepackagename=thunderbird_spn, distroseries=hoary)
+
     >>> form = {
     ...     'field.sourcepackagename': 'thunderbird',
     ...     'field.actions.continue': 'Update',
@@ -211,6 +244,8 @@
     >>> login('[e-mail]')
     >>> hoary.status = DistroSeriesStatus.CURRENT
     >>> grumpy_series = ubuntu.getSeries('grumpy')
+    >>> spph = factory.makeSourcePackagePublishingHistory(
+    ...     sourcepackagename=sourcepackagename, distroseries=grumpy_series)
     >>> grumpy_series.status = DistroSeriesStatus.FROZEN
 
     >>> a_user = factory.makePerson(name="hedgehog")

=== 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-13 15:11:15 +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 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
+    http://launchpad.dev/evolution/trunk/+ubuntupkg
+
+    >>> anon_browser.open('http://launchpad.dev/evolution/+packages')
+    >>> anon_browser.getLink(url='/evolution/trunk/+ubuntupkg')
     Traceback (most recent call last):
     ...
     LinkNotFoundError

=== modified file 'lib/lp/registry/stories/productseries/xx-productseries-index.txt'
--- lib/lp/registry/stories/productseries/xx-productseries-index.txt	2009-10-30 15:40:55 +0000
+++ lib/lp/registry/stories/productseries/xx-productseries-index.txt	2010-01-13 15:11:15 +0000
@@ -130,14 +130,11 @@
    This series is not packaged in any distribution series.
 
 
-The driver sees two packaging links near the distribution packaging.
+The driver sees a packaging link near the distribution packaging.
 
     >>> driver_browser.getLink('Link to Ubuntu package')
     <Link ... url='http://launchpad.dev/firefox/trunk/+ubuntupkg'>
 
-    >>> driver_browser.getLink('Link package')
-    <Link ... url='http://launchpad.dev/firefox/trunk/+addpackage'>
-
 There is a section that lists related links to this series
 
     >>> anon_browser.getLink('View series for the Mozilla Firefox project')

=== modified file 'lib/lp/registry/templates/product-packages.pt'
--- lib/lp/registry/templates/product-packages.pt	2009-11-12 23:24:53 +0000
+++ lib/lp/registry/templates/product-packages.pt	2010-01-13 15:11:15 +0000
@@ -85,9 +85,9 @@
       </table>
 
       <ul class="table-actions"
-        tal:condition="series/menu:overview/add_package/linked">
+        tal:condition="series/menu:overview/ubuntupkg/linked">
         <li>
-          <a tal:replace="structure series/menu:overview/add_package/fmt:link" />
+          <a tal:replace="structure series/menu:overview/ubuntupkg/fmt:link" />
         </li>
       </ul>
     </div>

=== modified file 'lib/lp/registry/templates/productseries-portlet-packages.pt'
--- lib/lp/registry/templates/productseries-portlet-packages.pt	2009-10-22 19:16:03 +0000
+++ lib/lp/registry/templates/productseries-portlet-packages.pt	2010-01-13 15:11:15 +0000
@@ -39,8 +39,5 @@
     <li>
       <a tal:replace="structure context/menu:overview/ubuntupkg/fmt:icon-link" />
     </li>
-    <li>
-      <a tal:replace="structure context/menu:overview/add_package/fmt:icon-link" />
-    </li>
   </ul>
 </div>

=== modified file 'lib/lp/registry/templates/productseries-ubuntupkg.pt'
--- lib/lp/registry/templates/productseries-ubuntupkg.pt	2009-10-21 21:16:09 +0000
+++ lib/lp/registry/templates/productseries-ubuntupkg.pt	2010-01-13 15:11:15 +0000
@@ -17,31 +17,14 @@
           translations from Ubuntu to the
           <span tal:replace="context/product/displayname">Firefox</span> team.
         </p>
-
         <p>
-          Use
-          <a tal:replace="structure context/menu:overview/add_package/fmt:link" />
-           to link this series to packages in other distribution series.
+          Verify that the version packaged in the Ubuntu series is based on
+          the <tal:series replace="context/title" />. Don&rsquo;t link "HEAD",
+          "MAIN", "MASTER", or "TRUNK" to an Ubuntu package unless you are
+          certain that the package is based on the trunk of development.
         </p>
-      </div>
-
-      <div class="portlet">
-        <div metal:use-macro="context/@@launchpad_form/form">
-          <div metal:fill-slot="extra_info">
-            <h2>
-              Package in
-              <span tal:replace="view/default_distroseries/title" />
-            </h2>
-
-            <p>
-              Ensure both the <tal:series replace="context/title" /> and the
-              Ubuntu package are exactly correct. Don&rsquo;t link "HEAD",
-              "MAIN", or "TRUNK" to an Ubuntu package unless you are
-              absolutely certain that the package is really based on the
-              trunk of development.
-            </p>
-          </div>
-        </div>
+
+        <div metal:use-macro="context/@@launchpad_form/form" />
       </div>
 
       <div class="portlet"

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-01-11 23:43:59 +0000
+++ lib/lp/testing/factory.py	2010-01-13 15:11:15 +0000
@@ -277,10 +277,11 @@
     def makeGPGKey(self, owner):
         """Give 'owner' a crappy GPG key for the purposes of testing."""
         key_id = self.getUniqueHexString(digits=8).upper()
+        fingerprint = key_id + 'A' * 32
         return getUtility(IGPGKeySet).new(
             owner.id,
             keyid=key_id,
-            fingerprint='A' * 40,
+            fingerprint=fingerprint,
             keysize=self.getUniqueInteger(),
             algorithm=GPGKeyAlgorithm.R,
             active=True,