-
Notifications
You must be signed in to change notification settings - Fork 15
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
PP-11681 Replace 'base-client' with 'axios-base-client' in 'ledger.client' #4187
PP-11681 Replace 'base-client' with 'axios-base-client' in 'ledger.client' #4187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really - just some minor comments
}, defaultOptions, options) | ||
return baseClient.get(configuration) | ||
const transactionWithAccountOverride = async function transactionWithAccountOverride (id, options = {}) { | ||
const url = urlJoin(defaultOptions.baseUrl,'/v1/transaction', id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add a check as per line 24?
Check if the baseUrl is the default URL / coming from options object.
await ledgerClient.transactionWithAccountOverride('id', { baseUrl: 'https://example.com' }) | ||
|
||
expect(configureSpy.getCall(0).args[0]).to.equal('http://127.0.0.1:8006/v1/transaction/id?override_account_id_restriction=true') | ||
// expect(configureSpy.getCall(0).args[0]).to.equal('https://example.com/v1/transaction/id?override_account_id_restriction=true') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommented as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work - would be good to get another set of eyes to check one of these PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just need to squash commits.
4ef4230
to
64a4848
Compare
64a4848
to
e961cf6
Compare
- Change ‘ledger.client’ methods to use ‘axios-base-client’. - Refactor call parameters initialisation. - Use ‘async/await’ syntax. - Use baseUrl from options parameter if present otherwise use default envvar.
e961cf6
to
4488478
Compare
https://payments-platform.atlassian.net/browse/PP-11681
WHAT
HOW
All automated tests should pass without the need to be amended.
Existing functionality should remain unaffected.