Make SDK Errors Public #1159
Make SDK Errors Public #1159
Conversation
| // MARK: - Equatable Conformance | ||
|
|
||
| public static func == (lhs: BTCardError, rhs: BTCardError) -> Bool { | ||
| lhs.errorCode == rhs.errorCode | ||
| } |
There was a problem hiding this comment.
This is required for any of the enums with associated types. It it's able to resolve the equality of the error codes automatically.
| @@ -66,7 +66,7 @@ class VenmoViewController: PaymentButtonBaseViewController { | |||
| progressBlock("Got a nonce 💎!") | |||
| completionBlock(venmoAccount) | |||
| } catch { | |||
| if (error as NSError).code == 10 { | |||
| if error as? BTVenmoError == .canceled { | |||
There was a problem hiding this comment.
This is slick! Much improved over forcing merchants to look at our markdown list of codes and hard code the int values
There was a problem hiding this comment.
🏈 Punt, but curious your thoughts.
For V7, we could get to something like this if error == .canceled { ... } if the error type we return is BT SDK specific. Is that what you're thinking as the V7 goal?
There was a problem hiding this comment.
Yeah - I think that would be dope if we return strongly typed errors. The only case it could be a little weird is we would need to wrap any errors from core. I think in most cases we do that anyways but I know there are a few instances where we return a core error directly. Though I don't think it should be a blocker for strongly typed errors.
| errorCode: BTThreeDSecureError.canceled.errorCode, | ||
| completion: completionHandler | ||
| ) | ||
| completionHandler(nil, BTThreeDSecureError.canceled) |
There was a problem hiding this comment.
This refactor pointed out a flaw in this logic. As we weren't actually sending the canceled error here and instead constructing an error similar to cancel, merchants would need to check for both the code and the raw canceled error. Instead above we can construct the NSError for the error case, but should have been sending our cancel error here. Without this updated logic we actually don't fire our cancel analytics event in BTThreeDSecureClient.
There was a problem hiding this comment.
Good catch! I actually think that resolves this ticket too (DTBTSDK-3276). I also noticed we don't have a proper unit test for the cancel case from cardinal, which I included in that ticket. Maybe we update that ticket to be unit test specific since your PR here fixes the bug?
There was a problem hiding this comment.
Ah - nice! Yeah this should resolve that issue. I think we can make the ticket you created test specific and include this fix here. Is it worth calling out as a separate CHANGELOG point?
There was a problem hiding this comment.
is there any chance we can include the test coverage work in this PR, since we're making the change now? I'm really leery about skipping tests for another day...
There was a problem hiding this comment.
I poked around at testing this and it actually looks like nothing in this method is tested. Mainly because it involves testing CardinalValidationDelegate which uses a class with no inits CardinalSession. I can't figure out a good way to test this closed class CardinalSession which is required for a mock of this class. @scannillo not sure if you have any other ideas on testing this but imo it seems like it'd be a really large initiative outside of the scope of this PR.
|
|
||
| /// 0. Invalid API client | ||
| case invalidAPIClient | ||
|
|
||
| static var errorDomain: String { | ||
| public static var errorDomain: String { |
There was a problem hiding this comment.
Do all the enum properties need to be public to resolve the merchant casting issue? errorDomain feels internal (but maybe it's a swifty thing).
There was a problem hiding this comment.
Yeah - it needs to be public to satisfy protocol requirements since we are making the enum as a whole public.
There was a problem hiding this comment.
we need a taylor swift emoji for "swifty" things 😄
There was a problem hiding this comment.
Love this work! Small tweak to CHANGELOG and other general thoughts.
If we're changing code, we should incorporate test coverage into those changes, too. We absolutely can and should take the time to do the right thing:TM:. I have no problem having higher level conversations if we get pushback about this.
|
|
||
| /// 0. Invalid API client | ||
| case invalidAPIClient | ||
|
|
||
| static var errorDomain: String { | ||
| public static var errorDomain: String { |
There was a problem hiding this comment.
we need a taylor swift emoji for "swifty" things 😄
| errorCode: BTThreeDSecureError.canceled.errorCode, | ||
| completion: completionHandler | ||
| ) | ||
| completionHandler(nil, BTThreeDSecureError.canceled) |
There was a problem hiding this comment.
is there any chance we can include the test coverage work in this PR, since we're making the change now? I'm really leery about skipping tests for another day...
Co-authored-by: Holly Richko <[email protected]>
|
Leaving testing for a future PR as it involves wrapping a final Cardinal class without an init. None of the Cardinal delegate is tested at all so figuring out how/if we test it should be a separate workstream. |
Summary of changes
NSErrorand can do error comparison in a more Swift-y way (see Demo app ViewController updates as an example)Checklist
Authors