Merge lp:~edwin-grubbs/launchpad/bug-272343-packaging-views into lp:launchpad

[Edwin Grubbs]

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

Remove PackagingAddView and productseries-packaging.pt.

Implementation details
----------------------

Removed PackagingAddView and moved the next_url, cancel_url,
and validate() code from PackagingAddView to ProductSeriesUbuntuPackagingView,
which used to subclass from it.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/packaging.py
    lib/lp/registry/browser/productseries.py

Removed file:
    lib/lp/registry/templates/productseries-packaging.pt

Converted tests for +addpackage to test +ubuntupkg unless the
test is obsolete.
    lib/lp/registry/browser/tests/packaging-views.txt

Make the error clearer when a menu item's method is removed
but is still referenced in the menu.links attribute.
    lib/canonical/launchpad/webapp/menu.py

Tests
-----

./bin/test -vv -t 'packaging-views.txt'

Demo and Q/A
------------

* Open a project series.
  * Click on "Link to Ubuntu package".
  * There should be no change in behavior.

[Curtis Hovey (sinzui)]

Hi Edwin.

I really appreciate your refactoring to remove the obsolete view. There is a conflict with DistroSeriesStatus that you need to fix. Adi renamed it SeriesStatus yesterday.

I have some hesitation about the assert in the doctest. There was a bad test in the past that wrote the assert wrongly and we did not discover it until after users reported errors. There is nothing wrong with your use, but printing the expected items would make it clear what you expected to be in the vocabulary tokens.

review: Approve

[Edwin Grubbs (edwin-grubbs)]

> Hi Edwin.
>
> I really appreciate your refactoring to remove the obsolete view. There is a
> conflict with DistroSeriesStatus that you need to fix. Adi renamed it
> SeriesStatus yesterday.
>
> I have some hesitation about the assert in the doctest. There was a bad test
> in the past that wrote the assert wrongly and we did not discover it until
> after users reported errors. There is nothing wrong with your use, but
> printing the expected items would make it clear what you expected to be in the
> vocabulary tokens.

Hi Curtis,

Thanks for the review. Here is the incremental diff:

{{{
=== modified file 'lib/lp/registry/browser/tests/packaging-views.txt'
--- lib/lp/registry/browser/tests/packaging-views.txt 2010-01-22 16:44:39 +0000
+++ lib/lp/registry/browser/tests/packaging-views.txt 2010-01-22 16:51:28 +0000
@@ -27,8 +27,9 @@
     []
     >>> transaction.commit()

-The view has a label and requires a distro series, source package name,
-and a packaging contents.
+The view has a label and requires a distro series and a source package name.
+The distroseries field's vocabulary is the same as the ubuntu.series
+attribute.

     >>> view = create_view(productseries, '+ubuntupkg')
     >>> print view.label
@@ -43,13 +44,19 @@
     >>> print view.cancel_url
     http://launchpad.dev/hot/hotter

- >>> series_names = [series.name for series in ubuntu.series]
+ >>> for series in ubuntu.series:
+ ... print series.name
+ breezy-autotest
+ grumpy
+ hoary
+ warty
     >>> view.setUpFields()
- >>> vocabulary_tokens = [
- ... term.token
- ... for term in view.form_fields['distroseries'].field.vocabulary
- ... ]
- >>> assert vocabulary_tokens == series_names
+ >>> for term in view.form_fields['distroseries'].field.vocabulary:
+ ... print term.token
+ breezy-autotest
+ grumpy
+ hoary
+ warty

     >>> form = {
     ... 'field.distroseries': 'hoary',
}}}

[Curtis Hovey (sinzui)]

On Fri, 2010-01-22 at 16:53 +0000, Edwin Grubbs wrote:
> > Hi Edwin.
> >
> > I really appreciate your refactoring to remove the obsolete view. There is a
> > conflict with DistroSeriesStatus that you need to fix. Adi renamed it
> > SeriesStatus yesterday.
> >
> > I have some hesitation about the assert in the doctest. There was a bad test
> > in the past that wrote the assert wrongly and we did not discover it until
> > after users reported errors. There is nothing wrong with your use, but
> > printing the expected items would make it clear what you expected to be in the
> > vocabulary tokens.
>
> Hi Curtis,
>
> Thanks for the review. Here is the incremental diff:

These change look good. thank you.

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

=== modified file 'lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt'
--- lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2009-12-05 13:36:12 +0000
+++ lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2010-01-22 22:32:18 +0000
@@ -114,7 +114,6 @@
 >>> check("/firefox")
 >>> check("/firefox/+series")
 >>> check("/firefox/1.0")
->>> check("/firefox/1.0/+addpackage", auth=True)
 >>> check("/firefox/1.0/+ubuntupkg", auth=True)
 >>> check("/firefox/1.0/+edit", auth=True)
 >>> check("/firefox/1.0/+specs")
 
=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py	2009-08-13 00:51:50 +0000
+++ lib/canonical/launchpad/webapp/menu.py	2010-01-22 22:32:18 +0000
@@ -229,7 +229,12 @@
         pass
 
     def _buildLink(self, name):
-        method = getattr(self, name)
+        method = getattr(self, name, None)
+        # Since Zope traversals hides the root cause of an AttributeError,
+        # an AssertionError is raised explaining what went wrong.
+        if method is None:
+            raise AssertionError(
+                '%r does not define %r method.' % (self, name))
         linkdata = method()
         # The link need only provide ILinkData.  We need an ILink so that
         # we can set attributes on it like 'name' and 'url' and 'linked'.
 
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-01-06 13:42:17 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-01-22 22:32:18 +0000
@@ -1594,13 +1594,6 @@
             template="../templates/productseries-table-releases.pt"/>
     </browser:pages>
     <browser:page
-        name="+addpackage"
-        for="lp.registry.interfaces.productseries.IProductSeries"
-        class="lp.registry.browser.packaging.PackagingAddView"
-        facet="overview"
-        permission="launchpad.View"
-        template="../templates/productseries-packaging.pt"/>
-    <browser:page
         name="+ubuntupkg"
         facet="overview"
         template="../templates/productseries-ubuntupkg.pt"
 
=== modified file 'lib/lp/registry/browser/packaging.py'
--- lib/lp/registry/browser/packaging.py	2010-01-11 16:26:52 +0000
+++ lib/lp/registry/browser/packaging.py	2010-01-22 22:32:18 +0000
@@ -4,7 +4,6 @@
 __metaclass__ = type
 
 __all__ = [
-    'PackagingAddView',
     'PackagingDeleteView',
     ]
 
@@ -14,82 +13,8 @@
 from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
 
 from canonical.launchpad import _
-from lp.registry.interfaces.packaging import (
-    IPackaging, IPackagingUtil)
-from canonical.launchpad.webapp import canonical_url
+from lp.registry.interfaces.packaging import IPackagingUtil
 from canonical.launchpad.webapp.launchpadform import action, LaunchpadFormView
-from canonical.launchpad.webapp.menu import structured
-
-
-class PackagingAddView(LaunchpadFormView):
-    schema = IPackaging
-    field_names = ['distroseries', 'sourcepackagename', 'packaging']
-    default_distroseries = None
-
-    @property
-    def label(self):
-        """See `LaunchpadFormView`."""
-        return 'Packaging of %s in distributions' % self.context.displayname
-
-    page_title = label
-
-    @property
-    def next_url(self):
-        """See `LaunchpadFormView`."""
-        return canonical_url(self.context)
-
-    cancel_url = next_url
-
-    def validate(self, data):
-        productseries = self.context
-        sourcepackagename = data.get('sourcepackagename', None)
-        distroseries = data.get('distroseries', self.default_distroseries)
-        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,
-            sourcepackagename=sourcepackagename,
-            distroseries=distroseries):
-            # The series packaging conflicts with itself.
-            message = _(
-                "This series is already packaged in %s." %
-                distroseries.displayname)
-            self.setFieldError('sourcepackagename', message)
-        elif packaging_util.packagingEntryExists(
-            sourcepackagename=sourcepackagename,
-            distroseries=distroseries):
-            # The series package conflicts with another series.
-            sourcepackage = distroseries.getSourcePackage(
-                sourcepackagename.name)
-            message = structured(
-                'The <a href="%s">%s</a> package in %s is already linked to '
-                'another series.' %
-                (canonical_url(sourcepackage),
-                 sourcepackagename.name,
-                 distroseries.displayname))
-            self.setFieldError('sourcepackagename', message)
-        else:
-            # The distroseries and sourcepackagename are not already linked
-            # to this series, or any other series.
-            pass
-
-    @action('Continue', name='continue')
-    def continue_action(self, action, data):
-        productseries = self.context
-        getUtility(IPackagingUtil).createPackaging(
-            productseries, data['sourcepackagename'], data['distroseries'],
-            data['packaging'], owner=self.user)
 
 
 class PackagingDeleteView(LaunchpadFormView):
 
