Merge lp:~sinzui/launchpad/register-project-maintainer into lp:launchpad

Proposed by Curtis Hovey
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 15721
Proposed branch: lp:~sinzui/launchpad/register-project-maintainer
Merge into: lp:launchpad
Diff against target: 262 lines (+141/-7)
4 files modified
lib/canonical/launchpad/icing/css/forms.css (+3/-0)
lib/lp/registry/browser/product.py (+22/-3)
lib/lp/registry/browser/tests/project-add-views.txt (+2/-0)
lib/lp/registry/browser/tests/test_product.py (+114/-4)
To merge this branch: bzr merge lp:~sinzui/launchpad/register-project-maintainer
Reviewer Review Type Date Requested Status
Ian Booth (community) Approve
Review via email: mp+117335@code.launchpad.net

Commit message

Allow the project registrant to choose the maintainer.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

This looks great. Perhaps a subsequent branch could make it so that the disclaim owner checkbox, when ticked, hides or disables the owner field. This will make it clearer to the user what's about to happen I think.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
73 margin: 0.2em 0 0.5em 0.2em;73 margin: 0.2em 0 0.5em 0.2em;
74 color: #666;74 color: #666;
75 }75 }
76.subordinate[type="checkbox"] + label + .formHelp {
77 margin-left: 3.4em;
78 }
76.listbox {79.listbox {
77 /* a scrolling list of checkboxes or radio buttons */80 /* a scrolling list of checkboxes or radio buttons */
78 border: 1px solid #8cacbb;81 border: 1px solid #8cacbb;
7982
=== 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 @@
20082008
2009 _field_names = ['displayname', 'name', 'title', 'summary',2009 _field_names = ['displayname', 'name', 'title', 'summary',
2010 'description', 'homepageurl', 'licenses', 'license_info',2010 'description', 'homepageurl', 'licenses', 'license_info',
2011 'owner',
2011 ]2012 ]
2012 schema = IProduct2013 schema = IProduct
2013 step_name = 'projectaddstep2'2014 step_name = 'projectaddstep2'
@@ -2021,6 +2022,11 @@
2021 custom_widget('homepageurl', TextWidget, displayWidth=30)2022 custom_widget('homepageurl', TextWidget, displayWidth=30)
2022 custom_widget('licenses', LicenseWidget)2023 custom_widget('licenses', LicenseWidget)
2023 custom_widget('license_info', GhostWidget)2024 custom_widget('license_info', GhostWidget)
2025 custom_widget(
2026 'owner', PersonPickerWidget, header="Select the maintainer",
2027 show_create_team_link=True)
2028 custom_widget(
2029 'disclaim_maintainer', CheckBoxWidget, cssClass="subordinate")
20242030
2025 @property2031 @property
2026 def main_action_label(self):2032 def main_action_label(self):
@@ -2048,12 +2054,20 @@
2048 return 'Check for duplicate projects'2054 return 'Check for duplicate projects'
2049 return 'Registration details'2055 return 'Registration details'
20502056
2057 @property
2058 def initial_values(self):
2059 return {'owner': self.user.name}
2060
2051 def setUpFields(self):2061 def setUpFields(self):
2052 """See `LaunchpadFormView`."""2062 """See `LaunchpadFormView`."""
2053 super(ProjectAddStepTwo, self).setUpFields()2063 super(ProjectAddStepTwo, self).setUpFields()
2054 self.form_fields = (self.form_fields +2064 hidden_names = ('__visited_steps__', 'license_info')
2065 hidden_fields = self.form_fields.select(*hidden_names)
2066 visible_fields = self.form_fields.omit(*hidden_names)
2067 self.form_fields = (visible_fields +
2055 self._createDisclaimMaintainerField() +2068 self._createDisclaimMaintainerField() +
2056 create_source_package_fields())2069 create_source_package_fields() +
2070 hidden_fields)
20572071
2058 def _createDisclaimMaintainerField(self):2072 def _createDisclaimMaintainerField(self):
2059 """Return a Bool field for disclaiming maintainer.2073 """Return a Bool field for disclaiming maintainer.
@@ -2150,6 +2164,11 @@
2150 def validateStep(self, data):2164 def validateStep(self, data):
2151 """See `MultiStepView`."""2165 """See `MultiStepView`."""
2152 ProductLicenseMixin.validate(self, data)2166 ProductLicenseMixin.validate(self, data)
2167 if data.get('disclaim_maintainer') and self.errors:
2168 # The checkbox supersedes the owner text input.
2169 errors = [error for error in self.errors if error[0] == 'owner']
2170 for error in errors:
2171 self.errors.remove(error)
21532172
2154 @property2173 @property
2155 def label(self):2174 def label(self):
@@ -2167,7 +2186,7 @@
2167 if disclaim_maintainer:2186 if disclaim_maintainer:
2168 owner = getUtility(ILaunchpadCelebrities).registry_experts2187 owner = getUtility(ILaunchpadCelebrities).registry_experts
2169 else:2188 else:
2170 owner = self.user2189 owner = data.get('owner')
2171 return getUtility(IProductSet).createProduct(2190 return getUtility(IProductSet).createProduct(
2172 registrant=self.user,2191 registrant=self.user,
2173 owner=owner,2192 owner=owner,
21742193
=== 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 @@
135135
136 # The form keys have the 'field.' prefix here because the form data will136 # The form keys have the 'field.' prefix here because the form data will
137 # be processed.137 # be processed.
138 >>> registrant = factory.makePerson()
138 >>> form = {139 >>> form = {
139 ... 'field.displayname': 'Snowdog',140 ... 'field.displayname': 'Snowdog',
140 ... 'field.name': 'snowdog',141 ... 'field.name': 'snowdog',
@@ -142,6 +143,7 @@
142 ... 'field.summary': 'By-tor and the Snowdog',143 ... 'field.summary': 'By-tor and the Snowdog',
143 ... 'field.licenses': ['PYTHON'],144 ... 'field.licenses': ['PYTHON'],
144 ... 'field.license_info': '',145 ... 'field.license_info': '',
146 ... 'field.owner': registrant.name,
145 ... 'field.__visited_steps__': '%s|%s' % (147 ... 'field.__visited_steps__': '%s|%s' % (
146 ... ProjectAddStepOne.step_name, ProjectAddStepTwo.step_name),148 ... ProjectAddStepOne.step_name, ProjectAddStepTwo.step_name),
147 ... 'field.actions.continue': 'Continue',149 ... 'field.actions.continue': 'Continue',
148150
=== 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 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8import transaction
8from lazr.restful.interfaces import IJSONRequestCache9from lazr.restful.interfaces import IJSONRequestCache
9from zope.component import getUtility10from zope.component import getUtility
10from zope.schema.vocabulary import SimpleVocabulary11from zope.schema.vocabulary import SimpleVocabulary
1112
12from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items13from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
13from lp.app.enums import ServiceUsage14from lp.app.enums import ServiceUsage
14from lp.registry.interfaces.person import CLOSED_TEAM_POLICY15from lp.registry.browser.product import (
16 ProjectAddStepOne,
17 ProjectAddStepTwo,
18 )
19from lp.registry.interfaces.person import (
20 CLOSED_TEAM_POLICY,
21 TeamSubscriptionPolicy,
22 )
15from lp.registry.interfaces.product import (23from lp.registry.interfaces.product import (
16 IProductSet,24 IProductSet,
17 License,25 License,
@@ -21,6 +29,7 @@
21from lp.testing import (29from lp.testing import (
22 BrowserTestCase,30 BrowserTestCase,
23 login_celebrity,31 login_celebrity,
32 login_person,
24 person_logged_in,33 person_logged_in,
25 TestCaseWithFactory,34 TestCaseWithFactory,
26 )35 )
@@ -73,9 +82,31 @@
73 def setUp(self):82 def setUp(self):
74 super(TestProductAddView, self).setUp()83 super(TestProductAddView, self).setUp()
75 self.product_set = getUtility(IProductSet)84 self.product_set = getUtility(IProductSet)
76 # Marker allowing us to reset the config.85
77 config.push(self.id(), '')86 def makeForm(self, action):
78 self.addCleanup(config.pop, self.id())87 if action == 1:
88 return {
89 'field.actions.continue': 'Continue',
90 'field.__visited_steps__': ProjectAddStepOne.step_name,
91 'field.displayname': 'Fnord',
92 'field.name': 'fnord',
93 'field.title': 'fnord',
94 'field.summary': 'fnord summary',
95 }
96 else:
97 return {
98 'field.actions.continue': 'Continue',
99 'field.__visited_steps__': '%s|%s' % (
100 ProjectAddStepOne.step_name, ProjectAddStepTwo.step_name),
101 'field.displayname': 'Fnord',
102 'field.name': 'fnord',
103 'field.title': 'fnord',
104 'field.summary': 'fnord summary',
105 'field.owner': '',
106 'field.licenses': ['MIT'],
107 'field.license_info': '',
108 'field.disclaim_maintainer': 'off',
109 }
79110
80 def test_staging_message_is_not_demo(self):111 def test_staging_message_is_not_demo(self):
81 view = create_initialized_view(self.product_set, '+new')112 view = create_initialized_view(self.product_set, '+new')
@@ -83,11 +114,90 @@
83 self.assertTrue(message is not None)114 self.assertTrue(message is not None)
84115
85 def test_staging_message_is_demo(self):116 def test_staging_message_is_demo(self):
117 config.push(self.id(), '')
118 self.addCleanup(config.pop, self.id())
86 self.useFixture(DemoMode())119 self.useFixture(DemoMode())
87 view = create_initialized_view(self.product_set, '+new')120 view = create_initialized_view(self.product_set, '+new')
88 message = find_tag_by_id(view.render(), 'staging-message')121 message = find_tag_by_id(view.render(), 'staging-message')
89 self.assertEqual(None, message)122 self.assertEqual(None, message)
90123
124 def test_step_two_initialize(self):
125 # Step two collects additional license, owner, and packaging info.
126 registrant = self.factory.makePerson(name='pting')
127 transaction.commit()
128 login_person(registrant)
129 form = self.makeForm(action=1)
130 view = create_initialized_view(self.product_set, '+new', form=form)
131 owner_widget = view.view.widgets['owner']
132 self.assertEqual('pting', view.view.initial_values['owner'])
133 self.assertEqual('Select the maintainer', owner_widget.header)
134 self.assertIs(True, owner_widget.show_create_team_link)
135 disclaim_widget = view.view.widgets['disclaim_maintainer']
136 self.assertEqual('subordinate', disclaim_widget.cssClass)
137 self.assertEqual(
138 ['displayname', 'name', 'title', 'summary', 'description',
139 'homepageurl', 'licenses', 'license_info', 'owner',
140 '__visited_steps__'],
141 view.view.field_names)
142 self.assertEqual(
143 ['displayname', 'name', 'title', 'summary', 'description',
144 'homepageurl', 'licenses', 'owner', 'disclaim_maintainer',
145 'source_package_name', 'distroseries', '__visited_steps__',
146 'license_info'],
147 [f.__name__ for f in view.view.form_fields])
148
149 def test_owner_can_be_team(self):
150 # An owner can be any valid user or team selected.
151 registrant = self.factory.makePerson()
152 team = self.factory.makeTeam(
153 subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
154 transaction.commit()
155 login_person(registrant)
156 form = self.makeForm(action=2)
157 form['field.owner'] = team.name
158 view = create_initialized_view(self.product_set, '+new', form=form)
159 self.assertEqual(0, len(view.view.errors))
160 product = self.product_set.getByName('fnord')
161 self.assertEqual(team, product.owner)
162
163 def test_disclaim_maitainer_supersedes_owner(self):
164 # When the disclaim_maintainer is selected, the owner field is ignored
165 # and the registry team is made the maintainer.
166 registrant = self.factory.makePerson()
167 login_person(registrant)
168 form = self.makeForm(action=2)
169 form['field.owner'] = registrant.name
170 form['field.disclaim_maintainer'] = 'on'
171 view = create_initialized_view(self.product_set, '+new', form=form)
172 self.assertEqual(0, len(view.view.errors))
173 product = self.product_set.getByName('fnord')
174 self.assertEqual('registry', product.owner.name)
175
176 def test_owner_is_requried_without_disclaim_maitainer(self):
177 # A valid owner name is required if disclaim_maintainer is
178 # not selected.
179 registrant = self.factory.makePerson()
180 login_person(registrant)
181 form = self.makeForm(action=2)
182 form['field.owner'] = ''
183 del form['field.disclaim_maintainer']
184 view = create_initialized_view(self.product_set, '+new', form=form)
185 self.assertEqual(1, len(view.view.errors))
186 self.assertEqual('owner', view.view.errors[0][0])
187
188 def test_disclaim_maitainer_empty_supersedes_owner(self):
189 # Errors for the owner field are ignored when disclaim_maintainer is
190 # selected.
191 registrant = self.factory.makePerson()
192 login_person(registrant)
193 form = self.makeForm(action=2)
194 form['field.owner'] = ''
195 form['field.disclaim_maintainer'] = 'on'
196 view = create_initialized_view(self.product_set, '+new', form=form)
197 self.assertEqual(0, len(view.view.errors))
198 product = self.product_set.getByName('fnord')
199 self.assertEqual('registry', product.owner.name)
200
91201
92class TestProductView(TestCaseWithFactory):202class TestProductView(TestCaseWithFactory):
93 """Tests the ProductView."""203 """Tests the ProductView."""