Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V7] Add Encodable protocol for Local Payments. #1468

Merged
merged 4 commits into from
Dec 2, 2024
Merged

Conversation

richherrera
Copy link
Contributor

@richherrera richherrera commented Nov 20, 2024

Summary of changes

  • Add encodable types for post bodies used in the Local Payment flow.
  • Local Payment flow has been tested in the Demo app to ensure the dictionaries are still constructed as expected.

Checklist

  • [ ] Added a changelog entry
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

@richherrera richherrera requested a review from a team as a code owner November 20, 2024 17:23
case currencyCode = "currency_iso_code"
case paymentTypeCountryCode = "payment_type_country_code"
case merchantAccountID = "merchant_account_id"
case email = "payer_email"
Copy link
Contributor Author

@richherrera richherrera Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing Android, I noticed we’re not sending the same parameters, such as email, phone, givenName, surname, bic, and merchantAccountId. I verified with the LPMS team the keys needed to send these parameters. It seems the gateway supports both camel case and snake case, web and Android use camel case.

@richherrera richherrera added the v7 label Nov 21, 2024
Copy link
Contributor

@agedd agedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! 🚀

@@ -9,7 +9,12 @@ import BraintreeDataCollector
#endif

/// Used to initialize a local payment flow
@objcMembers public class BTLocalPaymentRequest: NSObject {
/// The POST body for v1/local_payments/create
@objcMembers public class BTLocalPaymentRequest: NSObject, Encodable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take it or leave it but we could also consider separating the encodable portion out into a new file, we've done that in most places where we've made this change, like here for example: https://github.com/braintree/braintree_ios/blob/main/Sources/BraintreeSEPADirectDebit/SEPADebitAccountsRequest.swift

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated: 3e00b0e what do you think??

Comment on lines 95 to 102
if let address {
try container.encodeIfPresent(address.streetAddress, forKey: .streetAddress)
try container.encodeIfPresent(address.extendedAddress, forKey: .extendedAddress)
try container.encodeIfPresent(address.locality, forKey: .locality)
try container.encodeIfPresent(address.countryCodeAlpha2, forKey: .countryCodeAlpha2)
try container.encodeIfPresent(address.postalCode, forKey: .postalCode)
try container.encodeIfPresent(address.region, forKey: .region)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take it or leave it - instead of needing a custom Encoder implementation, we could have the init take in a BTPostalAddress param, but then the private properties on this struct are the actual encodable types.

Roughly something like:

private let amount: String
private var streetAddress: String?
private var extendedAddress: String?

init(amount: String, ... the other stuff, address: BTPostalAddress?) {
    self.amount = amount 
    self.streetAddress = address.streetAddress
    self.extendedAddress = address.extendedAddress
}

Then we wouldn't need to override encode().

Which is more readable? I go back and forth sometimes with the custom encoder being more or less explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: 3e00b0e

Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@richherrera richherrera merged commit 530f948 into v7 Dec 2, 2024
7 of 8 checks passed
@richherrera richherrera deleted the move-to-encodable branch December 2, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants