[APAM-563] add Session and modify intent confirm request to support simplifying flow#264
[APAM-563] add Session and modify intent confirm request to support simplifying flow#264ferdian-airwallex wants to merge 6 commits intofeature/APAM-561from
Conversation
| val minPaymentAmount: Double? = null, | ||
| /** | ||
| * The first payment amount. Optional if payment agreement type is VARIABLE | ||
| */ |
There was a problem hiding this comment.
Nit: Would it be worth using BigDecimal here instead of Double? The rest of the SDK uses BigDecimal for monetary values (e.g. AirwallexSession.amount), and Double can introduce subtle precision issues with financial amounts. This would apply to fixedPaymentAmount, maxPaymentAmount, minPaymentAmount, and firstPaymentAmount.
| /** | ||
| * The maximum payment amount that can be charged for a single payment. | ||
| * Optional if payment_amount_type is VARIABLE | ||
| */ |
There was a problem hiding this comment.
Suggestion: paymentAmountType has constrained values (FIXED / VARIABLE) and periodUnit (line 112) has constrained values (DAY / WEEK / MONTH / YEAR). Would it make sense to use typed enums with a .value property here, similar to how NextTriggeredBy and MerchantTriggerReason are defined? That would give us compile-time safety and keep things consistent across the codebase.
| private val paymentIntent: PaymentIntent, | ||
| private val countryCode: String, | ||
| private val googlePayOptions: GooglePayOptions? = null | ||
| ) : ObjectBuilder<Session> { |
There was a problem hiding this comment.
Question: I noticed that AirwallexPaymentSession (which the KDoc references as similar) implements PaymentIntentResolvableSession, but Session does not. This means the isExpressCheckout extension property and the PaymentIntentProvider / PaymentIntentSource patterns won't be available for Session instances.
Is this intentional for the simplifying flow? If so, it might be helpful to note that in the KDoc so future readers understand the distinction. Just want to make sure nothing was missed!
There was a problem hiding this comment.
yes I just realized also while actually trying to use the Session in the next ticket. adding it soon!
| * For example, period=1 and period_unit=MONTH means monthly billing. | ||
| * Required when merchant_trigger_reason = scheduled | ||
| */ | ||
| val period: Int? = null, |
There was a problem hiding this comment.
PaymentSchedule is not aligned with the API documentation. There should only be period and period_unit according to the api doc. Is this intentional?
There was a problem hiding this comment.
lol thanks I read it wrong, I thought the start_date & total_billing_cycles are under payment_schedule. I'll move those to TermOfUse
…aram types for consent options
2 participants
everything is just new code but unused. this is the base necessity for the future tasks
note: the destination is APAM-561, which is actually a merged branch between
APAM-551-2andAPAM-555will recreate the APAM-561 branch from develop branch when the previous project is merged