React-native-iap: BAD CONSUMPTION FUNCTIONALITY!!

[lwansbrough]

refreshAllItems is very poorly named!! The implementation uses fetchHistory on iOS which is non-mutative operation, and uses consumeItem on Android, which destroys purchase history! This is either very bad naming, or very poor purchase flow behaviour! I'll nominate it for both!

I can't think of a single case where I'd want to immediately consume all purchased items. What I would expect is that refreshAllItems re-synchronizes the device purchase history with Google Play and the App Store (ie. getPurchases) -- this should absolutely be changed so more people don't make this mistake.

I even checked the source code to make sure I had the right idea about what was going on, but I got as far as getPurchases on the Android side and stopped reading. I didn't notice this module was automatically consuming those purchases!!

Here's my suggestion for a fix: rename refreshAllItems to refreshPurchases, and ONLY fetch the user's purchase history. Do NOT consume items automatically. Consuming behaviour is it's own flow which should be managed by the application developer, ie explicitly calling consumeItem(...) or something similar.

[lwansbrough]

https://developer.android.com/google/play/billing/billing_integrate.html

You typically don't want to implement consumption for in-app products that are purchased once and provide a permanent effect (for example, a premium upgrade).

[lwansbrough]

Here's what you need instead of getPurchases: you need getPurchaseHistory. https://developer.android.com/google/play/billing/billing_reference.html#getPurchaseHistory

[hyochan]

@lwansbrough Hi! thank you for really important testing. As you can see we are out of time just focusing on this module. However, I am trying hard to maintain this so others can use in-app-purchase functionality easy as possible. Thank you for the link, I've found out that they implemented new api since 2017.05 for getPurchaseHistory. I've just changed method name refreshAllItems to fetchHistory, I've found out their characteristic was totally different since ios control the purchase process itself. Also if you look in the readme, there is getPurchasedItemsAndroid method for android which query for purchased items in android.

Could you please try this commit in dev branch which I've just updated? Since I may take some time setting the testing environment, I wish I can hear from you if this is what you want.

Thank you again for the help. If you are fine with this commit, I'll publish this to 0.2.17.

[lwansbrough]

@dooboolab I've taken my somewhat rough criticism and have turned it into constructive change! Here's my work so far: https://github.com/TrackerNetwork/react-native-iap/commit/25e7e9b278cd02e9333590252e0c9e0684ed7182

I've refactored a considerable portion of the code base (very much a work in progress still) with the intent of making it more stable and more readable. I've also cleaned up the API a bit (removed platform specific functionality) in favour of an easier to maintain core set of functionality.

Thanks for your response + for confirming my suspicions. Unfortunately I think there may be some lasting impact on a few thousand of my users. :(

[hyochan]

@lwansbrough Sorry to hear that. Be sure module version without 1.0.0 release is under construction and needs some testing before actually using it in your running app. Would you mind me committing changes to npm module 0.2.17? Could you work from there?

I've seen your changes, and there are lots of changes that should be made to 0.3.0 release after all. Hope you look into index.d.ts file and also the Readme.md.

I'll just publish the updates to 0.2.17 and yours will be 0.3.0 if you contribute to our repository.

Thank you for the work again.

[lwansbrough]

@dooboolab I think getting version out 0.2.17 out when you can would be a great start.

[javier-artiles]

@lwansbrough Thank you for clarifying these concepts and sending the PR. After your experience with the code, what do you think are areas we can improve moving forward ? I must admit I have a vested interest (my own RN apps out there) in this library being robust and well documented, so I'd like to put time aside to contribute here.

[hyochan]

@javier-artiles
You can see the remaining issues in this repo. Testing local receipt validation might be useful too. Also, before all that, we need to test @lwansbrough code and update readme and announce users for migration who's already using our module. I am running out of time and testing is bit delayed. Sorry for that.

[javier-artiles]

@dooboolab No worries, I know it can be hard to balance personal and day job projects. Will start by spending some time reading through the codebase, @lwansbrough fork, and currently reported issues.

[hyochan]

[email protected] has released. Be sure there is bug in [email protected] so please try [email protected].

[lwansbrough]

@dooboolab Please refer to this issue again, as the method has been reimplemented. We need to find a better name for this action. It is very misleading and in cases where someone doesn't pay very close attention to the documentation, they can irreversibly delete transaction information.

[lwansbrough]

Perhaps it would be more appropriate to make a new issue - I haven't checked if the source code does the same thing as it used to, but it sounds like it does based on the description.

[hyochan]

@lwansbrough Alright what about calling the method name consumeAllProducts? Someone need this method because sometimes they need to wait for 24 hours if they refunded the item and try to buy the same thing again. If the app doesn't care about transaction information, this method is quite useful.

[lwansbrough]

Yeah fair enough, I can see the use case is valid. consumeAllProducts is good, or you could call it dangerouslyRefreshAllItems - as long as it’s clear the method does something destructive.

[hyochan]

@lwansbrough I've renamed refreshItems to consumeAllItems in npm 1.0.1. Closing this again.