Reference id mixup fix #147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pedronveloso
commented
Jul 18, 2024
| @@ -235,7 +226,6 @@ internal class PayKitAnalyticsEventDispatcherImpl( | |||
| createActions = apiActionsAsJson, | |||
| createChannel = CHANNEL_IN_APP, | |||
| createRedirectUrl = redirectUri?.let { PiiString(redirectUri) }, | |||
| createReferenceId = possibleReferenceId?.let { PiiString(possibleReferenceId) }, | |||
There was a problem hiding this comment.
Since this field his hidden from ES2, there's no point even sending it to begin with.
pedronveloso
commented
Jul 18, 2024
| @@ -188,7 +206,12 @@ internal class CashAppPayImpl( | |||
| currentState = UpdatingCustomerRequest | |||
|
|
|||
| // Network request. | |||
| val networkResult = networkManager.updateCustomerRequest(clientId, requestId, paymentActions) | |||
| val networkResult = networkManager.updateCustomerRequest( | |||
There was a problem hiding this comment.
I made some of these functions have named parameters to increase readability, and also because several of these params share the same type, so this makes it less likely we pass the wrong thing into the wrong field without it being caught in a review.
pedronveloso
commented
Jul 18, 2024
| @@ -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?) { | |||
There was a problem hiding this comment.
scopeId was supposed to be an open val, so I'm sort of piggybacking this fix. This doesn't affect anything for us yet - but its the right thing if in the future we need to loop trough a collection of CashAppPayPaymentAction for whatever reason.
There was a problem hiding this comment.
nope. merchants shouldn't be using the top class definition at all (and the field previously wasn't accessible that way)
mmroz
approved these changes
Jul 18, 2024
pedronveloso
added a commit
that referenced
this pull request
Jul 19, 2024
* Fix Retrieve Existing request ID behavior for PROCESSING state (#95) * Fix Retrieve Existing request ID behavior for PENDING state * Add distinction between Pending & Processing * Code cleanup (and research on proguard rules being handed to consumers) (#98) * Add proguard rules to project for automatic client consumption * Fix constant name after renaming class * Concrete R8 rules are actually not needed * Add enum for GrantType (#99) * Update Robolectric dependency (#102) * Update changelog * Use Epoch usec for Analytics timestamp (#107) * Add UT for Analytics (#108) * Refresh Auth Token for Customer Request (#109) * Initial support for Unauthorized Customer Request refresh * Add useful docs about token refresh window * Improve logging for CashAppCashAppPayImpl * Use Duration instead of Long for better time handling * Remove not needed call to interrupt * Schedule auth token refresh on `startWithExistingCustomerRequest` when status is "pending" * Support deferred auth token refresh upon authorize call * Better thread safety and handling * Update comment * Add new Refreshing state (#110) * Add Refreshing state * Use better explicit tag for logging * Abstract thread managment away from main class (#113) * Redact PII for logs and metrics (#112) * Redact PII for logs and metrics * Add UTs for PiiString * Swap redact with unredacted defaults * Redact more fields; move redaction step to encoding * Simplify internal class names (#115) * Modify CashAppPayButton to allow for dynamic styles (#116) * Modify CashAppPayButton to allow for dynamic styles * Update CHANGELOG * Update testing frameworks and link baseline * Add PR template (#120) * Logging abstraction (#119) * Create new module for logging * CashPayLogger implementation, and apply it to Analytics module * Add CAP logger to Core module * Create lint baseline file for logging module * Fix tests * Change Analytics implementation to log only Warn level or above * Add support to observe logging changes * Add some UT for logging module * Add UTs for CashAppLoggerImpl * Add some useful documentation to tests * Set default PR template (#121) * Make Initializer open to modification (#123) * Update Spotless Plugin and Ktlint version (#124) * Add minify rules (#125) * Add consumer-rules for minify * Bump release version; update release notes * Add more minify rules * Update Core dependency on OkHttp (#126) * Add consumer-rules for minify * Bump release version; update release notes * Add more minify rules * Update Core dependency on OkHttp * Update Android target version (#133) * Increase compile version to API 33 * Update CHANGELOG * Update AGP to 8.1.2 (#134) * Update AGP to 8.1.2 * Update Java version for CI to 17 * Update AGP & Gradle (#138) * Update AGP and Gradle * Update Robolectric dependency * Pedro/sdk kotlin 1.8 (#139) * Update AGP version * Update Kotlin to 1.8.x, along with various library dependencies * Update Dev App versions and baseline lint files * Update AGP and dev app version * Use reference_id param correctly (#145) * Add Start Screen to split different playgrouds within Dev App * Improve webview logs display * Increase log history size * Fix accountReferenceId param missing from network call * Update some of the project libraries * Fix test related to size of log history * Minor changes based on PR feedback * 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 * Prepare release 2.5.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
account_reference_idis a unique field of on-file payments, andreference_idis a top-level field across both on-file and one-time payments. This PR addresses that distinction, by exposing and using the correct field.How did you accomplish this?
Because
reference_idis a top-level field, it is therefore NOT part of the request Actions, and we had to add 1 extra parameter to a few of our public functions.Visual:
Testing both field, and correctly assessing that they're being sent and recognized correctly by the backend:
Screen_recording_20240718_211137.mp4