Skip to content
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

Card: Remove Approve Order Listener Pattern #305

Merged
merged 18 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
## unreleased
* CorePayments
* Fix issue that causes analytics version number to always be `null`
* Breaking Changes
* CardPayments
* Remove `ApproveOrderListener` type
* Add `CardApproveOrderCallback` type
* Add `CardApproveOrderResult` type
* Remove `CardClient.approveOrder(CardRequest)` method
* Add `CardClient.approveOrder(CardRequest, CardApproveOrderCallback)` method
* Add `CardClient.finishApproveOrder(Intent, String)` method
* Add `CardFinishApproveOrderResult` type
* Remove `CardResult` type

## 2.0.0-beta1 (2024-11-20)
* Breaking Changes
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.paypal.android.cardpayments

import androidx.annotation.MainThread

fun interface CardApproveOrderCallback {
/**
* Called when the order is approved.
*
* @param result [CardApproveOrderResult] result with details
*/
@MainThread
fun onCardApproveOrderResult(result: CardApproveOrderResult)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. I don't think it's possible to do this in iOS with protocols.
Do you feel like this is something merchant would want us to handle?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we need to deliver results on the main thread, but it is convenient when updating the UI after executing a transaction.

It's up to y'all if you want to do the same. I feel like the closest equivalent to this annotation in Swift would be @MainActor.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.paypal.android.cardpayments

import com.paypal.android.corepayments.PayPalSDKError

sealed class CardApproveOrderResult {

data class Success(
val orderId: String,
val status: String? = null,
val didAttemptThreeDSecureAuthentication: Boolean = false
) : CardApproveOrderResult()

data class AuthorizationRequired(val authChallenge: CardAuthChallenge) : CardApproveOrderResult()
data class Failure(val error: PayPalSDKError) : CardApproveOrderResult()
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.braintreepayments.api.BrowserSwitchFinalResult
import com.braintreepayments.api.BrowserSwitchOptions
import com.braintreepayments.api.BrowserSwitchStartResult
import com.paypal.android.corepayments.BrowserSwitchRequestCodes
import com.paypal.android.corepayments.PayPalSDKError
import org.json.JSONObject

internal class CardAuthLauncher(
Expand Down Expand Up @@ -60,6 +61,25 @@ internal class CardAuthLauncher(
}
}

fun completeApproveOrderAuthRequest(
intent: Intent,
authState: String
): CardFinishApproveOrderResult =
when (val finalResult = browserSwitchClient.completeRequest(intent, authState)) {
is BrowserSwitchFinalResult.Success -> parseApproveOrderSuccessResult(finalResult)

is BrowserSwitchFinalResult.Failure -> {
// TODO: remove error codes and error description from project; the built in
// Throwable type already has a message property and error codes are only required
// for iOS Error protocol conformance
val message = "Browser switch failed"
val browserSwitchError = PayPalSDKError(0, message, reason = finalResult.error)
CardFinishApproveOrderResult.Failure(browserSwitchError)
}

BrowserSwitchFinalResult.NoResult -> CardFinishApproveOrderResult.NoResult
}

fun completeAuthRequest(intent: Intent, authState: String): CardStatus =
when (val finalResult = browserSwitchClient.completeRequest(intent, authState)) {
is BrowserSwitchFinalResult.Success -> parseBrowserSwitchSuccessResult(finalResult)
Expand All @@ -69,7 +89,6 @@ internal class CardAuthLauncher(

private fun parseBrowserSwitchSuccessResult(result: BrowserSwitchFinalResult.Success): CardStatus =
when (result.requestCode) {
BrowserSwitchRequestCodes.CARD_APPROVE_ORDER -> parseApproveOrderSuccessResult(result)
BrowserSwitchRequestCodes.CARD_VAULT -> parseVaultSuccessResult(result)
else -> CardStatus.NoResult
}
Expand All @@ -88,14 +107,19 @@ internal class CardAuthLauncher(
}

private fun parseApproveOrderSuccessResult(
finalResult: BrowserSwitchFinalResult.Success,
): CardStatus {
val orderId = finalResult.requestMetadata?.optString(METADATA_KEY_ORDER_ID)
return if (orderId == null) {
CardStatus.ApproveOrderError(CardError.unknownError, null)
finalResult: BrowserSwitchFinalResult.Success
): CardFinishApproveOrderResult =
if (finalResult.requestCode == BrowserSwitchRequestCodes.CARD_APPROVE_ORDER) {
val orderId = finalResult.requestMetadata?.optString(METADATA_KEY_ORDER_ID)
if (orderId == null) {
CardFinishApproveOrderResult.Failure(CardError.unknownError)
} else {
CardFinishApproveOrderResult.Success(
orderId = orderId,
didAttemptThreeDSecureAuthentication = true
)
}
} else {
val result = CardResult(orderId = orderId, didAttemptThreeDSecureAuthentication = true)
CardStatus.ApproveOrderSuccess(result)
CardFinishApproveOrderResult.NoResult
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import kotlinx.coroutines.launch
/**
* Use this client to approve an order with a [Card].
*
* @property approveOrderListener listener to receive callbacks from [CardClient.approveOrder].
* @property cardVaultListener listener to receive callbacks form [CardClient.vault].
*/
class CardClient internal constructor(
Expand All @@ -29,8 +28,6 @@ class CardClient internal constructor(
private val dispatcher: CoroutineDispatcher
) {

var approveOrderListener: ApproveOrderListener? = null

// NEXT MAJOR VERSION: rename to vaultListener
/**
* @suppress
Expand All @@ -54,12 +51,14 @@ class CardClient internal constructor(
)

// NEXT MAJOR VERSION: Consider renaming approveOrder() to confirmPaymentSource()

/**
* Confirm [Card] payment source for an order.
*
* @param cardRequest [CardRequest] for requesting an order approval
* @param callback [CardApproveOrderCallback] callback for receiving result asynchronously
*/
fun approveOrder(cardRequest: CardRequest) {
fun approveOrder(cardRequest: CardRequest, callback: CardApproveOrderCallback) {
// TODO: deprecate this method and offer auth challenge integration pattern (similar to vault)
approveOrderId = cardRequest.orderId
analytics.notifyApproveOrderStarted(cardRequest.orderId)
Expand All @@ -69,22 +68,27 @@ class CardClient internal constructor(
is ConfirmPaymentSourceResult.Success -> {
if (response.payerActionHref == null) {
Copy link
Collaborator

@KunJeongPark KunJeongPark Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this syntax looks really similar to completion handler. So callback in place of listener pattern is safer for memory management?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's similar. The main difference now is there's a callback for every async method. With the listener pattern, there is only one listener, and it needs to be set by the merchant before starting any of the payment flows.

analytics.notifyApproveOrderSucceeded(response.orderId)
val result =
response.run { CardResult(orderId = orderId, status = status?.name) }
approveOrderListener?.onApproveOrderSuccess(result)
val result = response.run {
CardApproveOrderResult.Success(
orderId = orderId,
status = status?.name
)
}
callback.onCardApproveOrderResult(result)
} else {
analytics.notifyApproveOrderAuthChallengeReceived(cardRequest.orderId)
approveOrderListener?.onApproveOrderThreeDSecureWillLaunch()

val url = Uri.parse(response.payerActionHref)
val authChallenge = CardAuthChallenge.ApproveOrder(url, cardRequest)
approveOrderListener?.onApproveOrderAuthorizationRequired(authChallenge)
val result = CardApproveOrderResult.AuthorizationRequired(authChallenge)
Copy link
Collaborator

@KunJeongPark KunJeongPark Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting that you have this intermediate state on the same result type.
So there is result type for API call and final result type for the merchant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does have three possible results. Alternatively, I thought about bundling the payer_action_required status into the Failure case but this felt more natural.

Technically it isn't a failure. PPaaS just needs more information before the buyer can continue.

callback.onCardApproveOrderResult(result)
}
}

is ConfirmPaymentSourceResult.Failure -> {
analytics.notifyApproveOrderFailed(cardRequest.orderId)
approveOrderListener?.onApproveOrderFailure(response.error)
val result = CardApproveOrderResult.Failure(response.error)
callback.onCardApproveOrderResult(result)
}
}
}
Expand Down Expand Up @@ -134,6 +138,9 @@ class CardClient internal constructor(

/**
* Present an auth challenge received from a [CardClient.approveOrder] or [CardClient.vault] result.
* @param activity [ComponentActivity] activity reference used to present a Chrome Custom Tab.
* @param authChallenge [CardAuthChallenge] auth challenge to present
* (see [CardApproveOrderResult.AuthorizationRequired])
*/
fun presentAuthChallenge(
activity: ComponentActivity,
Expand Down Expand Up @@ -173,6 +180,25 @@ class CardClient internal constructor(
}
}

fun finishApproveOrder(intent: Intent, authState: String): CardFinishApproveOrderResult {
val result = authChallengeLauncher.completeApproveOrderAuthRequest(intent, authState)
when (result) {
is CardFinishApproveOrderResult.Success ->
analytics.notifyApproveOrderAuthChallengeSucceeded(result.orderId)

is CardFinishApproveOrderResult.Failure ->
analytics.notifyApproveOrderAuthChallengeFailed(null)

CardFinishApproveOrderResult.Canceled ->
analytics.notifyApproveOrderAuthChallengeCanceled(null)

else -> {
// no analytics tracking required at the moment
}
}
return result
}

fun completeAuthChallenge(intent: Intent, authState: String): CardStatus {
val status = authChallengeLauncher.completeAuthRequest(intent, authState)
when (status) {
Expand All @@ -194,26 +220,11 @@ class CardClient internal constructor(
cardVaultListener?.onVaultFailure(PayPalSDKError(1, "User Canceled"))
}

is CardStatus.ApproveOrderError -> {
analytics.notifyApproveOrderAuthChallengeFailed(status.orderId)
approveOrderListener?.onApproveOrderFailure(status.error)
}

is CardStatus.ApproveOrderSuccess -> {
analytics.notifyApproveOrderAuthChallengeSucceeded(status.result.orderId)
approveOrderListener?.onApproveOrderSuccess(status.result)
}

is CardStatus.ApproveOrderCanceled -> {
analytics.notifyApproveOrderAuthChallengeCanceled(status.orderId)
approveOrderListener?.onApproveOrderCanceled()
}

is CardStatus.UnknownError -> {
Log.d("PayPalSDK", "An unknown error occurred: ${status.error.message}")
}

CardStatus.NoResult -> {
else -> {
// ignore
}
}
Expand All @@ -224,7 +235,6 @@ class CardClient internal constructor(
* Call this method at the end of the card flow to clear out all observers and listeners
*/
fun removeObservers() {
approveOrderListener = null
cardVaultListener = null
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.paypal.android.cardpayments

import com.paypal.android.corepayments.PayPalSDKError

sealed class CardFinishApproveOrderResult {
data class Success(
val orderId: String,
val status: String? = null,
val didAttemptThreeDSecureAuthentication: Boolean = false
) : CardFinishApproveOrderResult()

data class Failure(val error: PayPalSDKError) : CardFinishApproveOrderResult()
data object Canceled : CardFinishApproveOrderResult()
data object NoResult : CardFinishApproveOrderResult()
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ import com.paypal.android.corepayments.PayPalSDKError

sealed class CardStatus {

class ApproveOrderError(val error: PayPalSDKError, val orderId: String?) : CardStatus()
class ApproveOrderSuccess(val result: CardResult) : CardStatus()
class ApproveOrderCanceled(val orderId: String?) : CardStatus()

class VaultError(val error: PayPalSDKError) : CardStatus()
class VaultSuccess(val result: CardVaultResult) : CardStatus()
class VaultCanceled(val setupTokenId: String?) : CardStatus()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ class CardAuthLauncherUnitTest {
}

@Test
fun `completeAuthRequest() returns approve order success`() {
fun `completeApproveOrderAuthRequest() returns approve order success`() {
sut = CardAuthLauncher(browserSwitchClient)

val scheme = "com.paypal.android.demo"
Expand All @@ -164,13 +164,12 @@ class CardAuthLauncherUnitTest {
browserSwitchClient.completeRequest(intent, "pending request")
} returns finalResult

val status = sut.completeAuthRequest(intent, "pending request")
as CardStatus.ApproveOrderSuccess
val result = sut.completeApproveOrderAuthRequest(intent, "pending request")
as CardFinishApproveOrderResult.Success

val cardResult = status.result
assertEquals("fake-order-id", cardResult.orderId)
assertTrue(cardResult.didAttemptThreeDSecureAuthentication)
assertNull(cardResult.status)
assertEquals("fake-order-id", result.orderId)
assertTrue(result.didAttemptThreeDSecureAuthentication)
assertNull(result.status)
}

@Test
Expand Down
Loading
Loading