Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] pass source_currency params properly #1

Merged
merged 2 commits into from
Mar 11, 2015
Merged

Conversation

nerdylocks
Copy link
Contributor

addresses RCNX-84
also, use api.ripple.com as a default

[TEST] update build payment method

also add fixtures

[TASK] add lodash

@@ -7,6 +7,7 @@ describe('Ripple REST Client buildPayment', function() {

before(function () {
client = new Client({
api: 'http://localhost:5990/',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be hitting api.ripple.com

Copy link

Choose a reason for hiding this comment

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

In a test? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you're looking at an older version of the PR. Have fixed that. @wltsmrz

On Wednesday, March 11, 2015, wltsmrz notifications@github.com wrote:

In test/build_payment.js
#1 (comment)
:

@@ -7,6 +7,7 @@ describe('Ripple REST Client buildPayment', function() {

before(function () {
client = new Client({

  •  api: 'http://localhost:5990/',
    

In a test? Why?


Reply to this email directly or view it on GitHub
https://github.com/ripple/ripple-rest-client/pull/1/files#r26258157.

Abiy Seifu | Software Engineer
Ripple Labs, Inc.
abiy@ripple.com | www.ripple.com

Copy link

Choose a reason for hiding this comment

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

What did you fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, removed http://localhost:5990/ so it tests against api.ripple.com.
updated. thanks for catching it @wltsmrz

On Wed, Mar 11, 2015 at 2:42 PM, wltsmrz notifications@github.com wrote:

In test/build_payment.js
#1 (comment)
:

@@ -7,6 +7,7 @@ describe('Ripple REST Client buildPayment', function() {

before(function () {
client = new Client({

  •  api: 'http://localhost:5990/',
    

What did you fix?


Reply to this email directly or view it on GitHub
https://github.com/ripple/ripple-rest-client/pull/1/files#r26258904.

Abiy Seifu | Software Engineer
Ripple Labs, Inc.
abiy@ripple.com | www.ripple.com

Copy link

Choose a reason for hiding this comment

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

Why would you use api.ripple.com in a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're testing agains the api.ripple.com because we want to catch breaks in future ripple-rest releases. In a future pull request, we'll have offline testing to make sure our internal (ripple-rest-client) logics work.

@nerdylocks nerdylocks force-pushed the fix/source_currency branch 2 times, most recently from 3a440f4 to 479bc1f Compare March 11, 2015 20:46
}
sourceCurrenciesParam.source_currencies = sourceCurrenciesString;
sourceCurrenciesString = opts.source_currencies.join(',');
sourceCurrenciesString += !_.isEmpty(opts.from_issuer) ? '%20' + opts.from_issuer : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do encodeURI('+') instead?

@nerdylocks nerdylocks force-pushed the fix/source_currency branch 2 times, most recently from 6d00bca to 2982594 Compare March 11, 2015 21:54
@cdm6a
Copy link
Contributor

cdm6a commented Mar 11, 2015

LGTM.

Abiy Seifu added 2 commits March 11, 2015 15:39
addresses RCNX-84
also, use api.ripple.com as a default

[TEST] update build payment method

add fixtures for buildPayment responses
nerdylocks pushed a commit that referenced this pull request Mar 11, 2015
[FIX] pass source_currency params properly
@nerdylocks nerdylocks merged commit 8fbd782 into master Mar 11, 2015
@nerdylocks nerdylocks deleted the fix/source_currency branch March 11, 2015 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants