[in_app_purchase] Fix the bug that prevent restored subscription transactions from being completed #2872

ziggycrane · 2020-07-13T11:44:58Z

Description

*transactionsSetter only contained single transaction for productId, but subscriptions have multiple transactions and when status changed to restored for one of them it was impossible to complete them, because you could not access them from Flutter. Remade transactionsSetter be NSMutableDictionary<NSString *, NSMutableArray<SKPaymentTransaction *> *> instead of NSMutableDictionary<NSString *, SKPaymentTransaction >.

Related Issues

flutter/flutter#53534
flutter/flutter#57903

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

[ x ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[ x ] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
[ x ] All existing and new tests are passing.
[ x ] I updated/added relevant documentation (doc comments with ///).
[ x ] The analyzer (flutter analyze) does not report any problems on my PR.
[ x ] I read and followed the [Flutter Style Guide].
[ x ] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
[ x ] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
[ x ] I updated CHANGELOG.md to add a description of the change.
[ x ] I signed the [CLA].
[ x ] I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
[ x ] No, this is not a breaking change.

…g completed

googlebot · 2020-07-13T11:45:06Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ziggycrane · 2020-07-13T11:46:51Z

@googlebot I signed it!

…

On Mon, Jul 13, 2020 at 2:45 PM googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 *Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.* Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it. ------------------------------ What to do if you already signed the CLA Individual signers - It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. Corporate signers - Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version <https://opensource.google/docs/cla/#troubleshoot>). - The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - The email used to register you as an authorized contributor must also be attached to your GitHub account <https://github.com/settings/emails>. ℹ️ *Googlers: Go here <https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fflutter%2Fplugins%2Fpull%2F2872> for more info*. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2872 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2XN7HEZLQG46UU6Q3ZKETR3LXVBANCNFSM4OYMW6ZA> .

googlebot · 2020-07-13T11:46:57Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

kinex · 2020-07-26T12:09:59Z

packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m

              details:call.arguments]);
    return;
  }
  @try {
+    for (SKPaymentTransaction *transaction in transactions) {
+      [self.paymentQueueHandler finishTransaction:transaction];
+    }


Is it ok to abort the loop if an exception is thrown or should you move try/catch inside the loop.

Is there a way for the user to know which transactions are failed when finishing?

LHLL · 2020-07-30T19:46:30Z

packages/in_app_purchase/ios/Classes/FIAPaymentQueueHandler.m

@@ -81,7 +81,11 @@ - (void)paymentQueue:(SKPaymentQueue *)queue
      //    will become impossible for clients to finish deferred transactions when needed.
      // 2. Using product identifiers can help prevent clients from purchasing the same
      //    subscription more than once by accident.
-      self.transactionsSetter[transaction.payment.productIdentifier] = transaction;
+      if ([self.transactionsSetter objectForKey:transaction.payment.productIdentifier] == nil) {


This == nil is unnecessary.

How so? You only want to init a new array to that key if there aren't an array with transactions for that key already.

LHLL · 2020-07-30T19:51:48Z

packages/in_app_purchase/ios/Classes/FIAPaymentQueueHandler.m

@@ -81,7 +81,11 @@ - (void)paymentQueue:(SKPaymentQueue *)queue
      //    will become impossible for clients to finish deferred transactions when needed.
      // 2. Using product identifiers can help prevent clients from purchasing the same
      //    subscription more than once by accident.
-      self.transactionsSetter[transaction.payment.productIdentifier] = transaction;
+      if ([self.transactionsSetter objectForKey:transaction.payment.productIdentifier] == nil) {
+        self.transactionsSetter[transaction.payment.productIdentifier] = [NSMutableArray array];


Prefer using light weight generic:

NSMutableArray<SKPaymentTransaction *> * transactionArray = [NSMutableArray array]; self.transactionsSetter[transaction.payment.productIdentifier] = transactionArray;

LHLL · 2020-07-30T19:52:59Z

packages/in_app_purchase/ios/Classes/FIAPaymentQueueHandler.m

+      continue;
+    }
+
+    [self.transactionsSetter[productId] removeObjectsInArray:[self.transactionsSetter[productId] filteredArrayUsingPredicate:[NSPredicate predicateWithFormat:@"transactionIdentifier == %@", transaction.transactionIdentifier]]];


Consider making this a single line to help formatting.

kinex · 2020-07-31T06:41:42Z

FYI, I applied this pull request to my production app. I still see exceptions "storekit_duplicate_product_object, There is a pending transaction for the same product identifier." and users are contacting me because their purchased in-app is not working,

ziggycrane · 2020-07-31T06:43:43Z

You will see the issue until you complete all the purchases. That part you still need to do yourself, this fix allows you do that.

…

On Fri, 31 Jul 2020 at 09:41, kinex ***@***.***> wrote: FYI, I applied this pull request to my production app. I still see exceptions "storekit_duplicate_product_object, There is a pending transaction for the same product identifier." and users are contacting me because their purchased in-app is not working, — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2872 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2XN7HHZBDACSZFLAQOPI3R6JRTLANCNFSM4OYMW6ZA> .

kinex · 2020-07-31T06:58:19Z

I am already doing that:

  Future _onPurchaseUpdatedStreamData(
      List<PurchaseDetails> purchaseDetailsList) async {
    for (final purchaseDetails in purchaseDetailsList) {
        if (purchaseDetails.pendingCompletePurchase) {
          final billingResult =
              await _connection.completePurchase(purchaseDetails);
        } 
      }
    }
  }

d9media · 2020-07-31T10:55:32Z

I am already doing that:

  Future _onPurchaseUpdatedStreamData(
      List<PurchaseDetails> purchaseDetailsList) async {
    for (final purchaseDetails in purchaseDetailsList) {
        if (purchaseDetails.pendingCompletePurchase) {
          final billingResult =
              await _connection.completePurchase(purchaseDetails);
        } 
      }
    }
  }

I do it like that as well but it does not seem to work always

if (Platform.isIOS)
                       await InAppPurchaseConnection.instance
                           .completePurchase(purchaseDetails)
                   })

Another contributor to #53534 suggested this method so I've implemented this method and call it on button press before sending the purchase request

  Future _maybeClearTransactions() async {
    if (Platform.isIOS) {
      var paymentWrapper = SKPaymentQueueWrapper();
      var transactions = await paymentWrapper.transactions();
      for (var i = 0; i < transactions.length; i++) {
        await paymentWrapper.finishTransaction(transactions[i]);
      }
      await Future.delayed(Duration(milliseconds: 500));
    }
  }

kinex · 2020-07-31T11:22:13Z

@d9media Actually I included "_maybeClearTransactions" also into my previous production update (excluding the magic delay at the end... maybe it was a mistake to exclude it) and call it before sending the purchase request.

d9media · 2020-07-31T14:28:24Z

@d9media Actually I included "_maybeClearTransactions" also into my previous production update (excluding the magic delay at the end... maybe it was a mistake to exclude it) and call it before sending the purchase request.

Yeah I'm not sure if that really matters because were still gettings calls of customers not able to complete purchase. I think much comes down to ziggyscrane observation here in regards to when to call queryPastPurchases and subscribing to purchase updates. From my understanding the stream will not receive restored purchases.

kinex · 2020-07-31T21:18:56Z

@ziggycrane Any reason why you did not remove the unnecessary (as you said and I agree) check from this method:

- (BOOL)addPayment:(SKPayment *)payment {
  if (self.transactionsSetter[payment.productIdentifier]) {
    return NO;
  }
  [self.queue addPayment:payment];
  return YES;
}

Actually, I would remove transactionSetter and any references to it totally. At least I could not find any really useful purpose for it. Other place where it is referenced from is InAppPurchasePlugin.m/finishTransaction. The way it is used there looks also unnecessary and most probably only causing issues.

So there is relatively complex logic (which relies on OS callbacks to work "correctly") trying to maintain transactionSetter, a variable that is not really needed and only adding complexity and uncertainty to this plugin. So I would just remove it.

kinex · 2020-08-07T11:41:32Z

I implemented the fixes I described above in this PR #2911

# Conflicts: # packages/in_app_purchase/CHANGELOG.md # packages/in_app_purchase/pubspec.yaml

ziggycrane · 2020-08-08T08:27:45Z

@kinex you can't realy remove it, because there were reason why it was implemented in the first place. If customer starts new transactions without previous ones being completed it will result in trouble.

ziggycrane · 2020-08-08T08:29:21Z

@kinex I could not repeat the error with these changes - if you complete the purchases before starting new one everything works fine.

kinex · 2020-08-08T10:43:50Z

@kinex you can't realy remove it, because there were reason why it was implemented in the first place. If customer starts new transactions without previous ones being completed it will result in trouble.

Can you clarify what kind of troubles exactly? In which scenario could this happen? I still think it can be removed safely. You can also compare this to other Flutter plugin flutter_inapp_purchase which has longer history than this one so it is more stable too. It doesn't have any unnecessary checks, it just calls SKPaymentQueue.addPayment like I do in my PR now.

