Launchpad itself

Code review comment for lp:~edwin-grubbs/launchpad/bug-99395-linking-sourcepackages-to-projects

bug-99395-linking-sourcepackages-to-projects
Merge into devel

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2010-02-18:

> 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:

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

+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):

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

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

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

Hi Brad,

-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_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/packaging/xx-sourcepackage-packaging.txt'
--- lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-02-12 11:45:37 +0000
+++ lib/lp/registry/stories/packaging/xx-sourcepackage-packaging.txt	2010-02-17 03:27:19 +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'))

« Back to merge proposal