search

braintree / braintree-android-google-payment

Google Payment component for our Braintree AndroidSDK
MIT License
3 stars 10 forks source link

Add check in `requestPayment` to avoid NPE #7

Closed Epreuve closed 5 years ago

Epreuve commented 5 years ago

This fix will return an exception if requestPayment is called when Google Pay is not enabled. Ideally this would be caught in isReadyToPay, however it's possible though not advised to jump straight to calling requestPayment and get a Null Pointer Exception if some values are not set in the configuration.

scannillo commented 5 years ago

Can you clarify/outline the issue that triggered this null pointer in the first place?

Where was the null pointer being thrown? Is this the issue someone had about a nil env variable or something else? Just trying to get the full context !

Epreuve commented 5 years ago

Can you clarify/outline the issue that triggered this null pointer in the first place?

Where was the null pointer being thrown? Is this the issue someone had about a nil env variable or something else? Just trying to get the full context !

Sure thing! This aims to fix a use case where someone essentially skips calling isReadyToPay first, and jumps straight to calling requestPayment. The former method does check the config to confirm the merchant is configured for and has enabled Google Pay (and the merchant has properly added Google Play Services).

The latter though, assumes that certain configuration values exist, and will trigger a null pointer exception if the merchant is not properly configured (or Google Play Services is not properly added which results in an Illegal State Exception).

This aims to prevent the latter case and return an exception to the onError listener so the merchant can recover from the error and act accordingly.

However, some issues exist now with the setup of the existing tests since the test env isn't setup to accommodate such a check, so I need to go back and adjust the setup of these tests or figure something out to prevent this new functionality from incorrectly causing these tests to fail.

scannillo commented 5 years ago

Thanks for explaining!

This change and its placement look good to me. Will review again once tests are sorted out 👍

demerino commented 5 years ago

We can't write a test for this?

Epreuve commented 5 years ago

We can't write a test for this?

We can! The issue is that the other existing tests were setup in such a way that they never had to account for this check. So, I have to work out how to adjust the setup for those tests to avoid incorrectly failing (as they're testing for a happy path, and so should assume this check and the exception will not throw/return)