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

feat: add an option to disable payment detection in the request client #201

Merged
merged 10 commits into from
Apr 21, 2020

Conversation

vrolland
Copy link
Contributor

No description provided.

@vrolland vrolland requested review from rittme, benjlevesque, lumtis, AdamREQ and yomarion and removed request for rittme and benjlevesque April 17, 2020 13:39
@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage increased (+0.009%) to 90.362% when pulling 6360070 on get-balance-optional into 7b7913b on master.

packages/request-client.js/src/api/request.ts Outdated Show resolved Hide resolved
packages/request-client.js/src/api/request.ts Show resolved Hide resolved
@@ -1461,6 +1461,83 @@ describe('index', () => {
);
sinon.restore();
});

it('can skip the get the balance of a request', async function(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a check that getBalance was not called. We do this to improve performance, if the balance field is null but getBalance was still called, we didn't achieve much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to test expecting putting private paymentNetwork: PaymentTypes.IPaymentNetwork | null = null; to public in the request class.
@rittme 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.

after rethinking it, We return the previous balance to avoid loosing information

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. Can't you add a spy to getBalance and make sure it's not called?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the payment network is nested in the class as private.

Copy link
Contributor

@rittme rittme Apr 21, 2020

Choose a reason for hiding this comment

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

private stuff sucks because of this :/ If we're sure we're not calling getBalance, I'm fine with leaving like this for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you solve this with any ?

packages/request-client.js/src/api/request-network.ts Outdated Show resolved Hide resolved
packages/request-client.js/src/api/request-network.ts Outdated Show resolved Hide resolved
@@ -1461,6 +1461,83 @@ describe('index', () => {
);
sinon.restore();
});

it('can skip the get the balance of a request', async function(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. Can't you add a spy to getBalance and make sure it's not called?

@vrolland vrolland requested a review from rittme April 20, 2020 14:51
@rittme rittme merged commit 035302f into master Apr 21, 2020
@rittme rittme deleted the get-balance-optional branch April 21, 2020 01:48
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

4 participants