feat: Create payment actions service - #1658
Conversation
mzlangreder
changed the title
4c61612 to
0e294a3
Compare
* Payment Action Id is not needed as it's read from the authFingerprint * Unit Test updated to reflect unvaulted card flow
* Added KDoc for PaymentActionsService * Added unit tests to cover updated enum cases
8029239 to
6a237ab
Compare
saralvasquez
left a comment
saralvasquez
left a comment
There was a problem hiding this comment.
This looks great! Just a couple questions and nits
| PaymentActionResult.Success(paymentAction) | ||
| } | ||
| } | ||
| } catch (jex: JSONException) { |
There was a problem hiding this comment.
Interesting naming convention jex. Just out of curiosity, is that a personal preference or style standard?
| ): PaymentActionResult { | ||
| return try { | ||
| braintreeClient | ||
| .sendGraphQLPOST(buildSetPaymentActionPaymentMethodQuery(paymentMethod)) |
There was a problem hiding this comment.
nit: since the method name is so long this is starting to feel a bit difficult to parse in a single line, but I don't feel super strongly
|
|
||
| private val paymentMethodVariables = JSONObject().apply { | ||
| put( | ||
| "paymentMethodDetails", |
There was a problem hiding this comment.
Just double checking it should be paymentMethodDetails and not paymentMethod. I don't think I've seen what the payment method input is supposed to look like yet
| every { paymentActionSelectionSet() } returns selectionSet | ||
| } | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) |
There was a problem hiding this comment.
Is this something we can put on the class so it doesn't need to be added to every test?
| "setPaymentActionPaymentMethod": { | ||
| "paymentAction": { | ||
| "id": "pa123", | ||
| "status": "requires_payment_method" |
There was a problem hiding this comment.
I think this is more of a general feature question but I can't seem to answer it using the technical decision docs I've seen. What use case will the setPaymentActionPaymentMethod query be called and return a success but also have the status requires_payment_method. Isn't that status to signify that setPaymentActionPaymentMethod has to be called?
Again I'm probably under informed so please let me know if there's a doc I need to be referring to
| .let { paymentActionJson -> | ||
| PaymentAction( | ||
| id = paymentActionJson.getString(ID_KEY), | ||
| status = paymentActionJson.getString(STATUS_KEY).toPaymentActionStatus() |
There was a problem hiding this comment.
same thing came up on iOS: getString throws if id is missing, but not if it's an empty string — so that currently returns a Success with an unusable id. worth adding a check so empty fails the same way as missing, matching iOS.
| braintreeClient | ||
| .sendGraphQLPOST(buildSetPaymentActionPaymentMethodQuery(paymentMethod)) | ||
| .toPaymentActionsResult() | ||
| } catch (exception: IOException) { |
There was a problem hiding this comment.
wondering if we should widen the exception type or if it's okay as-is 👀 in the case sendGraphQLPOST can throw anything besides IOException
3 participants
Summary of changes
AI Usage
Which AI Agent Was Used?
How was AI used?
Review ADR to match up expected graphQL shapes and writing unit tests.
Estimated AI Code Contribution
Checklist
Authors