Add BTVenmoError.canceled case

[scannillo]

Summary of changes

Fixes #1085. Previously, we returned a completion of (nil, nil) when the Venmo flow is canceled by the user. This results in a no-op for merchants using our async-await function wrappers.

This PR aligns with how the SEPA & PayPal modules handle cancelation cases, by returning an error to the merchant (either via the completion, or throwing if using async-await).

• Add a .canceled error case to BTVenmoError
• Clarify docstring
• Update SDK_ERROR_CODES.md

Checklist

• Added a changelog entry

Authors

@scannillo

[scannillo]

CI is having a field day

[sshropshire]

nit: I know we can reference the error codes markdown, but this code would read better if there was a static constant used in place of 10.

[scannillo]

That's a good call. And on top of that, I think it should be easier for a merchant to check against our error codes (a merchant asked about it in #1080) versus needing them to check our SDK_ERROR_CODES.md file list.

Something like : if error.code == BTVenmoError.cancel.code { // do things } would be nice to expose for merchants.

@jaxdesmarais What do you think? Should we ticket this work?

[jaxdesmarais]

Yeah, I think I would still lean towards exposing the errors as a whole publicly for merchants using Swift. Since many of our errors use associated types, we can't expose those to Obj-C publicly as only Int type enums can be exposed.

If we wanted to expose them publicly for Obj-C merchants we would need to move the error codes into their own enum without associated types and construct the error messages at the callsite or in a method where we can include the details we are currently including with the associated type when constructing the localizedDescription.

[scannillo]

Yeh - there are ways to do it without having to move the error creation to the callsite, though it makes our internal <Feature>Error.swift files a little uglier. Something like this:

enum BTAmericanExpressError: Int, Error, CustomNSError, LocalizedError {

    case unknown

    case noRewardsData

    case deallocated

    static var errorDomain: String {
        "com.braintreepayments.BTAmericanExpressErrorDomain"
    }
    
    var errorCode: Int {
        switch self {
        case .unknown:
            return BTAmericanExpressErrorCode.unknown.rawValue
        case .noRewardsData:
            return BTAmericanExpressErrorCode.noRewardsData.rawValue
        case .deallocated:
            return BTAmericanExpressErrorCode.deallocated.rawValue
        }
    }
    
    var errorDescription: String? {
        switch self {
        case .unknown:
            return "An unknown error occurred. Please contact support."

        case .noRewardsData:
            return "No American Express Rewards data was returned. Please contact support."

        case .deallocated:
            return "BTAmericanExpressClient has been deallocated."
        }
    }
}


/// Public for merchants to reference (accessible in both Swift & ObjC)
@objc public enum BTAmericanExpressErrorCode: Int {
    
    case unknown = 0
    case noRewardsData = 1
    case deallocated = 2
}

There might even be a slicker way to do that switch statement, but something like this would solve the issue

[jaxdesmarais]

Ah, yeah I hadn't thought about just extracting the code portion.

Yeah, I guess the question in that case would be the merchant experience. With the above the callsite would look something like:

if (error as? NSError)?.code == BTAmericanExpressErrorCode.noRewardsData.rawValue {
    // do something for this specific error
}

vs making the enum public the callsite could look something like:

switch error as? BTAmericanExpressError {
case . noRewardsData:
    // do something for this specific error
default:
    // do something else for other errors
}

I think you could technically use a switch for the first but it'd be something like switch ((error as? NSError)?.code as? BTJSONErrorCode). It really comes down to how we want to support Obj-C vs Swift first in the future, which is up to us! We can certainly play around with some other options and see what feels best or engage the merchant that opened the issue to see what their ideal world would be.

[scannillo]

Thats a great point. I added this convo to our "iOS Minor Version Improvements" doc!

[sshropshire]

q: is this a breaking change?

[scannillo]

Nope, since we are only adding an error code & not changing existing ones we didn't think this was breaking

[scannillo]

The docs for the async method also needs this update!

[jaxdesmarais]

CI is having a field day

Should be fixed now! See PR: #1089