Merge lp:~jcsackett/juju-gui/remove-charm-extension-in-browser-charm into lp:juju-gui/experimental

[j.c.sackett]

Removes Charm extension from BrowserCharm

In order to be able to safely remove Charm, BrowserCharm cannot extend it.
-BrowserCharm now extends Model
-BrowserCharm now supports the initialization functions that Charm does (e.g.
setting attr from the provided ID)
-BrowserCharm now has the same `load` pipeline functions (e.g. `sync`)
-Tests have been updated or added to demonstrate new support
-Drive by test cleanup--there were many tests that checked the same exact
things; I've merged these to mitigate the damage from some of the test
repetition I've had to temporarily add.

https://codereview.appspot.com/12608043/

[j.c.sackett (jcsackett)]

Reviewers: mp+179005_code.launchpad.net,

Message:
Please take a look.

Description:
Removes Charm extension from BrowserCharm

In order to be able to safely remove Charm, BrowserCharm cannot extend
it.
-BrowserCharm now extends Model
-BrowserCharm now supports the initialization functions that Charm does
(e.g.
setting attr from the provided ID)
-BrowserCharm now has the same `load` pipeline functions (e.g. `sync`)
-Tests have been updated or added to demonstrate new support
-Drive by test cleanup--there were many tests that checked the same
exact
things; I've merged these to mitigate the damage from some of the test
repetition I've had to temporarily add.

https://code.launchpad.net/~jcsackett/juju-gui/remove-charm-extension-in-browser-charm/+merge/179005

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12608043/

Affected files:
   A [revision details]
   M app/models/charm.js
   M test/test_model.js

[Richard Harding (rharding)]

LGTM getting closer!

