launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10181
[Merge] lp:~rharding/launchpad/garden_client into lp:launchpad
Richard Harding has proposed merging lp:~rharding/launchpad/garden_client into lp:launchpad with lp:~rharding/launchpad/tab_client as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rharding/launchpad/garden_client/+merge/116330
= Summary =
In the process of getting some LoC points towards the bug fix in another
branch this performs some cleanup of the code in the client.js.
== Implementation Notes ==
Move all objects/methods not attached to the module (thus private) to the
top of the file.
Fix 2-space lines with 4-space indentation.
Change the ErrorHandler, FormErrorHandler, PatchPlugin to be created via
Y.Base.create().
Assign objects straight to module vs creating them as vars and then later
perform a:
module.SomeObject = SomeObject
Fixed a comment block to match.
Removing some double spaces lines to try to keep things consistant as there
are various 2-line/1-line spacing between objects and methods.
Make sure variables are declared (querystring) so that they don't leak to the
global window scope.
And some line length cleanup due to the increased tab depth from the prev
branch.
== Tests ==
lib/lp/app/javascript/tests/test_lp_client.html
== Lint ==
Linting changed files:
lib/lp/app/javascript/client.js
== LoC Qualification ==
Negative LoC
--
https://code.launchpad.net/~rharding/launchpad/garden_client/+merge/116330
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/garden_client into lp:launchpad.
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2012-07-23 18:09:20 +0000
+++ lib/lp/app/javascript/client.js 2012-07-23 18:09:20 +0000
@@ -7,6 +7,75 @@
* @module Y.lp.client
*/
YUI.add('lp.client', function(Y) {
+ // Private code used only in the module.
+ var NOTIFICATION_INFO = {
+ 'level10': {
+ 'selector': '.debug.message',
+ 'css_class': 'debug message'
+ },
+ 'level20': {
+ 'selector': '.informational.message',
+ 'css_class': 'informational message'
+ },
+ 'level30': {
+ 'selector': '.warning.message',
+ 'css_class': 'warning message'
+ },
+ 'level40': {
+ 'selector': '.error.message',
+ 'css_class': 'error message'
+ }
+ };
+
+ var update_cached_object = function (cache_name, cache, entry) {
+ var fields_changed = [];
+ var name;
+ var html_name;
+ for (name in cache) {
+ if (cache.hasOwnProperty(name)) {
+ var old_value = cache[name];
+ var new_value = entry.get(name);
+ if (name !== 'lp_html') {
+ if (old_value !== new_value) {
+ fields_changed.push(name);
+ cache[name] = new_value;
+ var field_updated_event_name =
+ 'lp:' + cache_name + ':' + name + ':changed';
+ var new_value_html = entry.getHTML(name);
+ var event = {
+ 'name': name,
+ 'old_value': old_value,
+ 'new_value': new_value,
+ 'new_value_html': new_value_html,
+ 'entry': entry
+ };
+ Y.fire(field_updated_event_name, event);
+ }
+ }
+ else {
+ // Since we don't care here about the content, we aren't
+ // using the values here to determine if the field has
+ // changed, so we can just update the cache.
+ for (html_name in old_value) {
+ if (old_value.hasOwnProperty(html_name)) {
+ old_value[html_name] = new_value[html_name];
+ }
+ }
+ }
+ }
+ }
+
+ if (fields_changed.length > 0) {
+ var event_name = 'lp:' + cache_name + ':changed';
+ var event_ = {
+ 'fields_changed': fields_changed,
+ 'entry': entry
+ };
+ Y.fire(event_name, event_);
+ }
+ };
+
+
var module = Y.namespace('lp.client');
@@ -120,11 +189,11 @@
};
module.normalize_uri = function(uri) {
- /* Converts an absolute URI into a relative URI.
-
- Appends the root to a relative URI that lacks the root.
-
- Does nothing to a relative URI that includes the root.*/
+ /**
+ * Converts an absolute URI into a relative URI.
+ * Appends the root to a relative URI that lacks the root.
+ * Does nothing to a relative URI that includes the root.
+ */
var host_start = uri.indexOf('//');
if (host_start !== -1) {
var host_end = uri.indexOf('/', host_start+2);
@@ -183,7 +252,6 @@
}
};
-
/**
* Get the URL of the view for an Entry
* @method get_view_url
@@ -195,15 +263,15 @@
*/
module.get_view_url = function(entry, view_name, namespace, query){
entry_url = Y.lp.get_url_path(entry.get('web_link'));
- querystring = Y.QueryString.stringify(query);
+ var querystring = Y.QueryString.stringify(query);
if (querystring !== '') {
querystring = '?' + querystring;
}
return (
- entry_url + '/' + view_name + '/++' + namespace + '++' + querystring);
+ entry_url + '/' + view_name + '/++' +
+ namespace + '++' + querystring);
};
-
/**
* Get the URL of the form for a view for an Entry
* @method get_form_url
@@ -215,7 +283,6 @@
return module.get_view_url(entry, view_name, 'form');
};
-
/**
* Load the model for a view.
*
@@ -237,7 +304,6 @@
io_provider.io(url, y_config);
};
-
module.add_accept = function(config, headers) {
if (headers === undefined) {
headers = {};
@@ -258,77 +324,27 @@
return data;
};
- var update_cached_object = function (cache_name, cache, entry)
- {
- var fields_changed = [];
- var name;
- var html_name;
- for (name in cache) {
- if (cache.hasOwnProperty(name)) {
- var old_value = cache[name];
- var new_value = entry.get(name);
- if (name !== 'lp_html') {
- if (old_value !== new_value) {
- fields_changed.push(name);
- cache[name] = new_value;
- var field_updated_event_name =
- 'lp:' + cache_name + ':' + name + ':changed';
- var new_value_html = entry.getHTML(name);
- var event = {
- 'name': name,
- 'old_value': old_value,
- 'new_value': new_value,
- 'new_value_html': new_value_html,
- 'entry': entry
- };
- Y.fire(field_updated_event_name, event);
- }
- }
- else {
- // Since we don't care here about the content, we aren't using the
- // values here to determine if the field has changed, so we can just
- // update the cache.
- for (html_name in old_value) {
- if (old_value.hasOwnProperty(html_name)) {
- old_value[html_name] = new_value[html_name];
- }
- }
- }
- }
- }
-
- if (fields_changed.length > 0) {
- var event_name = 'lp:' + cache_name + ':changed';
- var event_ = {
- 'fields_changed': fields_changed,
- 'entry': entry
- };
- Y.fire(event_name, event_);
- }
- };
-
-
module.update_cache = function(entry) {
- if (!entry) {
- return;
- }
- var original_uri = entry.lp_original_uri;
- var full_uri = module.get_absolute_uri(original_uri);
- var name;
- var cached_object;
- for (name in LP.cache) {
- if (LP.cache.hasOwnProperty(name)) {
- cached_object = LP.cache[name];
- /*jslint continue:true*/
- if (!Y.Lang.isValue(cached_object)) {
- continue;
- }
- if (cached_object.self_link === full_uri) {
- Y.log(name + ' cached object has been updated.');
- update_cached_object(name, cached_object, entry);
- }
- }
- }
+ if (!entry) {
+ return;
+ }
+ var original_uri = entry.lp_original_uri;
+ var full_uri = module.get_absolute_uri(original_uri);
+ var name;
+ var cached_object;
+ for (name in LP.cache) {
+ if (LP.cache.hasOwnProperty(name)) {
+ cached_object = LP.cache[name];
+ /*jslint continue:true*/
+ if (!Y.Lang.isValue(cached_object)) {
+ continue;
+ }
+ if (cached_object.self_link === full_uri) {
+ Y.log(name + ' cached object has been updated.');
+ update_cached_object(name, cached_object, entry);
+ }
+ }
+ }
};
module.wrap_resource_on_success = function(ignore, response, args) {
@@ -364,25 +380,6 @@
}
};
- var NOTIFICATION_INFO = {
- 'level10': {
- 'selector': '.debug.message',
- 'css_class': 'debug message'
- },
- 'level20': {
- 'selector': '.informational.message',
- 'css_class': 'informational message'
- },
- 'level30': {
- 'selector': '.warning.message',
- 'css_class': 'warning message'
- },
- 'level40': {
- 'selector': '.error.message',
- 'css_class': 'error message'
- }
- };
-
/**
* Display a list of notifications - error, warning, informational or debug.
* @param notifications An json encoded array of (level, message) tuples.
@@ -455,8 +452,7 @@
// The resources that come together to make Launchpad.
// A hosted file resource.
-
- var HostedFile = function(client, uri, content_type, contents) {
+ module.HostedFile = function(client, uri, content_type, contents) {
/* A binary file manipulable through the web service. */
this.lp_client = client;
this.uri = uri;
@@ -465,8 +461,7 @@
this.io_provider = client.io_provider;
};
- HostedFile.prototype = {
-
+ module.HostedFile.prototype = {
'lp_save' : function(config) {
/* Write a new version of this file back to the web service. */
var on = config.on;
@@ -482,7 +477,8 @@
'data': hosted_file.contents,
'sync': this.lp_client.sync
};
- this.io_provider.io(module.normalize_uri(hosted_file.uri), y_config);
+ this.io_provider.io(module.normalize_uri(hosted_file.uri),
+ y_config);
},
'lp_delete' : function(config) {
@@ -498,12 +494,10 @@
}
};
- module.HostedFile = HostedFile;
-
- var Resource = function() {
+ module.Resource = function() {
/* The base class for objects retrieved from Launchpad's web service. */
};
- Resource.prototype = {
+ module.Resource.prototype = {
'init': function(client, representation, uri) {
/* Initialize a resource with its representation and URI. */
this.lp_client = client;
@@ -533,9 +527,9 @@
}
// If the response is 404, it means we have a hosted file that
- // doesn't exist yet. If the response is 303 and goes off to another
- // site, that means we have a hosted file that does exist. Either way
- // we should turn the failure into a success.
+ // doesn't exist yet. If the response is 303 and goes off to
+ // another site, that means we have a hosted file that does exist.
+ // Either way we should turn the failure into a success.
var on_success = on.success;
var old_on_failure = on.failure;
on.failure = function(ignore, response, args) {
@@ -543,7 +537,7 @@
var original_url = args[1];
if (response.status === module.HTTP_NOT_FOUND ||
response.status === module.HTTP_SEE_ALSO) {
- var file = new HostedFile(client, original_url);
+ var file = new module.HostedFile(client, original_url);
return on_success(file);
} else if (old_on_failure !== undefined) {
return old_on_failure(ignore, response, args);
@@ -554,43 +548,41 @@
'named_get': function(operation_name, config) {
/* Get the result of a named GET operation on this resource. */
- return this.lp_client.named_get(this.lp_original_uri, operation_name,
+ return this.lp_client.named_get(this.lp_original_uri,
+ operation_name,
config);
},
'named_post': function(operation_name, config) {
/* Trigger a named POST operation on this resource. */
- return this.lp_client.named_post(this.lp_original_uri, operation_name,
+ return this.lp_client.named_post(this.lp_original_uri,
+ operation_name,
config);
}
};
- module.Resource = Resource;
-
-
// The service root resource.
- Root = function(client, representation, uri) {
+ module.Root = function(client, representation, uri) {
/* The root of the Launchpad web service. */
this.init(client, representation, uri);
};
- Root.prototype = new Resource();
-
- module.Root = Root;
-
-
- var Collection = function(client, representation, uri) {
+ module.Root.prototype = new module.Resource();
+
+ module.Collection = function(client, representation, uri) {
/* A grouped collection of objets from the Launchpad web service. */
var index, entry;
this.init(client, representation, uri);
for (index = 0 ; index < this.entries.length ; index++) {
entry = this.entries[index];
- this.entries[index] = new Entry(client, entry, entry.self_link);
+ this.entries[index] = new module.Entry(client,
+ entry,
+ entry.self_link);
}
};
- Collection.prototype = new Resource();
+ module.Collection.prototype = new module.Resource();
- Collection.prototype.lp_slice = function(on, start, size) {
+ module.Collection.prototype.lp_slice = function(on, start, size) {
/* Retrieve a subset of the collection.
:param start: Where in the collection to start serving entries.
@@ -600,9 +592,7 @@
{on: on, start: start, size: size});
};
- module.Collection = Collection;
-
- var Entry = function(client, representation, uri) {
+ module.Entry = function(client, representation, uri) {
/* A single object from the Launchpad web service. */
this.lp_client = client;
this.lp_original_uri = uri;
@@ -620,19 +610,19 @@
}
};
- Entry.prototype = new Resource();
+ module.Entry.prototype = new module.Resource();
// Augment with Attribute so that we can listen for attribute change events.
- Y.augment(Entry, Y.Attribute);
+ Y.augment(module.Entry, Y.Attribute);
- Entry.prototype.mark_as_dirty = function(event) {
+ module.Entry.prototype.mark_as_dirty = function(event) {
/* Respond to an event triggered by modification to an Entry's field. */
if (event.newVal !== event.prevVal) {
this.dirty_attributes.push(event.attrName);
}
};
- Entry.prototype.lp_save = function(config) {
+ module.Entry.prototype.lp_save = function(config) {
/* Write modifications to this entry back to the web service. */
var representation = {};
var entry = this;
@@ -648,12 +638,12 @@
this.dirty_attributes = [];
};
- Entry.prototype.lookup_value = function(key) {
+ module.Entry.prototype.lookup_value = function(key) {
/* A common getter interface between Entrys and non-Entrys. */
return this.get(key);
};
- Entry.prototype.getHTML = function(key) {
+ module.Entry.prototype.getHTML = function(key) {
var lp_html = this.get('lp_html');
if (lp_html) {
// First look for the key.
@@ -671,17 +661,14 @@
return null;
};
- module.Entry = Entry;
-
// The Launchpad client itself.
-
- var Launchpad = function(config) {
+ module.Launchpad = function(config) {
/* A client that makes HTTP requests to Launchpad's web service. */
this.io_provider = module.get_configured_io_provider(config);
this.sync = (config ? config.sync : false);
};
- Launchpad.prototype = {
+ module.Launchpad.prototype = {
'get': function (uri, config) {
/* Get the current state of a resource. */
var on = Y.merge(config.on);
@@ -750,7 +737,9 @@
{ on: { success: old_on_success,
failure: on.failure } });
}
- return module.wrap_resource_on_success(undefined, response, args);
+ return module.wrap_resource_on_success(undefined,
+ response,
+ args);
};
var client = this;
var update_cache = false;
@@ -814,7 +803,7 @@
|| representation.total_size_link !== undefined) {
// It's a list. Treat it as a collection;
// it should be slicable.
- return new Collection(this, representation, uri);
+ return new module.Collection(this, representation, uri);
} else if (Y.Lang.isObject(representation)) {
// It's an Array or mapping. Recurse into it.
if (Y.Lang.isArray(representation)) {
@@ -840,18 +829,15 @@
}
} else if (representation.resource_type_link.search(
/\/#service-root$/) !== -1) {
- return new Root(this, representation, uri);
+ return new module.Root(this, representation, uri);
} else if (representation.total_size === undefined) {
- return new Entry(this, representation, uri);
+ return new module.Entry(this, representation, uri);
} else {
- return new Collection(this, representation, uri);
+ return new module.Collection(this, representation, uri);
}
}
};
- module.Launchpad = Launchpad;
-
-
/**
* Helper object for handling XHR failures.
* clearProgressUI() and showError() need to be defined by the callsite
@@ -859,12 +845,7 @@
*
* @class ErrorHandler
*/
- var ErrorHandler;
- ErrorHandler = function(config) {
- ErrorHandler.superclass.constructor.apply(this, arguments);
- };
-
- Y.extend(ErrorHandler, Y.Base, {
+ module.ErrorHandler = Y.Base.create('client-error-handler', Y.Base, [], {
/**
* Clear the progress indicator.
*
@@ -898,8 +879,8 @@
* @method handleError
* @param ioId The request id.
* @param response The XHR call response object.
- * @return {Boolean} Return true if the error has been fully processed and
- * any further generic error handling is not required.
+ * @return {Boolean} Return true if the error has been fully processed
+ * and any further generic error handling is not required.
*/
handleError: function(ioId, response) {
return false;
@@ -917,8 +898,8 @@
var self = this;
return function(ioId, o) {
self.clearProgressUI();
- // Perform any user specified error handling. If true is returned,
- // we do not do any further processing.
+ // Perform any user specified error handling. If true is
+ // returned, we do not do any further processing.
if( self.handleError(ioId, o) ) {
return;
}
@@ -951,21 +932,8 @@
}
});
- module.ErrorHandler = ErrorHandler;
-
- var FormErrorHandler;
- FormErrorHandler = function(config) {
- FormErrorHandler.superclass.constructor.apply(this, arguments);
- };
-
- FormErrorHandler.ATTRS = {
- form: {
- value: null
- }
- };
-
- Y.extend(FormErrorHandler, ErrorHandler, {
-
+ module.FormErrorHandler = Y.Base.create('client-form-error-handler',
+ module.ErrorHandler, [], {
// Clear any errors on the form.
clearFormErrors: function() {
Y.all('.error.message').remove(true);
@@ -973,8 +941,8 @@
Y.all('div.error').removeClass('error');
},
- // If the XHR call returns a form validation error, we display the errors
- // on the form.
+ // If the XHR call returns a form validation error, we display the
+ // errors on the form.
handleError: function(ioId, response) {
if (response.status === 400
&& response.statusText === 'Validation') {
@@ -1051,15 +1019,19 @@
return response.status + ' ' + response.statusText;
}
}
- });
-
- module.FormErrorHandler = FormErrorHandler;
-
-
-}, "0.1",
- {"requires":["attribute", "io", "querystring", "json-parse",
- "json-stringify", "lp"]});
-
+ }, {
+ ATTRS: {
+ form: {
+ value: null
+ }
+ }
+ });
+
+
+}, "0.1", {
+ requires: ["attribute", "base", "io", "querystring", "json-parse",
+ "json-stringify", "lp"]
+});
YUI.add('lp.client.plugins', function (Y) {
@@ -1078,81 +1050,8 @@
* @class PATCHPlugin
* @extends Widget
*/
- var PATCHPlugin = function PATCHPlugin () {
- PATCHPlugin.superclass.constructor.apply(this, arguments);
- };
-
- Y.mix(PATCHPlugin, {
- /**
- * The identity of the plugin.
- *
- * @property PATCHPlugin.NAME
- * @type String
- * @static
- */
- NAME: 'PATCHPlugin',
-
- /**
- * The namespace of the plugin.
- *
- * @property PATCHPlugin.NS
- * @type String
- * @static
- */
- NS: 'patcher',
-
- /**
- * Static property used to define the default attribute configuration of
- * this plugin.
- *
- * @property PATCHPlugin.ATTRS
- * @type Object
- * @static
- */
- ATTRS : {
- /**
- * Name of the attribute to patch.
- *
- * @attribute patch
- * @type String
- */
- patch: {},
-
- /**
- * URL of the resource to PATCH.
- *
- * @attribute resource
- * @type String
- */
- resource: {},
-
- /**
- * Should the resulting field get the value from the lp_html
- * attribute?
- *
- * @attribute use_html
- * @type Boolean
- */
- use_html: false,
-
- /**
- * The function to use to format the returned result into a form that
- * can be inserted into the page DOM.
- *
- * The default value is a function that simply returns the result
- * unmodified.
- *
- * @attribute formatter
- * @type Function
- * @default null
- */
- formatter: {
- valueFn: function() { return this._defaultFormatter; }
- }
- }});
-
- Y.extend(PATCHPlugin, Y.Plugin.Base, {
-
+ module.PATCHPlugin = Y.Base.create('client-plugin-patch',
+ Y.Plugin.Base, [], {
/**
* Configuration parameters that will be passed through to the lp.client
* call.
@@ -1171,15 +1070,17 @@
*/
initializer: function(config) {
if (!Y.Lang.isString(config.patch)) {
- Y.error("missing config: 'patch' containing the attribute name");
+ Y.error(
+ "missing config: 'patch' containing the attribute name");
}
if (!Y.Lang.isString(config.resource)) {
- Y.error("missing config: 'resource' containing the URL to patch");
+ Y.error(
+ "missing config: 'resource' containing the URL to patch");
}
- // Save the config object that the user passed in so that we can pass
- // any extra parameters through to the lp.client constructor.
+ // Save the config object that the user passed in so that we can
+ // pass any extra parameters through to the lp.client constructor.
this.extra_config = config || {};
this.extra_config.accept = 'application/json;include=lp_html';
@@ -1250,8 +1151,8 @@
},
/**
- * Return the webservice Entry object attribute that is to be shown in the
- * page DOM.
+ * Return the webservice Entry object attribute that is to be shown in
+ * the page DOM.
*
* This function may be overridden in various ways.
*
@@ -1259,8 +1160,8 @@
* @protected
* @param result {Entry|String} A Launchpad webservice Entry object, or
* the unmodified result string if the default Content-Type wasn't used.
- * @param attribute {String} The resource attribute that the PATCH request
- * was sent to.
+ * @param attribute {String} The resource attribute that the PATCH
+ * request was sent to.
* @return {String|Node} A string or Node instance to be inserted into
* the DOM.
*/
@@ -1275,10 +1176,76 @@
}
}
}
+ }, {
+ /**
+ * The identity of the plugin.
+ *
+ * @property PATCHPlugin.NAME
+ * @type String
+ * @static
+ */
+ NAME: 'PATCHPlugin',
+
+ /**
+ * The namespace of the plugin.
+ *
+ * @property PATCHPlugin.NS
+ * @type String
+ * @static
+ */
+ NS: 'patcher',
+
+ /**
+ * Static property used to define the default attribute configuration of
+ * this plugin.
+ *
+ * @property PATCHPlugin.ATTRS
+ * @type Object
+ * @static
+ */
+ ATTRS : {
+ /**
+ * Name of the attribute to patch.
+ *
+ * @attribute patch
+ * @type String
+ */
+ patch: {},
+
+ /**
+ * URL of the resource to PATCH.
+ *
+ * @attribute resource
+ * @type String
+ */
+ resource: {},
+
+ /**
+ * Should the resulting field get the value from the lp_html
+ * attribute?
+ *
+ * @attribute use_html
+ * @type Boolean
+ */
+ use_html: false,
+
+ /**
+ * The function to use to format the returned result into a form
+ * that can be inserted into the page DOM.
+ *
+ * The default value is a function that simply returns the result
+ * unmodified.
+ *
+ * @attribute formatter
+ * @type Function
+ * @default null
+ */
+ formatter: {
+ valueFn: function() { return this._defaultFormatter; }
+ }
+ }
});
- module.PATCHPlugin = PATCHPlugin;
-
}, "0.1", {
- requires: ["plugin", "dump", "lazr.editor", "lp.client"]
+ requires: ["base", "plugin", "dump", "lazr.editor", "lp.client"]
});
Follow ups
