Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Convert the bundle details page to use the charmstore apiv4 endpoint #676

Merged
merged 6 commits into from Dec 12, 2014

Conversation

hatched
Copy link
Contributor

@hatched hatched commented Dec 11, 2014

The bundles details page now makes a request to apiv4 for its information when loading.

@hatched
Copy link
Contributor Author

hatched commented Dec 11, 2014

To QA

Because this makes the bundle details page request from apiv4 any id's formatted in the apiv3 style will fail. This is only an issue if the user clicks one of the bundles on the 'interesting' results. I'm going to change the interesting/featured results over to the apiv4 endpoint in the next branch so this issue won't persist long.

To properly test these changes perform a search (searches use the apiv4 endpoint) then click on one of the bundle token results to load the bundle details page. I was using 'mongodb' as the search query during development.

@jujugui
Copy link
Contributor

jujugui commented Dec 11, 2014

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/juju-gui/2307/
Test PASSed.

@jujugui
Copy link
Contributor

jujugui commented Dec 11, 2014

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/juju-gui/2308/
Test PASSed.

@@ -90,11 +90,16 @@ YUI.add('charmstore-api', function(Y) {
@param {String} endpoint The endpoint to call at the charmstore.
@param {Object} query The query parameters that are required for the
request.
@param {Boolean} extension Any extension to add to the endpoint
then /meta/any needs to be appended to the end of the endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't make a whole lot of sense to me, looks like maybe two sentences got spliced together? /meta/any doesn't appear anywhere in the code below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, extension may not be the best name, as it makes me think of a file extension, when it looks like '/archive/filename.ext' is what's generally being passed in here. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this description must have been a copy paste fail or something. The 'extension' gets added to the endpoint so you can pass in and '/meta/any' or '/archive' and it'll get added between the filters and the endpoint. I'll fix this up

@makyo
Copy link
Contributor

makyo commented Dec 11, 2014

Thanks @hatched I think this looks good. I QAed in an incognito window because chrome still thinks api.jujucharms.com is evil, but things looked good as I played around with the browser and looked at bundles, much faster. I only had one small comment, up to you. 👍 from me, though I'd like you to get at least one more review, given how big this branch is.

// This technique will create a version with a capitalized key so we
// need to delete it from the host object. To protect against keys
// which are already lower case then we test to make sure we don't
// delte those.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"don't delete those"

@kadams54
Copy link
Contributor

Nice work @hatched, looks like a pretty wide-reaching change. 👍 from me with the exception of the tiny typo noted above :-)

@hatched
Copy link
Contributor Author

hatched commented Dec 12, 2014

Thanks a lot for the reviews and qa's! I'm going to ship this branch as-is then fix these typos in the follow-up as I've already branches from this branch. :shipit:

@jujugui
Copy link
Contributor

jujugui commented Dec 12, 2014

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge

jujugui added a commit that referenced this pull request Dec 12, 2014
Convert the bundle details page to use the charmstore apiv4 endpoint

The bundles details page now makes a request to apiv4 for its information when loading.
@jujugui jujugui merged commit 05c6b26 into juju:develop Dec 12, 2014
@hatched hatched deleted the bundle-details-apiv4 branch December 16, 2014 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants