launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03184
[Merge] lp:~danilo/launchpad/refactor-overlay-setup into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/refactor-overlay-setup into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/refactor-overlay-setup/+merge/56146
= Refactor overlay setup =
At the moment, overlay setup is too ingrained in what the overlays do and you can't easily construct overlays with different purposes on the same page (eg. one for editing a subscription and another for adding a new subscription).
== Proposed fix ==
Decouple setup from page loading: mostly moves code around and does away with one very nasty hack for passing stuff around.
Will fix lint after the review (unless I introduced the problems).
== Tests ==
lib/lp/registry/javascript/tests/test_structural_subscription.html
== Demo and Q/A ==
Nothing breaks on:
https://launchpad.dev/firefox/+subscriptions
https://launchpad.dev/firefox
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/javascript/structural-subscription.js
./lib/lp/registry/javascript/structural-subscription.js
655: Line exceeds 78 characters.
693: Line exceeds 78 characters.
1067: Line exceeds 78 characters.
--
https://code.launchpad.net/~danilo/launchpad/refactor-overlay-setup/+merge/56146
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/refactor-overlay-setup into lp:launchpad.
=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js 2011-04-01 17:55:51 +0000
+++ lib/lp/registry/javascript/structural-subscription.js 2011-04-04 12:28:56 +0000
@@ -311,42 +311,15 @@
add_subscription_overlay.on('cancel', clean_up);
}
-/*
- * Modify the DOM to insert a link or two into the global actions portlet.
- * If structural subscriptions already exist then a 'modify' link is
- * added. Otherwise, just the 'add' link is put into the portlet.
- *
- * @method setup_subscription_links
- * @param {String} overlay_id Id of the overlay element.
- * @param {String} content_box_id Id of the element on the page where
- * the overlay is anchored.
- */
-function setup_subscription_links(overlay_id, content_box_id) {
- // Modify the menu-link-subscribe-to-bug-mail link to be visible.
- var link = Y.one('.menu-link-subscribe_to_bug_mail');
- if (!Y.Lang.isValue(link)) {
- Y.fail('"Subscribe to bug mail" link not found.');
- }
- link.removeClass('invisible-link');
- link.addClass('visible-link');
- link.on('click', function(e) {
- // Only proceed if the form content is already available.
- if (add_subscription_overlay) {
- e.halt();
- // We always set up the overlay as a blank canvas, in case it was
- // used before.
- clear_overlay(Y.one(content_box_id));
- add_subscription_overlay.show();
- }
- });
- link.addClass('js-action');
-} // setup_subscription_links
-
/**
* Reset the overlay form to initial values.
*/
-function clear_overlay(content_node) {
- set_recipient(content_node, false, undefined);
+function clear_overlay(content_node, no_recipient_picker) {
+ if (no_recipient_picker) {
+ set_recipient_label(content_node, undefined);
+ } else {
+ set_recipient(content_node, false, undefined);
+ }
content_node.one('[name="name"]').set('value', '');
set_checkboxes(
content_node, LP.cache.statuses, LP.cache.statuses);
@@ -733,9 +706,9 @@
.appendChild(container)
.one('#structural-subscription-context-title')
.set('text', LP.cache.context.title);
+ add_recipient_picker(content_node, hide_recipient_picker);
var accordion = create_accordion('#accordion-overlay', content_node);
- add_recipient_picker(container, hide_recipient_picker);
// Set up click handlers for the events radio buttons.
var radio_group = Y.all('#events input');
@@ -787,35 +760,11 @@
* the overlay will be anchored.
*/
namespace.setup_bug_subscriptions = function(config) {
- validate_config(config);
- Y.on('domready', function() {
- if (Y.Lang.isValue(config.lp_client)) {
- // Tests can specify an lp_client if they want to.
- namespace.lp_client = config.lp_client;
- } else {
- // Setup the Launchpad client.
- setup_client();
- }
+ // Return if pre-setup fails.
+ if (!pre_setup(config))
+ return;
- var overlay_id = setup_overlay(config.content_box, true);
- var submit_button = Y.Node.create(
- '<button type="submit" name="field.actions.create" ' +
- 'value="Save Changes" class="lazr-pos lazr-btn" ' +
- '>OK</button>');
- // This is a bit of an odd approach, but it lets us retrofit code
- // without a large refactoring. When edit_subscription_handler is
- // called, context.filter_info will have the information about the
- // filter that is being edited.
- var context = {};
- create_overlay(config.content_box, overlay_id, submit_button,
- function (form_data) {
- return edit_subscription_handler(context, form_data);});
- fill_in_bug_subscriptions(config, context);
- // We need to initialize the help links. They may have already been
- // initialized except for the ones we added, so setupHelpTrigger
- // is idempotent. Notice that this is old MochiKit code.
- forEach(findHelpLinks(), setupHelpTrigger);
- }, window);
+ fill_in_bug_subscriptions(config);
};
/*
@@ -923,95 +872,149 @@
}
}
+/* Sets the recipient label according to the filter on the overlay.
+ * Overlay must not have a recipient picker, but a simple recipient label.
+ */
+function set_recipient_label(content_node, filter_info) {
+ var recipient_label = content_node.one('input[name="recipient"] + span'),
+ teams = LP.cache.administratedTeams;
+ if (filter_info !== undefined && filter_info.subscriber_is_team) {
+ var i;
+ for (i=0; i<teams.length; i++) {
+ if (teams[i].link === filter_info.subscriber_link){
+ recipient_label.set('text', teams[i].title);
+ break;
+ }
+ }
+ } else {
+ recipient_label.set('text', 'Yourself');
+ }
+};
+
+/* Sets filter statuses and importances on the overlay based on the filter
+ * data.
+ */
+function set_filter_statuses_and_importances(content_node, filter) {
+ var is_lifecycle = filter.bug_notification_level==='Lifecycle',
+ statuses = filter.statuses,
+ importances = filter.importances;
+ if (is_lifecycle) {
+ statuses = LP.cache.statuses;
+ importances = LP.cache.importances;
+ } else {
+ // An absence of values is equivalent to all values.
+ if (statuses.length === 0) {
+ statuses = LP.cache.statuses;
+ }
+ if (importances.length === 0) {
+ importances = LP.cache.importances;
+ }
+ }
+ set_checkboxes(content_node, LP.cache.statuses, statuses);
+ set_checkboxes(
+ content_node, LP.cache.importances, importances);
+};
+
+/* Sets filter tags and tag matching options in the overlay based on the
+ * filter data.
+ */
+function set_filter_tags(content_node, filter) {
+ var is_lifecycle = filter.bug_notification_level==='Lifecycle';
+ content_node.one('[name="tags"]').set(
+ 'value', is_lifecycle ? '' : filter.tags.join(' '));
+ set_radio_buttons(
+ content_node, [MATCH_ALL, MATCH_ANY],
+ filter.find_all_tags ? MATCH_ALL : MATCH_ANY);
+};
+
+/* Sets filter notification level radio/check boxes in the overlay
+ * according to the filter data.
+ */
+function set_filter_notification_options(content_node, filter) {
+ var is_lifecycle = filter.bug_notification_level==='Lifecycle';
+ has_advanced_filters = !is_lifecycle && (
+ filter.statuses.length ||
+ filter.importances.length ||
+ filter.tags.length) > 0,
+ filters = has_advanced_filters ? [ADVANCED_FILTER] : [],
+ event = ADDED_OR_CHANGED;
+ // Chattiness: Lifecycle < Details < Discussion.
+ switch (filter.bug_notification_level) {
+ case 'Lifecycle':
+ event = ADDED_OR_CLOSED;
+ filters = [];
+ break;
+ case 'Details':
+ filters.push(FILTER_COMMENTS);
+ break;
+ // case 'Discussion': This is already handled/the default.
+ // default: If we get here then it is a programmer error.
+ }
+ set_radio_buttons(
+ content_node, [ADDED_OR_CLOSED, ADDED_OR_CHANGED], event);
+ set_checkboxes(
+ content_node, [FILTER_COMMENTS, ADVANCED_FILTER], filters);
+ handle_change(ADDED_OR_CHANGED, FILTER_WRAPPER, {duration: 0});
+ handle_change(ADVANCED_FILTER, ACCORDION_WRAPPER, {duration: 0});
+};
+
+/* Loads all data from the filter into the overlay for editing. */
+function load_overlay_with_filter_data(content_node, filter_info) {
+ var filter = filter_info.filter;
+ set_recipient_label(content_node, filter_info);
+ content_node.one('[name="name"]').set('value',filter.description);
+ set_filter_statuses_and_importances(content_node, filter);
+ set_filter_tags(content_node, filter);
+ set_filter_notification_options(content_node, filter);
+}
+
+/* Show an overlay for editing a subscription. */
+function show_edit_overlay(config, subscription, filter_info, filter_id) {
+ Y.one(config.content_box).empty();
+ var content_node = Y.one(config.content_box),
+ overlay_id = setup_overlay(config.content_box, true),
+ submit_button = Y.Node.create(
+ '<button type="submit" name="field.actions.create" ' +
+ 'value="Save Changes" class="lazr-pos lazr-btn" ' +
+ '>OK</button>');
+
+ clear_overlay(content_node, true);
+
+ var context = {
+ filter_info: filter_info,
+ filter_id: filter_id
+ };
+ create_overlay(
+ config.content_box, overlay_id, submit_button,
+ function (form_data) {
+ return edit_subscription_handler(context, form_data);});
+
+ load_overlay_with_filter_data(content_node, filter_info);
+ var title = subscription.target_title;
+ Y.one('#structural-subscription-context-title')
+ .set('text', title);
+ Y.one('#subscription-overlay-title')
+ .set('text', 'Edit subscription for '+title+' bugs');
+
+ // We need to initialize the help links. They may have already been
+ // initialized except for the ones we added, so setupHelpTrigger
+ // is idempotent. Notice that this is old MochiKit code.
+ forEach(findHelpLinks(), setupHelpTrigger);
+ add_subscription_overlay.show();
+};
+
/**
* Return an edit handler for the specified filter.
*/
-function make_edit_handler(subscription, filter_info, filter_id,
- config, context) {
+function make_edit_handler(subscription, filter_info, filter_id, config) {
// subscription is the filter's subscription.
// filter_info is the filter's information (from subscription.filters).
// filter_id is the numerical id for the filter, unique on the page.
// config is the configuration object used for the entire assembly of the
// page.
- // context is a way to communicate to the shared edit handler what filter
- // should be updated.
return function(e) {
- // Only proceed if the form content is already available.
- if (add_subscription_overlay) {
- e.halt();
- var content_node = Y.one(config.content_box),
- teams = LP.cache.administratedTeams,
- filter = filter_info.filter,
- is_lifecycle = filter.bug_notification_level==='Lifecycle',
- recipient_label = content_node.one(
- 'input[name="recipient"] + span'),
- statuses = filter.statuses,
- importances = filter.importances;
- if (filter_info.subscriber_is_team) {
- var i;
- for (i=0; i<teams.length; i++) {
- if (teams[i].link === filter_info.subscriber_link){
- recipient_label.set('text', teams[i].title);
- break;
- }
- }
- } else {
- recipient_label.set('text', 'Yourself');
- }
- content_node.one('[name="name"]').set('value',filter.description);
- if (is_lifecycle) {
- statuses = LP.cache.statuses;
- importances = LP.cache.importances;
- } else {
- // An absence of values is equivalent to all values.
- if (statuses.length === 0) {
- statuses = LP.cache.statuses;
- }
- if (importances.length === 0) {
- importances = LP.cache.importances;
- }
- }
- set_checkboxes(content_node, LP.cache.statuses, statuses);
- set_checkboxes(
- content_node, LP.cache.importances, importances);
- content_node.one('[name="tags"]').set(
- 'value', is_lifecycle ? '' : filter.tags.join(' '));
- set_radio_buttons(
- content_node, [MATCH_ALL, MATCH_ANY],
- filter.find_all_tags ? MATCH_ALL : MATCH_ANY);
- var has_advanced_filters = !is_lifecycle && (
- filter.statuses.length ||
- filter.importances.length ||
- filter.tags.length) > 0,
- filters = has_advanced_filters ? [ADVANCED_FILTER] : [],
- event = ADDED_OR_CHANGED;
- // Chattiness: Lifecycle < Details < Discussion.
- switch (filter.bug_notification_level) {
- case 'Lifecycle':
- event = ADDED_OR_CLOSED;
- filters = [];
- break;
- case 'Details':
- filters.push(FILTER_COMMENTS);
- break;
- // case 'Discussion': This is already handled/the default.
- // default: If we get here then it is a programmer error.
- }
- set_radio_buttons(
- content_node, [ADDED_OR_CLOSED, ADDED_OR_CHANGED], event);
- set_checkboxes(
- content_node, [FILTER_COMMENTS, ADVANCED_FILTER], filters);
- handle_change(ADDED_OR_CHANGED, FILTER_WRAPPER, {duration: 0});
- handle_change(ADVANCED_FILTER, ACCORDION_WRAPPER, {duration: 0});
- context.filter_info = filter_info;
- context.filter_id = filter_id;
- var title = subscription.target_title;
- Y.one('#structural-subscription-context-title')
- .set('text', title);
- Y.one('#subscription-overlay-title')
- .set('text', 'Edit subscription for '+title+' bugs');
- add_subscription_overlay.show();
- }
+ e.halt();
+ show_edit_overlay(config, subscription, filter_info, filter_id);
};
}
@@ -1049,7 +1052,7 @@
/**
* Attach activation (click) handlers to all of the edit links on the page.
*/
-function wire_up_edit_links(config, context) {
+function wire_up_edit_links(config) {
var listing = Y.one('#subscription-listing');
var subscription_info = LP.cache.subscription_info;
var filter_id = 0;
@@ -1064,7 +1067,7 @@
var node = Y.one('#subscription-filter-'+filter_id.toString());
var edit_link = node.one('a.edit-subscription');
var edit_handler = make_edit_handler(
- sub, filter_info, filter_id, config, context);
+ sub, filter_info, filter_id, config);
edit_link.on('click', edit_handler);
var delete_link = node.one('a.delete-subscription');
var delete_handler = make_delete_handler(
@@ -1079,7 +1082,7 @@
/**
* Populate the subscription list DOM element with subscription descriptions.
*/
-function fill_in_bug_subscriptions(config, context) {
+function fill_in_bug_subscriptions(config) {
validate_config(config);
var listing = Y.one('#subscription-listing');
@@ -1159,7 +1162,7 @@
}
listing.appendChild(top_node);
- wire_up_edit_links(config, context);
+ wire_up_edit_links(config);
}
/**
@@ -1267,20 +1270,16 @@
// Expose in the namespace for testing purposes.
namespace._validate_config = validate_config;
-/*
- * External entry point for configuring the structual subscription.
- * @method setup
- * @param {Object} config Object literal of config name/value pairs.
- * config.content_box is the name of an element on the page where
- * the overlay will be anchored.
+/* Do pre-setup checks and initalizations.
+ * Sets up the LP client and ensures the user is logged-in.
*/
-namespace.setup = function(config) {
+function pre_setup(config) {
validate_config(config);
// If the user is not logged in, then we need to defer to the
// default behaviour.
if (LP.links.me === undefined) {
- return;
+ return false;
}
if (Y.Lang.isValue(config.lp_client)) {
// Tests can specify an lp_client if they want to.
@@ -1289,22 +1288,68 @@
// Setup the Launchpad client.
setup_client();
}
- // Create the overlay.
+ return true;
+};
+
+/* Show the overlay for creating a new subscription.
+ */
+function show_add_overlay(config) {
+ Y.one(config.content_box).empty();
var overlay_id = setup_overlay(config.content_box);
- // Create the subscription links on the page.
- setup_subscription_links(overlay_id, config.content_box);
+ clear_overlay(Y.one(config.content_box), false);
var submit_button = Y.Node.create(
'<button type="submit" name="field.actions.create" ' +
'value="Create subscription" class="lazr-pos lazr-btn" '+
'>OK</button>');
+
create_overlay(config.content_box, overlay_id, submit_button,
save_subscription);
// We need to initialize the help links. They may have already been
// initialized except for the ones we added, so setupHelpTrigger
// is idempotent. Notice that this is old MochiKit code.
forEach(findHelpLinks(), setupHelpTrigger);
-}; // setup
+ add_subscription_overlay.show()
+ return overlay_id;
+};
+
+/*
+ * Modify a link to pop up a subscription overlay.
+ *
+ * @method setup_subscription_link
+ * @param {String} link_id Id of the link element.
+ * @param {String} overlay_id Id of the overlay element.
+ */
+function setup_subscription_link(config, link_id) {
+ // Modify the menu-link-subscribe-to-bug-mail link to be visible.
+ var link = Y.one(link_id);
+ if (!Y.Lang.isValue(link)) {
+ Y.fail('Link to set as the pop-up link not found.');
+ }
+ link.removeClass('invisible-link');
+ link.addClass('visible-link');
+ link.on('click', function(e) {
+ e.halt();
+ show_add_overlay(config);
+ });
+ link.addClass('js-action');
+} // setup_subscription_links
+
+/*
+ * External entry point for configuring the structural subscription.
+ * @method setup
+ * @param {Object} config Object literal of config name/value pairs.
+ * config.content_box is the name of an element on the page where
+ * the overlay will be anchored.
+ */
+namespace.setup = function(config) {
+ // Return if pre-setup fails.
+ if (!pre_setup(config))
+ return;
+
+ // Create the subscription links on the page.
+ setup_subscription_link(config, '.menu-link-subscribe_to_bug_mail');
+}; // setup
}, '0.1', {requires: [
'dom', 'node', 'lazr.anim', 'lazr.formoverlay', 'lazr.overlay',
Follow ups
