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

Determine "Apple Pay capable" based on available card types from the project #805

Merged
merged 18 commits into from
Sep 3, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Aug 19, 2019

📲 What

Previously, we were determining Apple Pay capabilities from hardcoded logic. This PR updates that logic to instead use the availableCardTypes field on the project, and also adds the business logic of showing/hiding the Apple Pay button in the new PledgePaymentMethodsViewController. The logic for determining supported networks is shared, and is now fully tested.

🤔 Why

Part of Apple Pay implementation on the new Pledge screen.

🛠 How

We now have a new availableCardTypes field on the Project model. We can use this array to determine which supportedNetworks to request from the method PassKit function PKPaymentAuthorizationViewController.canMakePayments(usingNetworks:). There is still fallback logic which mimics the backend logic, in case availableCardTypes is nil.

✅ Acceptance criteria

  • TBD

⏰ TODO

  • IMPORTANT we're still waiting for the backend to update the availableCardType strings that are being returned to match the GraphQL CreditCardType schema names so this feature will work

"VISA",
"DISCOVER",
"JCB",
"DINERS",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that DINERS is actually not a supportedNetwork according to Apple Pay, so it's filtered out.

@@ -4,6 +4,7 @@ import Prelude
import Runes

public struct Project {
public var availableCardTypes: [String]?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is optional because the project JSON object we get from the /discover endpoint doesn't have this field (it's only a partial project), and we use the same Project model to decode that payload.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could also consider coalescing an empty array if we wanted this to be non-optional 🤔 but I guess having nil here is more explicit about not having the information rather than there are no available card types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'd rather keep it nil to make it clear that the info is not available.

}

public static func supportedNetworks(for project: Project) -> [PKPaymentNetwork] {
guard let availableCardTypes = project.availableCardTypes else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

availableCardTypes should never be nil in this context, but decided to add a fallback just in case 🤷‍♀

@ksr-ci-bot
Copy link

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@ifbarrera ifbarrera added the blocked a PR that is blocked for external reasons label Aug 19, 2019
@ifbarrera ifbarrera marked this pull request as ready for review August 19, 2019 19:42
Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Looking good, minor comments

@@ -4,6 +4,7 @@ import Prelude
import Runes

public struct Project {
public var availableCardTypes: [String]?
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could also consider coalescing an empty array if we wanted this to be non-optional 🤔 but I guess having nil here is more explicit about not having the information rather than there are no available card types.

Library/PKPaymentAuthorizationViewController+Helpers.swift Outdated Show resolved Hide resolved
@justinswart justinswart added needs review and removed blocked a PR that is blocked for external reasons labels Aug 22, 2019
Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

One last question

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Nice lgtm!

@ifbarrera ifbarrera merged commit 27490b6 into master Sep 3, 2019
@ifbarrera ifbarrera deleted the apple-pay-capable branch September 3, 2019 20:58
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.

None yet

3 participants