=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-01-20 15:41:25 +0000
+++ lib/lp/registry/browser/productseries.py	2010-01-22 22:32:18 +0000
@@ -59,25 +59,27 @@
 from lp.registry.browser.structuralsubscription import (
     StructuralSubscriptionMenuMixin,
     StructuralSubscriptionTargetTraversalMixin)
+from lp.registry.interfaces.packaging import (
+    IPackaging, IPackagingUtil)
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.productserieslanguage import (
     IProductSeriesLanguageSet)
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from canonical.launchpad.webapp import (
-    action, ApplicationMenu, canonical_url, custom_widget,
-    enabled_with_permission, LaunchpadEditFormView,
-    LaunchpadView, Link, Navigation, NavigationMenu, StandardLaunchpadFacets,
-    stepthrough, stepto)
+    ApplicationMenu, canonical_url, enabled_with_permission, LaunchpadView,
+    Link, Navigation, NavigationMenu, StandardLaunchpadFacets, stepthrough,
+    stepto)
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.interfaces import NotFoundError
+from canonical.launchpad.webapp.launchpadform import (
+    action, custom_widget, LaunchpadEditFormView, LaunchpadFormView)
 from canonical.launchpad.webapp.menu import structured
 from canonical.widgets.textwidgets import StrippedTextWidget
 
 from lp.registry.browser import (
     MilestoneOverlayMixin, RegistryDeleteViewMixin)
-from lp.registry.browser.packaging import PackagingAddView
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.productseries import IProductSeries
 
@@ -161,8 +163,7 @@
     facet = 'overview'
     links = [
         'edit', 'delete', 'driver', 'link_branch', 'branch_add', 'ubuntupkg',
-        'add_package', 'create_milestone', 'create_release',
-        'rdf', 'subscribe',
+        'create_milestone', 'create_release', 'rdf', 'subscribe',
         ]
 
     @enabled_with_permission('launchpad.Edit')
@@ -210,12 +211,6 @@
         text = 'Link to Ubuntu package'
         return Link('+ubuntupkg', text, icon='add')
 
-    @enabled_with_permission('launchpad.View')
-    def add_package(self):
-        """Return a link to link this series to a sourcepackage."""
-        text = 'Link package'
-        return Link('+addpackage', text, icon='add')
-
     @enabled_with_permission('launchpad.Edit')
     def create_milestone(self):
         """Return a link to create a milestone."""
@@ -367,8 +362,9 @@
         return None
 
 
-class ProductSeriesUbuntuPackagingView(PackagingAddView):
+class ProductSeriesUbuntuPackagingView(LaunchpadFormView):
 
+    schema = IPackaging
     field_names = ['sourcepackagename', 'distroseries']
     page_title = 'Ubuntu source packaging'
     label = page_title
@@ -386,6 +382,13 @@
             # The package has never been set.
             self.default_sourcepackagename = None
 
