Venmo Result Objects Refactor #828
Conversation
| handleError(authError); | ||
| return; | ||
| venmoClient.createPaymentAuthRequest(requireActivity(), venmoRequest, (paymentAuthRequest) -> { | ||
| if (paymentAuthRequest instanceof VenmoPaymentAuthRequest.Failure) { |
There was a problem hiding this comment.
Do we still think VenmoPaymentAuthRequest is the best naming or is it too wordy for the Java integration?
There was a problem hiding this comment.
I think it's fine unless you had ideas on another name, though we probably would want to update everything not just Venmo if so.
| * Callback for receiving result of | ||
| * {@link VenmoClient#tokenize(VenmoPaymentAuthResult, VenmoTokenizeCallback)}. | ||
| */ | ||
| interface VenmoInternalCallback { |
There was a problem hiding this comment.
Internal and external methods shared the same callback for delivering (nonce, exception) results - I opted to only refactor the public facing callbacks for now, so kept this one and renamed it to internal. Future code cleanup will be to migrate all internal code to use the single result object pattern too.
| */ | ||
| void onResult(boolean isReadyToPay, @Nullable Exception error); | ||
| void onVenmoReadinessResult(@NonNull VenmoReadinessResult venmoReadinessResult); |
There was a problem hiding this comment.
This PR changes all public callbacks to follow the result method naming pattern of on<ObjectBeingReturned>Result. It can provide some code clarity if merchants are creating multiple callbacks (rather than all having the same onResult method), but it also feels pretty verbose. What do you think?
There was a problem hiding this comment.
Yeah it feels a bit verbose but imo not too bad. We do have SEPADirectDebit all over so not sure anything can top that with verbosity 🙃
There was a problem hiding this comment.
In most cases I assume it'll be reduced to a lambda (venmoReadinessResult) -> {}. At the same time, it does offer flexibility to merchants who aren't using lambdas.
| /** | ||
| * There was an [error] creating the request | ||
| */ | ||
| class Failure(val error: Exception) : VenmoPaymentAuthRequest() |
There was a problem hiding this comment.
I'm not sure if Failure feels right for returning a Request object - it might give the impression that the request itself failed, but in this case it's that the creating of the request failed. Is this naming pattern clear or is there a better way?
| /** | ||
| * An [error] occurred when determining if the user is ready to pay with Venmo. | ||
| */ | ||
| class Failure(val error: Exception) : VenmoReadinessResult() |
There was a problem hiding this comment.
Do we prefer three result options here (explicitly ready, explicitly not ready, or error), or would it be better to combine not ready and failure? The failure is only if there's an error fetching the config so we can't determine readiness. In that case it would be ReadyToPay() or NotReadyToPay(error?) with an optional error.
Merchants likely will handle not ready and failure in the same way. But, they do have the option if a user is not ready to pay, to call showVenmoInCGooglePayStore. They might not want to call that on the failure case though.
There was a problem hiding this comment.
🤔 do we think merchants would call showVenmoInGooglePayStore with the result or when instantiating the GooglePayClient if they have a failure here? If it's with the GooglePayClient I would expect we could handle them both in 1 (ready and failure) result as they would want to parse the result in either case. That would give them time to set showVenmoInGooglePayStore in either case if desired.
But this is also assuming showVenmoInGooglePayStore is somewhere in the Google module and not here so I may be misunderstanding.
There was a problem hiding this comment.
Ah no showVenmoInGooglePlayStore is a method on VenmoClient (confusing but it's unrelated to Google Pay - it just brings the user to the Venmo app in the app store
There was a problem hiding this comment.
Oh! Lol that makes way more sense. Thanks for the explanation. 🙏
| /** | ||
| * Result of tokenizing a Venmo account | ||
| */ | ||
| sealed class VenmoResult { |
There was a problem hiding this comment.
I dropped the Payment from this result object naming - open to opinions if it should be VenmoPaymentResult instead
| /** | ||
| * A request used to launch the Venmo app for continuation of the Venmo payment flow. | ||
| */ | ||
| sealed class VenmoPaymentAuthRequest { |
There was a problem hiding this comment.
nit: There's some interesting interop cases between Kotlin and Java with sealed classes.
| assertEquals( | ||
| "Cannot collect customer data when ECD is disabled. Enable this feature in the Control Panel to collect this data.", | ||
| captor.getValue().getMessage()); | ||
| ((VenmoPaymentAuthRequest.Failure) paymentAuthRequest).getError().getMessage()); |
There was a problem hiding this comment.
Seeing now that the class interop works well in the Java unit test. I'd only heard about the interop before, never seen it in action. 👍
Summary of changes
VenmoClient#isReadyToPay->VenmoPaymentReadinessResultVenmoClient#createPaymentAuthRequest->VenmoPaymentAuthRequestVenmoClient#tokenize->VenmoResultThe naming schemes with the second two result objects will be used across modules so please weigh in if we want to make changes to those!
Checklist
Authors