From 67845e0aca85e63839887664ad29f56a719346bb Mon Sep 17 00:00:00 2001 From: Pedro Veloso Date: Fri, 19 Jul 2024 10:07:35 -0700 Subject: [PATCH] Reference id mixup fix (#147) * Correct distinction between reference_id and account_reference_id * Spotless apply * Make function changes be backwards compatible * Update CHANGELOG * Fix UTs --- CHANGELOG.md | 5 ++- .../java/app/cash/paykit/core/CashAppPay.kt | 11 +++++- .../app/cash/paykit/core/NetworkManager.kt | 2 + .../PayKitAnalyticsEventDispatcherImpl.kt | 11 ------ .../cash/paykit/core/impl/CashAppPayImpl.kt | 37 +++++++++++++++---- .../paykit/core/impl/NetworkManagerImpl.kt | 23 ++++++++++-- .../cash/paykit/core/models/common/Action.kt | 3 ++ .../request/CustomerRequestDataFactory.kt | 14 +++---- .../models/sdk/CashAppPayPaymentAction.kt | 12 +++--- .../paykit/core/CashAppPayExceptionsTests.kt | 2 +- .../cash/paykit/core/CashAppPayStateTests.kt | 4 +- 11 files changed, 84 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9c9fa74..4ec1210a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ -# 2.5.0 - UPCOMING +# 2.5.0 + + - Fix correct usage of `account_reference_id` in `OnFileAction` + - Add `reference_id` as a parameter of creating a customer request. This is to bring the Android SDK to parity with iOS and Web. # 2.4.0 diff --git a/core/src/main/java/app/cash/paykit/core/CashAppPay.kt b/core/src/main/java/app/cash/paykit/core/CashAppPay.kt index b2d0cdbd..2f22479f 100644 --- a/core/src/main/java/app/cash/paykit/core/CashAppPay.kt +++ b/core/src/main/java/app/cash/paykit/core/CashAppPay.kt @@ -45,9 +45,10 @@ interface CashAppPay { * @param paymentAction A wrapper class that contains all of the necessary ingredients for building a customer requests. * Look at [PayKitPaymentAction] for more details. * @param redirectUri The URI for Cash App to redirect back to your app. If you do not set this, back navigation from CashApp might not work as intended. + * @param referenceId Optional identifier generated by you for this request, typically used to associate the resource with a record in an external system. */ @WorkerThread - fun createCustomerRequest(paymentAction: CashAppPayPaymentAction, redirectUri: String?) + fun createCustomerRequest(paymentAction: CashAppPayPaymentAction, redirectUri: String?, referenceId: String? = null) /** * Create customer request given list of [CashAppPayPaymentAction]. @@ -57,9 +58,11 @@ interface CashAppPay { * @param paymentActions A wrapper class that contains all of the necessary ingredients for building one or more customer requests. * Look at [PayKitPaymentAction] for more details. * @param redirectUri The URI for Cash App to redirect back to your app. If you do not set this, back navigation from CashApp might not work as intended. + * @param referenceId Optional identifier generated by you for this request, typically used to associate the resource with a record in an external system. + * */ @WorkerThread - fun createCustomerRequest(paymentActions: List, redirectUri: String?) + fun createCustomerRequest(paymentActions: List, redirectUri: String?, referenceId: String? = null) /** * Update an existing customer request given its [requestId] and the updated definitions contained within [CashAppPayPaymentAction]. @@ -69,11 +72,13 @@ interface CashAppPay { * @param requestId ID of the request we intent do update. * @param paymentAction A wrapper class that contains all of the necessary ingredients for updating a customer request for a given [requestId]. * Look at [CashAppPayPaymentAction] for more details. + * @param referenceId Optional identifier generated by you for this request, typically used to associate the resource with a record in an external system. */ @WorkerThread fun updateCustomerRequest( requestId: String, paymentAction: CashAppPayPaymentAction, + referenceId: String? = null, ) /** @@ -84,11 +89,13 @@ interface CashAppPay { * @param requestId ID of the request we intent do update. * @param paymentActions A wrapper class that contains all of the necessary ingredients for updating one more more customer requests that share the same [requestId]. * Look at [CashAppPayPaymentAction] for more details. + * @param referenceId Optional identifier generated by you for this request, typically used to associate the resource with a record in an external system. */ @WorkerThread fun updateCustomerRequest( requestId: String, paymentActions: List, + referenceId: String? = null, ) /** diff --git a/core/src/main/java/app/cash/paykit/core/NetworkManager.kt b/core/src/main/java/app/cash/paykit/core/NetworkManager.kt index c5f184bb..f507d99c 100644 --- a/core/src/main/java/app/cash/paykit/core/NetworkManager.kt +++ b/core/src/main/java/app/cash/paykit/core/NetworkManager.kt @@ -28,12 +28,14 @@ internal interface NetworkManager { clientId: String, paymentActions: List, redirectUri: String?, + referenceId: String?, ): NetworkResult @Throws(IOException::class) fun updateCustomerRequest( clientId: String, requestId: String, + referenceId: String?, paymentActions: List, ): NetworkResult diff --git a/core/src/main/java/app/cash/paykit/core/analytics/PayKitAnalyticsEventDispatcherImpl.kt b/core/src/main/java/app/cash/paykit/core/analytics/PayKitAnalyticsEventDispatcherImpl.kt index 777531e2..f560789d 100644 --- a/core/src/main/java/app/cash/paykit/core/analytics/PayKitAnalyticsEventDispatcherImpl.kt +++ b/core/src/main/java/app/cash/paykit/core/analytics/PayKitAnalyticsEventDispatcherImpl.kt @@ -47,7 +47,6 @@ import app.cash.paykit.core.models.request.CustomerRequestDataFactory.CHANNEL_IN import app.cash.paykit.core.models.response.CustomerResponseData import app.cash.paykit.core.models.response.Grant import app.cash.paykit.core.models.sdk.CashAppPayPaymentAction -import app.cash.paykit.core.models.sdk.CashAppPayPaymentAction.OnFileAction import app.cash.paykit.core.network.MoshiProvider import app.cash.paykit.core.utils.Clock import app.cash.paykit.core.utils.ClockRealImpl @@ -218,14 +217,6 @@ internal class PayKitAnalyticsEventDispatcherImpl( val moshiAdapter: JsonAdapter> = moshi.adapter() val apiActionsAsJson: String = moshiAdapter.toJson(apiActions) - // Inner payload of the ES2 event. - var possibleReferenceId: String? = null - for (paymentAction in paymentKitActions) { - if (paymentAction is OnFileAction) { - possibleReferenceId = paymentAction.accountReferenceId - } - } - return AnalyticsCustomerRequestPayload( sdkVersion, userAgent, @@ -235,7 +226,6 @@ internal class PayKitAnalyticsEventDispatcherImpl( createActions = apiActionsAsJson, createChannel = CHANNEL_IN_APP, createRedirectUrl = redirectUri?.let { PiiString(redirectUri) }, - createReferenceId = possibleReferenceId?.let { PiiString(possibleReferenceId) }, environment = sdkEnvironment, ) } @@ -285,7 +275,6 @@ internal class PayKitAnalyticsEventDispatcherImpl( customerId = customerResponseData?.customerProfile?.id, customerCashTag = customerResponseData?.customerProfile?.cashTag, requestId = customerResponseData?.id, - referenceId = customerResponseData?.referenceId, environment = sdkEnvironment, ) } diff --git a/core/src/main/java/app/cash/paykit/core/impl/CashAppPayImpl.kt b/core/src/main/java/app/cash/paykit/core/impl/CashAppPayImpl.kt index 8fdd95df..8e80afdb 100644 --- a/core/src/main/java/app/cash/paykit/core/impl/CashAppPayImpl.kt +++ b/core/src/main/java/app/cash/paykit/core/impl/CashAppPayImpl.kt @@ -120,8 +120,12 @@ internal class CashAppPayImpl( analyticsEventDispatcher.sdkInitialized() } - override fun createCustomerRequest(paymentAction: CashAppPayPaymentAction, redirectUri: String?) { - createCustomerRequest(listOf(paymentAction), redirectUri) + override fun createCustomerRequest( + paymentAction: CashAppPayPaymentAction, + redirectUri: String?, + referenceId: String?, + ) { + createCustomerRequest(listOf(paymentAction), redirectUri, referenceId) } /** @@ -132,7 +136,11 @@ internal class CashAppPayImpl( * Look at [PayKitPaymentAction] for more details. */ @WorkerThread - override fun createCustomerRequest(paymentActions: List, redirectUri: String?) { + override fun createCustomerRequest( + paymentActions: List, + redirectUri: String?, + referenceId: String?, + ) { enforceRegisteredStateUpdatesListener() // Validate [paymentActions] is not empty. @@ -145,7 +153,12 @@ internal class CashAppPayImpl( currentState = CreatingCustomerRequest // Network call. - val networkResult = networkManager.createCustomerRequest(clientId, paymentActions, redirectUri) + val networkResult = networkManager.createCustomerRequest( + clientId = clientId, + paymentActions = paymentActions, + redirectUri = redirectUri, + referenceId = referenceId, + ) when (networkResult) { is Failure -> { currentState = CashAppPayExceptionState(networkResult.exception) @@ -159,8 +172,12 @@ internal class CashAppPayImpl( } } - override fun updateCustomerRequest(requestId: String, paymentAction: CashAppPayPaymentAction) { - updateCustomerRequest(requestId, listOf(paymentAction)) + override fun updateCustomerRequest( + requestId: String, + paymentAction: CashAppPayPaymentAction, + referenceId: String?, + ) { + updateCustomerRequest(requestId, listOf(paymentAction), referenceId) } /** @@ -175,6 +192,7 @@ internal class CashAppPayImpl( override fun updateCustomerRequest( requestId: String, paymentActions: List, + referenceId: String?, ) { enforceRegisteredStateUpdatesListener() @@ -188,7 +206,12 @@ internal class CashAppPayImpl( currentState = UpdatingCustomerRequest // Network request. - val networkResult = networkManager.updateCustomerRequest(clientId, requestId, paymentActions) + val networkResult = networkManager.updateCustomerRequest( + clientId = clientId, + requestId = requestId, + referenceId = referenceId, + paymentActions = paymentActions, + ) when (networkResult) { is Failure -> { currentState = CashAppPayExceptionState(networkResult.exception) diff --git a/core/src/main/java/app/cash/paykit/core/impl/NetworkManagerImpl.kt b/core/src/main/java/app/cash/paykit/core/impl/NetworkManagerImpl.kt index 578f75bc..3beaec92 100644 --- a/core/src/main/java/app/cash/paykit/core/impl/NetworkManagerImpl.kt +++ b/core/src/main/java/app/cash/paykit/core/impl/NetworkManagerImpl.kt @@ -81,8 +81,14 @@ internal class NetworkManagerImpl( clientId: String, paymentActions: List, redirectUri: String?, + referenceId: String?, ): NetworkResult { - val customerRequestData = CustomerRequestDataFactory.build(clientId, redirectUri, paymentActions) + val customerRequestData = CustomerRequestDataFactory.build( + clientId = clientId, + redirectUri = redirectUri, + referenceId = referenceId, + paymentActions = paymentActions, + ) val createCustomerRequest = CreateCustomerRequest( idempotencyKey = UUID.randomUUID().toString(), customerRequestData = customerRequestData, @@ -103,16 +109,27 @@ internal class NetworkManagerImpl( override fun updateCustomerRequest( clientId: String, requestId: String, + referenceId: String?, paymentActions: List, ): NetworkResult { val customerRequestData = - CustomerRequestDataFactory.build(clientId, null, paymentActions, isRequestUpdate = true) + CustomerRequestDataFactory.build( + clientId = clientId, + redirectUri = null, + referenceId = referenceId, + paymentActions = paymentActions, + isRequestUpdate = true, + ) val createCustomerRequest = CreateCustomerRequest( customerRequestData = customerRequestData, ) // Record analytics. - analyticsEventDispatcher?.updatedCustomerRequest(requestId, paymentActions, customerRequestData.actions) + analyticsEventDispatcher?.updatedCustomerRequest( + requestId = requestId, + paymentKitActions = paymentActions, + apiActions = customerRequestData.actions, + ) return executeNetworkRequest( PATCH, diff --git a/core/src/main/java/app/cash/paykit/core/models/common/Action.kt b/core/src/main/java/app/cash/paykit/core/models/common/Action.kt index 6f05a3d2..9dda99f9 100644 --- a/core/src/main/java/app/cash/paykit/core/models/common/Action.kt +++ b/core/src/main/java/app/cash/paykit/core/models/common/Action.kt @@ -15,6 +15,7 @@ */ package app.cash.paykit.core.models.common +import app.cash.paykit.core.models.pii.PiiString import com.squareup.moshi.Json import com.squareup.moshi.JsonClass @@ -28,4 +29,6 @@ data class Action( val scopeId: String, @Json(name = "type") val type: String, + @Json(name = "account_reference_id") + val accountReferenceId: PiiString? = null, ) diff --git a/core/src/main/java/app/cash/paykit/core/models/request/CustomerRequestDataFactory.kt b/core/src/main/java/app/cash/paykit/core/models/request/CustomerRequestDataFactory.kt index 7383626c..24c59810 100644 --- a/core/src/main/java/app/cash/paykit/core/models/request/CustomerRequestDataFactory.kt +++ b/core/src/main/java/app/cash/paykit/core/models/request/CustomerRequestDataFactory.kt @@ -33,20 +33,15 @@ object CustomerRequestDataFactory { fun build( clientId: String, redirectUri: String?, + referenceId: String?, paymentActions: List, isRequestUpdate: Boolean = false, ): CustomerRequestData { val actions = ArrayList(paymentActions.size) - var possibleReferenceId: String? = null for (paymentAction in paymentActions) { when (paymentAction) { - is OnFileAction -> { - actions.add(buildFromOnFileAction(clientId, paymentAction)) - if (paymentAction.accountReferenceId != null) { - possibleReferenceId = paymentAction.accountReferenceId - } - } + is OnFileAction -> actions.add(buildFromOnFileAction(clientId, paymentAction)) is OneTimeAction -> actions.add(buildFromOneTimeAction(clientId, paymentAction)) } } @@ -56,14 +51,14 @@ object CustomerRequestDataFactory { actions = actions, channel = null, redirectUri = null, - referenceId = possibleReferenceId?.let { PiiString(it) }, + referenceId = referenceId?.let { PiiString(it) }, ) } else { CustomerRequestData( actions = actions, channel = CHANNEL_IN_APP, redirectUri = redirectUri?.let { PiiString(it) }, - referenceId = possibleReferenceId?.let { PiiString(it) }, + referenceId = referenceId?.let { PiiString(it) }, ) } } @@ -78,6 +73,7 @@ object CustomerRequestDataFactory { return Action( scopeId = scopeIdOrClientId, type = PAYMENT_TYPE_ON_FILE, + accountReferenceId = onFileAction.accountReferenceId?.let { PiiString(it) }, ) } diff --git a/core/src/main/java/app/cash/paykit/core/models/sdk/CashAppPayPaymentAction.kt b/core/src/main/java/app/cash/paykit/core/models/sdk/CashAppPayPaymentAction.kt index d660d6ba..9ef153fd 100644 --- a/core/src/main/java/app/cash/paykit/core/models/sdk/CashAppPayPaymentAction.kt +++ b/core/src/main/java/app/cash/paykit/core/models/sdk/CashAppPayPaymentAction.kt @@ -20,7 +20,7 @@ import app.cash.paykit.core.CashAppPay /** * This class holds the information necessary for [CashAppPay.createCustomerRequest] to be executed. */ -sealed class CashAppPayPaymentAction(scopeId: String?) { +sealed class CashAppPayPaymentAction(open val scopeId: String?, open val referenceId: String?) { /** * Describes an intent for a client to charge a customer a given amount. @@ -38,8 +38,9 @@ sealed class CashAppPayPaymentAction(scopeId: String?) { data class OneTimeAction( val currency: CashAppPayCurrency?, val amount: Int?, - val scopeId: String? = null, - ) : CashAppPayPaymentAction(scopeId) + override val scopeId: String? = null, + override val referenceId: String? = null, + ) : CashAppPayPaymentAction(scopeId, referenceId) /** * Describes an intent for a client to store a customer's account, allowing a client to create payments @@ -49,8 +50,9 @@ sealed class CashAppPayPaymentAction(scopeId: String?) { * @param accountReferenceId Identifier of the account or customer associated to the on file action. */ data class OnFileAction( - val scopeId: String? = null, + override val scopeId: String? = null, val accountReferenceId: String? = null, + override val referenceId: String? = null, ) : - CashAppPayPaymentAction(scopeId) + CashAppPayPaymentAction(scopeId, referenceId) } diff --git a/core/src/test/java/app/cash/paykit/core/CashAppPayExceptionsTests.kt b/core/src/test/java/app/cash/paykit/core/CashAppPayExceptionsTests.kt index 24280a8a..0f7a8a02 100644 --- a/core/src/test/java/app/cash/paykit/core/CashAppPayExceptionsTests.kt +++ b/core/src/test/java/app/cash/paykit/core/CashAppPayExceptionsTests.kt @@ -56,7 +56,7 @@ class CashAppPayExceptionsTests { val listener = mockk(relaxed = true) payKit.registerForStateUpdates(listener) - every { networkManager.createCustomerRequest(any(), any(), any()) } returns NetworkResult.failure( + every { networkManager.createCustomerRequest(any(), any(), any(), any()) } returns NetworkResult.failure( Exception("bad"), ) payKit.createCustomerRequest(FakeData.oneTimePayment, FakeData.REDIRECT_URI) diff --git a/core/src/test/java/app/cash/paykit/core/CashAppPayStateTests.kt b/core/src/test/java/app/cash/paykit/core/CashAppPayStateTests.kt index 5d643e71..50da4d08 100644 --- a/core/src/test/java/app/cash/paykit/core/CashAppPayStateTests.kt +++ b/core/src/test/java/app/cash/paykit/core/CashAppPayStateTests.kt @@ -76,7 +76,7 @@ class CashAppPayStateTests { val listener = mockk(relaxed = true) payKit.registerForStateUpdates(listener) - every { networkManager.createCustomerRequest(any(), any(), any()) } returns NetworkResult.failure( + every { networkManager.createCustomerRequest(any(), any(), any(), any()) } returns NetworkResult.failure( Exception("bad"), ) @@ -95,6 +95,7 @@ class CashAppPayStateTests { any(), any(), any(), + any(), ) } returns NetworkResult.failure( Exception("bad"), @@ -180,6 +181,7 @@ class CashAppPayStateTests { any(), any(), any(), + any(), ) } returns customerTopLevelResponse