Merge lp:~benji/juju-gui/charmworld-api-3 into lp:juju-gui/experimental

[Benji York]

Add charmworld v3 API support.

The v2 API was changed to be a subclass of v3 so that removing v2 when it is
retired will be easy. There is a feature flag to enable the v3 API.

https://codereview.appspot.com/13368056/

[Richard Harding (rharding)]

LGTM with the config revert

#9 - revert config change

review: Approve

[Benji York (benji)]

*** Submitted:

More/tweaked tests for the charmworld v2 API

The code was also tweaked in directions suggested by the new/refactored tests.

[Gary Poster (gary)]

Hey. I *think* this LGTM but would like some clarifications. This
would have been a great branch for some pre-review comments, at least
for me.

https://codereview.appspot.com/13368056/diff/1/app/store/charm.js
File app/store/charm.js (left):

https://codereview.appspot.com/13368056/diff/1/app/store/charm.js#oldcode114
app/store/charm.js:114: result = result + '-1';
Don't we still want this for APIv2?

https://codereview.appspot.com/13368056/diff/1/app/store/charm.js#oldcode353
app/store/charm.js:353: @method filepath
why don't we need this now? was it never used? I don't see it in your
APIv2 so...I'm confused.

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

https://codereview.appspot.com/13368056/diff/1/app/store/charm.js#newcode546
app/store/charm.js:546: * Charmworld API version 2 interface.
I'm assuming this has no changes?

https://codereview.appspot.com/13368056/

[Gary Poster (gary)]

Cool, thanks for the explanations. It would have been nice to also see
the new tests, but anyway, LGTM, fly away. :-)

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

https://codereview.appspot.com/13368056/diff/1/app/store/charm.js#newcode549
app/store/charm.js:549: * @extends {Base}
My main comment, in line with your reply above, is that explaining the
situation would be nice. Beyond that, Maybe @extends is supposed to say
"{APIv3}"? <shrug>

https://codereview.appspot.com/13368056/

=== modified file 'app/app.js'
--- app/app.js	2013-09-13 19:52:55 +0000
+++ app/app.js	2013-09-17 19:00:33 +0000
@@ -1379,7 +1379,11 @@
           } else {
             cfg.apiHost = window.juju_config.charmworldURL;
           }
-          return new Y.juju.charmworld.APIv2(cfg);
+          if (window.flags.charmworldv3) {
+            return new Y.juju.charmworld.APIv3(cfg);
+          } else {
+            return new Y.juju.charmworld.APIv2(cfg);
+          }
         }
       },
 

=== modified file 'app/store/charm.js'
--- app/store/charm.js	2013-09-17 17:56:07 +0000
+++ app/store/charm.js	2013-09-17 19:00:33 +0000
@@ -109,10 +109,6 @@
         }
         result = defaultSeries + '/' + result;
       }
-      if (/\-(\d+|HEAD)/.exec(result) === null) {
-        // Add in a revision placeholder
-        result = result + '-1';
-      }
       return result;
     }
 
@@ -122,14 +118,14 @@
   });
 
   /**
-   * Api helper for the charmworld API v2.
+   * Charmworld API version 3 interface.
    *
-   * @class APIv2
+   * @class APIv3
    * @extends {Base}
    *
    */
-  ns.APIv2 = Y.Base.create('APIv2', Y.Base, [], {
-    _apiRoot: 'api/2',
+  ns.APIv3 = Y.Base.create('APIv3', Y.Base, [], {
+    _apiRoot: 'api/3',
 
     /**
       * Send the actual request and handle response from the api.
@@ -159,7 +155,7 @@
      * @param {Object} bindScope the scope of *this* in the callbacks.
      */
     autocomplete: function(filters, callbacks, bindScope) {
-      var endpoint = 'charms';
+      var endpoint = 'search';
       // Force that this is an autocomplete call to perform matching on the
       // start of names vs a fulltext search.
       filters.autocomplete = 'true';
@@ -263,8 +259,8 @@
       @return {Promise} A promise for a newer charm ID or undefined.
     */
     promiseUpgradeAvailability: function(charm, cache) {
-      // Get the charm's store ID, then replace the version number
-      // with '-HEAD' to retrieve the latest version of the charm.
+      // Get the charm's store ID, then remove the version number to retrieve
+      // the latest version of the charm.
       var storeId, revision;
       if (charm instanceof Y.Model) {
         storeId = charm.get('storeId');
@@ -273,7 +269,7 @@
         storeId = charm.url;
         revision = parseInt(charm.revision, 10);
       }
-      storeId = storeId.replace(/-\d+$/, '-HEAD');
+      storeId = storeId.replace(/-\d+$/, '');
       // XXX By using a cache we hide charm versions that have become available
       // since we last requested the most recent version.
       return this.promiseCharm(storeId, cache)
@@ -296,7 +292,7 @@
      * @param {Object} bindScope the scope of *this* in the callbacks.
      */
     search: function(filters, callbacks, bindScope) {
-      var endpoint = 'charms';
+      var endpoint = 'search';
       if (bindScope) {
         callbacks.success = Y.bind(callbacks.success, bindScope);
         callbacks.failure = Y.bind(callbacks.failure, bindScope);
@@ -346,25 +342,6 @@
     },
 
     /**
-      Generate the API path to a file.
-      This is useful when generating links and references in HTML to a file
-      but not actually fetching the file itself.
-
-      @method filepath
-      @param {String} charmID The id of the charm to grab the file from.
-      @param {String} filename The name of the file to generate a path to.
-
-     */
-    filepath: function(charmID, filename) {
-      return this.get('apiHost') + [
-        this._apiRoot,
-        'charm',
-        charmID,
-        'file',
-        filename].join('/');
-    },
-
-    /**
       Generate the API path to a charm icon.
       This is useful when generating links and references in HTML to the
       charm's icon and is constructing the correct icon based on reviewed
@@ -390,11 +367,11 @@
         // The following regular expression removes everything up to the
         // colon portion of the quote and leaves behind a charm ID.
         charmID = charmID.replace(/^[^:]+:/, '');
-
         return this.get('apiHost') + [
           this._apiRoot,
           'charm',
           charmID,
+          'file',
           'icon.svg'].join('/');
       }
     },
@@ -487,7 +464,7 @@
         callbacks.failure = Y.bind(callbacks.failure, bindScope);
       }
 
-      this._makeRequest('charms/interesting', callbacks);
+      this._makeRequest('search/interesting', callbacks);
     },
 
     /**
@@ -565,6 +542,191 @@
     }
   });
 
+  /**
+   * Charmworld API version 2 interface.
+   *
+   * @class APIv2
+   * @extends {Base}
+   *
+   */
+  ns.APIv2 = Y.Base.create('APIv2', ns.APIv3, [], {
+    _apiRoot: 'api/2',
+
+    /**
+     * Api call to fetch autocomplete suggestions based on the current term.
+     *
+     * @method autocomplete
+     * @param {Object} query the filters data object for search.
+     * @param {Object} filters the filters data object for search.
+     * @param {Object} callbacks the success/failure callbacks to use.
+     * @param {Object} bindScope the scope of *this* in the callbacks.
+     */
+    autocomplete: function(filters, callbacks, bindScope) {
+      var endpoint = 'charms';
+      // Force that this is an autocomplete call to perform matching on the
+      // start of names vs a fulltext search.
+      filters.autocomplete = 'true';
+      filters.limit = 5;
+      if (bindScope) {
+        callbacks.success = Y.bind(callbacks.success, bindScope);
+        callbacks.failure = Y.bind(callbacks.failure, bindScope);
+      }
+      this._makeRequest(endpoint, callbacks, filters);
+    },
+
+    /**
+     * Api call to fetch a charm's details, with an optional local cache.
+     *
+     * @method charmWithCache
+     * @param {String} charmID The charm to fetch This is the fully qualified
+     *   charm name in the format scheme:series/charm-revision.
+     * @param {Object} callbacks The success/failure callbacks to use.
+     * @param {Object} bindScope The scope of "this" in the callbacks.
+     * @param {ModelList} [cache] a local cache of browser charms.
+     * @param {String} [defaultSeries='precise'] The series to use if none is
+     *  specified in the charm ID.
+     */
+    charm: function(charmID, callbacks, bindScope, cache, defaultSeries) {
+      if (bindScope) {
+        callbacks.success = Y.bind(callbacks.success, bindScope);
+      }
+      if (cache) {
+        var charm = cache.getById(charmID);
+        if (charm) {
+          // If the charm was found in the cache, then we can declare success
+          // without ever making a request to charmworld.
+          Y.soon(function() {
+            // Since there wasn't really a request, there is no data, so we
+            // pass an empty object as the "data" parameter.
+            callbacks.success({}, charm);
+          });
+          return;
+        } else {
+          var successCB = callbacks.success;
+          callbacks.success = function(data) {
+            var charm = new Y.juju.models.Charm(data.charm);
+            if (data.metadata) {
+              charm.set('metadata', data.metadata);
+            }
+            cache.add(charm);
+            successCB(data, charm);
+          };
+        }
+      }
+      charmID = this.apiHelper.normalizeCharmId(charmID, defaultSeries);
+      // If the charm ID does not have a revision number (or "HEAD"), add one.
+      if (/\-(\d+|HEAD)/.exec(charmID) === null) {
+        // Add in a revision placeholder.  Any value will do, v2 of the
+        // charmworld API ignores revision numbers.
+        charmID = charmID + '-1';
+      }
+      this._charm(charmID, callbacks, bindScope);
+    },
+
+    /**
+      Promises to return the latest charm ID for a given charm if a newer one
+      exists; this also caches the newer charm if one is available.
+
+      @method promiseUpgradeAvailability
+      @param {Charm} charm An existing charm potentially in need of an upgrade.
+      @param {ModelList} cache A local cache of browser charms.
+      @return {Promise} A promise for a newer charm ID or undefined.
+    */
+    promiseUpgradeAvailability: function(charm, cache) {
+      // Get the charm's store ID, then replace the version number
+      // with '-HEAD' to retrieve the latest version of the charm.
+      var storeId, revision;
+      if (charm instanceof Y.Model) {
+        storeId = charm.get('storeId');
+        revision = parseInt(charm.get('revision'), 10);
+      } else {
+        storeId = charm.url;
+        revision = parseInt(charm.revision, 10);
+      }
+      storeId = storeId.replace(/-\d+$/, '-HEAD');
+      // XXX By using a cache we hide charm versions that have become available
+      // since we last requested the most recent version.
+      return this.promiseCharm(storeId, cache)
+        .then(function(latest) {
+            var latestVersion = parseInt(latest.charm.id.split('-').pop(), 10);
+            if (latestVersion > revision) {
+              return latest.charm.id;
+            }
+          }, function(e) {
+            throw e;
+          });
+    },
+
+    /**
+     * Api call to search charms
+     *
+     * @method search
+     * @param {Object} filters the filters data object for search.
+     * @param {Object} callbacks the success/failure callbacks to use.
+     * @param {Object} bindScope the scope of *this* in the callbacks.
+     */
+    search: function(filters, callbacks, bindScope) {
+      var endpoint = 'charms';
+      if (bindScope) {
+        callbacks.success = Y.bind(callbacks.success, bindScope);
+        callbacks.failure = Y.bind(callbacks.failure, bindScope);
+      }
+      this._makeRequest(endpoint, callbacks, filters);
+    },
+
+    /**
+      Generate the API path to a charm icon.
+      This is useful when generating links and references in HTML to the
+      charm's icon and is constructing the correct icon based on reviewed
+      status and categories on the charm.
+
+      @method iconpath
+      @param {String} charmID The id of the charm to grab the icon for.
+      @return {String} The URL of the charm's icon.
+     */
+    iconpath: function(charmID) {
+      // If this is a local charm, then we need use a hard coded path to the
+      // default icon since we cannot fetch its category data or its own
+      // icon.
+      // XXX: #1202703 - this is a short term fix for the bug. Need longer
+      // term solution.
+      if (charmID.indexOf('local:') === 0) {
+        return this.get('apiHost') +
+            'static/img/charm_160.svg';
+
+      } else {
+        // Get the charm ID from the service.  In some cases, this will be
+        // the charm URL with a protocol, which will need to be removed.
+        // The following regular expression removes everything up to the
+        // colon portion of the quote and leaves behind a charm ID.
+        charmID = charmID.replace(/^[^:]+:/, '');
+        return this.get('apiHost') + [
+          this._apiRoot,
+          'charm',
+          charmID,
+          'icon.svg'].join('/');
+      }
+    },
+
+    /**
+     * Fetch the interesting landing content from the charmworld api.
+     *
+     * @method interesting
+     * @return {Object} data loaded from the api call.
+     *
+     */
+    interesting: function(callbacks, bindScope) {
+      if (bindScope) {
+        callbacks.success = Y.bind(callbacks.success, bindScope);
+        callbacks.failure = Y.bind(callbacks.failure, bindScope);
+      }
+
+      this._makeRequest('charms/interesting', callbacks);
+    }
+  }, {
+    ATTRS: {}
+  });
+
 }, '0.1.0', {
   requires: [
     'datasource-io',

=== modified file 'test/test_charm_store.js'
--- test/test_charm_store.js	2013-09-17 17:56:07 +0000
+++ test/test_charm_store.js	2013-09-17 19:00:33 +0000
@@ -20,8 +20,244 @@
 
 (function() {
 
+  describe('Charmworld API v3 interface', function() {
+    var Y, models, conn, data, juju, utils, charmworld, hostname, api;
+
+
+    before(function(done) {
+      Y = YUI(GlobalConfig).use(
+          'datasource-local', 'json-stringify', 'juju-charm-store',
+          'datasource-io', 'io', 'array-extras', 'juju-charm-models',
+          'juju-tests-utils',
+          function(Y) {
+            juju = Y.namespace('juju');
+            charmworld = Y.namespace('juju.charmworld');
+            models = Y.namespace('juju.models');
+            utils = Y.namespace('juju-tests').utils;
+            done();
+          });
+    });
+
+    beforeEach(function() {
+      hostname = 'http://charmworld.example/';
+      api = new charmworld.APIv3({apiHost: hostname});
+    });
+
+    it('constructs the api url correctly based on apiHost', function() {
+      var ds = api.get('datasource');
+
+      ds.get('source').should.eql(hostname + 'api/3/');
+
+      // And it should work without a trailing / as well.
+      hostname = hostname.slice(0, -1);
+      api = new charmworld.APIv3({apiHost: hostname});
+      ds = api.get('datasource');
+      ds.get('source').should.eql(hostname + '/api/3/');
+    });
+
+    it('handles loading interesting content correctly', function(done) {
+      var data = [];
+
+      data.push({responseText: Y.JSON.stringify({summary: 'wowza'})});
+      api.set('datasource', new Y.DataSource.Local({source: data}));
+
+      api.interesting({
+        success: function(data) {
+          data.summary.should.equal('wowza');
+          done();
+        },
+        failure: function(data, request) {
+        }
+      }, this);
+
+    });
+
+    it('handles searching correctly', function(done) {
+      var data = [],
+          url;
+      data.push({responseText: Y.JSON.stringify({name: 'foo'})});
+      // Create a monkeypatched datasource we can use to track the generated
+      // apiEndpoint
+      var datasource = new Y.DataSource.Local({source: data});
+      datasource.realSendRequest = datasource.sendRequest;
+      datasource.sendRequest = function(params) {
+        url = params.request;
+        datasource.realSendRequest(params);
+      };
+
+      api.set('datasource', datasource);
+      api.search({text: 'foo'}, {
+        success: function(data) {
+          assert.equal('search?text=foo', url);
+          assert.equal('foo', data.name);
+          done();
+        },
+        failure: function(data, request) {
+        }
+      }, this);
+      api.destroy();
+    });
+
+    it('constructs cateogry icon paths correctly', function() {
+      var iconPath = api.buildCategoryIconPath('app-servers');
+      assert.equal(
+          iconPath,
+          hostname + 'static/img/category-app-servers-bw.svg');
+    });
+
+    it('makes charm requests to correct URL', function(done) {
+      api._makeRequest = function(endpoint, callbacks, filters) {
+        assert.equal(endpoint, 'charm/CHARM-ID');
+        done();
+      };
+
+      api._charm('CHARM-ID');
+    });
+
+    it('can use a cache to avoid requesting charm data', function(done) {
+      var should_not_happen = function() {
+        assert.isTrue(false, 'Oops, this should not have been called.');
+        done();
+      };
+      var CACHED_CHARM = 'CACHED-CHARM';
+
+      var callbacks = {
+        success: function(data, charm) {
+          assert.equal(charm, CACHED_CHARM);
+          done();
+        },
+        failure: should_not_happen
+      };
+
+      api._makeRequest = should_not_happen;
+
+      var cache = {
+        getById: function(charmID) {
+          return CACHED_CHARM;
+        }};
+
+      api.charm('CHARM-ID', callbacks, false, cache);
+
+    });
+
+    it('will make a request on a cache miss', function(done) {
+      var should_not_happen = function() {
+        assert.isTrue(false, 'Oops, this should not have been called.');
+        done();
+      };
+      var CACHED_CHARM = 'CACHED-CHARM';
+
+      var callbacks = {
+        success: function(data, charm) {
+          assert.equal(charm, CACHED_CHARM);
+          done();
+        },
+        failure: should_not_happen
+      };
+
+      api._makeRequest = function() {
+        // If this was called, then the test is successful.
+        done();
+      };
+
+      var cache = {
+        getById: function(charmID) {
+          return null;
+        }};
+
+      api.charm('CHARM-ID', callbacks, false, cache);
+
+    });
+
+    it('makes autocomplete requests to correct URL', function(done) {
+      var noop = function() {};
+
+      api._makeRequest = function(endpoint, callbacks, filters) {
+        assert.equal(endpoint, 'search');
+        done();
+      };
+
+      api.autocomplete({text: 'mys'}, {'success': noop});
+    });
+
+    it('makes autocomplete requests with right query flag', function(done) {
+      var noop = function() {};
+
+      api._makeRequest = function(endpoint, callbacks, filters) {
+        assert.equal(filters.autocomplete, 'true');
+        done();
+      };
+
+      api.autocomplete({text: 'mys'}, {'success': noop});
+    });
+
+    it('constructs iconpaths correctly', function() {
+      var iconPath = api.iconpath('precise/mysql-1');
+      assert.equal(
+          iconPath,
+          hostname + 'api/3/charm/precise/mysql-1/file/icon.svg');
+    });
+
+    it('constructs an icon path for local charms', function() {
+      var iconPath = api.iconpath('local:precise/mysql-1');
+      assert.equal(iconPath, hostname + 'static/img/charm_160.svg');
+    });
+
+    it('removes cs: from the icon path when necessary', function() {
+      var iconPath = api.iconpath('cs:precise/mysql-1');
+      assert.equal(
+          iconPath,
+          hostname + 'api/3/charm/precise/mysql-1/file/icon.svg');
+    });
+
+    it('can fetch a charm via a promise', function(done) {
+      // The "promiseCharm" method is just a promise-wrapped version of the
+      // "charm" method.
+      var DATA = 'DATA';
+      var CHARM = 'CHARM';
+      api.charm = function(charmID, callbacks) {
+        callbacks.success(DATA, CHARM);
+      };
+      api.promiseCharm('CHARM-ID', null, 'precise')
+        .then(function(data) {
+            assert.equal(data, DATA);
+            done();
+          });
+    });
+
+    it('finds upgrades for charms - upgrade available', function(done) {
+      var store = utils.makeFakeStore();
+      var charm = new models.Charm({url: 'cs:precise/wordpress-10'});
+      store.promiseUpgradeAvailability(charm)
+        .then(function(upgrade) {
+            assert.equal(upgrade, 'precise/wordpress-15');
+            done();
+          }, function(error) {
+            assert.isTrue(false, 'We should not get here.');
+            done();
+          });
+    });
+
+    it('finds upgrades for charms - no upgrade available', function(done) {
+      var store = utils.makeFakeStore();
+      var charm = new models.Charm({url: 'cs:precise/wordpress-15'});
+      store.promiseUpgradeAvailability(charm)
+        .then(function(upgrade) {
+            assert.isUndefined(upgrade);
+            done();
+          }, function(error) {
+            assert.isTrue(false, 'We should not get here');
+            done();
+          });
+    });
+
+  });
+
+  // The tests below are based on copies of the v3 tests above so that removing
+  // support for the v2 charmworld API will be easy.  However, it means that
+  // any edits made to these tests may need to be made to the v3 tests above.
   describe('Charmworld API v2 interface', function() {
-    var Y, models, conn, env, app, container, data, juju, utils, charmworld,
+    var Y, models, conn, data, juju, utils, charmworld,
         hostname, api;
 
 
@@ -99,13 +335,6 @@
       api.destroy();
     });
 
-    it('constructs filepaths correctly', function() {
-      var iconPath = api.filepath('precise/mysql-1', 'icon.svg');
-      assert.equal(
-          iconPath,
-          hostname + 'api/2/charm/precise/mysql-1/file/icon.svg');
-    });
-
     it('constructs cateogry icon paths correctly', function() {
       var iconPath = api.buildCategoryIconPath('app-servers');
       assert.equal(
@@ -258,8 +487,7 @@
   });
 
   describe('Charmworld API Helper', function() {
-    var Y, models, conn, env, app, container, data, juju, utils, charmworld,
-        hostname;
+    var Y, models, conn, data, juju, utils, charmworld, hostname;
 
 
     before(function(done) {
@@ -282,20 +510,60 @@
 
     it('can normalize charm names for lookup', function() {
       var apiHelper = new charmworld.ApiHelper({});
+      // If the charm ID does not include a series, the given default seriese
+      // is used to fill our the charm ID.
       assert.equal(apiHelper.normalizeCharmId('wordpress', 'precise'),
-          'precise/wordpress-1');
-      assert.equal(apiHelper.normalizeCharmId('precise/wordpress', 'precise'),
-          'precise/wordpress-1');
+          'precise/wordpress');
+      // If no default series is given, "precise" is used.
+      assert.equal(apiHelper.normalizeCharmId('wordpress'),
+          'precise/wordpress');
+      // If a series is provided, the default serise is ignored.
+      assert.equal(apiHelper.normalizeCharmId('quantal/wordpress', 'precise'),
+          'quantal/wordpress');
+      // A charm ID with series and name but no revision is unchanged.
       assert.equal(apiHelper.normalizeCharmId('precise/wordpress'),
-          'precise/wordpress-1');
+          'precise/wordpress');
+      // A charm ID with series, name, and revision is unchanged.
       assert.equal(apiHelper.normalizeCharmId('precise/wordpress-10'),
           'precise/wordpress-10');
+      // A leading charm store scheme identifier will be stripped.
       assert.equal(apiHelper.normalizeCharmId('cs:precise/wordpress-10'),
           'precise/wordpress-10');
-      assert.equal(apiHelper.normalizeCharmId('precise/wordpress-HEAD'),
-          'precise/wordpress-HEAD');
-      assert.equal(apiHelper.normalizeCharmId('cs:precise/wordpress-HEAD'),
-          'precise/wordpress-HEAD');
+    });
+
+  });
+
+  describe('Charmworld API feature flag support', function() {
+    var Y, models, conn, juju, app;
+
+
+    before(function(done) {
+      Y = YUI(GlobalConfig).use(
+          'juju-gui',
+          'datasource-local', 'json-stringify', 'juju-charm-store',
+          'datasource-io', 'io', 'array-extras', 'juju-charm-models',
+          'juju-tests-utils',
+          function(Y) {
+            juju = Y.namespace('juju');
+            done();
+          });
+    });
+
+    afterEach(function() {
+      app.destroy();
+      window.flags = {};
+    });
+
+    it('enables the charmworld v2 API if not set', function() {
+      assert.deepEqual(window.flags, {});
+      app = new Y.juju.App({});
+      assert.equal(app.get('store').name, 'APIv2');
+    });
+
+    it('enables the charmworld v3 API if set', function() {
+      window.flags.charmworldv3 = true;
+      app = new Y.juju.App({});
+      assert.equal(app.get('store').name, 'APIv3');
     });
 
   });