Merge lp:~sinzui/launchpad/edit-packaging-oops-bug-456622 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to fix oopses for empty form values in +edit-packaging.

    lp:~sinzui/launchpad/edit-packaging-oops-bug-456622
    Diff size: 169
    Launchpad bug: https://bugs.launchpad.net/bugs/456622
                   https://bugs.launchpad.net/bugs/438090
                   https://bugs.launchpad.net/bugs/276417
    Test command: ./bin/test -vvt "lp.registry.*sourcepackage-views"
    Pre-implementation: No one. This look similar to other OOPs I fixed in
       the past week.
    Target release: 3.1.10

= Fix oopses for empty form values in +edit-packaging =

Bug 456622 [OOPS submitting empty +edit-packaging page]
    As seen in OOPS-1388EA532 a NoneError: None isn't acceptable as a value
    for Packaging.productseries is raised when the form is submitted with the
    product series field empty

Bug 438090 ["Define upstream link" form text is gibberish]
    * The form should not refer to things that don't exist in Launchpad,
      such as "product series".
    * The form label should use standard Launchpad capitalization and
      punctuation, i.e. "Project series:" rather than "Product Series".
    * The page title should be "Link to an upstream project".
    * The language should be free from grammar errors.

Bug 276417 [Launchpad thanks you for package-upstream link no-op]
    * An informational alert appears, falsely claiming "Upstream link updated,
      thank you!".

== Rules ==

Bug 456622 [OOPS submitting empty +edit-packaging page]
    * Add validation to the form
    * Add setupFields to force the productseries field to be required.

Bug 438090 ["Define upstream link" form text is gibberish]
    * Updated the schema to be clear about what is expected
    * Fix the forms label/page_title to: Link to an upstream project

Bug 276417 [Launchpad thanks you for package-upstream link no-op]
    * Return early if the update action is a no-op.

== QA ==

Visit staging.
    * Choose to edit a sourcepackage link
    * Verify that the form ask you to choose a *Project* series.
    * Verify it is listed in the field.
    * Change nothing and Choose update
    * Verify you are not thanks for changing the link
    * Choose edit again.
    * Empty the field and choose update
    * Verify the form reports a field error.

== Lint ==

Linting changed files:
  lib/lp/registry/browser/sourcepackage.py
  lib/lp/registry/browser/tests/sourcepackage-views.txt
  lib/lp/registry/interfaces/sourcepackage.py

== Test ==

    * lib/lp/registry/browser/tests/sourcepackage-views.txt
      * Verified the new page title.
      * Verified that the widget has the correct default value.
      * Added a test to verify the form explains that the project series
        must be chosen.
      * Added a test to verify a change notification is not shown when there
        is no change.

== Implementation ==

    * lib/lp/registry/browser/sourcepackage.py
      * Updated the page title.
      * Added setupFields to make the field required--it is not required
        by the object's main schema.
      * Added a setupWidgets to set the default value of the HTML field.
      * Added a validate method to raise a clear error message.
      * Updated the change...

[Brad Crittenden (bac)]

Looks good.

review: Approve (code)

=== modified file 'lib/lp/bugs/stories/distribution/xx-distribution-upstream-bug-report.txt'
--- lib/lp/bugs/stories/distribution/xx-distribution-upstream-bug-report.txt	2009-09-28 12:59:33 +0000
+++ lib/lp/bugs/stories/distribution/xx-distribution-upstream-bug-report.txt	2009-10-27 18:37:15 +0000
@@ -194,7 +194,8 @@
     >>> user_browser.open("http://bugs.launchpad.dev/ubuntu/+upstreamreport")
     >>> user_browser.getLink('link').click()
     >>> print user_browser.title
-    Define upstream link : ...
+    Link to an upstream project : ...
+
 
 If you are logged in and can edit the upstream project, you can also set
 a bug tracker and a bug supervisor. Let's first fix Firefox to stop using

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py	2009-10-16 15:00:55 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2009-10-27 18:37:15 +0000
@@ -18,6 +18,9 @@
 from apt_pkg import ParseSrcDepends
 from zope.component import getUtility, getMultiAdapter
 from zope.app.form.interfaces import IInputWidget
