Merge lp:~edwin-grubbs/launchpad/bug-99395-linking-sourcepackages-to-projects into lp:launchpad

[Edwin Grubbs]

Made $sourcepackage/+edit-packaging a two step form since choosing a product series by entering $product/$series was confusing.

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

Make $sourcepackage/+edit-packaging a two step form since users are
confused by having to enter $project/$series.

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

The launchpadform was displaying "(Optional)" next to readonly widgets,
which seems silly.
    lib/canonical/launchpad/webapp/launchpadform.py

Converted SourcePackageChangeUpstreamView to MultiStepView.
    lib/lp/registry/browser/sourcepackage.py
    lib/lp/registry/stories/distribution/xx-distribution-packages.txt

Added multistep info and info on creating a new series if needed.
    lib/lp/registry/templates/sourcepackage-edit-packaging.pt

Tests
-----

./bin/test -vv -t xx-distribution-packages.txt

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

* Open https://launchpad.dev/ubuntu/warty/+source/iceweasel/+edit-packaging
    * Enter a project name.
    * Click "Continue".
    * Select a series.
    * Click "Change".
    * Verify that the series has changed on the sourcepackage page.

[Brad Crittenden (bac)]

Edwin this branch looks great. I expected the multistep stuff to be much harder. Thanks for a nice branch and thorough explanations.

review: Approve (code)

[Edwin Grubbs (edwin-grubbs)]

> Edwin this branch looks great. I expected the multistep stuff to be much
> harder. Thanks for a nice branch and thorough explanations.

Hi Brad,

Here are some tests that were broken by +edit-packaging being two steps
now. I also have changes to browser/sourcepackage.py, since I had
erroneously edit the field's default without copying the field, so the
default value was propagated to other views and was not only a bad value
but also a stale storm object. I'm setting the default manually, since
passing in the render_context is more complicated for the multistep views.

-Edwin

Incremental diff:

=== modified file 'lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt'
--- lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt 2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt 2010-02-17 03:16:46 +0000
@@ -37,8 +37,9 @@
 Let's follow the link and specify the packaging information.

     >>> user_browser.getLink('updating the packaging information').click()
- >>> user_browser.getControl(name='field.productseries').value = (
- ... 'thunderbird/trunk')
+ >>> user_browser.getControl(name='field.product').value = 'thunderbird'
+ >>> user_browser.getControl('Continue').click()
+ >>> user_browser.getControl(name='field.productseries').value = ['trunk']
     >>> user_browser.getControl('Change').click()

 Now the upstream product will be chosen automatically also for pmount.

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py 2010-02-16 18:56:38 +0000
+++ lib/lp/registry/browser/sourcepackage.py 2010-02-18 19:27:48 +0000
@@ -28,6 +28,8 @@
 from zope.schema.vocabulary import (
     getVocabularyRegistry, SimpleVocabulary, SimpleTerm)

+from lazr.restful.interface import copy_field
+
 from canonical.widgets import LaunchpadRadioWidget

 from canonical.launchpad import helpers
@@ -143,8 +145,8 @@

 class SourcePackageChangeUpstreamStepOne(StepView):
     """A view to set the `IProductSeries` of a sourcepackage."""
- schema = IProductSeries
- _field_names = ['product']
+ schema = Interface
+ _field_names = []

     step_name = 'sourcepackage_change_upstream_step1'
     template = ViewPageTemplateFile(
@@ -158,7 +160,12 @@
         super(SourcePackageChangeUpstreamStepOne, self).setUpFields()
         series = self.context.productseries
         if series is not None:
- self.form_fields['product'].field.default = series.product
+ default = series.product
+ else:
+ default = None
+ product_field = copy_field(
+ IProductSeries['product'], default=default)
+ self.form_fields += Fields(product_field)

     @property
     def cancel_url(self):

=== modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
--- lib/lp/registry/browser/tests/sourcepackage-views.txt 2010-02-12 14:04:16 +0000
+++ lib/lp/registry/browser/tests/sourcepackage-views.txt 2010-02-18 20:37:41 +0000
@@ -22,35 +22,67 @@
     >>> print view.page_title
     Link to an upstream project

- >>> print view.cancel...

=== modified file 'lib/canonical/launchpad/webapp/launchpadform.py'
--- lib/canonical/launchpad/webapp/launchpadform.py	2009-08-04 00:41:49 +0000
+++ lib/canonical/launchpad/webapp/launchpadform.py	2010-02-19 01:53:17 +0000
@@ -362,6 +362,12 @@
         # widgets.
         if not IInputWidget.providedBy(widget):
             return False
+
+        # Do not show for readonly fields.
+        context = getattr(widget, 'context', None)
+        if getattr(context, 'readonly', None):
+            return False
+
         # Do not show the marker for required widgets or always submitted
         # widgets.  Everything else gets the marker.
         return not (widget.required or

=== modified file 'lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt'
--- lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/bugs/stories/bug-also-affects/xx-also-affects-upstream-default-values.txt	2010-02-19 01:53:17 +0000
@@ -37,8 +37,9 @@
 Let's follow the link and specify the packaging information.
 
     >>> user_browser.getLink('updating the packaging information').click()
-    >>> user_browser.getControl(name='field.productseries').value = (
-    ...     'thunderbird/trunk')
+    >>> user_browser.getControl(name='field.product').value = 'thunderbird'
+    >>> user_browser.getControl('Continue').click()
+    >>> user_browser.getControl(name='field.productseries').value = ['trunk']
     >>> user_browser.getControl('Change').click()
 
 Now the upstream product will be chosen automatically also for pmount.

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2010-02-12 11:45:37 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2010-02-19 01:53:17 +0000
@@ -18,11 +18,13 @@
 
 from apt_pkg import ParseSrcDepends
 from cgi import escape
+from z3c.ptcompat import ViewPageTemplateFile
+from zope.app.form.browser import DropdownWidget
+from zope.app.form.interfaces import IInputWidget
 from zope.component import getUtility, getMultiAdapter
-from zope.app.form.interfaces import IInputWidget
-from zope.formlib.form import Fields, FormFields
+from zope.formlib.form import Fields
 from zope.interface import Interface
-from zope.schema import Choice
+from zope.schema import Choice, TextLine
 from zope.schema.vocabulary import (
     getVocabularyRegistry, SimpleVocabulary, SimpleTerm)
 
@@ -31,6 +33,7 @@
 from canonical.widgets import LaunchpadRadioWidget
 
 from canonical.launchpad import helpers
+from canonical.launchpad.browser.multistep import MultiStepView, StepView
 from lp.bugs.browser.bugtask import BugTargetTraversalMixin
 from canonical.launchpad.browser.packagerelationship import (
     relationship_builder)
@@ -39,13 +42,15 @@
 from lp.services.worlddata.interfaces.country import ICountry
 from lp.registry.interfaces.packaging import IPackaging
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.product import IProductSet
+from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from canonical.launchpad import _
 from canonical.launchpad.webapp import (
     action, ApplicationMenu, custom_widget, GetitemNavigation,
-    LaunchpadEditFormView, LaunchpadFormView, Link, redirection,
-    StandardLaunchpadFacets, stepto)
+    LaunchpadFormView, Link, redirection, StandardLaunchpadFacets, stepto)
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
@@ -138,53 +143,136 @@
         return Link('+gethelp', 'Help and support options', icon='info')
 
 
-class SourcePackageChangeUpstreamView(LaunchpadEditFormView):
-    """A view to set the `IProductSeries` of a sourcepackage."""
-    schema = ISourcePackage
-    field_names = ['productseries']
-
-    label = 'Link to an upstream project'
-    page_title = label
-
-    @property
-    def cancel_url(self):
-        return canonical_url(self.context)
-
-    def setUpFields(self):
-        """ See `LaunchpadFormView`.
-
-        The productseries field is required by the view.
-        """
-        super(SourcePackageChangeUpstreamView, self).setUpFields()
-        field = copy_field(ISourcePackage['productseries'], required=True)
-        self.form_fields = self.form_fields.omit('productseries')
-        self.form_fields = self.form_fields + FormFields(field)
-
-    def setUpWidgets(self):
-        """See `LaunchpadFormView`.
-
-        Set the current `IProductSeries` as the default value.
-        """
-        super(SourcePackageChangeUpstreamView, self).setUpWidgets()
-        if self.context.productseries is not None:
-            widget = self.widgets.get('productseries')
-            widget.setRenderedValue(self.context.productseries)
-
-    def validate(self, data):
-        productseries = data.get('productseries', None)
-        if productseries is None:
-            message = "You must choose a project series."
-            self.setFieldError('productseries', message)
-
-    @action(_("Change"), name="change")
-    def change(self, action, data):
+class SourcePackageChangeUpstreamStepOne(StepView):
+    """A view to set the `IProductSeries` of a sourcepackage."""
+    schema = Interface
+    _field_names = []
+
+    step_name = 'sourcepackage_change_upstream_step1'
+    template = ViewPageTemplateFile(
+        '../templates/sourcepackage-edit-packaging.pt')
+    label = 'Link to an upstream project'
+    page_title = label
+    step_description = 'Choose project'
+    product = None
+
+    def setUpFields(self):
+        super(SourcePackageChangeUpstreamStepOne, self).setUpFields()
+        series = self.context.productseries
+        if series is not None:
+            default = series.product
+        else:
+            default = None
+        product_field = copy_field(
+            IProductSeries['product'], default=default)
+        self.form_fields += Fields(product_field)
+
+    @property
+    def cancel_url(self):
+        return canonical_url(self.context)
+
+    def main_action(self, data):
+        """See `MultiStepView`."""
+        self.next_step = SourcePackageChangeUpstreamStepTwo
+        self.request.form['product'] = data['product']
+
+
+class SourcePackageChangeUpstreamStepTwo(StepView):
+    """A view to set the `IProductSeries` of a sourcepackage."""
+    schema = IProductSeries
+    _field_names = ['product']
+
+    step_name = 'sourcepackage_change_upstream_step2'
+    template = ViewPageTemplateFile(
+        '../templates/sourcepackage-edit-packaging.pt')
+    label = 'Link to an upstream project'
+    page_title = label
+    step_description = 'Choose project series'
+    product = None
+
+    # The DropdownWidget is used, since the VocabularyPickerWidget
+    # does not support visible=False to turn it into a hidden input
+    # to continue passing the variable in the form.
+    custom_widget('product', DropdownWidget, visible=False)
+    custom_widget('productseries', LaunchpadRadioWidget)
+
+    @property
+    def cancel_url(self):
+        return canonical_url(self.context)
+
+    def setUpFields(self):
+        super(SourcePackageChangeUpstreamStepTwo, self).setUpFields()
+
+        # The vocabulary for the product series is overridden to just
+        # include active series from the product selected in the
+        # previous step.
+        product_name = self.request.form['field.product']
+        self.product = getUtility(IProductSet)[product_name]
+        series_list = [
+            series for series in self.product.series
+            if series.status != SeriesStatus.OBSOLETE
+            ]
+        dev_focus = self.product.development_focus
+        if dev_focus in series_list:
+            series_list.remove(dev_focus)
+        vocab_terms = [
+            SimpleTerm(series, series.name, series.name)
+            for series in series_list
+            ]
+        dev_focus_term = SimpleTerm(
+            dev_focus, dev_focus.name, "%s (Recommended)" % dev_focus.name)
+        vocab_terms.insert(0, dev_focus_term)
+
+        # If the product is not being changed, then the current
+        # productseries can be the default choice. Otherwise,
+        # it will not exist in the vocabulary.
+        if (self.context.productseries is not None
+            and self.context.productseries.product == self.product):
+            series_default = self.context.productseries
+        else:
+            series_default = None
+
+        productseries_choice = Choice(
+            __name__='productseries',
+            title=_("Series"),
+            description=_("The series in this project."),
+            vocabulary=SimpleVocabulary(vocab_terms),
+            default=series_default,
+            required=True)
+
+        # The product selected in the previous step should be displayed,
+        # but a widget can't be readonly and pass its value with the
+        # form, so the real product field passes the value, and this fake
+        # product field displays it.
+        display_product_field = TextLine(
+            __name__='fake_product',
+            title=_("Project"),
+            default=self.product.displayname,
+            readonly=True)
+
+        self.form_fields = (
+            Fields(display_product_field, productseries_choice)
+            + self.form_fields)
+
+    main_action_label = u'Change'
+    def main_action(self, data):
         productseries = data['productseries']
+        # Because it is part of a multistep view, the next_url can't
+        # be set until the action is called, or it will skip the step.
+        self.next_url = canonical_url(self.context)
         if self.context.productseries == productseries:
             # There is nothing to do.
             return
         self.context.setPackaging(productseries, self.user)
         self.request.response.addNotification('Upstream link updated.')
-        self.next_url = canonical_url(self.context)
+
+
+class SourcePackageChangeUpstreamView(MultiStepView):
+    """A view to set the `IProductSeries` of a sourcepackage."""
+    page_title = SourcePackageChangeUpstreamStepOne.page_title
+    label = SourcePackageChangeUpstreamStepOne.label
+    total_steps = 2
+    first_step = SourcePackageChangeUpstreamStepOne
 
 
 class SourcePackageView:

=== modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
--- lib/lp/registry/browser/tests/sourcepackage-views.txt	2010-02-12 14:04:16 +0000
+++ lib/lp/registry/browser/tests/sourcepackage-views.txt	2010-02-19 01:53:17 +0000
@@ -22,35 +22,67 @@
     >>> print view.page_title
     Link to an upstream project
 
-    >>> print view.cancel_url
+    >>> print view.view.cancel_url
     http://launchpad.dev/youbuntu/busy/+source/bonkers
 
 
-The view allows the logged in user to change product series field. The value
-of the product series field is None by default because it is not required to
-create a source package.
-
-    >>> view.field_names
-    ['productseries']
-
-    >>> print view.widgets.get('productseries')._getFormValue()
+The view allows the logged in user to change product series field. The
+value of the product field is None by default because it is not required
+to create a source package.
+
+    # The product field is added in setUpFields().
+    >>> view.view.field_names
+    ['__visited_steps__']
+    >>> [form_field.__name__ for form_field in view.view.form_fields]
+    ['__visited_steps__', 'product']
+
+    >>> print view.view.widgets.get('product')._getFormValue()
     <BLANKLINE>
 
     >>> print package.productseries
     None
 
+This is a multistep view. In the first step, the product is specified.
+
+    >>> print view.view.__class__.__name__
+    SourcePackageChangeUpstreamStepOne
+    >>> print view.view.request.form
+    {'field.__visited_steps__': 'sourcepackage_change_upstream_step1'}
+
     >>> login_person(product.owner)
     >>> form = {
-    ...     'field.productseries': 'bonkers/crazy',
-    ...     'field.actions.change': 'Change',
+    ...     'field.product': 'bonkers',
+    ...     'field.actions.continue': 'Continue',
     ...     }
+    >>> form.update(view.view.request.form)
     >>> view = create_initialized_view(
     ...     package, name='+edit-packaging', form=form,
     ...     principal=product.owner)
-    >>> view.errors
+    >>> view.view.errors
     []
 
-    >>> print view.next_url
+In the second step, one of the series of the previously selected
+product can be chosen from a list of options.
+
+    >>> print view.view.__class__.__name__
+    SourcePackageChangeUpstreamStepTwo
+    >>> print view.view.request.form['field.__visited_steps__']
+    sourcepackage_change_upstream_step1|sourcepackage_change_upstream_step2
+    >>> [term.token for term in view.view.widgets['productseries'].vocabulary]
+    ['trunk', 'crazy']
+
+    >>> form = {
+    ...     'field.__visited_steps__': 'sourcepackage_change_upstream_step2',
+    ...     'field.product': 'bonkers',
+    ...     'field.productseries': 'crazy',
+    ...     'field.actions.continue': 'continue',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     package, name='+edit-packaging', form=form,
+    ...     principal=product.owner)
+
+    >>> ignored = view.view.render()
+    >>> print view.view.next_url
     http://launchpad.dev/youbuntu/busy/+source/bonkers
 
     >>> for notification in view.request.response.notifications:
@@ -62,26 +94,40 @@
 
     >>> transaction.commit()
 
-The form shows the current product series if it is set.
+The form shows the current product if it is set.
 
     >>> view = create_initialized_view(package, name='+edit-packaging')
-    >>> print view.widgets.get('productseries')._getFormValue().name
+
+    >>> print view.view.widgets.get('product')._getFormValue().name
+    bonkers
+
+If the same product as the current product series is selected,
+then the current product series will be the selected option.
+
+    >>> form = {
+    ...     'field.product': 'bonkers',
+    ...     'field.actions.continue': 'Continue',
+    ...     }
+    >>> form.update(view.view.request.form)
+    >>> view = create_initialized_view(
+    ...     package, name='+edit-packaging', form=form,
+    ...     principal=product.owner)
+    >>> print view.view.widgets.get('productseries')._getFormValue().name
     crazy
 
-The form requires a product series. An error is raised if the field is left
+The form requires a product. An error is raised if the field is left
 empty.
 
     >>> form = {
-    ...     'field.productseries': '',
-    ...     'field.actions.change': 'Change',
+    ...     'field.product': '',
+    ...     'field.actions.continue': 'Continue',
     ...     }
     >>> view = create_initialized_view(
     ...     package, name='+edit-packaging', form=form,
     ...     principal=product.owner)
-    >>> for error in view.errors:
+    >>> for error in view.view.errors:
     ...     print error
-    ('productseries', u'Project series', RequiredMissing())
-    You must choose a project series.
+    ('product', u'Project', RequiredMissing())
 
 Submitting the same product series as the current packaging is not an error,
 but there is no notification message that the upstream link was updated.
@@ -93,7 +139,7 @@
     >>> view = create_initialized_view(
     ...     package, name='+edit-packaging', form=form,
     ...     principal=product.owner)
-    >>> view.errors
+    >>> view.view.errors
     []
 
     >>> print view.request.response.notifications

=== modified file 'lib/lp/registry/stories/distribution/xx-distribution-packages.txt'
--- lib/lp/registry/stories/distribution/xx-distribution-packages.txt	2010-02-09 15:38:13 +0000
+++ lib/lp/registry/stories/distribution/xx-distribution-packages.txt	2010-02-19 01:53:17 +0000
@@ -284,8 +284,18 @@
     >>> print user_browser.url
     http://launchpad.dev/ubuntu/warty/+source/iceweasel/+edit-packaging
 
+In step one the project is specified.
+
     >>> user_browser.getControl(
-    ...     name='field.productseries').value = "firefox/trunk"
+    ...     name='field.product').value = "firefox"
+    >>> user_browser.getControl('Continue').click()
+
+In step two, one of the series for that project can be selected.
+
+    >>> series_control = user_browser.getControl(name='field.productseries')
+    >>> print series_control.options
+    ['trunk', '1.0']
+    >>> series_control.value = ['trunk']
     >>> user_browser.getControl('Change').click()
 
 Go back to the source page, and now the upstream's description is shown and

