[v7] Make APIClient Non-optional & Update BTAmericanExpressClient Init
#1527
Conversation
jaxdesmarais
changed the title
BTAmexClientBTAmericanExpressClient Init
| @@ -3,7 +3,9 @@ import BraintreeCore | |||
|
|
|||
| class PaymentButtonBaseViewController: BaseViewController { | |||
|
|
|||
| // TODO: remove API client in final PR | |||
There was a problem hiding this comment.
Added some TODOs throughout this PR to update in our final PR for this work
| } catch { | ||
| completion(nil, error) | ||
|
|
||
| if authorization.type == .invalidAuthorization { |
There was a problem hiding this comment.
On android we do this check in each request (POST, GET, etc). Since fetchOrReturnRemoteConfiguration is the first API call in our SDK it made sense to me to throw the error here instead vs adding to each of the HTTP/GraphQLHTTP methods. This will allow us to return an error before making any API calls in the SDK if the auth is invalid.
There was a problem hiding this comment.
We could miss something by nesting it within fetchOrReturnRemoteConfiguration. Actually, this PR as-is misses doing auth string validation for shopperInsightsClient.getRecommendedPaymentMethods() calls, since it hits apiClient.post() directly.
There was a problem hiding this comment.
IMO it will be better to call the auth string checks explicitly, vs relying on fetchOrReturnRemoteConfiguration(), which is already kind of confusing
There was a problem hiding this comment.
Ah, good catch, yes we do miss that currently. By call the auth string checks explicitly what do you mean? We want to make the network request in ignore mefetchOrReturnRemoteConfiguration before returning an error and move these to the get/post/etc requests?
There was a problem hiding this comment.
Ah, nvm we can still do them before that network call. Updated here: 15bed08 and checked that getRecommendedPaymentMethods would hit that block as well.
|
UI tests are expected to fail currently, we have ticket DTMOBILES-1372 to address those failures. |
| let type = AuthorizationType.invalidAuthorization | ||
| let configURL: URL | ||
| let bearer: String | ||
| let originalValue: String |
There was a problem hiding this comment.
None of these values are actually going to be used / we're just setting these properties to conform to the protocol but never using them functionally.
I crossed this bridge when messing around with this in this branch. I wonder if there's a cleaner pattern.
There was a problem hiding this comment.
Potentially but I do think there is some value in enforcing them for the valid auth types. Since they won't be used it seems harmless. We also do something similar on Android.
| if authorization.type == .invalidAuthorization { | ||
| completion(nil, BTAPIClientError.invalidAuthorization(authorization.originalValue)) | ||
| } else { | ||
| Task { @MainActor in |
There was a problem hiding this comment.
Can you explain why we're dispatching to MainActor in our network layer? I feel like that should be up to the merchant when they get our async result from tokenize() with either an error or success
There was a problem hiding this comment.
This is not a change in code, it's just moving this logic into an else. But our code expects fetch or return config to be run on the main thread according to this test:
Co-authored-by: scannillo <[email protected]>
…raintree/braintree_ios into update-apiClient-init-and-amex
Co-authored-by: Rich Herrera <[email protected]>
Summary of changes
BTAmericanExpressClient(authorization:)InvalidAuthorizationtype - this matches the pattern used on AndroidBTAPIClientinitializer to require an authorization stringBTAPIClientError.invalidAuthorizationPaymentButtonBaseViewControllerwith new authorization stringAmexViewControllerdemo app with new initializerTODOs for final PR cleanupChecklist
Authors