APM Mobile - Add new API for handling payment method change #1
Conversation
|
Humm wondering if this library has any testing to cover & validate all the changes we will made. Also, do we have a list of Would make sense to put it inside the apm-mobile packages as a mono-repo. |
| ) { | ||
| throw new Error( | ||
| `Only "${SHIPPING_ADDRESS_CHANGE_EVENT}" and "${SHIPPING_OPTION_CHANGE_EVENT}" event listeners are supported.` | ||
| `Only "${SHIPPING_ADDRESS_CHANGE_EVENT}" and "${SHIPPING_OPTION_CHANGE_EVENT} and "${PAYMENT_METHOD_CHANGE_EVENT}"" event listeners are supported.` |
There was a problem hiding this comment.
| `Only "${SHIPPING_ADDRESS_CHANGE_EVENT}" and "${SHIPPING_OPTION_CHANGE_EVENT} and "${PAYMENT_METHOD_CHANGE_EVENT}"" event listeners are supported.` | |
| `Only "${SHIPPING_ADDRESS_CHANGE_EVENT}", "${SHIPPING_OPTION_CHANGE_EVENT} and "${PAYMENT_METHOD_CHANGE_EVENT}"" event listeners are supported.` |
There was a problem hiding this comment.
(This is a nitpick suggestion just to improve readability. This change is not essential.)
| @@ -14,6 +14,7 @@ import type { | |||
| PaymentShippingType, | |||
| PaymentDetailsIOS, | |||
| PaymentDetailsIOSRaw, | |||
| PaymentMethod | |||
There was a problem hiding this comment.
The PaymentMethod type only contains one attribute: a payment method type string. Maybe it would be simpler to just handle that information as a string without wrapping it in an object.
There was a problem hiding this comment.
it matches the response body of the new API, so we need to have a object that includes attribute paymentMethodType
| @@ -106,26 +109,33 @@ export default class PaymentRequest { | |||
| _serializedModifierData: string; | |||
| _details: Object; | |||
| _options: Object; | |||
| _creditDebitDetails: Object; | |||
There was a problem hiding this comment.
Would it make sense to name this object _cardType? If that name is accurate, I think it would be clear and concise. I might just be misunderstanding the usage of this object though.
There was a problem hiding this comment.
_creditDebitDetails includes all the details of both debit and credit cards :)
| _handlePaymentMethodChange(paymentMethod: PaymentMethod) { | ||
| this._paymentMethod = paymentMethod.paymentMethodType; |
There was a problem hiding this comment.
If we treated paymentMethod as a string, we could simplify this to:
| _handlePaymentMethodChange(paymentMethod: PaymentMethod) { | |
| this._paymentMethod = paymentMethod.paymentMethodType; | |
| _handlePaymentMethodChange(paymentMethod: string) { | |
| this._paymentMethod = paymentMethod; |
There was a problem hiding this comment.
paymentMethod is handling the response body
| completion:(nonnull void (^)(NSArray<PKPaymentSummaryItem *> * _Nonnull))completion | ||
| { | ||
| self.paymentMethodCompletion = completion; | ||
| NSString *type = [self paymentMethodTypeToString:paymentMethod.type]; |
There was a problem hiding this comment.
Do we need to explicitly define paymentMethodTypeToString somewhere?
There was a problem hiding this comment.
yes, paymentMethodTypeToString is defined at line 447 in ReactNativePayments.m
| } | ||
| else if (this._paymentMethod === 'PKPaymentMethodTypeDebit') { | ||
| this._details = this._creditDebitDetails.debit; | ||
| } |
There was a problem hiding this comment.
This is just prototype code (nice prototype code with unit tests!), but this is one part that i'd add a todo for. Handle the else statement.
There was a problem hiding this comment.
good point for clarification, the to-do has been added.
we have another investigation card about that the supported cards can only be shown up in apple pay flow to avoid other card problem, and this is handle in this investigation card https://trello.com/c/jXpIHzT6/2280-investigate-apm-mobile-only-show-up-the-supported-cards.
anncheng999
force-pushed
the
annieColdApplePay
branch
from
74d92ed to
142f58e
Compare
| _userDismissSubscription: any; // TODO: - add proper type annotation | ||
| _userAcceptSubscription: any; // TODO: - add proper type annotation | ||
| _gatewayErrorSubscription: any; // TODO: - add proper type annotation | ||
| _shippingAddressChangesCount: number; | ||
| _paymentMethodChangesCount: number; |
There was a problem hiding this comment.
Follow-up: Do we need to handle this the way it's handled on lines 319-326 for the shipping address change event?
| _handlePaymentMethodChange(paymentMethod: PaymentMethod) { | ||
| this._paymentMethod = paymentMethod.paymentMethodType; | ||
|
|
||
| if(this._paymentMethod === 'PKPaymentMethodTypeCredit') { |
There was a problem hiding this comment.
Follow-up: This should be moved up to the application layer and the library should be more generic.
We are no longer changing behavior for how PaymentRequest handles its details constructor argument, so the test changes from PR #1 are no longer valid. Revert these. There is a better way of changing PaymentRequestUpdateEvent, so revert all the changes from PR #1 and then re-do the tests in a following commit.
No description provided.