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 user canceled #517

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

max-critcrew
Copy link
Contributor

See: #516

fixed various issues that caused the payment flow not call the listeners (e.g. when the user canceled or item was unavailable, etc.)

Some of the additionally triggered listeners like developer error might trigger double now, but better double then not receiving the errors. If any library author could double check that'd be awesome.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rror) might come double now, but better double then not receiving the errors
|| responseCode == BillingClient.BillingResponseCode.ITEM_UNAVAILABLE
|| responseCode == BillingClient.BillingResponseCode.DEVELOPER_ERROR
|| responseCode == BillingClient.BillingResponseCode.ERROR
|| responseCode == BillingClient.BillingResponseCode.ITEM_ALREADY_OWNED
Copy link
Member

Choose a reason for hiding this comment

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

BillingClient.BillingResponseCode.ITEM_ALREADY_OWNED should be removed here as it is already handled in the else branch above

@serggl
Copy link
Member

serggl commented Feb 3, 2022

@max-critcrew can you please clean up all the extra changes in the PR? (anything except BillingProcessor changes?)
Also do ming explaining what error codes will 'double'?

@max-critcrew
Copy link
Contributor Author

@serggl Without the extra changes, it wouldn't compile on https://jitpack.io/ - so these are necessary in order to make it work for testing. Do you know any other way how I can make it work?

Absolutely agree about the BillingClient.BillingResponseCode.ITEM_ALREADY_OWNED that was unnecessary, I removed that.

About possible double issues: I am not entirely sure which of the errors that I am catching here and calling reportBillingError for are also catched otherwise. I'd expect that the creators of the plugin also tested some of these and made some of these trigger. I know for sure that the DEVELOPER_ERROR can trigger sometimes. Therefore, I am not certain which errors now might trigger twice since. Since it's not entirely clear which errors come up here and which ones were already catched and reported to the listeners, I chose to better propagate errors like DEVELOPER_ERROR twice than not at all.

@serggl
Copy link
Member

serggl commented Feb 10, 2022

@max-critcrew this project is published via maven central, so these extra changes are not required. Also the licence key in the sample project does not really needs to be changed to compile, right?
The ideal PR should only contain changes related the bug fix, nothing extra. Happy to merge this when you'll clean up this a little

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

2 participants