Unfortunately I didn't have very much time to investigate why exactly the current implementation based on the transactionsSetter does not work. My best guess is that the SKPaymentTransactionObserver events do not work always as your code expects them to work. Maybe there have been made some assumptions how it works and things have changed in some iOS update. But investigating that is waste of time in my opinion, because you don't need the whole logic.

I have been forced to test these different fix attempts with my production app as my own Sandbox environment is somehow broken and I need the fix ASAP. As I reported earlier this PR did not fix the issues in my app. I saw this from Crashlytics and several (angry) users also contacted me directly. But this is of course only my test results. Maybe it depends also on how you use the plugin. It would be interesting to know if some other developer has managed to solve the issues with this PR (preferably in the production environment).

Anyway, I didn't have any more time to wait for possible other fixes (already too many 1-star reviews, angry users and lost money) so I decided to fix this myself. I am sharing my fixes in the PR #2911. I am testing this again with my production app and with real users. Two days after releasing it I am already confident the issues were finally resolved. The same users who have had the issue 1-2 weeks are now reporting it was solved. I haven't seen any related exceptions in Crashlytics either. But I will continue tracking this of course, only 2 days behind.

So I guess you have two options:

Review and merge my PR [in_app_purchase] Removed maintaining own cache of transactions #2911
OR
If you want to keep transactionsSetter for whatever reason, investigate further how the SKPaymentTransactionObserver events really work in different situations and with different OS versions and fix updating the transactionsSetter properly.

LHLL

LGTM, thanks for the fix.

…sactions from being completed (flutter#2872) * Fix the bug that prevent restored subscription transactions from being completed * Update changelog * increased version * fixed removing transactions from transactionsSetter * Fixed CHANGELOGS conflicts * transactionsSetter code formating updates * fixed formating

ziggycrane added 2 commits July 13, 2020 14:04

Fix the bug that prevent restored subscription transactions from bein…

300f223

…g completed

Update changelog

17af9f9

ziggycrane requested review from cyanglaz, LHLL and mklim as code owners July 13, 2020 11:44

googlebot added the cla: no label Jul 13, 2020

googlebot added cla: yes and removed cla: no labels Jul 13, 2020

ziggycrane added 2 commits July 13, 2020 14:55

increased version

b90c6c1

fixed removing transactions from transactionsSetter

b52fca1

kinex reviewed Jul 26, 2020

View reviewed changes

LHLL reviewed Jul 30, 2020

View reviewed changes

ziggycrane added 3 commits August 8, 2020 10:52

Merge branch 'plugins' into in_app_purchases

bb53423

# Conflicts: # packages/in_app_purchase/CHANGELOG.md # packages/in_app_purchase/pubspec.yaml

Fixed CHANGELOGS conflicts

6611eff

transactionsSetter code formating updates

cd52a97

fixed formating

6b4b8b1

ziggycrane requested a review from LHLL August 14, 2020 07:59

LHLL approved these changes Aug 14, 2020

View reviewed changes

LHLL mentioned this pull request Aug 25, 2020

[in_app_purchase] Removed maintaining own cache of transactions #2911

Merged

13 tasks

Merge branch 'master' into in_app_purchases

230449e

LHLL merged commit ba09b7f into flutter:master Sep 9, 2020

[in_app_purchase] Fix the bug that prevent restored subscription transactions from being completed #2872

[in_app_purchase] Fix the bug that prevent restored subscription transactions from being completed #2872

Uh oh!

Conversation

ziggycrane commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

googlebot commented Jul 13, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

ziggycrane commented Jul 13, 2020 via email

Uh oh!

googlebot commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinex commented Jul 31, 2020

Uh oh!

ziggycrane commented Jul 31, 2020 via email

Uh oh!

kinex commented Jul 31, 2020

Uh oh!

d9media commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinex commented Jul 31, 2020

Uh oh!

d9media commented Jul 31, 2020

Uh oh!

kinex commented Jul 31, 2020

Uh oh!

kinex commented Aug 7, 2020

Uh oh!

ziggycrane commented Aug 8, 2020

Uh oh!

ziggycrane commented Aug 8, 2020

Uh oh!

kinex commented Aug 8, 2020

Uh oh!

LHLL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziggycrane commented Jul 13, 2020 •

edited

Loading

d9media commented Jul 31, 2020 •

edited

Loading