yellow team mailing list archive
-
yellow team
-
Mailing list archive
-
Message #01704
[Merge] lp:~benji/juju-gui/bug-1074336 into lp:juju-gui
Benji York has proposed merging lp:~benji/juju-gui/bug-1074336 into lp:juju-gui.
Requested reviews:
Juju GUI Hackers (juju-gui)
For more details, see:
https://code.launchpad.net/~benji/juju-gui/bug-1074336/+merge/135261
refactored manageUnitsMixin and added tests
Full details:
- refactored manageUnitsMixin and added tests for it
- added documentation for manageUnitsMixin
- reconsidered our object literal style, updated the style guide to reflect my
current notions, and used the style in the code as a proof of concept
- captured current naming practice in the style guide (camelCase everywhere)
- tweaked all of the tests so the test container element is "display: none"
when possible and "visibility: hidden" otherwise
- transmuted an "apply" to a slightly simpler "call"
https://codereview.appspot.com/6845065/
--
https://code.launchpad.net/~benji/juju-gui/bug-1074336/+merge/135261
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~benji/juju-gui/bug-1074336 into lp:juju-gui.
=== modified file 'app/store/env.js'
--- app/store/env.js 2012-10-15 13:20:38 +0000
+++ app/store/env.js 2012-11-20 22:04:20 +0000
@@ -103,7 +103,7 @@
var tid = msg.request_id;
if (tid in this._txn_callbacks) {
console.log('Env: Dispatch Rpc');
- this._txn_callbacks[tid].apply(this, [msg]);
+ this._txn_callbacks[tid].call(this, msg);
delete this._txn_callbacks[tid];
}
}
=== modified file 'app/views/service.js'
--- app/views/service.js 2012-11-08 16:23:16 +0000
+++ app/views/service.js 2012-11-20 22:04:20 +0000
@@ -15,61 +15,118 @@
// Mixin attributes
events: {
'#num-service-units': {
- keydown: 'modifyUnits',
- blur: 'resetUnits'
+ keydown: 'handleKeyEvent',
+ blur: 'resetUnitInputBox'
}
},
- resetUnits: function() {
- var container = this.get('container'),
- field = container.one('#num-service-units');
+ /**
+ * Retrieve the input box that holds the user-specified number of units.
+ *
+ * @return {Object} An input box node.
+ */
+ _getInputBox: function() {
+ return this.get('container').one('#num-service-units');
+ },
+
+ /**
+ * Overwrite the number of units in the input field to the real number of
+ * units contained in the model and enable the input field.
+ *
+ * @method resetUnitInputBox
+ * @return {undefined} Side effects only.
+ */
+ resetUnitInputBox: function() {
+ var field = this._getInputBox();
field.set('value', this.get('model').get('unit_count'));
field.set('disabled', false);
},
- modifyUnits: function(ev) {
- if (ev.keyCode !== ESC && ev.keyCode !== ENTER) {
- return;
- }
- var container = this.get('container'),
- field = container.one('#num-service-units');
-
- if (ev.keyCode === ESC) {
- this.resetUnits();
- }
- if (ev.keyCode !== ENTER) { // If not Enter keyup...
- return;
- }
- ev.halt(true);
-
- if (/^\d+$/.test(field.get('value'))) {
- this._modifyUnits(parseInt(field.get('value'), 10));
- } else {
- this.resetUnits();
- }
- },
-
+ /**
+ * Interpret a key pressed inside the requested number of units field.
+ *
+ * @param {Number} keyCode The key that was pressed.
+ * @return {Boolean} True if the key press event should be halted.
+ */
+ _handleKeyPress: function(keyCode, fieldValue) {
+ if (keyCode === ESC) {
+ this.resetUnitInputBox();
+ return false;
+ } else if (keyCode === ENTER) {
+ var numberRegex = /^\d+$/;
+ if (numberRegex.test(fieldValue)) {
+ this._modifyUnits(parseInt(fieldValue, 10));
+ } else {
+ // The user entered something crazy. Reset the input box.
+ this.resetUnitInputBox();
+ }
+ return true;
+ }
+ return false;
+ },
+
+ /**
+ * Extract the relevant information from a key press event and call the key
+ * press handler.
+ *
+ * @param {Object} ev The event.
+ * @return {undefined} Side effects only.
+ */
+ handleKeyEvent: function(ev) {
+ var field = this._getInputBox();
+ ev.halt(this._handleKeyPress(ev.keyCode, field.get('value')));
+ },
+
+ /**
+ * Tell the backend that we want a new number of units for a service.
+ *
+ * @method _modifyUnits
+ * @param {Number} requested_unit_count The number of units the user wants.
+ * @return {undefined} Side effects only.
+ */
_modifyUnits: function(requested_unit_count) {
var service = this.get('model'),
unit_count = service.get('unit_count'),
- field = this.get('container').one('#num-service-units'),
- env = this.get('env');
-
+ field = this._getInputBox(),
+ env = this.get('env'),
+ db = this.get('db');
if (requested_unit_count < 1) {
console.log('You must have at least one unit');
field.set('value', unit_count);
return;
}
-
+ // We do not want the user changing the requested value while a request
+ // is outstanding.
+ field.set('disabled', true);
var delta = requested_unit_count - unit_count;
+ this._requestUnitDelta(delta, service);
+ },
+
+ /**
+ * Ask the backend to add or remove a given number of units.
+ *
+ * @param {Number} delta The number of units to add or remove.
+ * @param {String} service The service for which units should be added or
+ * removed.
+ * @param {Function} getUnits A function that returns all the units for a
+ * given service.
+ * @param {Function} addUnits A function that will add units for a service.
+ * @param {Function} removeUnits A function that will remove units for a
+ * service.
+ * @return {undefined} Side effects only.
+ */
+ _requestUnitDelta: function(delta, service) {
+ var env = this.get('env'),
+ db = this.get('db');
if (delta > 0) {
- // Add units!
+ // We need more units.
env.add_unit(
service.get('id'), delta,
Y.bind(this._addUnitCallback, this));
} else if (delta < 0) {
+ // We need fewer units.
delta = Math.abs(delta);
- var units = this.get('db').units.get_units_for_service(service),
+ var units = db.units.get_units_for_service(service),
unit_ids_to_remove = [];
for (var i = units.length - 1;
@@ -82,7 +139,6 @@
Y.bind(this._removeUnitCallback, this)
);
}
- field.set('disabled', true);
},
_addUnitCallback: function(ev) {
@@ -868,6 +924,7 @@
});
views.service = ServiceView;
+ views.unitCountMixin = manageUnitsMixin;
}, '0.1.0', {
requires: ['panel',
=== modified file 'docs/style-guide.rst'
--- docs/style-guide.rst 2012-11-07 18:41:11 +0000
+++ docs/style-guide.rst 2012-11-20 22:04:20 +0000
@@ -15,6 +15,23 @@
indention will be applied.
+Variable declaration
+====================
+
+Since global variables kill kittens, we use "var" when introducing a new
+variable. Our beautifier does not handle more than one variable
+declaration per "var" very well, so each variable should have its own,
+like so:
+
+ var a = 1;
+ var b = 2;
+
+Not like this:
+
+ var a = 1,
+ b = 2;
+
+
For loops
=========
@@ -25,8 +42,16 @@
Whitespace
==========
-No trailing whitespace on lines or at the end of the file (i.e., the file
-should end with a non-blank line).
+No trailing whitespace on lines or at the end of the file (i.e., the
+file should end with a non-blank line). Look at the file using xxd if
+you are unsure.
+
+
+Naming
+======
+
+Class names should be (upper) CamelCase, method, attribute, and variable
+names should be (lower) camelCase.
Object literal formatting
@@ -34,12 +59,18 @@
Things you should do:
-- object literals have their opening brace on a new line and the first
- value (if any) is on the same line (i.e., the opening brace is not on
- a line by itself)
-- there is a space after the opening brace of an object literal
-- object literals have their closing brace on the same line as their
- last value (i.e., the closing brace is not on a line by itself)
+- object literals have their opening brace on the same line as the
+ equals sign (e.g., "var foo = {"
+- multi-line object literals have a newline after the opening brace
+- multi-line object literals have their closing brace on a line by
+ itself (like functions)
+- nested object literals may have their closing brace on the same line
+ (like functions); e.g.:
+
+ var a = {
+ b: {
+ c: 42
+ }};
Things the beautifier should do (given an input line with no indentation
at all):
@@ -47,14 +78,12 @@
- all of the keys of an object literal line up (the second and
subsequent key names are indented 2 spaces more than the indentation
of the opening brace of the object literal
-- lines after a line that ends in an equals sign are indented 4 (more)
- spaces
-- lines after a line that ends in a colon are indented 6 (more) spaces
+- lines after a line that ends in a brace are indented 2 (more) spaces
An example::
- var values =
- { int_good: '1',
+ var values = {
+ int_good: '1',
int_bad: 'nope!'},
schema =
{ int_good:
=== modified file 'test/index.html'
--- test/index.html 2012-11-20 15:35:30 +0000
+++ test/index.html 2012-11-20 22:04:20 +0000
@@ -33,8 +33,12 @@
<script src="test_endpoints.js"></script>
<script src="test_application_notifications.js"></script>
<script src="test_charm_store.js"></script>
+<<<<<<< TREE
<script src="test_app_hotkeys.js"></script>
<script src="test_notifier_widget.js"></script>
+=======
+ <script src="test_manageUnitsMixin.js"></script>
+>>>>>>> MERGE-SOURCE
<script>
YUI().use('node', 'event', function(Y) {
=== modified file 'test/test_app.js'
--- test/test_app.js 2012-11-14 14:34:31 +0000
+++ test/test_app.js 2012-11-20 22:04:20 +0000
@@ -132,7 +132,10 @@
before(function(done) {
Y = YUI(GlobalConfig).use(['juju-gui', 'juju-tests-utils'], function(Y) {
- container = Y.Node.create('<div id="test" class="container"></div>');
+ container = Y.Node.create('<div/>')
+ .addClass('container')
+ .hide();
+ container.hide();
done();
});
@@ -193,7 +196,9 @@
env = new Y.juju.Environment({conn: conn});
env.connect();
conn.open();
- container = Y.Node.create('<div id="test" class="container"></div>');
+ container = Y.Node.create('<div/>')
+ .addClass('container')
+ .hide();
data = [];
app = new Y.juju.App(
{ container: container,
=== modified file 'test/test_application_notifications.js'
--- test/test_application_notifications.js 2012-11-19 16:36:12 +0000
+++ test/test_application_notifications.js 2012-11-20 22:04:20 +0000
@@ -39,7 +39,7 @@
'juju-tests-utils',
'node-event-simulate'],
function(Y) {
- applicationContainer = Y.Node.create('<div id="test-container" />');
+ applicationContainer = Y.Node.create('<div/>').hide();
applicationContainer.appendTo(Y.one('body'));
notificationsContainer = Y.Node.create('<div id="notifications" />');
=== modified file 'test/test_charm_collection_view.js'
--- test/test_charm_collection_view.js 2012-09-26 23:31:13 +0000
+++ test/test_charm_collection_view.js 2012-11-20 22:04:20 +0000
@@ -105,7 +105,7 @@
// Ensure the search results are rendered inside the container.
it('must correctly render the search results', function() {
- var container = Y.Node.create('<div id="test-container" />');
+ var container = Y.Node.create('<div/>').hide();
var MyView = Y.Base.create('MyView', CharmCollectionView, [], {
// Overriding to check the results as they are rendered in
// the container. Subclassing is required because render() is
=== modified file 'test/test_charm_view.js'
--- test/test_charm_view.js 2012-11-07 12:32:38 +0000
+++ test/test_charm_view.js 2012-11-20 22:04:20 +0000
@@ -54,7 +54,7 @@
created: 1340206387.539},
proof: {}};
- container = Y.Node.create('<div id="test-container" />');
+ container = Y.Node.create('<div/>').hide();
Y.one('#main').append(container);
CharmView = juju.views.charm;
// Use a local charm store.
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js 2012-11-15 20:48:30 +0000
+++ test/test_environment_view.js 2012-11-20 22:04:20 +0000
@@ -103,8 +103,15 @@
});
beforeEach(function(done) {
+<<<<<<< TREE
container = Y.Node.create('<div id="test-container" />');
Y.one('body').prepend(container);
+=======
+ container = Y.Node.create('<div/>')
+ .set('id', 'test-container')
+ .setStyle('visibility', 'hidden');
+ Y.one('body').append(container);
+>>>>>>> MERGE-SOURCE
db = new models.Database();
db.on_delta({data: environment_delta});
done();
=== added file 'test/test_manageUnitsMixin.js'
--- test/test_manageUnitsMixin.js 1970-01-01 00:00:00 +0000
+++ test/test_manageUnitsMixin.js 2012-11-20 22:04:20 +0000
@@ -0,0 +1,142 @@
+'use strict';
+
+(function() {
+ describe('unit management mixin', function() {
+ var views, Y, container, mixin, ENTER, ESC;
+
+ before(function(done) {
+ Y = YUI(GlobalConfig).use(
+ 'juju-views', 'juju-models', 'base', 'node', 'json-parse',
+ 'juju-env', 'node-event-simulate', 'juju-tests-utils',
+ function(Y) {
+ views = Y.namespace('juju.views');
+ mixin = views.unitCountMixin;
+ ENTER = Y.Node.DOM_EVENTS.key.eventDef.KEY_MAP.enter;
+ ESC = Y.Node.DOM_EVENTS.key.eventDef.KEY_MAP.esc;
+ done();
+ });
+ });
+
+ after(function(done) {
+ done();
+ });
+
+ beforeEach(function(done) {
+ container = Y.Node.create('<div id=\"test-container\"/>').hide();
+ Y.one('#main').append(container);
+ done();
+ });
+
+ afterEach(function(done) {
+ container.remove(true);
+ done();
+ });
+
+ it('should be able to get the unit number input box', function() {
+ var fauxField = {};
+ var fauxContainer = {
+ one: function(selector) {
+ assert.equal(selector, '#num-service-units');
+ return fauxField;
+ }};
+ var fauxThis = {
+ container: fauxContainer,
+ get: function(name) {
+ return this[name];
+ }};
+ var returned = mixin._getInputBox.call(fauxThis);
+ assert.strictEqual(returned, fauxField);
+ });
+
+ it('should be able to reset the unit number input box', function() {
+ var unitsRequested;
+ var fieldValues = {};
+ var fauxField = {
+ set: function(name, value) {
+ fieldValues[name] = value;
+ }};
+ var fauxThis = {
+ _getInputBox: function() {
+ return fauxField;
+ },
+ get: function(name) {
+ return this[name];
+ },
+ _modifyUnits: function(n) {
+ unitsRequested = n;
+ }};
+ mixin._handleKeyPress.call(fauxThis, ENTER, '10');
+ assert.equal(unitsRequested, 10);
+ });
+
+ it('should pass key events through to _handleKeyPress', function() {
+ // The handleKeyEvent function just extracts the pertinent details from
+ // the event and the input field and passes them to _handleKeyPress.
+ var fauxEvent = {
+ keyCode: 42,
+ halt: function() {}};
+ var fauxInputBox = {
+ get: function(name) {
+ assert.equal(name, 'value');
+ return '999';
+ }};
+ var passedKeyCode;
+ var passedFieldValue;
+ var fauxThis = {
+ _getInputBox: function() {return fauxInputBox;},
+ _handleKeyPress: function(keyCode, fieldValue) {
+ passedKeyCode = keyCode;
+ passedFieldValue = fieldValue;
+ }};
+ mixin.handleKeyEvent.call(fauxThis, fauxEvent);
+ assert.strictEqual(passedKeyCode, 42);
+ assert.strictEqual(passedFieldValue, '999');
+ });
+
+ it('should have keydown and blur event handlers', function() {
+ assert.isTrue('keydown' in mixin.events['#num-service-units']);
+ assert.isTrue('blur' in mixin.events['#num-service-units']);
+ });
+
+ it('should reset the unit count when the user presses escape', function() {
+ var inputBoxReset = false;
+ var fakeThis = {
+ resetUnitInputBox: function() {
+ inputBoxReset = true;
+ }};
+ mixin._handleKeyPress.call(fakeThis, ESC, 999);
+ assert.isTrue(inputBoxReset);
+ });
+
+ it('should reset the unit count a non-number is entered', function() {
+ var inputBoxReset = false;
+ var fakeThis = {
+ resetUnitInputBox: function() {
+ inputBoxReset = true;
+ }};
+ mixin._handleKeyPress.call(fakeThis, ENTER, 'this is not a number');
+ assert.isTrue(inputBoxReset);
+ });
+
+ it('must change the number of units when a number is entered', function() {
+ var unitsRequested;
+ var fakeThis = {
+ _modifyUnits: function(n) {
+ unitsRequested = n;
+ }};
+ mixin._handleKeyPress.call(fakeThis, ENTER, '10');
+ assert.equal(unitsRequested, 10);
+ });
+
+ it('should be able to apply a delta to the number of units', function() {
+ var unitsRequested;
+ var fakeThis = {
+ _modifyUnits: function(n) {
+ unitsRequested = n;
+ }};
+ mixin._handleKeyPress.call(fakeThis, ENTER, '10');
+ assert.equal(unitsRequested, 10);
+ });
+
+ });
+}) ();
=== modified file 'test/test_service_view.js'
--- test/test_service_view.js 2012-11-08 18:16:07 +0000
+++ test/test_service_view.js 2012-11-20 22:04:20 +0000
@@ -23,7 +23,7 @@
env = new (Y.namespace('juju')).Environment({conn: conn});
env.connect();
conn.open();
- container = Y.Node.create('<div id="test-container" />');
+ container = Y.Node.create('<div/>').hide();
Y.one('#main').append(container);
db = new models.Database();
charm = new models.Charm({id: 'cs:precise/mysql-7', description: 'A DB'});
=== modified file 'undocumented'
--- undocumented 2012-11-12 15:19:43 +0000
+++ undocumented 2012-11-20 22:04:20 +0000
@@ -151,7 +151,6 @@
app/views/service.js _addUnitCallback
app/views/service.js _destroyCallback
app/views/service.js _exposeServiceCallback
-app/views/service.js _modifyUnits
app/views/service.js _removeRelationCallback
app/views/service.js _removeUnitCallback
app/views/service.js _setConfigCallback
@@ -167,9 +166,7 @@
app/views/service.js getHeight
app/views/service.js getServiceTabs
app/views/service.js initializer
-app/views/service.js modifyUnits
app/views/service.js render
-app/views/service.js resetUnits
app/views/service.js saveConfig
app/views/service.js showErrors
app/views/service.js unexposeService
Follow ups
