Merge lp:~deryck/launchpad/actually-save-info-type-on-project-create into lp:launchpad

[Deryck Hodge]

Ensure the project creation web UI doesn't error when saving, and ensure that it also saves information_type correctly, when the new private projects feature UI is enabled.

This branch fixes a couple bugs discovered in the new private projects UI for project creation.

The JavaScript needed to be updated to fill in a value for license_info if the "Other/Proprietary" option was selected. This will prevent the form validation from throwing an error. I added a test inside the excellent test coverage for this new js code.

Also, the project creation web UI was not saving information type. This changes the form to save PUBLIC unless the feature flag is set, then we will save the value returned in form data. I added a couple tests here, too, that ensure that information_type is saved correctly whether the private projects feature flag is set or not/

There are some long lines of lint in the js file, which I'll cleanup before landing.

[Aaron Bentley (abentley)]

This looks reasonable.

Setting information_type to PUBLIC is a bit of a DRY violation, since createProject takes care of that when unspecified or None. It's not the only DRY violation in this regard, as the EnumCol declares a default (but I can't figure out how to access it).

Similarly, declaring private_projects_flag is a DRY violation. I would prefer if you moved it to a module-level variable, since it's already used in two other places.

review: Approve

[Deryck Hodge (deryck)]

Sure, I can make those changes. Thanks for the review.

[Deryck Hodge (deryck)]

I realized now there's no need to hide the form data get behind a feature flag since None is an acceptable value. I've updated again, which makes this a little cleaner still.

[Aaron Bentley (abentley)]

Nice.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/browser/product.py	2012-10-04 12:58:22 +0000
@@ -204,6 +204,7 @@
 
 OR = ' OR '
 SPACE = ' '
+PRIVATE_PROJECTS_FLAG = 'disclosure.private_projects.enabled'
 
 
 class ProductNavigation(
@@ -1974,8 +1975,7 @@
         hidden_names = ['__visited_steps__', 'license_info']
         hidden_fields = self.form_fields.select(*hidden_names)
 
-        private_projects_flag = 'disclosure.private_projects.enabled'
-        private_projects = bool(getFeatureFlag(private_projects_flag))
+        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
         if not private_projects or not IProductSet.providedBy(self.context):
             hidden_names.extend([
                 'information_type', 'bug_supervisor', 'driver'])
@@ -2018,8 +2018,7 @@
         self.widgets['source_package_name'].visible = False
         self.widgets['distroseries'].visible = False
 
-        private_projects_flag = 'disclosure.private_projects.enabled'
-        private_projects = bool(getFeatureFlag(private_projects_flag))
+        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
 
         if private_projects and IProductSet.providedBy(self.context):
             self.widgets['information_type'].value = InformationType.PUBLIC
@@ -2091,8 +2090,7 @@
             for error in errors:
                 self.errors.remove(error)
 
-        private_projects_flag = 'disclosure.private_projects.enabled'
-        private_projects = bool(getFeatureFlag(private_projects_flag))
+        private_projects = bool(getFeatureFlag(PRIVATE_PROJECTS_FLAG))
         if private_projects:
             if data.get('information_type') != InformationType.PUBLIC:
                 for required_field in ('bug_supervisor', 'driver'):
@@ -2118,6 +2116,7 @@
             owner = getUtility(ILaunchpadCelebrities).registry_experts
         else:
             owner = data.get('owner')
+
         return getUtility(IProductSet).createProduct(
             registrant=self.user,
             bug_supervisor=data.get('bug_supervisor', None),
@@ -2131,6 +2130,7 @@
             homepageurl=data.get('homepageurl'),
             licenses=data['licenses'],
             license_info=data['license_info'],
+            information_type=data.get('information_type'),
             project=project)
 
     def link_source_package(self, product, data):

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-09-10 13:24:03 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-10-04 12:58:22 +0000
@@ -11,7 +11,10 @@
 from zope.schema.vocabulary import SimpleVocabulary
 
 from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
-from lp.app.enums import ServiceUsage
+from lp.app.enums import (
+    InformationType,
+    ServiceUsage,
+    )
 from lp.registry.browser.product import (
     ProjectAddStepOne,
     ProjectAddStepTwo,
@@ -25,6 +28,7 @@
     License,
     )
 from lp.services.config import config
+from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     BrowserTestCase,
@@ -106,6 +110,7 @@
                 'field.licenses': ['MIT'],
                 'field.license_info': '',
                 'field.disclaim_maintainer': 'off',
+                'field.information_type': 0,
                 }
 
     def test_view_data_model(self):
@@ -211,6 +216,40 @@
         product = self.product_set.getByName('fnord')
         self.assertEqual('registry', product.owner.name)
 
+    def test_information_type_saved_new_product_default(self):
+        # information_type should be PUBLIC by default for new projects.
+        # if the private projects feature flag is not enabled.
+        registrant = self.factory.makePerson()
+        login_person(registrant)
+        form = self.makeForm(action=2)
+        form['field.information_type'] = 'PROPRIETARY'
+        form['field.owner'] = registrant.name
+        view = create_initialized_view(self.product_set, '+new', form=form)
+        self.assertEqual(0, len(view.view.errors))
+        product = self.product_set.getByName('fnord')
+        self.assertEqual(InformationType.PUBLIC, product.information_type)
+
+    def test_information_type_saved_new_product_updated(self):
+        # information_type will be updated if passed in via form data,
+        # if the private projects feature flag is enabled.
+        with FeatureFixture({u'disclosure.private_projects.enabled': u'on'}):
+            registrant = self.factory.makePerson()
+            login_person(registrant)
+            form = self.makeForm(action=2)
+            form['field.information_type'] = 'PROPRIETARY'
+            form['field.owner'] = registrant.name
+            form['field.driver'] = registrant.name
+            form['field.maintainer'] = registrant.name
+            form['field.bug_supervisor'] = registrant.name
+            form['field.licenses'] = License.OTHER_PROPRIETARY.title
+            form['field.license_info'] = 'Commericial Subscription'
+            view = create_initialized_view(
+                self.product_set, '+new', form=form)
+            self.assertEqual(0, len(view.view.errors))
+            product = self.product_set.getByName('fnord')
+            self.assertEqual(
+                InformationType.PROPRIETARY, product.information_type)
+
 
 class TestProductView(TestCaseWithFactory):
     """Tests the ProductView."""

=== modified file 'lib/lp/registry/javascript/product_views.js'
--- lib/lp/registry/javascript/product_views.js	2012-09-24 14:10:10 +0000
+++ lib/lp/registry/javascript/product_views.js	2012-10-04 12:58:22 +0000
@@ -176,7 +176,8 @@
            // selected task.
            step_title.set('innerHTML', 'Step 2 (of 2) Registration details');
 
-           var reset_height = this.get('search_results').getComputedStyle('height');
+           var reset_height = this.get('search_results').getComputedStyle(
+                'height');
            this.get('search_results').setStyle('height', reset_height);
            Y.lp.ui.effects.slide_in(this.get('search_results')).run();
 
@@ -196,8 +197,10 @@
          *
          */
         _information_type_change: function (value) {
-            var driver_cont = Y.one('input[name="field.driver"]').ancestor('td');
-            var bug_super_cont = Y.one('input[name="field.bug_supervisor"]').ancestor('td');
+            var driver_cont = Y.one(
+                'input[name="field.driver"]').ancestor('td');
+            var bug_super_cont = Y.one(
+                'input[name="field.bug_supervisor"]').ancestor('td');
 
             // The license code is nested in another table.
             var license = Y.one('input[name="field.licenses"]');
@@ -218,6 +221,8 @@
                 var commercial = 'input[value="' + COMMERCIAL_LICENSE + '"]';
                 Y.one(commercial).set('checked', 'checked');
                 license.set('value', COMMERCIAL_LICENSE);
+                Y.one('textarea[name="field.license_info"]').set(
+                    'value', 'Launchpad 30-day trial commerical license');
             } else {
                 driver_cont.hide();
                 bug_super_cont.hide();
@@ -306,9 +311,9 @@
          * @param {Object}
          */
         initialize: function (cfg) {
-            // The details button is only visible when JavaScript is enabled, but
-            // the H3 separator is only visible when JavaScript is disabled.
-            // Neither is displayed on the step 1 page.
+            // The details button is only visible when JavaScript is enabled,
+            // but the H3 separator is only visible when JavaScript is
+            // disabled.  Neither is displayed on the step 1 page.
             this._show_separator(false);
         },
 
@@ -334,7 +339,8 @@
             // If we've been here before (e.g. there was an error in
             // submitting step 2), jump to continuing the registration.
             var marker = this.get('marker');
-            if (marker && marker.getAttribute('value').search(/hidesearch/) >= 0) {
+            if (marker &&
+                marker.getAttribute('value').search(/hidesearch/) >= 0) {
                 this._complete_registration(null);
             }
         }

=== modified file 'lib/lp/registry/javascript/tests/test_product_views.js'
--- lib/lp/registry/javascript/tests/test_product_views.js	2012-09-24 14:10:10 +0000
+++ lib/lp/registry/javascript/tests/test_product_views.js	2012-10-04 12:58:22 +0000
@@ -135,6 +135,13 @@
             var new_license = Y.one('input[name="field.licenses"]');
             Y.Assert.areEqual('OTHER_PROPRIETARY', new_license.get('value'),
                               'License is updated to a commercial selection');
+
+            // license_info must also be filled in to ensure we don't
+            // get form validation errors.
+            var license_info = Y.one('textarea[name="field.license_info"]');
+            Y.Assert.areEqual(
+                'Launchpad 30-day trial commerical license',
+                license_info.get('value'));
         }
     }));