-
Notifications
You must be signed in to change notification settings - Fork 296
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
Fix Bug where fetchPaymentMethodNonces
Return Incorrect Type
#1100
Changes from all commits
28e36d2
9319995
a9a4057
c821e2e
a086353
6cbd5ed
1d58414
0310ecb
58da865
9cee196
4b89b34
c36f238
388c36a
bf0a676
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,18 +53,80 @@ import Foundation | |
return nil | ||
} | ||
|
||
if let completionHandler = completionHandler { | ||
if let completionHandler { | ||
return completionHandler(json) | ||
} | ||
|
||
if json?["nonce"].isString != false { | ||
if json?["nonce"].isString == false { | ||
return nil | ||
} | ||
|
||
let type = json?["type"].asString() | ||
|
||
if type == "CreditCard", let cardType = json?["details"]["cardType"].asString() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The below logic mirrors the behavior of iOS v5 with regards to the type. Though this is with the constraints of not being able to cast to unknown nonce types in Swift. The load method used previously was a very hacky workaround and does not map to Swift - the method ( |
||
return BTPaymentMethodNonce( | ||
nonce: json?["nonce"].asString() ?? "", | ||
type: self.cardType(from: cardType), | ||
isDefault: json?["default"].isTrue ?? false | ||
) | ||
} else if type == "ApplePayCard" { | ||
return BTPaymentMethodNonce( | ||
nonce: json?["nonce"].asString() ?? "", | ||
type: json?["details"]["cardType"].asString() ?? "ApplePayCard", | ||
isDefault: json?["default"].isTrue ?? false | ||
) | ||
} else if type == "PayPalAccount" { | ||
return BTPaymentMethodNonce( | ||
nonce: json?["nonce"].asString() ?? "", | ||
type: "PayPal", | ||
scannillo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isDefault: json?["default"].isTrue ?? false | ||
) | ||
} else if type == "VenmoAccount" { | ||
return BTPaymentMethodNonce( | ||
nonce: json?["nonce"].asString() ?? "", | ||
type: "Venmo", | ||
scannillo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isDefault: json?["default"].isTrue ?? false | ||
) | ||
} else { | ||
return BTPaymentMethodNonce( | ||
nonce: json?["nonce"].asString() ?? "", | ||
type: "Unknown", | ||
isDefault: json?["default"].isTrue ?? false | ||
) | ||
} | ||
} | ||
|
||
private func cardType(from cardType: String) -> String { | ||
let cardType = cardType.lowercased() | ||
|
||
if cardType == "american express" { | ||
return "AMEX" | ||
} else if cardType == "diners club" { | ||
return "DinersClub" | ||
} else if cardType == "unionpay" { | ||
return "UnionPay" | ||
} else if cardType == "discover" { | ||
return "Discover" | ||
} else if cardType == "mastercard" { | ||
return "MasterCard" | ||
} else if cardType == "jcb" { | ||
return "JCB" | ||
} else if cardType == "hiper" { | ||
return "Hiper" | ||
} else if cardType == "hipercard" { | ||
return "Hipercard" | ||
} else if cardType == "laser" { | ||
return "Laser" | ||
} else if cardType == "solo" { | ||
return "Solo" | ||
} else if cardType == "switch" { | ||
return "Switch" | ||
} else if cardType == "uk maestro" { | ||
return "UKMaestro" | ||
} else if cardType == "visa" { | ||
return "Visa" | ||
} | ||
|
||
return nil | ||
return "Unknown" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import XCTest | |
@testable import BraintreeCore | ||
|
||
class BTPaymentMethodNonceParser_Tests: XCTestCase { | ||
var parser : BTPaymentMethodNonceParser = BTPaymentMethodNonceParser() | ||
var parser: BTPaymentMethodNonceParser = BTPaymentMethodNonceParser() | ||
|
||
func testRegisterType_addsTypeToTypes() { | ||
parser.registerType("MyType") { _ -> BTPaymentMethodNonce? in return nil} | ||
|
@@ -67,4 +67,86 @@ class BTPaymentMethodNonceParser_Tests: XCTestCase { | |
XCTAssertEqual(unknownNonce.type, "Unknown") | ||
XCTAssertTrue(unknownNonce.isDefault) | ||
} | ||
|
||
func testSharedParser_whenTypeIsApplePayCard_returnsApplePayType() { | ||
let sharedParser = BTPaymentMethodNonceParser.shared | ||
let applePayCard = BTJSON(value: [ | ||
"consumed": false, | ||
"details": ["cardType": "American Express"], | ||
"isLocked": false, | ||
"nonce": "a-nonce", | ||
"securityQuestions": [] as [Any], | ||
"type": "ApplePayCard", | ||
] as [String: Any]) | ||
|
||
let applePayCardNonce = sharedParser.parseJSON(applePayCard, withParsingBlockForType: "ApplePayCard") | ||
|
||
XCTAssertEqual(applePayCardNonce?.nonce, "a-nonce") | ||
XCTAssertEqual(applePayCardNonce?.type, "American Express") | ||
} | ||
|
||
func testSharedParser_whenTypeIsCreditCard_returnsCardType() { | ||
let sharedParser = BTPaymentMethodNonceParser.shared | ||
|
||
let creditCardJSON = BTJSON(value: [ | ||
"consumed": false, | ||
"description": "ending in 31", | ||
"details": [ | ||
"cardType": "American Express", | ||
"lastTwo": "31", | ||
], | ||
"isLocked": false, | ||
"nonce": "0099b1d0-7a1c-44c3-b1e4-297082290bb9", | ||
"securityQuestions": ["cvv"], | ||
"threeDSecureInfo": NSNull(), | ||
"type": "CreditCard", | ||
"default": true | ||
] as [String: Any]) | ||
|
||
let cardNonce = sharedParser.parseJSON(creditCardJSON, withParsingBlockForType:"CreditCard")! | ||
|
||
XCTAssertEqual(cardNonce.nonce, "0099b1d0-7a1c-44c3-b1e4-297082290bb9") | ||
XCTAssertEqual(cardNonce.type, "AMEX") | ||
XCTAssertTrue(cardNonce.isDefault) | ||
} | ||
|
||
func testSharedParser_whenTypeIsVenmo_returnsVenmoType() { | ||
let sharedParser = BTPaymentMethodNonceParser.shared | ||
|
||
let venmoAccountJSON = BTJSON(value: [ | ||
"consumed": false, | ||
"description": "VenmoAccount", | ||
"details": ["username": "[email protected]"], | ||
"isLocked": false, | ||
"nonce": "a-nonce", | ||
"securityQuestions": [] as [Any], | ||
"type": "VenmoAccount", | ||
"default": true | ||
] as [String: Any]) | ||
|
||
let venmoAccountNonce = sharedParser.parseJSON(venmoAccountJSON, withParsingBlockForType: "VenmoAccount") | ||
|
||
XCTAssertEqual(venmoAccountNonce?.nonce, "a-nonce") | ||
XCTAssertEqual(venmoAccountNonce?.type, "Venmo") | ||
} | ||
|
||
func testSharedParser_whenTypeIsPayPal_returnsPayPalType() { | ||
let sharedParser = BTPaymentMethodNonceParser.shared | ||
|
||
let payPalAccountJSON = BTJSON(value: [ | ||
"consumed": false, | ||
"description": "[email protected]", | ||
"details": ["email": "[email protected]"], | ||
"isLocked": false, | ||
"nonce": "a-nonce", | ||
"securityQuestions": [] as [Any], | ||
"type": "PayPalAccount", | ||
"default": true | ||
] as [String: Any]) | ||
|
||
let payPalAccountNonce = sharedParser.parseJSON(payPalAccountJSON, withParsingBlockForType: "PayPalAccount") | ||
|
||
XCTAssertEqual(payPalAccountNonce?.nonce, "a-nonce") | ||
XCTAssertEqual(payPalAccountNonce?.type, "PayPal") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,9 @@ | ||
import XCTest | ||
@testable import BraintreePayPal | ||
|
||
class BTPaymentMethodNonceParser_PayPal_Tests: XCTestCase { | ||
func testSharedParser_whenTypeIsPayPal_returnsPayPalAccountNonce() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved the parser specific test |
||
let payPalAccountNonce = BTPayPalAccountNonce( | ||
json: BTJSON( | ||
value: [ | ||
"consumed": false, | ||
"description": "[email protected]", | ||
"details": [ | ||
"email": "[email protected]", | ||
], | ||
"isLocked": false, | ||
"nonce": "a-nonce", | ||
"securityQuestions": [] as [Any?], | ||
"type": "PayPalAccount", | ||
"default": true | ||
] as [String: Any] | ||
) | ||
) | ||
|
||
XCTAssertEqual(payPalAccountNonce?.nonce, "a-nonce") | ||
XCTAssertEqual(payPalAccountNonce?.type, "PayPal") | ||
XCTAssertEqual(payPalAccountNonce?.email, "[email protected]") | ||
XCTAssertTrue(payPalAccountNonce!.isDefault) | ||
XCTAssertNil(payPalAccountNonce?.creditFinancing) | ||
} | ||
final class BTPayPalAccountNonce_Tests: XCTestCase { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed this class and the tests since they were not actually testing the shared parser and the test names were not accurate. |
||
|
||
func testParsePayPalCreditFinancingAmount() { | ||
func testPayPalAccountNonce_returnsPayPalCreditFinancingAmount() { | ||
let payPalCreditFinancingAmount = BTJSON(value: [ | ||
"currency": "USD", | ||
"value": "123.45", | ||
|
@@ -41,7 +17,7 @@ class BTPaymentMethodNonceParser_PayPal_Tests: XCTestCase { | |
XCTAssertEqual(amount.value, "123.45") | ||
} | ||
|
||
func testParsePayPalCreditFinancing() { | ||
func testPayPalAccountNonce_returnsPayPalCreditFinancing() { | ||
let payPalCreditFinancing = BTJSON(value: [ | ||
"cardAmountImmutable": false, | ||
"monthlyPayment": [ | ||
|
@@ -93,7 +69,7 @@ class BTPaymentMethodNonceParser_PayPal_Tests: XCTestCase { | |
XCTAssertEqual(totalInterest.value, "456.78") | ||
} | ||
|
||
func testSharedParser_whenTypeIsPayPal_returnsPayPalAccountNonceWithCreditFinancingOffered() { | ||
func testPayPalAccountNonce_returnsPayPalAccountNonceWithCreditFinancingOffered() { | ||
let payPalAccountNonce = BTPayPalAccountNonce( | ||
json: BTJSON( | ||
value: [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've actually gotten multiple requests to move this back into core. We initially moved it to DropIn to get rid of circular dependencies, and we're planning to implement it the correct way in
v5
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting... we may have to rethink what we want to return to merchants if we move it back to core. Not sure if Android is capable of what we were doing in v5. We were using the
load
method to dynamically register types that don't exist in core (BTCardNonce
/PayPalNonce
/'VenmoNonce/
ApplePayNonce`) at compile time. We don't have the ability to do that hacky workaround in Swift and importing those modules into Core would cause a circular dependency. How are y'all planning to correctly implement it in v5?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was a hack in Android
v3
as well. We usedcompileOnly
in our Gradle files to basically create weak references in the dependency tree. That allowed Gradle to build successfully, but it caused some issues in the long run.By "correct" I'm thinking "in a non-hacky way." It's tough because there will probably be tradeoffs. An enum feels like the right way to me, but I'd be up to do some cross-platform pairing on this since we need a solution on Android too.