=== modified file 'lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt'
--- lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-02-17 13:16:21 +0000
+++ lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-02-19 01:53:17 +0000
@@ -23,8 +23,9 @@
 project. He sets the upstream packaging link and sees that it is set.
 
     >>> user_browser.getLink('Set upstream link').click()
-    >>> user_browser.getControl(
-    ...     name="field.productseries").value = 'thunderbird/trunk'
+    >>> user_browser.getControl(name='field.product').value = 'thunderbird'
+    >>> user_browser.getControl('Continue').click()
+    >>> user_browser.getControl(name='field.productseries').value = ['trunk']
     >>> user_browser.getControl("Change").click()
     >>> print extract_text(find_tag_by_id(
     ...     user_browser.contents, 'upstreams'))

=== modified file 'lib/lp/registry/templates/sourcepackage-edit-packaging.pt'
--- lib/lp/registry/templates/sourcepackage-edit-packaging.pt	2009-08-16 19:42:08 +0000
+++ lib/lp/registry/templates/sourcepackage-edit-packaging.pt	2010-02-19 01:53:17 +0000
@@ -10,11 +10,23 @@
 <div metal:fill-slot="main">
 
   <div metal:use-macro="context/@@launchpad_form/form">
-    <p metal:fill-slot="extra_info">
-      Links from distribution packages to upstream project series let
-      distribution and upstream maintainers share bugs, patches, and
-      translations efficiently.
-    </p>
+    <div metal:fill-slot="extra_info">
+      <h2 class="legend" id="step-title">Step
+        <tal:step_number tal:replace="view/step_number"/>
+        (of <tal:total_steps tal:replace="view/total_steps"/>):
+        <tal:step_description tal:replace="view/step_description"/>
+      </h2>
+      <p>
+        Links from distribution packages to upstream project series let
+        distribution and upstream maintainers share bugs, patches, and
+        translations efficiently.
+      </p>
+    </div>
+
+    <div metal:fill-slot="extra_bottom" tal:condition="view/product">
+      If you need a new series created, contact the owner of
+      <a tal:content="structure view/product/fmt:link"/>.
+    </div>
   </div>
 
 </div>