➕ Add the success/failure handling from Dart side for Apple Pay

[YazeedAlKhalaf]

This PR is a suggestion to be added to the pay plugin, I think it will make the experience better since now the developer can make the Apple Pay sheet result match their result.

Please give me comments not he implementation and if you have any suggestions I could make to get this PR merged.

Before: the plugin would always return payment successful in Apple Pay status
Now: the plugin will run the completion handler when Dart side sends the status wanted, either success or failure. This is useful when you need to check with your backend server if the payment went through or not and match your app's status with the native sheet's status.

📱 Video Showcase
Screen.Recording.2023-08-03.at.1.17.51.AM.mp4

[YazeedAlKhalaf]

@JlUgia can you please take a look?

[JlUgia]

Hi @YazeedAlKhalaf.
Thanks a lot for your contribution.
I added some comments. Let me know your thoughts and continue from there.

[YazeedAlKhalaf]

@JlUgia Thanks for your review and comments.

I will resolve them once I have time since I am currently busy with school work and finals.

[YazeedAlKhalaf]

@JlUgia can you please take a look at my comments and commit so we can move forward with this PR?

[JlUgia]

Hey @YazeedAlKhalaf, were you able to take a look at this commet?

[YazeedAlKhalaf]

@JlUgia I am facing issues with local development, the packages use the published versions.

So should I create multiple PRs? and wait for you to publish the changes I need before moving on?

Because otherwise I would be pushing broken code that doesn't work, not sure what I should do. I read the contributing guide but found nothing related to this.

[JlUgia]

You can use dependency_overrides in your pubspec.yaml during development, and revert back when you are ready to submit the code. You can see an example in the example project – https://github.com/google-pay/flutter-plugin/blob/main/pay/example/pubspec.yaml#L39

[YazeedAlKhalaf]

@JlUgia but then I will be pushing code with red lines, it is okay?

[YazeedAlKhalaf]

@JlUgia I added a video at the top and made the changes you suggested, kindly check it out.

[YazeedAlKhalaf]

@JlUgia were you able to look at the new changes?

[YazeedAlKhalaf]

@JlUgia hopefully you have had time to look at the changes, so that we can merge it 😁.

[Likhachev17]

Hi! I faced the same issue. Any updates?

[YazeedAlKhalaf]

@JlUgia

does the PR look good now? or it needs maybe more edits?

[JlUgia]

Hi @YazeedAlKhalaf
Taking a look at this now

[JlUgia]

This will fail silently if executed from Android. Since this can only happen at development time I'd let the Error surface back for the developer to fix their code.
Alternatively, and if you think the error is not informative enough, we can craft our own exception/error.

[YazeedAlKhalaf]

okay, i think making a custom exception would be better in this case. can you check it?

[JlUgia]

As you probably noted, this plugin offers two integration paths.

1. Most of the business logic, including the pay client, is contained within the button components (eg.: ApplePayButton): This approach is great for folks who are happy with a default-based configuration and prefer to think of their apps as a collection of components.
2. The views and business logic are separate and the implementer is responsible for handling both separately: Using the Pay client directly to issue calls, and showing the raw button components in the UI when necessary.

To be consistent with that, we'd want to update the StatefulWidget under pay/lib/src/widgets/apple_pay_button.dart to handle the new logic too, and use it in the example.
One way to do that would be to offer an additional result callback in the Apple Pay button widget in the pay library that includes a handler (eg.: paymentConfirmation) that lets folks update the payment result to the iOS native end.

Let me know what you think

[YazeedAlKhalaf]

Oh I didn't notice the the two integration paths before 😅.

Regarding the addition of a new result callback, since we renamed the method updatePaymentStatus to updatePaymentResult, I think it will be confusing to users who see onPaymentResult and might link it to updatePaymentResult.

We can do a breaking a change to the API and make the onPaymentResult signature:

final bool Function(Map<String, dynamic> result) onPaymentResult;

And then the user can do their backend call here and return the result of the payment from this callback, which is required.

We can also make the return type of the callback nullable so existing users of the package who are not interested don't need to touch their code, but then there becomes two definitions of success, null or true. null would be the default state if the user didn't return anything.

What do you think? Should we introduce a new callback, or use the existing one?

I think using onPaymentResult makes more sense here since the names are related and we can do something like this in the PayButton:

  /// Callback function to respond to tap events.
  ///
  /// This is the default function for tap events. Calls the [onPressed]
  /// function if set, and initiates the payment process with the [paymentItems]
  /// specified.
  VoidCallback _defaultOnPressed(
    VoidCallback? onPressed,
    List<PaymentItem> paymentItems,
  ) {
    return () async {
      onPressed?.call();
      try {
        final result = await _payClient.showPaymentSelector(
          buttonProvider,
          paymentItems,
        );

        final isPaymentSuccessful = onPaymentResult(result);
        if (_supportedPlatforms.contains(TargetPlatform.iOS)) {
          await _payClient.updatePaymentResult(isPaymentSuccessful ?? true);
        }
      } catch (error) {
        onError?.call(error);
      }
    };
  }

and then we can keep the Google button signature the same, but have it return null for example when providing it to the super.

and for Apple button, we can expose the new signature for users.

What do you think?

[JlUgia]

Thank you for the proposal @YazeedAlKhalaf.
Your suggestion certainly supposed the least resistance path into getting the functionality out. I see three main disadvantages:

• It's a breaking change, although minor and quick to address.
• Using the return value of a function to trigger an additional action reduces code clarity, as it requires developers to read the docs to know what's happening for certain.
• Changing the signature of the base implementation to accommodate for one specific platform (child class) increases the tension between the implementations a little, forcing Google Pay and any other provider that uses the base implementation to adhere to it.

Using an additional callback for Apple Pay doesn't have any of the disadvantages above, although this approach also has some drawbacks:

• The internal logic is a little more intricate.
• Adding a second callback to the Apple Pay button implementation requires demoting the original callback in the base class to a late final, since it now needs to be overriden in the Apple button child class to call both callbacks if set. This can't be done in the initializer block because the second callback needs the Pay client to talk to the iOS native end, which is a class member. A way to circumvent that would be to create a second Pay client for that call only. To me, the late final demotion seems more appropriate.

I've put together a prototypical proposal that adds an extra callback for Apple Pay and lets developers choose the one that is best suited for them (the change may seem a little larger than necessary, since it includes a couple of type definitions and extra modifiers that can be now added). This is how the change would look for those who decide to use the confirm flow:

onApplePayResultWithConfirm(
    Map<String, dynamic> paymentResult, PaymentConfirmation handler) {
  // Complete the payment
  bool paymentCompletionResult = ...;
  handler.confirmPayment(paymentCompletionResult);
}

ApplePayButton(
  paymentConfiguration: PaymentConfiguration.fromJsonString(
    payment_configurations.defaultApplePay),
  paymentItems: _paymentItems,
  onPaymentResultWithConfirm: onApplePayResultWithConfirm,
)

I have a mild preference for the latter approach, and would like to hear your thoughts on it.

[YazeedAlKhalaf]

Looks great tbh, easier for the developers 👍.

I'll try to bundle it in this PR and hopefully we can see what to do next!

[YazeedAlKhalaf]

Thank you so much @JlUgia for your suggestion and PR!

I have implemented your changes with a small change, can you please look at it?

[JlUgia]

Hi YazeedAlKhalaf, thanks a lot for your contributions and patience. This is great!
I've re-reviewed the proposal and suggested some minor and two small changes.
I've included a few proposals for the small changes.

If you think it's easier for me to do a proposal commit for you to review, I'm happy to do that too.

[oleksii-pimenov]

Any updates guys?

[YazeedAlKhalaf]

@JlUgia It has been a while 😅, around 6 months! But I would appreciate if you could look at the new code.

[JlUgia]

Take a look at the comment
Also, it'd be helpful to strip out all style changes from this PR.

[JlUgia]

I was under the impression that we decided to go with the alternative callback approach (see comment and let me know)

[YazeedAlKhalaf]

ah okay, not sure why i did it with one method, but basically i need to keep the old method and have this new method also, right?

just want to make sure 😁