[in_app_purchase][iOS] fix iOS promotional offers (SKPaymentDiscountWrapper was not used properly) #6541

crystalstorm · 2022-10-06T08:58:45Z

Make iOS Promotional Offers in in_app_purchase plugin usable. SKPaymentDiscountWrapper was not being sent trough the method channel.
This is an update to an old (already closed) PR here

Resolves: [in-app-purchase][iOS] Promotional Offers in iOS not working

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…heck

cyanglaz · 2022-10-06T17:18:51Z

packages/in_app_purchase/in_app_purchase_storekit/ios/Classes/FIAObjectTranslator.m

@@ -277,7 +277,7 @@ + (SKPaymentDiscount *)getSKPaymentDiscountFromMap:(NSDictionary *)map
    return nil;
  }

-  if (!timestamp || ![timestamp isKindOfClass:NSNumber.class] || [timestamp intValue] <= 0) {
+  if (!timestamp || ![timestamp isKindOfClass:NSNumber.class] || [timestamp longLongValue] <= 0) {


Nice catch. Can we add an XCUnitTest for this? Maybe have the timestamp to be a very big long long number?

Done. I've added XCUnitTest using current timestamp number value. It's enough to cause the intValue overflow in negative number. If I use really big long long number, the intValue overflow goes positive again and passes the test. I'm not sure of the usefulness of this "not negative int" check in the original code.

cyanglaz · 2022-10-06T17:21:55Z

...p_purchase/in_app_purchase_storekit/lib/src/store_kit_wrappers/sk_payment_queue_wrapper.dart

@@ -405,7 +405,8 @@ class SKPaymentWrapper {
      'applicationUsername': applicationUsername,
      'requestData': requestData,
      'quantity': quantity,
-      'simulatesAskToBuyInSandbox': simulatesAskToBuyInSandbox
+      'simulatesAskToBuyInSandbox': simulatesAskToBuyInSandbox,
+      'paymentDiscount': paymentDiscount?.toMap(),


We should have some test for the toMap method, can we update the test to include the discount? If we don't have an existing test, do you mind adding a test for the toMap method?

Done. I've added a new test checking the values of the SKPaymentWrapper object and the result of the toMap method explicitly.

cyanglaz · 2022-10-06T17:23:44Z

...ages/in_app_purchase/in_app_purchase_storekit/lib/src/in_app_purchase_storekit_platform.dart

@@ -75,7 +75,10 @@ class InAppPurchaseStoreKitPlatform extends InAppPurchasePlatform {
            purchaseParam is AppStorePurchaseParam ? purchaseParam.quantity : 1,
        applicationUsername: purchaseParam.applicationUserName,
        simulatesAskToBuyInSandbox: purchaseParam is AppStorePurchaseParam &&
-            purchaseParam.simulatesAskToBuyInSandbox));
+            purchaseParam.simulatesAskToBuyInSandbox,
+        paymentDiscount: purchaseParam is AppStorePurchaseParam


We should test that the paymentDiscount is encoded and sent to method channel.

I see your point, that in fact the bug was that the discount parameter was not sent trough the method channel. But I am not sure on how to construct such a test?
In my understanding: now the discount is send trough AppStorePurchaseParam in the buyNonConsumable method, which is not returning a useful result to test. This method constructs SKPaymentWrapper with a discount parameter and sends it to _skPaymentQueueWrapper.addPayment which turns the SKPaymentWrapper to a map (which we test in the newly created test for the toMap method) and sends it trough the method channel. But the channel is not returning useful result for testing either. The encoding of the paymentDiscount is in the toMap method which we test now.
Can you elaborate please?

You might be able to intercept the method call in the FakeStoreKitPlatform and inspect the value?

I got your idea. Thanks! Done in the last commit.

…r SKPaymentWrapper.toMap()

packages/in_app_purchase/in_app_purchase_storekit/example/ios/RunnerTests/TranslatorTests.m

crystalstorm · 2022-10-10T17:22:44Z

@cyanglaz I don't understand while the last commit failed at the "submit-queue" test. When I read the details it seems related to android. I cannot see reference to the changes made in the PR here. Can you help?

cyanglaz · 2022-10-11T13:22:18Z

@cyanglaz I don't understand while the last commit failed at the "submit-queue" test. When I read the details it seems related to android. I cannot see reference to the changes made in the PR here. Can you help?

"submit-queue" is not related to this PR. It is an indication of the current tree's status. This CI is intended to block merging when tree is red (when the build is failing on master). Usually, if there is a "submit-queue" error, someone on the flutter team is working on a fix, the best action is to wait.

cyanglaz

LGTM mod nits, @stuartmorgan could you do a secondary review?

cyanglaz · 2022-10-12T16:30:19Z

packages/in_app_purchase/in_app_purchase_storekit/lib/src/types/app_store_purchase_param.dart

@@ -32,4 +33,7 @@ class AppStorePurchaseParam extends PurchaseParam {

  /// Quantity of the product user requested to buy.
  final int quantity;
+
+  /// Discount applied to the product (optional).


nits, the "optional" keyword is probably not necessary as the nullability of this parameter already indicates it is optional.
However, it might be better to add an explanation what does it mean to be null, something like:

/// /// The value is `null` when the product does not have a discount.

Done. Changed in the last commit.

cyanglaz · 2022-10-12T16:32:45Z

packages/in_app_purchase/in_app_purchase_storekit/test/fakes/fake_storekit_platform.dart

@@ -30,6 +30,7 @@ class FakeStoreKitPlatform {
  PlatformException? restoreException;
  SKError? testRestoredError;
  bool queueIsActive = false;
+  Map<String, dynamic>? discountReceived;


nits: Alternatively, we can make it non-nullable and initialized it to empty map, this can avoid many unwrapping.

Yes, collections should only be nullable if null and empty are conceptually different.

Done. Changed in the last commit.

stuartmorgan-g · 2022-10-12T16:54:49Z

packages/in_app_purchase/in_app_purchase_storekit/test/fakes/fake_storekit_platform.dart

@@ -30,6 +30,7 @@ class FakeStoreKitPlatform {
  PlatformException? restoreException;
  SKError? testRestoredError;
  bool queueIsActive = false;
+  Map<String, dynamic>? discountReceived;


Yes, collections should only be nullable if null and empty are conceptually different.

stuartmorgan-g · 2022-10-12T16:55:51Z

packages/in_app_purchase/in_app_purchase_storekit/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.3.3
+
+* Fixes `SKProductDiscountWrapper` with being sent trough the method channel as the expected parameter.


Typo: through

But higher level, the changelog should describe the issue from the client perspective, not the implementation details of the bug.

Changed in the last commit.

stuartmorgan-g · 2022-10-12T16:57:37Z

packages/in_app_purchase/in_app_purchase_storekit/ios/Classes/FIAObjectTranslator.m

@@ -237,7 +237,7 @@ + (NSDictionary *)getMapFromSKStorefront:(SKStorefront *)storefront

 + (SKPaymentDiscount *)getSKPaymentDiscountFromMap:(NSDictionary *)map
                                         withError:(NSString **)error {
-  if (!map || map.count <= 0) {
+  if (!map || [map isKindOfClass:[NSNull class]] || map.count <= 0) {


This code is checking for a type violation; NSNull is not a NSDictionary, so if this happens the caller has made an invalid call. If you have a path where this is happening, it needs to be fixed at the original argument extraction point, not here.

The original code never transferred through the method channel the discount parameter. So this check was stopped at the !map check, never trying the map.count <= 0 check and directly returning nil afterwards.
What happened when I transferred the discount key through the channel was that, when I was buying a product without discount, it was NULL but it did not stop at the !map it went to the next check map.count <= 0 and crashed.
So here I do not try to check for a type violation, but I am shielding the crashing check that comes next.
As for if it should be done here or in the caller. Why it is better in the caller?
Shouldn't the method take care of the argument's validation, as it does in the checks that come below?

when I was buying a product without discount, it was NULL

You mean it was NSNull, presumably, given the code you added. NULL is a typedef for 0, and is essentially the C equivalent of nil. NSNull is not nil/NULL, it is singleton class.

but it did not stop at the !map

That is expected; a pointer that is pointing to an NSNull instance is not nil.

it went to the next check map.count <= 0 and crashed.

Presumably the crash was an unrecognized selector sent to instance exception, because count is not a method of NSNull.

So here I do not try to check for a type violation

Yes, that is exactly what you are checking for. You are taking a pointer of type NSDictionary and checking whether it contains an NSNull instance instead of an NSDictionary instance. Which would be a type violation.

As for if it should be done here or in the caller. Why it is better in the caller?

This method takes an NSDictionary*, not an id. It is better in the caller because it is a type violation to pass an argument to this method that is not an NSDictionary, and an instance of NSNull is not an NSDictionary.

The comparable Dart code is:

class NSNull { } void methodHandler(dynamic args) { expectsAMap(args); } void expectsAMap(Map? map) { if (map != null) { print(map.length); } } void main() { NSNull nullArg = NSNull(); methodHandler(nullArg); }

The bug is this Dart code is methodHandler passing args to expectsAMap without checking whether it's a Map, not when expectsAMap uses map as if it were its declared type.

Shouldn't the method take care of the argument's validation, as it does in the checks that come below?

Methods should not have to validate that they haven't been called with types that violate their declarations. Other validation (like whether it's nil), sure.

The code below this is also wrong; values with unknown types should be type checked before assigning to a variable declared as a specific type, not after. Either the values can only be NSStrings, in which case the checks are pointless, or (more likely) they could be NSStrings or NSNulls, in which case the variables should be declared id, not NSString*.

I see your point. Thanks for the detailed explanation. Much appreciated! I've pushed a commit where I move the check out of there and into it's caller.

stuartmorgan-g · 2022-10-12T16:58:41Z

packages/in_app_purchase/in_app_purchase_storekit/pubspec.yaml

@@ -2,7 +2,7 @@ name: in_app_purchase_storekit
 description: An implementation for the iOS platform of the Flutter `in_app_purchase` plugin. This uses the StoreKit Framework.
 repository: https://github.com/flutter/plugins/tree/main/packages/in_app_purchase/in_app_purchase_storekit
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+in_app_purchase%22
-version: 0.3.2+2
+version: 0.3.3


This is a bugfix, so the version should be 0.3.2+3.

This adds a new field to the AppStorePurchaseParam so it should be a new feature. The PR description or CHANGELOG should probably be something like "Supports adding discount information to AppStorePurchaseParam", instead of "fixing".

Thanks for the correction there; I didn't look closely enough at the details and thought it was just populating something that was previously added but not wired up.

stuartmorgan-g · 2022-10-12T16:59:30Z

packages/in_app_purchase/in_app_purchase_storekit/test/fakes/fake_storekit_platform.dart

@@ -169,6 +171,25 @@ class FakeStoreKitPlatform {
      case '-[InAppPurchasePlugin addPayment:result:]':
        final String id = call.arguments['productIdentifier'] as String;
        final int quantity = call.arguments['quantity'] as int;
+
+        // in case of testing paymentDiscount


Comments should be full grammatically correct sentences, with capitalization and punctuation.

Changed in the last commit

stuartmorgan-g · 2022-10-12T17:01:12Z

packages/in_app_purchase/in_app_purchase_storekit/test/fakes/fake_storekit_platform.dart

+                call.arguments['paymentDiscount'];
+            discountReceived = <String, dynamic>{};
+            // can't cast directly the argument to Map<String,dynamic>, will receive:
+            // PlatformException(error, type '_InternalLinkedHashMap<Object?, Object?>' is not a subtype of type 'Map<String?, dynamic>' in type cast, null, null)


This comment sounds like you attempted to cast with as instead of cast<...>.

True. Changed it to a cast<> now. Thanks!

packages/in_app_purchase/in_app_purchase_storekit/test/fakes/fake_storekit_platform.dart

…o the caller

crystalstorm · 2022-10-13T17:51:19Z

@cyanglaz / @stuartmorgan should I squash all the commits before you accept the changes and decide to merge?

jmagman · 2022-10-14T19:13:35Z

@cyanglaz / @stuartmorgan should I squash all the commits before you accept the changes and decide to merge?

That's not necessary, GitHub will squash when it merges.

stuartmorgan-g

Looks good overall, just one more type issue.

stuartmorgan-g · 2022-10-21T19:59:54Z

packages/in_app_purchase/in_app_purchase_storekit/ios/Classes/InAppPurchasePlugin.m

-    SKPaymentDiscount *paymentDiscount = [FIAObjectTranslator
-        getSKPaymentDiscountFromMap:[paymentMap objectForKey:@"paymentDiscount"]
-                          withError:&error];
+    NSDictionary *paymentDiscountMap = [paymentMap objectForKey:@"paymentDiscount"];


This is still a type violation, it's just more local. If the right hand side of this assignment is NSNull, then it is incorrect to assign it to an NSDictionary*.

Either this should be an id, or you should use a helper like https://github.com/flutter/plugins/blob/main/packages/file_selector/file_selector_macos/macos/Classes/FileSelectorPlugin.swift#L198-L201 (written as ObjC of course) to do the extract/check/convert/assign in a single line here.

Doing the latter would allow you to remove the extra conditional you had to add at this layer since the called code already handles nil correctly.

(@cyanglaz Any chance you could plan to convert this plugin to Pigeon sometime in the nearish term, like early 2023? It seems like there's a fair amount of this kind of problem in the plugin, and we could eliminate all of it with a Pigeon conversion.)

Done. I did it with ObjC version of the helper that you've shown, one line without the condition check. Thanks! :)

stuartmorgan-g

LGTM, thanks for the iteration on the type issues!

…tDiscountWrapper was not used properly) (flutter/plugins#6541)

…rapper was not used properly) (flutter#6541)

fix payment discount (iOS Promotional Offers) into being actually usable

87b43a0

crystalstorm requested a review from cyanglaz as a code owner October 6, 2022 08:58

github-actions bot added p: in_app_purchase platform-ios labels Oct 6, 2022

fix long value for payment discount's timestamp overflowing integer c…

fa38cb5

…heck

crystalstorm mentioned this pull request Oct 6, 2022

fix payment discount (iOS Promotional Offers) into being actually usable #6099

Closed

11 tasks

cyanglaz suggested changes Oct 6, 2022

View reviewed changes

crystalstorm added 2 commits October 7, 2022 10:11

add XCT test regarding discount's timestamp overflow and dart test fo…

cd50399

…r SKPaymentWrapper.toMap()

patch format of new XCTest

8d9aa09

cyanglaz reviewed Oct 7, 2022

View reviewed changes

packages/in_app_purchase/in_app_purchase_storekit/example/ios/RunnerTests/TranslatorTests.m Show resolved Hide resolved

crystalstorm added 3 commits October 8, 2022 09:40

add members checks in the XCT unit test for the overflowing timestamp

62425f7

extend fakeStorekitPlatform to intercept paymentDiscount case

0bab6d1

fix failing test about type annotations

8fe0cd7

crystalstorm requested a review from cyanglaz October 12, 2022 03:13

cyanglaz approved these changes Oct 12, 2022

View reviewed changes

stuartmorgan-g suggested changes Oct 12, 2022

View reviewed changes

change discountReceived to not nullable and other PR review changes

badbd1d

crystalstorm force-pushed the bugfix/iap-payment-discount-2 branch from 0b58c4f to badbd1d Compare October 13, 2022 12:55

crystalstorm requested a review from stuartmorgan-g October 13, 2022 13:36

crystalstorm added 2 commits October 13, 2022 20:36

move the is-null check out of the getSKPaymentDiscountFromMap and int…

32c0424

…o the caller

fix format after the null-check move

f90c3f5

stuartmorgan-g suggested changes Oct 21, 2022

View reviewed changes

crystalstorm added 2 commits October 22, 2022 07:54

add helper for NSdictionary nil extract for given key

3621201

fix format issues

ea5cbcf

crystalstorm requested a review from stuartmorgan-g October 24, 2022 04:54

stuartmorgan-g approved these changes Oct 24, 2022

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 24, 2022

auto-submit bot merged commit 84f5ec6 into flutter:main Oct 24, 2022

engine-flutter-autoroll mentioned this pull request Oct 24, 2022

Roll Plugins from 7a7480a5f392 to 84f5ec608be4 (2 revisions) flutter/flutter#113942

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2022

84f5ec608 [in_app_purchase][iOS] fix iOS promotional offers (SKPaymen…

a6c1db9

…tDiscountWrapper was not used properly) (flutter/plugins#6541)

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[in_app_purchase][iOS] fix iOS promotional offers (SKPaymentDiscountW…

f4782d6

…rapper was not used properly) (flutter#6541)

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[in_app_purchase][iOS] fix iOS promotional offers (SKPaymentDiscountW…

7094f44

…rapper was not used properly) (flutter#6541)

		@@ -1,3 +1,7 @@
		## 0.3.3

		* Fixes `SKProductDiscountWrapper` with being sent trough the method channel as the expected parameter.

[in_app_purchase][iOS] fix iOS promotional offers (SKPaymentDiscountWrapper was not used properly) #6541

[in_app_purchase][iOS] fix iOS promotional offers (SKPaymentDiscountWrapper was not used properly) #6541

Uh oh!

Conversation

crystalstorm commented Oct 6, 2022

Pre-launch Checklist

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!

Uh oh!

crystalstorm commented Oct 10, 2022

Uh oh!

cyanglaz commented Oct 11, 2022

Uh oh!

cyanglaz left a comment

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!

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!

stuartmorgan-g Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

crystalstorm commented Oct 13, 2022

Uh oh!

jmagman commented Oct 14, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

stuartmorgan-g Oct 12, 2022 •

edited

Loading