launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04170
[Merge] lp:~jcsackett/launchpad/button-configs-break-pickers into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/button-configs-break-pickers into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #806179 in Launchpad itself: "PersonPickers broken from bad passed in config"
https://bugs.launchpad.net/launchpad/+bug/806179
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/button-configs-break-pickers/+merge/67047
Summary
=======
An earlier branch that set up configuration options for views regarding whether or not to show the extra buttons actually broke pickers by passing in "True" where javascript expects "true". This branch fixes that.
This sort of issue should go away once we address bug 799847, in particular the part about using the JSONCache.
Pre-implementation notes
========================
None.
Implementation details
======================
popup.py sets two variables regarding the assign me button and the remove assignee button, but sets them using python booleans. They've been updated to strings, which are interpreted in the form-macros into the appropriate javascript booleans (e.g. 'false' instead of False).
Callsites have been updated to pass in the correct assignments for these variables.
UPDATE: While waiting for a review, the revision this branch fixes was rolled back. This branch now removes that rollback, polluting the diff. The original diff is in a comment below.
Tests
=====
bin/test -vvc --layer=YUI
QA
==
Check that clicking the Choose fields (e.g. on launchpad.dev/evolution/+edit-people) properly opens the person picker.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/browser/team.py
lib/lp/app/widgets/popup.py
./lib/lp/registry/browser/team.py
552: local variable 'mailing_list' is assigned to but never used
Not sure what to do regaring the lint above; it seems like the sort of code that may accomplish something via side effect, and I can't find good tests to double check. I'll try to suss it out prior to landing it.
--
https://code.launchpad.net/~jcsackett/launchpad/button-configs-break-pickers/+merge/67047
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/button-configs-break-pickers into lp:launchpad.
=== modified file 'lib/lp/app/browser/lazrjs.py'
--- lib/lp/app/browser/lazrjs.py 2011-07-06 00:41:28 +0000
+++ lib/lp/app/browser/lazrjs.py 2011-07-06 14:19:07 +0000
@@ -238,7 +238,8 @@
def __init__(self, context, exported_field, default_html,
content_box_id=None, header='Select an item',
- step_title='Search', remove_button_text='Remove',
+ step_title='Search', assign_button_text='Pick Me',
+ remove_button_text='Remove Person',
null_display_value='None',
edit_view="+edit", edit_url=None, edit_title=''):
"""Create a widget wrapper.
@@ -251,6 +252,7 @@
Automatically generated if this is not provided.
:param header: The large text at the top of the picker.
:param step_title: Smaller line of text below the header.
+ :param assign_button_text: Override default button text: "Pick Me"
:param remove_button_text: Override default button text: "Remove"
:param null_display_value: This will be shown for a missing value
:param edit_view: The view name to use to generate the edit_url if
@@ -265,6 +267,7 @@
self.default_html = default_html
self.header = header
self.step_title = step_title
+ self.assign_button_text = assign_button_text
self.remove_button_text = remove_button_text
self.null_display_value = null_display_value
@@ -278,6 +281,7 @@
def config(self):
return dict(
header=self.header, step_title=self.step_title,
+ assign_button_text=self.assign_button_text,
remove_button_text=self.remove_button_text,
null_display_value=self.null_display_value,
show_remove_button=self.optional_field,
=== modified file 'lib/lp/app/javascript/lazr/picker/person_picker.js'
--- lib/lp/app/javascript/lazr/picker/person_picker.js 2011-07-06 03:20:46 +0000
+++ lib/lp/app/javascript/lazr/picker/person_picker.js 2011-07-06 14:19:07 +0000
@@ -23,6 +23,8 @@
var show_assign_me_button = true;
var show_remove_button = true;
+ var assign_button_text = 'Pick Me';
+ var remove_button_text = 'Remove Person';
if (cfg !== undefined) {
if (cfg.show_assign_me_button !== undefined) {
@@ -31,9 +33,17 @@
if (cfg.show_remove_button !== undefined) {
show_remove_button = cfg.show_remove_button;
}
+ if (cfg.assign_button_text !== undefined) {
+ assign_button_text = cfg.assign_button_text;
+ }
+ if (cfg.remove_button_text !== undefined) {
+ remove_button_text = cfg.remove_button_text;
+ }
}
this._show_assign_me_button = show_assign_me_button;
this._show_remove_button = show_remove_button;
+ this._assign_me_button_text = assign_button_text;
+ this._remove_button_text = remove_button_text;
},
hide: function() {
@@ -57,15 +67,13 @@
_renderPersonPickerUI: function() {
var remove_button, assign_me_button;
- var remove_button_text = "Remove assignee";
- var assign_me_button_text = "Assign to me";
if (this._show_remove_button) {
remove_button = Y.Node.create(
'<a class="yui-picker-remove-button bg-image" ' +
'href="javascript:void(0)" ' +
'style="background-image: url(/@@/remove); padding-right: ' +
- '1em">' + remove_button_text + '</a>');
+ '1em">' + this._remove_button_text + '</a>');
remove_button.on('click', this.remove, this);
this._extra_buttons.appendChild(remove_button);
this.remove_button = remove_button;
@@ -76,7 +84,7 @@
'<a class="yui-picker-assign-me-button bg-image" ' +
'href="javascript:void(0)" ' +
'style="background-image: url(/@@/person)">' +
- assign_me_button_text + '</a>');
+ this._assign_me_button_text + '</a>');
assign_me_button.on('click', this.assign_me, this);
this._extra_buttons.appendChild(assign_me_button);
this.assign_me_button = assign_me_button;
=== modified file 'lib/lp/app/javascript/picker_patcher.js'
--- lib/lp/app/javascript/picker_patcher.js 2011-07-06 03:20:46 +0000
+++ lib/lp/app/javascript/picker_patcher.js 2011-07-06 14:19:07 +0000
@@ -21,6 +21,7 @@
* @param {Object} config Object literal of config name/value pairs.
* config.header: a line of text at the top of the widget.
* config.step_title: overrides the subtitle.
+ * config.assign_button_text: Override the default 'Assign' text.
* config.remove_button_text: Override the default 'Remove' text.
* config.null_display_value: Override the default 'None' text.
* config.show_remove_button: Should the remove button be shown?
@@ -40,17 +41,12 @@
var show_remove_button = false;
var show_assign_me_button = false;
- var remove_button_text = 'Remove';
var null_display_value = 'None';
var show_search_box = true;
resource_uri = Y.lp.client.normalize_uri(resource_uri);
if (config !== undefined) {
- if (config.remove_button_text !== undefined) {
- remove_button_text = config.remove_button_text;
- }
-
if (config.null_display_value !== undefined) {
null_display_value = config.null_display_value;
}
=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js 2011-07-06 03:20:46 +0000
+++ lib/lp/app/javascript/tests/test_picker.js 2011-07-06 14:19:07 +0000
@@ -79,7 +79,6 @@
{
"step_title": "Choose someone",
"header": "Pick Someone",
- "remove_button_text": "Remove someone",
"null_display_value": "Noone",
"validate_callback": validate_callback
});
@@ -280,7 +279,8 @@
delete window.LP;
},
- create_picker: function(show_assign_me, show_remove, field_value) {
+ create_picker: function(
+ show_assign_me_button, show_remove_button, field_value) {
if (field_value !== undefined) {
var data_box = Y.one('#picker_id .yui3-activator-data-box');
data_box.appendChild(Y.Node.create('<a>Content</a>'));
@@ -292,8 +292,10 @@
"header": "Pick Someone",
"validate_callback": null,
"show_search_box": true,
- "show_assign_me_button": show_assign_me,
- "show_remove_button": show_remove
+ "show_assign_me_button": show_assign_me_button,
+ "show_remove_button": show_remove_button,
+ "assign_button_text": "Assign Moi",
+ "remove_button_text": "Remove someone"
};
this.picker = Y.lp.app.picker.addPickerPatcher(
this.vocabulary,
@@ -325,6 +327,22 @@
this._check_button_state('.yui-picker-remove-button', is_visible);
},
+ test_picker_assign_me_button_text: function() {
+ // The assign me button text is correct.
+ this.create_picker(true, true);
+ this.picker.render();
+ var assign_me_button = Y.one('.yui-picker-assign-me-button');
+ Assert.areEqual('Assign Moi', assign_me_button.get('innerHTML'));
+ },
+
+ test_picker_remove_button_text: function() {
+ // The remove button text is correct.
+ this.create_picker(true, true);
+ this.picker.render();
+ var remove_button = Y.one('.yui-picker-remove-button');
+ Assert.areEqual('Remove someone', remove_button.get('innerHTML'));
+ },
+
test_picker_has_assign_me_button: function() {
// The assign me button is shown.
this.create_picker(true, true);
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2011-07-06 00:41:28 +0000
+++ lib/lp/app/widgets/popup.py 2011-07-06 14:19:07 +0000
@@ -28,6 +28,11 @@
__call__ = ViewPageTemplateFile('templates/form-picker.pt')
picker_type = 'default'
+ # Provide default values for the following properties in case someone
+ # creates a vocab picker for a person instead if using the derived
+ # PersonPicker.
+ show_assign_me_button = 'false'
+ show_remove_button = 'false'
popup_name = 'popup-vocabulary-picker'
@@ -159,6 +164,8 @@
class PersonPickerWidget(VocabularyPickerWidget):
include_create_team_link = False
+ show_assign_me_button = 'true'
+ show_remove_button = 'true'
@property
def picker_type(self):
=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-07-06 03:20:46 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-07-06 14:19:07 +0000
@@ -43,6 +43,10 @@
extra_no_results_message: ${view/extra_no_results_message},
picker_type: '${view/picker_type}'
};
+ if (config.picker_type == 'person') {
+ config.show_assign_me_button = ${view/show_assign_me_button}
+ config.show_remove_button = ${view/show_remove_button}
+ }
var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
if (config.extra_no_results_message !== null) {
picker.before('resultsChange', function (e) {
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-07-06 00:41:28 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-07-06 14:19:07 +0000
@@ -894,6 +894,7 @@
assignee_content.get('id'),
{"step_title": step_title,
"header": "Change assignee",
+ "assign_button_text": "Assign Me",
"remove_button_text": "Remove assignee",
"null_display_value": "Unassigned",
"show_remove_button": conf.user_can_unassign,
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2011-07-06 00:41:28 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2011-07-06 14:19:07 +0000
@@ -106,6 +106,7 @@
from lp.code.model.branchtarget import PersonBranchTarget
from lp.code.model.sourcepackagerecipe import get_buildable_distroseries_set
from lp.registry.interfaces.series import SeriesStatus
+from lp.services.fields import PersonChoice
from lp.services.propertycache import cachedproperty
from lp.soyuz.model.archive import Archive
@@ -840,7 +841,7 @@
self.form_fields = self.form_fields.omit('daily_build_archive')
owner_field = self.schema['owner']
- any_owner_choice = Choice(
+ any_owner_choice = PersonChoice(
__name__='owner', title=owner_field.title,
description=(u"As an administrator you are able to reassign"
u" this branch to any person or team."),
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py 2011-07-06 00:41:28 +0000
+++ lib/lp/registry/browser/team.py 2011-07-06 14:19:07 +0000
@@ -70,6 +70,7 @@
LaunchpadRadioWidgetWithDescription,
)
from lp.app.widgets.owner import HiddenUserWidget
+from lp.app.widgets.popup import PersonPickerWidget
from lp.registry.browser.branding import BrandingChangeView
from lp.registry.interfaces.mailinglist import (
IMailingList,
@@ -983,8 +984,7 @@
failed_names = [person.displayname for person in failed_joins]
failed_list = ", ".join(failed_names)
- mapping=dict(
- this_team=target_team.displayname,
+ mapping = dict( this_team=target_team.displayname,
failed_list=failed_list)
if len(failed_joins) == 1:
@@ -1034,6 +1034,13 @@
schema = ITeamMember
label = "Select the new member"
+ # XXX: jcsackett 5.7.2011 The assignment of 'false' to the vars below
+ # should be changed to the more appropriate False bool when we're making
+ # use of the JSON cache to setup pickers, rather than assembling
+ # javascript in a a view hacro.
+ custom_widget(
+ 'newmember', PersonPickerWidget,
+ show_assign_me_button='false', show_remove_button='false')
@property
def page_title(self):
Follow ups
