launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03334
[Merge] lp:~wallyworld/launchpad/refactor-picker-template into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/refactor-picker-template into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #761331 in Launchpad itself: "Incorrect non-ajax url for vocabulary popup pickers"
https://bugs.launchpad.net/launchpad/+bug/761331
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/refactor-picker-template/+merge/57804
When a popup picker widget is rendered, you get a text field for direct entry of the value plus a link to invoke the popup to choose a value from a defined vocabulary. Where javascript is not available, there is supposed to be a non-ajax link that is rendered. This link is hard coded to "/people/" which is correct for a people picker but incorrect for a bug tracker picker for example. The code which renders the link needs to use the nonajax_uri property for the link.
== Pre-Implementation ==
I spoke with Curtis about the refactoring included in this mp and we agreed it would be good to do.
== Implementation ==
As well as fixing the bug, I refactored the popup picker implementation to conform with how other widgets are written in lp. That is, each widget has a page template file with javascript invoked by the domready event to wire up the javascript/ajax stuff. Previously, the popup widgets used a vocabulary.js.template file which had values substituted into it and the resulting javascript returned as part of the chooseLink() property. Now everything is done the same way and is easier to understand.
== Tests ==
Refactored the TestVocabularyPickerWidget.test_js_template_args() test. Renamed to test_widget_template_properties()
test -vvt TestVocabularyPickerWidget
test -vvt TestInlineEditPickerWidget
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/configure.zcml
lib/lp/app/widgets/configure.zcml
lib/lp/app/widgets/popup.py
lib/lp/app/widgets/templates/bugtracker-picker.pt
lib/lp/app/widgets/templates/form-picker-macros.pt
lib/lp/app/widgets/templates/form-picker.pt
lib/lp/app/widgets/tests/test_popup.py
lib/lp/bugs/browser/bugtarget.py
--
https://code.launchpad.net/~wallyworld/launchpad/refactor-picker-template/+merge/57804
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/refactor-picker-template into lp:launchpad.
=== modified file 'lib/lp/app/configure.zcml'
--- lib/lp/app/configure.zcml 2011-02-24 15:30:16 +0000
+++ lib/lp/app/configure.zcml 2011-04-15 03:03:34 +0000
@@ -11,4 +11,5 @@
i18n_domain="launchpad">
<include package=".browser"/>
<include package="lp.app.validators" />
+ <include package="lp.app.widgets" />
</configure>
=== modified file 'lib/lp/app/widgets/configure.zcml'
--- lib/lp/app/widgets/configure.zcml 2011-02-01 21:03:45 +0000
+++ lib/lp/app/widgets/configure.zcml 2011-04-15 03:03:34 +0000
@@ -20,4 +20,10 @@
permission="zope.Public"
/>
+ <browser:page
+ for="*"
+ name="form-picker-macros"
+ permission="zope.Public"
+ template="templates/form-picker-macros.pt"/>
+
</configure>
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2011-02-09 15:25:57 +0000
+++ lib/lp/app/widgets/popup.py 2011-04-15 03:03:34 +0000
@@ -8,7 +8,6 @@
__metaclass__ = type
import cgi
-import os
import simplejson
from z3c.ptcompat import ViewPageTemplateFile
@@ -110,7 +109,7 @@
:return: A string that will be passed to Y.Node.create()
so it needs to be contained in a single HTML element.
"""
- return None
+ return simplejson.dumps(None)
@property
def vocabulary_name(self):
@@ -127,43 +126,26 @@
% (choice.context, choice.__name__))
return choice.vocabularyName
- def js_template_args(self):
- """return a dict of args to configure the picker javascript."""
- if self.header is None:
- header = self.vocabulary.displayname
- else:
- header = self.header
-
- if self.step_title is None:
- step_title = self.vocabulary.step_title
- else:
- step_title = self.step_title
-
- return dict(
- vocabulary=self.vocabulary_name,
- header=header,
- step_title=step_title,
- show_widget_id=self.show_widget_id,
- input_id=self.name,
- extra_no_results_message=self.extra_no_results_message)
+ @property
+ def header_text(self):
+ return simplejson.dumps(self.header or self.vocabulary.displayname)
+
+ @property
+ def step_title_text(self):
+ return simplejson.dumps(self.step_title or self.vocabulary.step_title)
+
+ @property
+ def input_id(self):
+ return self.name
def chooseLink(self):
- js_file = os.path.join(os.path.dirname(__file__),
- 'templates/vocabulary-picker.js.template')
- js_template = open(js_file).read()
- args = self.js_template_args()
- js = js_template % simplejson.dumps(args)
- # If the YUI widget or javascript is not supported in the browser,
- # it will degrade to being this "Find..." link instead of the
- # "Choose..." link. This only works if a non-AJAX form is available
- # for the field's vocabulary.
if self.nonajax_uri is None:
css = 'unseen'
else:
css = ''
- return ('<span class="%s">(<a id="%s" href="/people/">'
- 'Find…</a>)</span>'
- '\n<script>\n%s\n</script>') % (css, self.show_widget_id, js)
+ return ('<span class="%s">(<a id="%s" href="%s">'
+ 'Find…</a>)</span>') % (
+ css, self.show_widget_id, self.nonajax_uri or '#')
@property
def nonajax_uri(self):
@@ -190,48 +172,18 @@
class BugTrackerPickerWidget(VocabularyPickerWidget):
+
+ __call__ = ViewPageTemplateFile('templates/bugtracker-picker.pt')
+
link_template = """
- or (<a id="%(activator_id)s" href="/bugs/bugtrackers/+newbugtracker"
- >Register an external bug tracker…</a>)
- <script>
- LPS.use('lp.bugs.bugtracker_overlay', function(Y) {
- if (Y.UA.ie) {
- return;
- }
- Y.on('domready', function () {
- // After the success handler finishes, it calls the
- // next_step function.
- var next_step = function(bug_tracker) {
- // Fill in the text field with either the name of
- // the newly created bug tracker or the name of an
- // existing bug tracker whose base_url matches.
- var bugtracker_text_box = Y.one(
- Y.DOM.byId('field.bugtracker.bugtracker'));
- if (bugtracker_text_box !== null) {
- bugtracker_text_box.set(
- 'value', bug_tracker.get('name'));
- // It doesn't appear possible to use onChange
- // event, so the onKeyPress event is explicitely
- // fired here.
- if (bugtracker_text_box.get('onkeypress')) {
- bugtracker_text_box.get('onkeypress')();
- }
- bugtracker_text_box.scrollIntoView();
- }
- }
- Y.lp.bugs.bugtracker_overlay.attach_widget({
- activate_node: Y.one('#%(activator_id)s'),
- next_step: next_step
- });
- });
- });
- </script>
+ or (<a id="create-bugtracker-link"
+ href="/bugs/bugtrackers/+newbugtracker"
+ >Register an external bug tracker…</a>)
"""
def chooseLink(self):
link = super(BugTrackerPickerWidget, self).chooseLink()
- link += self.link_template % dict(
- activator_id='create-bugtracker-link')
+ link += self.link_template
return link
@property
@@ -249,6 +201,7 @@
@property
def extra_no_results_message(self):
- return ("<strong>Didn't find the project you were looking for? "
+ return simplejson.dumps("<strong>Didn't find the project you were "
+ "looking for? "
'<a href="%s/+affects-new-product">Register it</a>.</strong>'
% canonical_url(self.context.context))
=== added file 'lib/lp/app/widgets/templates/bugtracker-picker.pt'
--- lib/lp/app/widgets/templates/bugtracker-picker.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/templates/bugtracker-picker.pt 2011-04-15 03:03:34 +0000
@@ -0,0 +1,41 @@
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ omit-tag="">
+
+<metal:form-picker use-macro="context/@@form-picker-macros/form-picker"/>
+<script tal:content="structure string:
+LPS.use('lp.bugs.bugtracker_overlay', function(Y) {
+ if (Y.UA.ie) {
+ return;
+ }
+ Y.on('domready', function () {
+ // After the success handler finishes, it calls the
+ // next_step function.
+ var next_step = function(bug_tracker) {
+ // Fill in the text field with either the name of
+ // the newly created bug tracker or the name of an
+ // existing bug tracker whose base_url matches.
+ var bugtracker_text_box = Y.one(
+ Y.DOM.byId('field.bugtracker.bugtracker'));
+ if (bugtracker_text_box !== null) {
+ bugtracker_text_box.set(
+ 'value', bug_tracker.get('name'));
+ // It doesn't appear possible to use onChange
+ // event, so the onKeyPress event is explicitely
+ // fired here.
+ if (bugtracker_text_box.get('onkeypress')) {
+ bugtracker_text_box.get('onkeypress')();
+ }
+ bugtracker_text_box.scrollIntoView();
+ }
+ }
+ Y.lp.bugs.bugtracker_overlay.attach_widget({
+ activate_node: Y.one('#create-bugtracker-link'),
+ next_step: next_step
+ });
+ });
+});
+"/>
+
+</tal:root>
=== added file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-04-15 03:03:34 +0000
@@ -0,0 +1,72 @@
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ omit-tag="">
+
+ <metal:form-picker define-macro="form-picker">
+ <tal:input replace="structure view/inputField" />
+
+ <tal:search_results tal:condition="not: view/hasValidInput">
+ <select tal:condition="view/matches">
+ <option
+ tal:repeat="match view/matches"
+ tal:attributes="value match/token;
+ selected python:path('match/token') == path('view/formToken');
+ onclick string:this.form['${view/name}'].value = this.value"
+ tal:content="string:${match/token}: ${match/title}"
+ />
+ </select>
+ </tal:search_results>
+
+ <tal:chooseLink replace="structure view/chooseLink" />
+ <script tal:content="structure string:
+ LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
+ if (Y.UA.ie) {
+ return;
+ }
+
+ // The vocabulary picker, created when used for the first time.
+ var picker = null;
+ function make_picker() {
+ var config = {
+ header: ${view/header_text},
+ step_title: ${view/step_title_text},
+ extra_no_results_message: ${view/extra_no_results_message}
+ };
+ var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
+ if (config.extra_no_results_message !== null) {
+ picker.before('resultsChange', function (e) {
+ var new_results = e.details[0].newVal;
+ if (new_results.length === 0) {
+ picker.set('footer_slot',
+ Y.Node.create(config.extra_no_results_message));
+ }
+ else {
+ picker.set('footer_slot', null);
+ }
+ });
+ }
+ picker.plug(Y.lazr.TextFieldPickerPlugin,
+ {input_element: '[id=\x22${view/input_id}\x22]'});
+ return picker;
+ }
+
+ Y.on('domready', function(e) {
+ // Sort out the Choose... link.
+ var show_widget_node = Y.one('#${view/show_widget_id}');
+
+ show_widget_node.set('innerHTML', 'Choose…');
+ show_widget_node.addClass('js-action');
+ show_widget_node.get('parentNode').removeClass('unseen');
+ show_widget_node.on('click', function (e) {
+ if (picker === null) {
+ picker = make_picker();
+ }
+ picker.show();
+ e.preventDefault();
+ });
+ });
+ });
+ "/>
+ </metal:form-picker>
+</tal:root>
=== modified file 'lib/lp/app/widgets/templates/form-picker.pt'
--- lib/lp/app/widgets/templates/form-picker.pt 2009-07-30 19:03:57 +0000
+++ lib/lp/app/widgets/templates/form-picker.pt 2011-04-15 03:03:34 +0000
@@ -1,18 +1,8 @@
-<tal:root xmlns:tal="http://xml.zope.org/namespaces/tal";
- omit-tag="">
-<tal:input replace="structure view/inputField" />
-
-<tal:search_results tal:condition="not: view/hasValidInput">
- <select tal:condition="view/matches">
- <option
- tal:repeat="match view/matches"
- tal:attributes="value match/token;
- selected python:path('match/token') == path('view/formToken');
- onclick string:this.form['${view/name}'].value = this.value"
- tal:content="string:${match/token}: ${match/title}"
- />
- </select>
-</tal:search_results>
-
-<tal:chooseLink replace="structure view/chooseLink" />
+<tal:root
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ omit-tag="">
+
+<metal:form-picker use-macro="context/@@form-picker-macros/form-picker"/>
+
</tal:root>
=== removed file 'lib/lp/app/widgets/templates/vocabulary-picker.js.template'
--- lib/lp/app/widgets/templates/vocabulary-picker.js.template 2011-02-03 21:08:23 +0000
+++ lib/lp/app/widgets/templates/vocabulary-picker.js.template 1970-01-01 00:00:00 +0000
@@ -1,47 +0,0 @@
-LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
- if (Y.UA.ie) {
- return;
- }
-
- // Args from python.
- var args = %s;
-
- // The vocabulary picker, created when used for the first time.
- var picker = null;
- function make_picker() {
- var config = {
- header: args.header,
- step_title: args.step_title,
- extra_no_results_message: args.extra_no_results_message
- };
- var picker = Y.lp.app.picker.create(args.vocabulary, config);
- if (config.extra_no_results_message !== null) {
- picker.before('resultsChange', function (e) {
- var new_results = e.details[0].newVal;
- if (new_results.length === 0) {
- picker.set('footer_slot',
- Y.Node.create(config.extra_no_results_message));
- }
- else {
- picker.set('footer_slot', null);
- }
- });
- }
- picker.plug(Y.lazr.TextFieldPickerPlugin,
- {input_element: '[id="' + args.input_id + '"]'});
- return picker;
- }
-
- // Sort out the "Choose..." link.
- var show_widget_node = Y.one('#' + args.show_widget_id);
- show_widget_node.set('innerHTML', 'Choose…');
- show_widget_node.addClass('js-action');
- show_widget_node.get('parentNode').removeClass('unseen');
- show_widget_node.on('click', function (e) {
- if (picker === null) {
- picker = make_picker();
- }
- picker.show();
- e.preventDefault();
- });
-});
=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py 2011-02-02 15:43:31 +0000
+++ lib/lp/app/widgets/tests/test_popup.py 2011-04-15 03:03:34 +0000
@@ -3,6 +3,7 @@
__metaclass__ = type
+import simplejson
from zope.schema.vocabulary import getVocabularyRegistry
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -25,19 +26,20 @@
self.vocabulary = vocabulary_registry.get(context, 'ValidTeamOwner')
self.request = LaunchpadTestRequest()
- def test_js_template_args(self):
+ def test_widget_template_properties(self):
picker_widget = VocabularyPickerWidget(
self.bound_field, self.vocabulary, self.request)
- js_template_args = picker_widget.js_template_args()
- self.assertEqual(
- 'ValidTeamOwner', js_template_args['vocabulary'])
- self.assertEqual(
- self.vocabulary.displayname, js_template_args['header'])
- self.assertEqual(
- self.vocabulary.step_title, js_template_args['step_title'])
- self.assertEqual(
- 'show-widget-field-teamowner', js_template_args['show_widget_id'])
- self.assertEqual(
- 'field.teamowner', js_template_args['input_id'])
- self.assertEqual(
- None, js_template_args['extra_no_results_message'])
+ self.assertEqual(
+ 'ValidTeamOwner', picker_widget.vocabulary_name)
+ self.assertEqual(
+ simplejson.dumps(self.vocabulary.displayname),
+ picker_widget.header_text)
+ self.assertEqual(
+ simplejson.dumps(self.vocabulary.step_title),
+ picker_widget.step_title_text)
+ self.assertEqual(
+ 'show-widget-field-teamowner', picker_widget.show_widget_id)
+ self.assertEqual(
+ 'field.teamowner', picker_widget.input_id)
+ self.assertEqual(
+ simplejson.dumps(None), picker_widget.extra_no_results_message)
