-
Notifications
You must be signed in to change notification settings - Fork 49
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
Upgrade in app purchase #1017
Upgrade in app purchase #1017
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.
Overall lgtm!
"if(!window.CdvPurchase || !window.CdvPurchase.store) return;", | ||
"const store = window.CdvPurchase.store;", | ||
"", | ||
"const eventName = eventsFunctionContext.getArgument(\"event\");", | ||
"const productId = eventsFunctionContext.getArgument(\"id\");", | ||
"", | ||
"const transactionCallback = (transaction) => {", | ||
" const products = transaction.products;", | ||
" // If the product being watched is in the list of products in the transaction, save into a variable.", | ||
" if (products.find(transactionProduct => transactionProduct.id === productId)) {", | ||
" const variable = runtimeScene.getVariables().get(eventsFunctionContext.getArgument(\"variableName\"));", | ||
" variable.setBoolean(true);", | ||
" }", | ||
"}", | ||
"", | ||
"store.when().approved((transaction) => {", | ||
" // If we're watching this event, call the callback.", | ||
" if (eventName === 'approved') {", | ||
" transactionCallback(transaction);", | ||
" }", | ||
" // Automatically verify the transaction on approved.", | ||
" transaction.verify();", | ||
"})", | ||
"", | ||
"// Automatically finish a receipt when verified.", | ||
"store.when().verified(receipt => receipt.finish())", | ||
"", | ||
"store.when().finished((transaction) => {", | ||
" // If we're watching this event, call the callback.", | ||
" if (eventName === 'finished') {", | ||
" transactionCallback(transaction);", | ||
" }", | ||
" // Nothing left to do when finished.", | ||
"})", |
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.
I have received multiple reports of the extension being broken & orders going unfulfilled after this update.
Taking a look now, this looks extremely wrong to me.
-
This adds a global event handlers for every event type every time this action is called? This is completely unnecessary, harmful to memory and performance, and will almost certainly lead to multiple calls of
.verify()
/.finish()
, which is not valid and likely will cause unintended behavior! -
This forces a forbidden behavior upon users: once this action has been called, and the purchase of the product is started, it will automatically do the steps to finalize the purchase. However, it is absolutely not how it is supposed to be done! Not letting the creator handle verification themselves is arguably not that big of a deal, but we cannot mark the purchases as finalized automatically. A purchase may only ever be marked as finished once the game code has successfully delivered the product that was bought, at this point we know for sure the item has not been delivered yet. Heck, this code will mark as finished ANY product that was bought, even ones for which the creator has not even put up this event listener for!
-
It is a very error prone implementation. If this action does not get called, a purchase of a product will never be processed since those event handlers will never have been set. The action is inconspicuous, doesn't seem and doesn't say in any way that it is crucial for the extension to function!
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.
Thanks for the feedback @arthuro555
I agree the implementation looks incomplete and was done in haste, to prevent creators from being stuck in publishing their app on the stores because of the Google Play deadline.
The tests I had done were fine on the example, but it may have been not enough.
I feel bad about having published this update now.
As you know well how this works, would you have time to improve it and make the extension less error-prone?
tested with example: GDevelopApp/GDevelop-examples#574
Mainly followed https://github.com/j3k0/cordova-plugin-purchase/wiki/HOWTO:-Migrate-to-v13 and API of functions for upgrade.