+from zope.formlib.form import FormFields
+
+from lazr.restful.interface import copy_field
 
 from canonical.launchpad import helpers
 from lp.bugs.browser.bugtask import BugTargetTraversalMixin
@@ -121,19 +124,50 @@
 
 
 class SourcePackageChangeUpstreamView(LaunchpadEditFormView):
+    """A view to set the `IProductSeries` of a sourcepackage."""
     schema = ISourcePackage
     field_names = ['productseries']
 
-    label = 'Define upstream link'
+    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):
-        self.context.setPackaging(data['productseries'], self.user)
+        productseries = data['productseries']
+        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)
 

=== modified file 'lib/lp/registry/browser/tests/sourcepackage-views.txt'
--- lib/lp/registry/browser/tests/sourcepackage-views.txt	2009-09-28 12:59:33 +0000
+++ lib/lp/registry/browser/tests/sourcepackage-views.txt	2009-10-27 18:37:15 +0000
@@ -13,21 +13,27 @@
     >>> package = factory.makeSourcePackage(
     ...     sourcepackagename=sourcepackagename, distroseries=distroseries)
 
-    >>> view = create_view(package, name='+edit-packaging')
+    >>> view = create_initialized_view(package, name='+edit-packaging')
     >>> print view.label
-     Define upstream link
+    Link to an upstream project
 
     >>> print view.page_title
-     Define upstream link
+    Link to an upstream project
 
     >>> print view.cancel_url
     http://launchpad.dev/youbuntu/busy/+source/bonkers
 
-The view allows the owner or admin to change productseries field.
+
+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()
+    <BLANKLINE>
+
     >>> print package.productseries
     None
 
@@ -51,3 +57,42 @@
 
     >>> print package.productseries.name
     crazy
+
+    >>> transaction.commit()
+
+The form shows the current product series if it is set.
+
+    >>> view = create_initialized_view(package, name='+edit-packaging')
+    >>> print view.widgets.get('productseries')._getFormValue().name
+    crazy
+
+The form requires a product series. An error is raised if the field is left
+empty.
+
+    >>> form = {
+    ...     'field.productseries': '',
+    ...     'field.actions.change': 'Change',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     package, name='+edit-packaging', form=form,
+    ...     principal=product.owner)
+    >>> for error in view.errors:
+    ...     print error
+    ('productseries', u'Project series', RequiredMissing())
+    You must choose a project series.
+
+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.
+
+    >>> form = {
+    ...     'field.productseries': 'bonkers/crazy',
+    ...     'field.actions.change': 'Change',
+    ...     }
+    >>> view = create_initialized_view(
+    ...     package, name='+edit-packaging', form=form,
+    ...     principal=product.owner)
+    >>> view.errors
+    []
+
+    >>> print view.request.response.notifications
+    []

=== modified file 'lib/lp/registry/interfaces/sourcepackage.py'
--- lib/lp/registry/interfaces/sourcepackage.py	2009-09-28 14:10:33 +0000
+++ lib/lp/registry/interfaces/sourcepackage.py	2009-10-27 18:37:15 +0000
@@ -90,14 +90,13 @@
     # This is really a reference to an IProductSeries.
     productseries = exported(
         ReferenceChoice(
-            title=_("Product Series"), required=False,
+            title=_("Project series"), required=False,
             vocabulary="ProductSeries",
             schema=Interface,
             description=_(
-                "The best guess we have as to the Launchpad ProductSeries "
-                "for this Source Package. Try find packaging information for "
-                "this specific distroseries then try parent series and "
-                "previous Ubuntu series.")))
+                "The registered project series that this source package."
+                "is based on. This series may be the same as the one that "
+                "earlier versions of this source packages were based on.")))
 
     releases = Attribute("The full set of source package releases that "
         "have been published in this distroseries under this source "