launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/register-project-maintainer into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #435767 in Launchpad itself: "too hard to register a project and its team"
  https://bugs.launchpad.net/launchpad/+bug/435767

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/register-project-maintainer/+merge/117335

Project are often maintained by teams, but that must be done in a
separate step. When the maintainer is changed after registration then
sharing must also be reconfigured. Launchpad needs to let users setup
project correctly from the start to avoid loosing data or disclosing
data to the wrong person.

--------------------------------------------------------------------

RULES

    Pre-implementation: wallyworld
    * Add the maintainer field to project registration.
    * The default value is the registrant, but it picker can be used
      to find or register a new team.
    * The user can still give the project to the Registry Admins.

QA

    * Visit https://qastaging.launchpad.net/projects/+new
    * Enter the information to get to step 2.
    * For the maintainer field, choose to create a new team.
    * Verify the new team is listed in the maintainer field.
    * Complete registration.
    * Verify the new team is the maintainer.
    * Verify the project shares everything with the maintainer.


LINT

    lib/canonical/launchpad/icing/css/forms.css
    lib/lp/registry/browser/product.py
    lib/lp/registry/browser/tests/project-add-views.txt
    lib/lp/registry/browser/tests/test_product.py


TEST

    ./bin/test -vv -t TestProductAddView lp.registry.browser.tests.test_product
    ./bin/test -vv -t project-add-views lp.registry.browser


IMPLEMENTATION

Update the css rules to indent the checkbox field description when the
checkbox is indented. This fixes the alignment on the old +edit people
page too. There may be other cases where this is fixed, but we rarely
provide descriptions for checkboxes that we add to schemas.
    lib/canonical/launchpad/icing/css/forms.css

Added the owner field to the list of field to render. I discovered that
the hidden fields in the form created white-space between the owner
field and disclaim_maintainer field, so I forced the hidden fields to
the end of the table. The validation() method was updated to ignore
owner field errors if the disclaim_maintainer field was checked; the
field can be empty of contain an invalid name because it will not be
used
    lib/lp/registry/browser/product.py
    lib/lp/registry/browser/tests/test_product.py

Updated an existing test to pass the required owner field.
    lib/lp/registry/browser/tests/project-add-views.txt
-- 
https://code.launchpad.net/~sinzui/launchpad/register-project-maintainer/+merge/117335
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/register-project-maintainer into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/css/forms.css'
--- lib/canonical/launchpad/icing/css/forms.css	2012-06-23 13:44:13 +0000
+++ lib/canonical/launchpad/icing/css/forms.css	2012-07-30 20:16:39 +0000
@@ -73,6 +73,9 @@
     margin: 0.2em 0 0.5em 0.2em;
     color: #666;
     }