+    @property
+    def next_url(self):
+        """See `LaunchpadFormView`."""
+        return canonical_url(self.context)
+
+    cancel_url = next_url
+
     def setUpFields(self):
         """See `LaunchpadFormView`.
 
@@ -428,11 +431,54 @@
             self.default_distroseries.distribution)
 
     def validate(self, data):
+        productseries = self.context
         sourcepackagename = data.get('sourcepackagename', None)
+        distroseries = data.get('distroseries', self.default_distroseries)
+
         if sourcepackagename == self.default_sourcepackagename:
             # The data has not changed, so nothing else needs to be done.
             return
-        super(ProductSeriesUbuntuPackagingView, self).validate(data)
+
+        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,
+            sourcepackagename=sourcepackagename,
+            distroseries=distroseries):
+            # The series packaging conflicts with itself.
+            message = _(
+                "This series is already packaged in %s." %
+                distroseries.displayname)
+            self.setFieldError('sourcepackagename', message)
+        elif packaging_util.packagingEntryExists(
+            sourcepackagename=sourcepackagename,
+            distroseries=distroseries):
+            # The series package conflicts with another series.
+            sourcepackage = distroseries.getSourcePackage(
+                sourcepackagename.name)
+            message = structured(
+                'The <a href="%s">%s</a> package in %s is already linked to '
+                'another series.' %
+                (canonical_url(sourcepackage),
+                 sourcepackagename.name,
+                 distroseries.displayname))
+            self.setFieldError('sourcepackagename', message)
+        else:
+            # The distroseries and sourcepackagename are not already linked
+            # to this series, or any other series.
+            pass
+
 
     @action('Update', name='continue')
     def continue_action(self, action, data):
 
=== modified file 'lib/lp/registry/browser/tests/packaging-views.txt'
--- lib/lp/registry/browser/tests/packaging-views.txt	2010-01-20 15:41:25 +0000
+++ lib/lp/registry/browser/tests/packaging-views.txt	2010-01-22 22:32:18 +0000
@@ -8,7 +8,7 @@
 ------------------------------
 
 Distro series sourcepackages can be linked to product series using the
-+addpackage named view.
++ubuntupkg named view.
 
     >>> from canonical.launchpad.interfaces.launchpad import (
     ...     ILaunchpadCelebrities)
@@ -25,31 +25,47 @@
     ...     product=product, name='hotter')
     >>> productseries.sourcepackages
     []
-
-The view has a label and requires a distro series, source package name,
-and a packaging contents.
-
-    >>> view = create_view(productseries, '+addpackage')
+    >>> transaction.commit()
+
+The view has a label and requires a distro series and a source package name.
+The distroseries field's vocabulary is the same as the ubuntu.series
+attribute.
+
+    >>> view = create_view(productseries, '+ubuntupkg')
     >>> print view.label
-    Packaging of hotter in distributions
+    Ubuntu source packaging
 
     >>> print view.page_title
-    Packaging of hotter in distributions
+    Ubuntu source packaging
 
     >>> print view.field_names
-    ['distroseries', 'sourcepackagename', 'packaging']
+    ['sourcepackagename', 'distroseries']
 
     >>> print view.cancel_url
     http://launchpad.dev/hot/hotter
 
+    >>> for series in ubuntu.series:
+    ...     print series.name
+    breezy-autotest
+    grumpy
+    hoary
+    warty
+    >>> view.setUpFields()
+    >>> for term in view.form_fields['distroseries'].field.vocabulary:
+    ...     print term.token
+    breezy-autotest
+    grumpy
+    hoary
+    warty
+
     >>> form = {
-    ...     'field.distroseries': 'ubuntu/hoary',
+    ...     'field.distroseries': 'hoary',
     ...     'field.sourcepackagename': 'hot',
     ...     'field.packaging': 'Primary Project',
     ...     'field.actions.continue': 'Continue',
     ...     }
     >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
+    ...     productseries, '+ubuntupkg', form=form)
     >>> view.errors
     []
     >>> for package in productseries.sourcepackages:
@@ -58,33 +74,19 @@
 
     >>> transaction.commit()
 
-It is an error to link a series to the same package and distro series twice.
-
-    >>> form = {
-    ...     'field.distroseries': 'ubuntu/hoary',
-    ...     'field.sourcepackagename': 'hot',
-    ...     'field.packaging': 'Primary Project',
-    ...     'field.actions.continue': 'Continue',
-    ...     }
-    >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
-    >>> for error in view.errors:
-    ...     print error
-    This series is already packaged in Hoary.
-
 Once a distro series sourcepackage is linked to a product series, no other
 product series can link to it.
 
     >>> other_productseries = factory.makeProductSeries(
     ...     product=product, name='hotest')
     >>> form = {
-    ...     'field.distroseries': 'ubuntu/hoary',
+    ...     'field.distroseries': 'hoary',
     ...     'field.sourcepackagename': 'hot',
     ...     'field.packaging': 'Primary Project',
     ...     'field.actions.continue': 'Continue',
     ...     }
     >>> view = create_initialized_view(
-    ...     other_productseries, '+addpackage', form=form)
+    ...     other_productseries, '+ubuntupkg', form=form)
     >>> for error in view.errors:
     ...     print error
     The <a href=".../hoary/+source/hot">hot</a> package in Hoary is already
@@ -93,52 +95,42 @@
 A source package name must be provided.
 
     >>> form = {
-    ...     'field.distroseries': 'ubuntu/hoary',
+    ...     'field.distroseries': 'hoary',
     ...     'field.sourcepackagename': '',
     ...     'field.packaging': 'Primary Project',
     ...     'field.actions.continue': 'Continue',
     ...     }
     >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
+    ...     productseries, '+ubuntupkg', form=form)
     >>> for error in view.errors:
     ...     print error
     ('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. 
+package must be published in the distro series.
 
     >>> vapor_spn = factory.makeSourcePackageName('vapor')
     >>> form = {
-    ...     'field.distroseries': 'ubuntu/hoary',
+    ...     'field.distroseries': 'hoary',
     ...     'field.sourcepackagename': 'vapor',
     ...     'field.packaging': 'Primary Project',
     ...     'field.actions.continue': 'Continue',
     ...     }
     >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
+    ...     productseries, '+ubuntupkg', 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.
-
-    >>> print view.default_distroseries
-    None
-
 
 Productseries linking Ubuntu packages
 -------------------------------------
 
-The +ubuntupkg named view is a subclass of the +addpackage named view. It
-allows the user to update the current linked Ubuntu package.
-
-    >>> from lp.registry.browser.packaging import PackagingAddView
+The +ubuntupkg named view allows the user to update the current linked
+Ubuntu package.
 
     >>> view = create_initialized_view(productseries, '+ubuntupkg')
-    >>> isinstance(view, PackagingAddView)
-    True
 
     >>> print view.label
     Ubuntu source packaging
@@ -314,34 +306,10 @@
 sorted by distribution with Ubuntu first and the rest in alphabetic
 order.
 
-    >>> form = {
-    ...     'field.distroseries': 'redhat/7.0',
-    ...     'field.sourcepackagename': 'hot',
-    ...     'field.packaging': 'Primary Project',
-    ...     'field.actions.continue': 'Continue',
-    ...     }
-
-    >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
-    >>> view.errors
-    []
-    >>> form = {
-    ...     'field.distroseries': 'debian/sarge',
-    ...     'field.sourcepackagename': 'hot',
-    ...     'field.packaging': 'Primary Project',
-    ...     'field.actions.continue': 'Continue',
-    ...     }
-
-    >>> view = create_initialized_view(
-    ...     productseries, '+addpackage', form=form)
-    >>> view.errors
-    []
     >>> view = create_initialized_view(product, name='+packages')
     >>> for distro_dict in view.distro_packaging:
     ...     print distro_dict['distribution'].name
     ubuntu
-    debian
-    redhat
 
 The +packages named view descends from PackagingDeleteView to provide remove
 link actions for the product's linked packages.
@@ -357,8 +325,6 @@
     >>> view = create_initialized_view(product, name='+packages')
     >>> for package in view.all_packaging:
     ...     print package.distroseries.name, package.productseries.name
-    sarge hotter
-    7.0 hotter
     grumpy hotter
     hoary hotter
 
@@ -373,8 +339,6 @@
     []
     >>> for package in view.all_packaging:
     ...     print package.distroseries.name, package.productseries.name
-    sarge hotter
-    7.0 hotter
     grumpy hotter
 
 
 
=== removed file 'lib/lp/registry/templates/productseries-packaging.pt'
--- lib/lp/registry/templates/productseries-packaging.pt	2009-09-12 06:11:08 +0000
+++ lib/lp/registry/templates/productseries-packaging.pt	1970-01-01 00:00:00 +0000
@@ -1,43 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  metal:use-macro="view/macro:page/main_only"
-  i18n:domain="launchpad"
->
-
-<body>
-
-<div metal:fill-slot="main">
-
-  <div metal:use-macro="context/@@launchpad_form/form">
-
-    <div metal:fill-slot="extra_info"
-      tal:define="sourcepackages context/sourcepackages">
-      <p>
-        Tell Launchpad about a package of
-        <tal:name replace="context/displayname" /> in a release of any
-        supported distribution.
-        <tal:existing condition="sourcepackages">
-          This series is packaged in the following places:
-        </tal:existing>
-      </p>
-
-      <ul class="bulleted"
-        tal:condition="sourcepackages">
-        <li tal:repeat="package sourcepackages">
-           <tal:distro replace="package/distroseries/distribution/name" />
-           <tal:series replace="package/distroseries/name" />
-           <a tal:content="package/name"
-              tal:attributes="href package/fmt:url">apache</a>
-        </li>
-      </ul>
-    </div>
-
-  </div>
-
-</div>
-    
-</body>
-</html>