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

753 implement apple pay and use the donate with apple pay button for support kiwix #1022

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Nov 3, 2024

Fixes: #753

Should be tested with real payments to see if it works end to end.

@rgaudin
Copy link
Member

rgaudin commented Nov 4, 2024

This is a UI PR ; please include a screencast or screenshots.

@BPerlakiH
Copy link
Collaborator Author

apple_pay_iphone.mov
apple_pay_ipad.mov
apple_pay_macos.mov

@kelson42
Copy link
Contributor

kelson42 commented Nov 4, 2024

@BPerlakiH Nice, but CI is red!

@BPerlakiH
Copy link
Collaborator Author

From what I suspect, this might require more integration into Stripe, to get the available payment options from there, and possibly make the payments appear on Stripe dashboard, but let's try it without it first.

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 407 lines in your changes missing coverage. Please review.

Project coverage is 24.61%. Comparing base (d79d153) to head (6a77700).

Files with missing lines Patch % Lines
Model/Payment.swift 0.00% 94 Missing ⚠️
Views/Settings/Settings.swift 0.00% 57 Missing ⚠️
Views/Payment/ListOfAmounts.swift 0.00% 54 Missing ⚠️
Views/Payment/CustomAmount.swift 0.00% 48 Missing ⚠️
Views/Payment/PaymentForm.swift 0.00% 48 Missing ⚠️
Views/Payment/PaymentSummary.swift 0.00% 35 Missing ⚠️
Model/StripeKiwix.swift 0.00% 33 Missing ⚠️
Views/Payment/PaymentThankYou.swift 0.00% 23 Missing ⚠️
Views/Buttons/SupportKiwixButton.swift 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1022      +/-   ##
==========================================
- Coverage   26.32%   24.61%   -1.72%     
==========================================
  Files         114      122       +8     
  Lines        6495     6887     +392     
==========================================
- Hits         1710     1695      -15     
- Misses       4785     5192     +407     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@kelson42
Copy link
Contributor

kelson42 commented Nov 5, 2024

@BPerlakiH I can not merge like this, what is going on with the CI must be properly understood and treated.

@rgaudin
Copy link
Member

rgaudin commented Nov 5, 2024

@BPerlakiH your marchant ID commit 3bee29b fixed the iOS build but not the macOS one. Indeed, it's not clear what's happening and is definitely specific to this PR's changes.

For clarity, I reran the CI workflow on main and it succeeded

@BPerlakiH
Copy link
Collaborator Author

@BPerlakiH your marchant ID commit 3bee29b fixed the iOS build but not the macOS one. Indeed, it's not clear what's happening and is definitely specific to this PR's changes.

For clarity, I reran the CI workflow on main and it succeeded

@rgaudin
I think the reason behind it is that the merchant certificate for Apple Pay is iOS only, can we either extend it to be all platform or create another one for macOS ?

@rgaudin
Copy link
Member

rgaudin commented Nov 6, 2024

OK, I think we're not aligned with what's in this PR. My understanding (@kelson42 please speak up if I'm mistaken) is that this PR should introduce donations on the App (via Apple Pay).

So when merging this, we would have a working donation UI that results in actual donated money on our Stripe account.

Unless I misunderstood, that's not where we're going.

  • I believe we must follow Stripe's documentation which entails creating a PaymentIntent on Stripe before asking ApplePay to process it.
  • That PaymentIntent will require a Stripe API Key. Here's the test-mode one: pk_test_oglo2v3Wc7ibH2oQe5oUDkhi. You can use Stripe test cards in Stripe-mode but for ApplePay, you must use a real card (wont be charged as long as using the test API key)
  • It seems quite clear to me that Stripe+ApplePay is only available on iOS. For macOS, the only option is web via Safari AFAICT. In this regard, we should simply open the donation URL on the browser.
  • Stripe doc suggest the paymentIntent to be created from a secure location (a new endpoint on one of our server) so that we can control the amount/currency requested. Given this is not a purchase but a donation and we allow users to select the amount, it sounds like unnecessary complications ATM

Obviously this is from reading Stripe's doc and I have no prior ApplePay experience so there might be other ways.

@BPerlakiH please let me know what you think of this

@BPerlakiH
Copy link
Collaborator Author

BPerlakiH commented Nov 7, 2024

Yes, I agree, from what I can see there's a couple of issues to be taken care of.

  1. we need the stripe integration in order to actually charge the given card. I tested it with the release build of iOS, and I could add my card to Apple Pay, and use it, but it's not charged, since it's not connecting to stripe (the payment provider).
  2. the stripe doc suggest that the for usual e-commerce the price / order tracking should be secured, for us it's not that important, so possibly the back-end integration can be omitted.
  3. the build issue remains for macOS. I tried to manually add the Apple Pay capability to the project (not via Xcodegen), and the problem is that I couldn't find a way to filter it down to iOS only as it supports all platforms:
Screenshot 2024-11-07 at 09 40 01
Screenshot 2024-11-07 at 09 38 31

@rgaudin
Copy link
Member

rgaudin commented Nov 7, 2024

From what I've seen online, we should be able to remove the entitlement/capability from project and add the merchantID to the Stripe dep config. Have you tried?

// async http call...
Task { [resultHandler] in

let paymentServer = StripeKiwix(endPoint: URL(string: "http://192.168.100.7:4242")!,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the test server url.

@BPerlakiH BPerlakiH force-pushed the 753-implement-apple-pay-and-use-the-donate-with-apple-pay-button-for-support-kiwix branch from 0f19c56 to 33d57a4 Compare November 22, 2024 11:28
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.

Implement Apple Pay and use the “Donate with Apple Pay” button for "Support Kiwix"
4 participants