+.subordinate[type="checkbox"] + label + .formHelp {
+    margin-left: 3.4em;
+    }
 .listbox {
     /* a scrolling list of checkboxes or radio buttons */
     border: 1px solid #8cacbb;

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-07-27 13:12:41 +0000
+++ lib/lp/registry/browser/product.py	2012-07-30 20:16:39 +0000
@@ -2008,6 +2008,7 @@
 
     _field_names = ['displayname', 'name', 'title', 'summary',
                     'description', 'homepageurl', 'licenses', 'license_info',
+                    'owner',
                     ]
     schema = IProduct
     step_name = 'projectaddstep2'
@@ -2021,6 +2022,11 @@
     custom_widget('homepageurl', TextWidget, displayWidth=30)
     custom_widget('licenses', LicenseWidget)
     custom_widget('license_info', GhostWidget)
+    custom_widget(
+        'owner', PersonPickerWidget, header="Select the maintainer",
+        show_create_team_link=True)
+    custom_widget(
+        'disclaim_maintainer', CheckBoxWidget, cssClass="subordinate")
 
     @property
     def main_action_label(self):
@@ -2048,12 +2054,20 @@
             return 'Check for duplicate projects'
         return 'Registration details'
 
+    @property
+    def initial_values(self):
+        return {'owner': self.user.name}
+
     def setUpFields(self):
         """See `LaunchpadFormView`."""
         super(ProjectAddStepTwo, self).setUpFields()
-        self.form_fields = (self.form_fields +
+        hidden_names = ('__visited_steps__', 'license_info')
+        hidden_fields = self.form_fields.select(*hidden_names)
+        visible_fields = self.form_fields.omit(*hidden_names)
+        self.form_fields = (visible_fields +
                             self._createDisclaimMaintainerField() +
-                            create_source_package_fields())
+                            create_source_package_fields() +
+                            hidden_fields)
 
     def _createDisclaimMaintainerField(self):
         """Return a Bool field for disclaiming maintainer.
@@ -2150,6 +2164,11 @@
     def validateStep(self, data):
         """See `MultiStepView`."""
         ProductLicenseMixin.validate(self, data)
+        if data.get('disclaim_maintainer') and self.errors:
+            # The checkbox supersedes the owner text input.
+            errors = [error for error in self.errors if error[0] == 'owner']
+            for error in errors:
+                self.errors.remove(error)
 
     @property
     def label(self):
@@ -2167,7 +2186,7 @@
         if disclaim_maintainer:
             owner = getUtility(ILaunchpadCelebrities).registry_experts
         else:
-            owner = self.user
+            owner = data.get('owner')
         return getUtility(IProductSet).createProduct(
             registrant=self.user,
             owner=owner,

=== modified file 'lib/lp/registry/browser/tests/project-add-views.txt'
--- lib/lp/registry/browser/tests/project-add-views.txt	2012-05-24 20:25:54 +0000
+++ lib/lp/registry/browser/tests/project-add-views.txt	2012-07-30 20:16:39 +0000
@@ -135,6 +135,7 @@
 
     # The form keys have the 'field.' prefix here because the form data will
     # be processed.
+    >>> registrant = factory.makePerson()
     >>> form = {
     ...     'field.displayname': 'Snowdog',
     ...     'field.name': 'snowdog',
@@ -142,6 +143,7 @@
     ...     'field.summary': 'By-tor and the Snowdog',
     ...     'field.licenses': ['PYTHON'],
     ...     'field.license_info': '',
+    ...     'field.owner': registrant.name,
     ...     'field.__visited_steps__': '%s|%s' % (
     ...         ProjectAddStepOne.step_name, ProjectAddStepTwo.step_name),
     ...     'field.actions.continue': 'Continue',

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-07-24 06:39:54 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-07-30 20:16:39 +0000
@@ -5,13 +5,21 @@
 
 __metaclass__ = type
 
+import transaction
 from lazr.restful.interfaces import IJSONRequestCache
 from zope.component import getUtility
 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.registry.interfaces.person import CLOSED_TEAM_POLICY
+from lp.registry.browser.product import (
+    ProjectAddStepOne,
+    ProjectAddStepTwo,
+    )
+from lp.registry.interfaces.person import (
+    CLOSED_TEAM_POLICY,
+    TeamSubscriptionPolicy,
+    )
 from lp.registry.interfaces.product import (
     IProductSet,
     License,
@@ -21,6 +29,7 @@
 from lp.testing import (
     BrowserTestCase,
     login_celebrity,
+    login_person,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -73,9 +82,31 @@
     def setUp(self):
         super(TestProductAddView, self).setUp()
         self.product_set = getUtility(IProductSet)
-        # Marker allowing us to reset the config.
-        config.push(self.id(), '')
-        self.addCleanup(config.pop, self.id())
+
+    def makeForm(self, action):
+        if action == 1:
+            return {
+                'field.actions.continue': 'Continue',
+                'field.__visited_steps__': ProjectAddStepOne.step_name,
+                'field.displayname': 'Fnord',
+                'field.name': 'fnord',
+                'field.title': 'fnord',
+                'field.summary': 'fnord summary',
+                }
+        else:
+            return {
+                'field.actions.continue': 'Continue',
+                'field.__visited_steps__': '%s|%s' % (
+                    ProjectAddStepOne.step_name, ProjectAddStepTwo.step_name),
+                'field.displayname': 'Fnord',
+                'field.name': 'fnord',
+                'field.title': 'fnord',
+                'field.summary': 'fnord summary',
+                'field.owner': '',
+                'field.licenses': ['MIT'],
+                'field.license_info': '',
+                'field.disclaim_maintainer': 'off',
+                }
 
     def test_staging_message_is_not_demo(self):
         view = create_initialized_view(self.product_set, '+new')
@@ -83,11 +114,90 @@
         self.assertTrue(message is not None)
 
     def test_staging_message_is_demo(self):
+        config.push(self.id(), '')
+        self.addCleanup(config.pop, self.id())
         self.useFixture(DemoMode())
         view = create_initialized_view(self.product_set, '+new')
         message = find_tag_by_id(view.render(), 'staging-message')
         self.assertEqual(None, message)
 
+    def test_step_two_initialize(self):
+        # Step two collects additional license, owner, and packaging info.
+        registrant = self.factory.makePerson(name='pting')
+        transaction.commit()
+        login_person(registrant)
+        form = self.makeForm(action=1)
+        view = create_initialized_view(self.product_set, '+new', form=form)
+        owner_widget = view.view.widgets['owner']
+        self.assertEqual('pting', view.view.initial_values['owner'])
+        self.assertEqual('Select the maintainer', owner_widget.header)
+        self.assertIs(True, owner_widget.show_create_team_link)
+        disclaim_widget = view.view.widgets['disclaim_maintainer']
+        self.assertEqual('subordinate', disclaim_widget.cssClass)
+        self.assertEqual(
+            ['displayname', 'name', 'title', 'summary', 'description',
+             'homepageurl', 'licenses', 'license_info', 'owner',
+             '__visited_steps__'],
+            view.view.field_names)
+        self.assertEqual(
+            ['displayname', 'name', 'title', 'summary', 'description',
+             'homepageurl', 'licenses', 'owner', 'disclaim_maintainer',
+             'source_package_name', 'distroseries', '__visited_steps__',
+             'license_info'],
+            [f.__name__ for f in view.view.form_fields])
+
+    def test_owner_can_be_team(self):
+        # An owner can be any valid user or team selected.
+        registrant = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
+        transaction.commit()
+        login_person(registrant)
+        form = self.makeForm(action=2)
+        form['field.owner'] = team.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(team, product.owner)
+
+    def test_disclaim_maitainer_supersedes_owner(self):
+        # When the disclaim_maintainer is selected, the owner field is ignored
+        # and the registry team is made the maintainer.
+        registrant = self.factory.makePerson()
+        login_person(registrant)
+        form = self.makeForm(action=2)
+        form['field.owner'] = registrant.name
+        form['field.disclaim_maintainer'] = 'on'
+        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('registry', product.owner.name)
+
+    def test_owner_is_requried_without_disclaim_maitainer(self):
+        # A valid owner name is required if disclaim_maintainer is
+        # not selected.
+        registrant = self.factory.makePerson()
+        login_person(registrant)
+        form = self.makeForm(action=2)
+        form['field.owner'] = ''
+        del form['field.disclaim_maintainer']
+        view = create_initialized_view(self.product_set, '+new', form=form)
+        self.assertEqual(1, len(view.view.errors))
+        self.assertEqual('owner', view.view.errors[0][0])
+
+    def test_disclaim_maitainer_empty_supersedes_owner(self):
+        # Errors for the owner field are ignored when disclaim_maintainer is
+        # selected.
+        registrant = self.factory.makePerson()
+        login_person(registrant)
+        form = self.makeForm(action=2)
+        form['field.owner'] = ''
+        form['field.disclaim_maintainer'] = 'on'
+        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('registry', product.owner.name)
+
 
 class TestProductView(TestCaseWithFactory):
     """Tests the ProductView."""