Merge lp:~jcsackett/juju-gui/search-api into lp:juju-gui/experimental

[j.c.sackett]

Adds call to API client for charm search

-Adds a method, search, to CharmworldAPI0, to make the search API call.
-Adds a test for it.

Nothing in this is wired into anything; it's used by a subsequent branch that
is doing the sidebar search view.

https://codereview.appspot.com/8612045/

[j.c.sackett (jcsackett)]

Reviewers: mp+158929_code.launchpad.net,

Message:
Please take a look.

Description:
Adds call to API client for charm search

-Adds a method, search, to CharmworldAPI0, to make the search API call.
-Adds a test for it.

Nothing in this is wired into anything; it's used by a subsequent branch
that
is doing the sidebar search view.

https://code.launchpad.net/~jcsackett/juju-gui/search-api/+merge/158929

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/store/charm.js
   M test/test_charm_store.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: test/test_charm_store.js
=== modified file 'test/test_charm_store.js'
--- test/test_charm_store.js 2013-03-26 20:36:21 +0000
+++ test/test_charm_store.js 2013-04-15 13:54:32 +0000
@@ -281,6 +281,38 @@

      });

+ it('handles searching correctly', function(done) {
+ var hostname = 'http://localhost',
+ api = new Y.juju.Charmworld0({
+ apiHost: hostname
+ }),
+ data = [];
+ 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}),
+ url;
+ datasource.realSendRequest = datasource.sendRequest;
+ datasource.sendRequest = function(params) {
+ url = params.request;
+ datasource.realSendRequest(params);
+ };
+
+ api.set('datasource', datasource);
+ var result = api.search('foo', {
+ success: function(data) {
+ assert.equal('charms?text=foo', url);
+ assert.equal('foo', data.name);
+ done();
+ },
+ failure: function(data, request) {
+ }
+ }, this);
+ });
    });

  })();

Index: app/store/charm.js
=== modified file 'app/store/charm.js'
--- app/store/charm.js 2013-04-08 21:03:12 +0000
+++ app/store/charm.js 2013-04-15 14:02:02 +0000
@@ -202,6 +202,21 @@
      },

      /**
+ * Api call to search charms
+ *
+ * @method search
+ * @param {String} text the search text.
+ */
+ search: function(text, 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, {text: text});
+ },
+
+ /**
       * Fetch the contents of a charm's file.
       *
       * @method file

[Jeff Pihach (hatch)]

LGTM with trivial and cleanup in the test.

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

https://codereview.appspot.com/8612045/diff/1/app/store/charm.js#newcode208
app/store/charm.js:208: * @param {String} text the search text.
Missing a couple params.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode286
test/test_charm_store.js:286: api = new Y.juju.Charmworld0({
should be destroyed?

https://codereview.appspot.com/8612045/

[Richard Harding (rharding)]

LGTM with a couple of notes on naming/var setup.

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

https://codereview.appspot.com/8612045/diff/1/app/store/charm.js#newcode208
app/store/charm.js:208: * @param {String} text the search text.
can we standardize on term for the search text? We'll use that in the
query params for the search and it'd be cool to keep it in sync.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode286
test/test_charm_store.js:286: api = new Y.juju.Charmworld0({
multi-line var should be on their own and not grouped. You can move the
url up here though to declare it in the function scope.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode299
test/test_charm_store.js:299: datasource.realSendRequest =
datasource.sendRequest;
I think you can just set this datasource on the Charmworld0 instance and
it'll use it.

https://codereview.appspot.com/8612045/

[j.c.sackett (jcsackett)]

Replies here, and changes coming.

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

https://codereview.appspot.com/8612045/diff/1/app/store/charm.js#newcode208
app/store/charm.js:208: * @param {String} text the search text.
On 2013/04/15 14:24:36, rharding wrote:
> can we standardize on term for the search text? We'll use that in the
query
> params for the search and it'd be cool to keep it in sync.

Right now we're actually using "text", unless I misunderstand you're
suggestion.

http://staging.jujucharms.com/api/0/charms?text=apache2

https://codereview.appspot.com/8612045/diff/1/app/store/charm.js#newcode208
app/store/charm.js:208: * @param {String} text the search text.
On 2013/04/15 14:13:56, jeff.pihach wrote:
> Missing a couple params.

Fixed.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js
File test/test_charm_store.js (right):

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode286
test/test_charm_store.js:286: api = new Y.juju.Charmworld0({
On 2013/04/15 14:13:56, jeff.pihach wrote:
> should be destroyed?

There are other tests creating 'api' and not destroying it. I have
however added destroy for api in this test, as it passes with it. We may
want to clean up the other tests, but I think that's out of scope for
this, since it's a bit more than a driveby fix.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode286
test/test_charm_store.js:286: api = new Y.juju.Charmworld0({
On 2013/04/15 14:24:36, rharding wrote:
> multi-line var should be on their own and not grouped. You can move
the url up
> here though to declare it in the function scope.

Done.

https://codereview.appspot.com/8612045/diff/1/test/test_charm_store.js#newcode299
test/test_charm_store.js:299: datasource.realSendRequest =
datasource.sendRequest;
On 2013/04/15 14:24:36, rharding wrote:
> I think you can just set this datasource on the Charmworld0 instance
and it'll
> use it.

Not sure I follow what you mean; I do set it as the datasource on the
instance a few lines down; do you mean reorder declaration and create
the instance after I've created this, and set it in the declaration?

https://codereview.appspot.com/8612045/

[lp:~jcsackett/juju-gui/search-api]

544. By j.c.sackett on 2013-04-15

Changes from review.

545. By j.c.sackett on 2013-04-15

Shut up, lint.

[j.c.sackett (jcsackett)]

*** Submitted:

Adds call to API client for charm search

-Adds a method, search, to CharmworldAPI0, to make the search API call.
-Adds a test for it.

Nothing in this is wired into anything; it's used by a subsequent branch
that
is doing the sidebar search view.

R=jeff.pihach, rharding
CC=
https://codereview.appspot.com/8612045

https://codereview.appspot.com/8612045/

=== modified file 'app/store/charm.js'
--- app/store/charm.js	2013-04-08 21:03:12 +0000
+++ app/store/charm.js	2013-04-15 15:50:39 +0000
@@ -202,6 +202,23 @@
     },
 
     /**
+     * Api call to search charms
+     *
+     * @method search
+     * @param {String} text the search text.
+     * @param {Object} callbacks the success/failure callbacks to use.
+     * @param {Object} bindScope the scope of *this* in the callbacks.
+     */
+    search: function(text, 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, {text: text});
+    },
+
+    /**
      * Fetch the contents of a charm's file.
      *
      * @method file

=== modified file 'test/test_charm_store.js'
--- test/test_charm_store.js	2013-03-26 20:36:21 +0000
+++ test/test_charm_store.js	2013-04-15 15:50:39 +0000
@@ -281,6 +281,39 @@
 
     });
 
+    it('handles searching correctly', function(done) {
+      var hostname = 'http://localhost',
+          data = [],
+          url;
+      var api = new Y.juju.Charmworld0({
+        apiHost: hostname
+      });
+      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);
+      var result = api.search('foo', {
+        success: function(data) {
+          assert.equal('charms?text=foo', url);
+          assert.equal('foo', data.name);
+          done();
+        },
+        failure: function(data, request) {
+        }
+      }, this);
+      api.destroy();
+    });
   });
 
 })();