https://codereview.appspot.com/12608043/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/12608043/diff/1/app/models/charm.js#newcode540
app/models/charm.js:540: if (data.config && data.config.options && !
data.options) {
space after the !

https://codereview.appspot.com/12608043/

[Madison Scott-Clary (makyo)]

LGTM, thanks!

https://codereview.appspot.com/12608043/

[lp:~jcsackett/juju-gui/remove-charm-extension-in-browser-charm]

935. By j.c.sackett on 2013-08-07

Merged trunk.

936. By j.c.sackett on 2013-08-07

Change from review.

[j.c.sackett (jcsackett)]

*** Submitted:

Removes Charm extension from BrowserCharm

In order to be able to safely remove Charm, BrowserCharm cannot extend
it.
-BrowserCharm now extends Model
-BrowserCharm now supports the initialization functions that Charm does
(e.g.
setting attr from the provided ID)
-BrowserCharm now has the same `load` pipeline functions (e.g. `sync`)
-Tests have been updated or added to demonstrate new support
-Drive by test cleanup--there were many tests that checked the same
exact
things; I've merged these to mitigate the damage from some of the test
repetition I've had to temporarily add.

R=rharding, matthew.scott
CC=
https://codereview.appspot.com/12608043

https://codereview.appspot.com/12608043/

=== modified file 'app/models/charm.js'
--- app/models/charm.js	2013-07-31 20:15:56 +0000
+++ app/models/charm.js	2013-08-07 21:02:27 +0000
@@ -216,7 +216,7 @@
       data.is_subordinate = data.subordinate || data.is_subordinate;
       // Because the old and new charm models have different places for
       // the options data, this handles the normalization.
-      if (data.config && data.config.options && ! data.options) {
+      if (data.config && data.config.options && !data.options) {
         data.options = data.config.options;
         delete data.config;
       }
@@ -397,10 +397,10 @@
    * Model to represent the Charms from the Charmworld2 Api.
    *
    * @class BrowserCharm
-   * @extends {Charm}
+   * @extends {Y.Model}
    *
    */
-  models.BrowserCharm = Y.Base.create('browser-charm', Charm, [], {
+  models.BrowserCharm = Y.Base.create('browser-charm', Y.Model, [], {
     // Only care about at most, this number of related charms per interface.
     maxRelatedCharms: 5,
 
@@ -486,8 +486,94 @@
         }
         if (cfg.id) {
           this.set('storeId', cfg.id);
-          this.set('id', this.get('scheme') + ':' + cfg.id);
-        }
+        }
+        if (cfg.url) {
+          this.set('id', cfg.url);
+        }
+      }
+      var id = this.get('id'),
+          parts = parseCharmId(id),
+          self = this;
+      if (!parts) {
+        throw 'Developers must initialize charms with a well-formed id.';
+      }
+      this.loaded = false;
+      this.on('load', function() { this.loaded = true; });
+      Y.Object.each(parts, function(value, key) {
+        this.set(key, value);
+      }, this);
+    },
+
+    sync: function(action, options, callback) {
+      if (action !== 'read') {
+        throw (
+            'Only use the "read" action; "' + action + '" not supported.');
+      }
+      if (Y.Lang.isValue(options.get_charm)) {
+        // This is an env.
+        options.get_charm(this.get('id'), function(response) {
+          if (response.err) {
+            callback(true, response);
+          } else if (response.result) {
+            callback(false, response.result);
+          } else {
+            // What's going on?  This does not look like either of our
+            // expected signatures.  Declare a loading error.
+            callback(true, response);
+          }
+        });
+      } else {
+        throw 'You must supply a get_charm function.';
+      }
+    },
+
+    parse: function(response) {
+      var data = Charm.superclass.parse.apply(this, arguments),
+          self = this;
+
+      // TODO (gary): verify whether is_subordinate is ever passed by pyjuju
+      // or juju core.  If not, remove the "|| data.is_subordinate" and change
+      // in the fakebackend and/or sandbox to send the expected thing there.
+      data.is_subordinate = data.subordinate || data.is_subordinate;
+      // Because the old and new charm models have different places for
+      // the options data, this handles the normalization.
+      if (data.config && data.config.options && !data.options) {
+        data.options = data.config.options;
+        delete data.config;
+      }
+      Y.each(data, function(value, key) {
+        if (!Y.Lang.isValue(value) ||
+            !self.attrAdded(key) ||
+            Y.Lang.isValue(self.get(key))) {
+          delete data[key];
+        }
+      });
+      if (data.owner === 'charmers') {
+        delete data.owner;
+      }
+      return data;
+    },
+
+    compare: function(other, relevance, otherRelevance) {
+      // Official charms sort before owned charms.
+      // If !X.owner, that means it is owned by charmers.
+      var owner = this.get('owner'),
+          otherOwner = other.get('owner');
+      if (!owner && otherOwner) {
+        return -1;
+      } else if (owner && !otherOwner) {
+        return 1;
+      // Relevance is next most important.
+      } else if (relevance && (relevance !== otherRelevance)) {
+        // Higher relevance comes first.
+        return otherRelevance - relevance;
+      // Otherwise sort by package name, then by owner, then by revision.
+      } else {
+        return (
+                (this.get('package_name').localeCompare(
+                other.get('package_name'))) ||
+                (owner ? owner.localeCompare(otherOwner) : 0) ||
+                (this.get('revision') - other.get('revision')));
       }
     },
 
@@ -562,6 +648,27 @@
       changelog: {
         value: {}
       },
+      //XXX jcsackett Aug 7 2013 This attribute is only needed until we turn
+      // on the service inspector. It's just used by the charm view you get when
+      // inspecting a service, and should be ripped out (along with tests) when
+      // we remove that view.
+      charm_path: {
+        /**
+         * Generate the charm store path from the attributes of the charm.
+         *
+         * @method getter
+         *
+         */
+        getter: function() {
+          var owner = this.get('owner');
+          return [
+            (owner ? '~' + owner : 'charms'),
+            this.get('series'),
+            (this.get('package_name') + '-' + this.get('revision')),
+            'json'
+          ].join('/');
+        }
+      },
       /**
        * Object of data about the source for this charm including bugs link,
        * log, revisions, etc.

=== modified file 'test/test_model.js'
--- test/test_model.js	2013-07-31 20:15:56 +0000
+++ test/test_model.js	2013-08-07 21:02:27 +0000
@@ -16,10 +16,9 @@
 with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
-
 'use strict';
 
-describe('charm normalization', function() {
+describe('Charm and BrowserCharm initialization', function() {
   var models;
 
   before(function(done) {
@@ -29,19 +28,101 @@
     });
   });
 
-  it('must create derived attributes from official charm id', function() {
+  it('must be able to create Charm', function() {
     var charm = new models.Charm(
-        {id: 'cs:precise/openstack-dashboard-0'});
-    charm.get('scheme').should.equal('cs');
-    var _ = expect(charm.get('owner')).to.not.exist;
-    charm.get('full_name').should.equal('precise/openstack-dashboard');
-    charm.get('charm_path').should.equal(
-        'charms/precise/openstack-dashboard-0/json');
+        {id: 'cs:~alt-bac/precise/openstack-dashboard-0'});
+    charm.get('scheme').should.equal('cs');
+    charm.get('owner').should.equal('alt-bac');
+    charm.get('series').should.equal('precise');
+    charm.get('package_name').should.equal('openstack-dashboard');
+    charm.get('revision').should.equal(0);
+    charm.get('full_name').should.equal(
+        '~alt-bac/precise/openstack-dashboard');
+    charm.get('charm_path').should.equal(
+        '~alt-bac/precise/openstack-dashboard-0/json');
+  });
+
+  it('must be able to create BrowserCharm', function() {
+    var charm = new models.BrowserCharm(
+        {id: 'cs:~alt-bac/precise/openstack-dashboard-0'});
+    charm.get('scheme').should.equal('cs');
+    charm.get('owner').should.equal('alt-bac');
+    charm.get('series').should.equal('precise');
+    charm.get('package_name').should.equal('openstack-dashboard');
+    charm.get('revision').should.equal(0);
+    charm.get('full_name').should.equal(
+        '~alt-bac/precise/openstack-dashboard');
+    charm.get('charm_path').should.equal(
+        '~alt-bac/precise/openstack-dashboard-0/json');
+  });
+
+  it('must not set "owner" for promulgated charms', function() {
+    var charm;
+    charm = new models.Charm({id: 'cs:precise/openstack-dashboard-0'});
+    assert.isUndefined(charm.get('owner'));
+
+    charm = new models.BrowserCharm({
+      id: 'cs:precise/openstack-dashboard-0'
+    });
+    assert.isUndefined(charm.get('owner'));
+  });
+
+  it('must be able to parse hyphenated owner names', function() {
+    // Note that an earlier version of the parsing code did not handle
+    // hyphens in user names, so this test intentionally includes one.
+    var charm;
+    charm = new models.Charm(
+        {id: 'cs:~marco-ceppi/precise/wordpress-17'});
+    charm.get('full_name').should.equal('~marco-ceppi/precise/wordpress');
+
+    charm = new models.BrowserCharm(
+        {id: 'cs:~marco-ceppi/precise/wordpress-17'});
+    charm.get('full_name').should.equal('~marco-ceppi/precise/wordpress');
+  });
+
+  it('must reject bad charm ids.', function() {
+    var charm;
+    try {
+      charm = new models.Charm({id: 'foobar'});
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal(
+          'Developers must initialize charms with a well-formed id.');
+    }
+
+    try {
+      charm = new models.BrowserCharm({id: 'foobar'});
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal(
+          'Developers must initialize charms with a well-formed id.');
+    }
+  });
+
+
+  it('must reject missing charm ids at initialization.', function() {
+    var charm;
+    try {
+      charm = new models.Charm();
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal(
+          'Developers must initialize charms with a well-formed id.');
+    }
+
+    try {
+      charm = new models.BrowserCharm();
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal(
+          'Developers must initialize charms with a well-formed id.');
+    }
   });
 
   it('can load options from both "options" and "config"', function() {
-    var options = {foo: 'bar'};
-    var charm = new models.Charm({
+    var options = {foo: 'bar'},
+        charm;
+    charm = new models.Charm({
       id: 'cs:precise/openstack-dashboard-0',
       options: options
     });
@@ -55,7 +136,7 @@
     assert.equal(charm.get('options'), options);
   });
 
-  it('must convert timestamps into time objects', function() {
+  it('must convert timestamps into time objects on Charm', function() {
     var time = 1349797266.032,
         date = new Date(time),
         charm = new models.Charm(
@@ -63,6 +144,13 @@
     charm.get('last_change').created.should.eql(date);
   });
 
+  it('must convert timestamps into time objects on BrowserCharm', function() {
+    var time = 1349797266.032,
+        date = new Date(time),
+        charm = new models.BrowserCharm(
+        { id: 'cs:precise/foo-9', last_change: {created: time / 1000} });
+    charm.get('last_change').created.should.eql(date);
+  });
 });
 
 describe('juju models', function() {
@@ -79,79 +167,6 @@
     window._gaq = [];
   });
 
-  it('must be able to create charm', function() {
-    var charm = new models.Charm(
-        {id: 'cs:~alt-bac/precise/openstack-dashboard-0'});
-    charm.get('scheme').should.equal('cs');
-    charm.get('owner').should.equal('alt-bac');
-    charm.get('series').should.equal('precise');
-    charm.get('package_name').should.equal('openstack-dashboard');
-    charm.get('revision').should.equal(0);
-    charm.get('full_name').should.equal(
-        '~alt-bac/precise/openstack-dashboard');
-    charm.get('charm_path').should.equal(
-        '~alt-bac/precise/openstack-dashboard-0/json');
-  });
-
-  it('must be able to parse real-world charm names', function() {
-    var charm = new models.Charm({id: 'cs:precise/openstack-dashboard-0'});
-    charm.get('full_name').should.equal('precise/openstack-dashboard');
-    charm.get('package_name').should.equal('openstack-dashboard');
-    charm.get('charm_path').should.equal(
-        'charms/precise/openstack-dashboard-0/json');
-    charm.get('scheme').should.equal('cs');
-    var _ = expect(charm.get('owner')).to.not.exist;
-    charm.get('series').should.equal('precise');
-    charm.get('package_name').should.equal('openstack-dashboard');
-    charm.get('revision').should.equal(0);
-  });
-
-  it('must be able to parse individually owned charms', function() {
-    // Note that an earlier version of the parsing code did not handle
-    // hyphens in user names, so this test intentionally includes one.
-    var charm = new models.Charm(
-        {id: 'cs:~marco-ceppi/precise/wordpress-17'});
-    charm.get('full_name').should.equal('~marco-ceppi/precise/wordpress');
-    charm.get('package_name').should.equal('wordpress');
-    charm.get('charm_path').should.equal(
-        '~marco-ceppi/precise/wordpress-17/json');
-    charm.get('revision').should.equal(17);
-  });
-
-  it('must reject bad charm ids.', function() {
-    try {
-      var charm = new models.Charm({id: 'foobar'});
-      assert.fail('Should have thrown an error');
-    } catch (e) {
-      e.should.equal(
-          'Developers must initialize charms with a well-formed id.');
-    }
-  });
-
-  it('must reject missing charm ids at initialization.', function() {
-    try {
-      var charm = new models.Charm();
-      assert.fail('Should have thrown an error');
-    } catch (e) {
-      e.should.equal(
-          'Developers must initialize charms with a well-formed id.');
-    }
-  });
-
-  it('must be able to create charm list', function() {
-    var c1 = new models.Charm(
-        { id: 'cs:precise/mysql-2',
-          description: 'A DB'}),
-        c2 = new models.Charm(
-        { id: 'cs:precise/logger-3',
-          description: 'Log sub'}),
-        clist = new models.CharmList();
-    clist.add([c1, c2]);
-    var names = clist.map(function(c) {return c.get('package_name');});
-    names[0].should.equal('mysql');
-    names[1].should.equal('logger');
-  });
-
   it('service unit list should be able to get units of a given service',
       function() {
         var sl = new models.ServiceList();
@@ -491,7 +506,169 @@
   });
 });
 
-describe('juju charm load', function() {
+// XXX jcsackett August 7 2013 This is a complete duplication of the next test
+// case, but we need to test independently for both models while we're merging
+// them. The Charm load suite below can be removed once the work is done.
+describe('BrowserCharm load', function() {
+  var Y, models, conn, env, app, container, fakeStore, data, juju;
+
+  before(function(done) {
+    Y = YUI(GlobalConfig).use(['juju-models', 'juju-gui', 'datasource-local',
+                               'juju-tests-utils', 'json-stringify',
+                               'juju-charm-store'], function(Y) {
+      models = Y.namespace('juju.models');
+      juju = Y.namespace('juju');
+      done();
+    });
+  });
+
+  beforeEach(function() {
+    conn = new (Y.namespace('juju-tests.utils')).SocketStub();
+    env = juju.newEnvironment({conn: conn});
+    env.connect();
+    conn.open();
+    container = Y.Node.create('<div id="test" class="container"></div>');
+    data = [];
+    fakeStore = new Y.juju.Charmworld2({});
+    fakeStore.set('datasource', {
+      sendRequest: function(params) {
+        params.callback.success({
+          response: {
+            results: [{
+              responseText: data
+            }]
+          }
+        });
+      }
+    });
+  });
+
+  afterEach(function() {
+    container.destroy();
+  });
+
+  it('will throw an exception with non-read sync', function() {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'});
+    try {
+      charm.sync('create');
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal('Only use the "read" action; "create" not supported.');
+    }
+    try {
+      charm.sync('update');
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal('Only use the "read" action; "update" not supported.');
+    }
+    try {
+      charm.sync('delete');
+      assert.fail('Should have thrown an error');
+    } catch (e) {
+      e.should.equal('Only use the "read" action; "delete" not supported.');
+    }
+  });
+
+  it('throws an error if you do not pass get_charm',
+     function() {
+       var charm = new models.BrowserCharm({id: 'local:precise/foo-4'});
+       try {
+         charm.sync('read', {});
+         assert.fail('Should have thrown an error');
+       } catch (e) {
+         e.should.equal(
+         'You must supply a get_charm function.');
+       }
+       try {
+         charm.sync('read', {env: 42});
+         assert.fail('Should have thrown an error');
+       } catch (e) {
+         e.should.equal(
+         'You must supply a get_charm function.');
+       }
+     });
+
+  it('must send request to juju environment for local charms', function() {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'}).load(env);
+    assert(!charm.loaded);
+    conn.last_message().op.should.equal('get_charm');
+  });
+
+  it('must handle success from local charm request', function(done) {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'}).load(
+        env,
+        function(err, response) {
+          assert(!err);
+          charm.get('summary').should.equal('wowza');
+          assert(charm.loaded);
+          done();
+        });
+    var response = conn.last_message();
+    response.result = { summary: 'wowza' };
+    env.dispatch_result(response);
+    // The test in the callback above should run.
+  });
+
+  it('parses the old charm model options location correctly', function(done) {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'}).load(
+        env,
+        function(err, response) {
+          assert(!err);
+          // This checks to make sure the parse mechanism is working properly
+          // for both the old ane new charm browser.
+          assert.equal(charm.get('options').default_log['default'], 'global');
+          done();
+        });
+    var response = conn.last_message();
+    response.result = {
+      config: {
+        options: {
+          default_log: {
+            'default': 'global',
+            description: 'Default log',
+            type: 'string'
+          }}}};
+    env.dispatch_result(response);
+  });
+
+  it('parses the new charm model options location correctly', function(done) {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'}).load(
+        env,
+        function(err, response) {
+          assert(!err);
+          // This checks to make sure the parse mechanism is working properly
+          // for both the old ane new charm browser.
+          assert.equal(charm.get('options').default_log['default'], 'global');
+          done();
+        });
+    var response = conn.last_message();
+    response.result = {
+      options: {
+        default_log: {
+          'default': 'global',
+          description: 'Default log',
+          type: 'string'
+        }}};
+    env.dispatch_result(response);
+  });
+
+  it('must handle failure from local charm request', function(done) {
+    var charm = new models.BrowserCharm({id: 'local:precise/foo-4'}).load(
+        env,
+        function(err, response) {
+          assert(err);
+          assert(response.err);
+          assert(!charm.loaded);
+          done();
+        });
+    var response = conn.last_message();
+    response.err = true;
+    env.dispatch_result(response);
+    // The test in the callback above should run.
+  });
+});
+
+describe('Charm load', function() {
   var Y, models, conn, env, app, container, fakeStore, data, juju;
 
   before(function(done) {