Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Feb 15, 2019

The objective-c portion of add payment flow.

Since there is no dart code in this PR, it could be hard to trace what is going on. Included 2 purchase flows below to better explain.

A regular flow of a user to process a purchase will be as such:

  1. User queries the products by -[InAppPurchasePlugin startProductRequest:result:]
  2. User creates a payment object using productID which is found in the result of the 1st step.
  3. User make a payment calling -[InAppPurchasePlugin addPayment:result:] using the payment object gotten from step 2.
  4. updatedTransaction in dart waiting for the transaction to be at state of 'purchased'
  5. User finish the transaction by calling -[InAppPurchasePlugin finishTransaction:result:] with the transaction gotten from step 4.

A app store purchase flow will be as such:

  1. Customer triggered the purchase in the app store.
  2. Programmer gets a notification in a callback named shouldAddStorePayment in dart.
  3. If shouldAddStorePayment return true, the payment object will be automatically added to the queue using -[InAppPurchasePlugin addPayment:result:] (called by our plugin code, not the programmer)
  4. Same as step 4 in the regular flow from this point.

flutter/flutter#26328

@cyanglaz cyanglaz requested a review from mklim February 15, 2019 21:17
SKPayment *payment = [self.paymentsCache objectForKey:productID];
// User can use payment object with mutable = true and add simulatesAskToBuyInSandBox = true to
// test the payment flow.
if (!payment || [paymentMap[@"mutable"] boolValue] == YES) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This ends up creating a mutable payment whenever payment isn't specified, not just when mutable is explicitly specified as an option. Is that something we want to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mutable keyword is indeed confusing. I created this for user to test the payment in sandbox, I have updated the mutable keyword to usePaymentObject in a different branch. Do you think changing the keyboard to usePaymentObject makes more sense here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that makes sense.

I'm still a little confused over the code silently creating an SkMutable payment object whenever it can't find a pre-existing payment in the map. Do we want to default to that or would it make sense to error if we can't find an existing payment instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you are right. Going to update. I am also going to re arrange the code a bit to make it more readable.

SKMutablePayment *mutablePayment = [[SKMutablePayment alloc] init];
mutablePayment.productIdentifier = productID;
NSNumber *quantity = [paymentMap objectForKey:@"quantity"];
if (quantity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we panic and bail here if we don't have a quantity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should update the quantity to default to 1 here, ill update.

for (SKPaymentTransaction *transcation in transactions) {
[maps addObject:[FIAObjectTranslator getMapFromSKPaymentTransaction:transcation]];
}
[self.callbackChannel invokeMethod:@"updatedTransaction" arguments:maps];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it would be a little clearer to name these methods "Transactions" here and below, since there's multiple transactions in each method.


- (void)handleTransactionsRemoved:(NSArray<SKPaymentTransaction *> *)transactions {
NSMutableArray *maps = [NSMutableArray new];
for (SKPaymentTransaction *transcation in transactions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/transcation/transaction/g

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Googlers can find more info about SignCLA and this PR by following this link.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Googlers can find more info about SignCLA and this PR by following this link.

@cyanglaz cyanglaz requested review from collinjackson and removed request for collinjackson February 20, 2019 00:54
@cyanglaz cyanglaz force-pushed the iap_make_payment_objc branch from 543862d to 253f67a Compare February 20, 2019 01:04
@cyanglaz cyanglaz force-pushed the iap_make_payment_objc branch from c8b670c to 7f9e5a9 Compare February 20, 2019 01:20
@googlebot
Copy link

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

@cyanglaz
Copy link
Contributor Author

Mistakenly sent out a bunch of review requests due to the file owners setting. Please ignore the requests.

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

// the product to process the payment.
SKProduct *product = [self.productsCache objectForKey:productID];
if (product) {
payment = [SKPayment paymentWithProduct:product];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can users set quantity this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not supported with the regular purchase flow, user will need to use the custom payment object.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Googlers can find more info about SignCLA and this PR by following this link.

@cyanglaz cyanglaz force-pushed the iap_make_payment_objc branch from ceb3960 to f3cdb83 Compare February 20, 2019 23:39
@googlebot
Copy link

CLAs look good, thanks!

Googlers can find more info about SignCLA and this PR by following this link.

@cyanglaz cyanglaz merged commit f92ba3a into flutter:master Feb 21, 2019
@cyanglaz cyanglaz deleted the iap_make_payment_objc branch February 21, 2019 07:45
idealopamp pushed a commit to idealopamp/plugins that referenced this pull request Feb 21, 2019
@cyanglaz cyanglaz changed the title Iap make payment objc [in_app_purchase] make payment objc Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants