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

[in_app_purchase] Convert storefront(), transactions(), canMakePayment(), and addPayment() to pigeon #5910

Merged
merged 33 commits into from
Jan 31, 2024

Conversation

LouiseHsu
Copy link
Contributor

@LouiseHsu LouiseHsu commented Jan 17, 2024

Part 1 of flutter/flutter#117910

This PR converts storefront(), addPayment(), transactions(), and canMakePayment().

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

This is looking great. I think there was one opportunity where the code review would be a bit easier if the new code was kept in the same space as the old code. The way pigeon should be used with the SetUp function and the type conversions is correct. Looks like we have some good tests that translated well and the objc is looking good.

I'd actually stop any forward progress and just clean up what you have and land it. This is already so much, break this conversion up into multiple PRs.

Did you run this through clang-tidy? There are a few places where it would be easier to read/review if there were some more newlines.

@@ -294,4 +295,59 @@ + (SKPaymentDiscount *)getSKPaymentDiscountFromMap:(NSDictionary *)map
return discount;
}

+ (nullable SKPaymentTransactionMessage *) convertTransactionToPigeon:(SKPaymentTransaction *) transaction {
SKPaymentTransactionMessage *msg = [SKPaymentTransactionMessage makeWithPayment:[self convertPaymentToPigeon:transaction.payment] transactionState:[self convertTransactionStateToPigeon:transaction.transactionState] originalTransaction:transaction.originalTransaction ? [self convertTransactionToPigeon:transaction.originalTransaction] : nil transactionTimeStamp:[NSNumber numberWithDouble:[transaction.transactionDate timeIntervalSince1970]] transactionIdentifier:transaction.transactionIdentifier
Copy link
Member

Choose a reason for hiding this comment

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

Can you insert some newlines here between arguments? The formatter should respect that. Traditionally objc would align things at the colon. I don't know what clang-tidy does by default.

switch (state) {
case SKPaymentTransactionStatePurchasing:
return SKPaymentTransactionStateMessagePurchasing;
break;
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these breaks.

paymentDiscount:[self convertPaymentDiscountToPigeon: payment.paymentDiscount]];
return msg;
}
return nil;
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this guard more specific to the API you are protecting? Right now it's around makeWithProductIdentifier.

If we do really want to return nil we should have a comment (maybe just a TODO if you haven't gotten around to it).

InAppPurchasePlugin *instance = [[InAppPurchasePlugin alloc] initWithRegistrar:registrar];
[registrar addMethodCallDelegate:instance channel:channel];
[registrar addApplicationDelegate:instance];
InAppPurchaseAPISetup([registrar messenger], instance);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why the existing code does this but I'd just put registrar.messenger.

Comment on lines -90 to -94
} else if ([@"-[SKPaymentQueue transactions]" isEqualToString:call.method]) {
[self getPendingTransactions:result];
} else if ([@"-[SKPaymentQueue storefront]" isEqualToString:call.method]) {
[self getStorefront:result];
} else if ([@"-[InAppPurchasePlugin startProductRequest:result:]" isEqualToString:call.method]) {
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@@ -410,78 +406,19 @@ - (void)testGetPendingTransactions {
shouldAddStorePayment:nil
updatedDownloads:nil
transactionCache:OCMClassMock(FIATransactionCache.class)];
[self.plugin handleMethodCall:call
result:^(id r) {
resultArray = r;
Copy link
Member

Choose a reason for hiding this comment

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

You should remove the __block directive above now that this isn't edited in a block anymore.

SKPaymentTransactionMessage *result = [self.plugin transactionsWithError:&error][0];

// How should I test this nicely without overriding isEquals?
// XCTAssertEqualObjects(result.payment, originalPigeon.payment);
Copy link
Member

Choose a reason for hiding this comment

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

I think you could do XCTAssertEqualObjects([result.payment encode], [originalPigeon.payment encode]) as a nifty trick. I know it's slightly fragile since we reserve the right to change that. Otherwise just make a helper function that returns BOOL if they are equal.

}

class SKErrorMessage {
// a lot of comparison operators are overriden in this class - do i add them here?
Copy link
Member

Choose a reason for hiding this comment

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

Nope. Any code written here will be thrown away by pigeon.

// @ObjCSelector('storefront')
SKStorefrontMessage storefront();

@ObjCSelector('addPayment:')
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the ObjCSelector here

@@ -56,6 +57,9 @@ NS_ASSUME_NONNULL_BEGIN
withError:(NSString *_Nullable *_Nullable)error
API_AVAILABLE(ios(12.2));

+ (nullable SKPaymentTransactionMessage *) convertTransactionToPigeon:(SKPaymentTransaction *) transaction;
Copy link
Member

Choose a reason for hiding this comment

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

These could have their own tests. Not a huge deal if they have transitive coverage.

@LouiseHsu LouiseHsu changed the title In app purchase pigeon conversion [in_app_purchase] Convert Jan 23, 2024
@LouiseHsu LouiseHsu changed the title [in_app_purchase] Convert [in_app_purchase] Convert 'storefront()', Jan 23, 2024
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Awesome, this is close. The big thing I noticed was just copying over the toList methods and there is also that stray sleep that needs to be accounted for. Good job keeping all the old tests around. I want to give this one last look holistically to make sure we are good, but considering the work you did to keep the tests aligned, we should be good.

@@ -439,7 +439,7 @@
buildSettings = {
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
CURRENT_PROJECT_VERSION = 1;
DEVELOPMENT_TEAM = "";
DEVELOPMENT_TEAM = S8QB4VV633;
Copy link
Member

Choose a reason for hiding this comment

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

please revert these. xcode does this automatically, it's annoying

+ (nullable SKErrorMessage *)convertSKErrorToPigeon:(NSError *)error {
SKErrorMessage *msg = [SKErrorMessage makeWithCode:@(error.code)
domain:error.domain
userInfo:error.userInfo];
Copy link
Member

Choose a reason for hiding this comment

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

This may be a problem if the error.userInfo contains data that isn't serializable by flutter. It might be better to keep this empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, can you explain what you mean? 👀 I think userInfo is at least guaranteed to be a dict

Copy link
Member

Choose a reason for hiding this comment

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

NSDictionary's are serializable by Flutter, but the encoder recurses into their contents which can be any NSObject which may not be serializable (see FlutterStandardCodec.mm). It looks like the behavior is that a "Message corrupted" exception will be thrown on the dart side if that did happen.

If throwing the corrupted exception was the old behavior, I'm fine keeping it.

transactionList.add(transaction);
InAppPurchaseStoreKitPlatform.observer.updatedTransactions(
transactions: <SKPaymentTransactionWrapper>[transaction]);
sleep(const Duration(milliseconds: 30));
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want this, right?


class FakeStoreKitPlatform {
class FakeStoreKitPlatform implements TestInAppPurchaseApi {
Copy link
Member

Choose a reason for hiding this comment

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

nice, glad this worked out for keeping old tests around


* Converted `storefront()`, `transactions()`, `addPayment()`, `canMakePayment` to pigeon.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be present tense

Suggested change
* Converted `storefront()`, `transactions()`, `addPayment()`, `canMakePayment` to pigeon.
* Converts `storefront()`, `transactions()`, `addPayment()`, `canMakePayment` to pigeon.

@@ -56,7 +57,18 @@ NS_ASSUME_NONNULL_BEGIN
withError:(NSString *_Nullable *_Nullable)error
API_AVAILABLE(ios(12.2));

+ (nullable SKPaymentTransactionMessage *)convertTransactionToPigeon:
(SKPaymentTransaction *)transaction;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This parameter should be marked nullable since it handles null values, right? Same for convertStorefrontToPigeon:, convertPaymentDiscountToPigeon:, and convertPaymentToPigeon:.

Suggested change
(SKPaymentTransaction *)transaction;
(nullable SKPaymentTransaction *)transaction;

SKPaymentTransactionMessage *msg = [SKPaymentTransactionMessage
makeWithPayment:[self convertPaymentToPigeon:transaction.payment]
transactionState:[self convertTransactionStateToPigeon:transaction.transactionState]
originalTransaction:transaction.originalTransaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to null check, since this method returns a nil value if it is null already?

@@ -31,6 +31,7 @@ dev_dependencies:
flutter_test:
sdk: flutter
json_serializable: ^6.0.0
pigeon: ^11.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

pigeon is currently at version 16. Could you regenerate at the latest version? Version 13 adds good quality of like changes for Objective-C: https://pub.dev/packages/pigeon/changelog#1300, so it will probably prevent your code from compiling without changes.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch @bparrishMines, this seems like the sort of thing that we should be checking in CI.

@gaaclarke
Copy link
Member

Notes from talking in person:

I went back and looked over this. I think this has sufficient automated testing so we are good there. There is some manual testing that we should do in the example app as a gut check before we merge this. Details are at https://github.com/flutter/packages/tree/main/packages/in_app_purchase/in_app_purchase/example

It seems like the iOS simulator is already setup to test this easily at least.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM! There is just 2 minor style fixes. Please manually test it before you merge, but looking at everything I think we are good. Thanks for the extra work to make the review easier and keeping the tests minimally changed.

@@ -603,4 +539,48 @@ - (void)testShowPriceConsentIfNeeded {
}
#endif

// Pigeon message deserializers for easy comparison

- (NSArray *)PaymentTransactionToList:(SKPaymentTransactionMessage *)paymentTransaction {
Copy link
Member

Choose a reason for hiding this comment

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

These methods need to start with lowercase.

@@ -603,4 +539,48 @@ - (void)testShowPriceConsentIfNeeded {
}
#endif

// Pigeon message deserializers for easy comparison
Copy link
Member

Choose a reason for hiding this comment

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

The comment should be written in proper English prose.

Suggested change
// Pigeon message deserializers for easy comparison
// The following methods are deserializer copied from Pigeon's output.

@LouiseHsu LouiseHsu merged commit 871a24b into flutter:main Jan 31, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 2, 2024
flutter/packages@5b48c44...d37fb0a

2024-02-02 32242716+ricardoamador@users.noreply.github.com Add a link the different possible Android virtual device configs (flutter/packages#6033)
2024-02-01 32242716+ricardoamador@users.noreply.github.com Update the emulator versions and expose cipd. (flutter/packages#6025)
2024-02-01 stuartmorgan@google.com [tool] Add details to missing gradle coverage error (flutter/packages#6029)
2024-02-01 stuartmorgan@google.com [file_selector] Fix comment typo (flutter/packages#6027)
2024-02-01 43054281+camsim99@users.noreply.github.com [camerax] Change `buildPreview` to return `Texture` versus `FutureBuilder` (flutter/packages#6021)
2024-02-01 engine-flutter-autoroll@skia.org Manual roll Flutter from c65ab4d to e02e207 (38 revisions) (flutter/packages#6028)
2024-02-01 engine-flutter-autoroll@skia.org Manual roll Flutter from 75a2e5b to c65ab4d (22 revisions) (flutter/packages#6026)
2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter from ace9181 to 75a2e5b (16 revisions) (flutter/packages#6017)
2024-02-01 JeroenWeener@users.noreply.github.com [webview_flutter] Support for handling basic authentication requests (flutter/packages#5727)
2024-01-31 stuartmorgan@google.com [tool] Extend `flutter test` workaround to other desktops (flutter/packages#6024)
2024-01-31 katelovett@google.com [two_dimensional_scrollables] Merged cells for TableView (flutter/packages#5917)
2024-01-31 tessertaha@gmail.com [rfw] Restore RFW to 100% coverage after `ButtonBar` update (flutter/packages#6020)
2024-01-31 louisehsu@google.com [in_app_purchase] Convert storefront(), transactions(), canMakePayment(), and addPayment() to pigeon (flutter/packages#5910)
2024-01-31 reidbaker@google.com [in_app_purchase] Add play country code api (flutter/packages#5941)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…t(), and addPayment() to pigeon (flutter#5910)

Part 1 of flutter/flutter#117910

This PR converts storefront(), addPayment(), transactions(), and
canMakePayment().


## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants