[V7] Add `Encodable` protocol for PayPal Checkout #1491

richherrera · 2025-01-02T18:21:22Z

Summary of changes

Add PayPalRequest protocol to define the contract for the base properties for BTPayPalCheckoutRequest and BTPayPalVaultRequest
Conform to the new protocol in the three concrete classes (BTPayPalRequest, BTPayPalCheckoutRequest and BTPayPalVaultRequest) to eliminate inheritance
Update docstrings for classes BTPayPalCheckoutRequest and BTPayPalVaultRequest
LineItems now conforms to the Encodable protocol.
Add encodable types for post bodies used in the PayPal Checkout flow.
PayPal Checkout has been tested in the Demo app to ensure the dictionaries are still constructed as expected.

Note: Since we have two different classes (PayPalCheckoutRequest and PayPalVaultRequest) used in PayPalClient, it's currently not possible to use Encodable until the same implementation is done for PayPalVaultRequest. We tested the flow, but we need to complete task DTMOBILES-1177 to be able to use Encodable.

Checklist

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

Authors

@richherrera

richherrera · 2025-01-02T18:38:23Z

Sources/BraintreePayPal/Models/PayPalCheckoutPOSTBody.swift

+        if payPalRequest.requestBillingAgreement {
+            self.requestBillingAgreement = payPalRequest.requestBillingAgreement
+
+            if let billingAgreementDescription = payPalRequest.billingAgreementDescription {


Currently, this property belongs to class BTPayPalRequest. After changing its properties to internal in a previous PR, merchants can no longer access these properties, meaning there's no way to configure, for example, "billingAgreementDescription" when creating an instance of BTPayPalCheckoutRequest or BTPayPalVaultRequest. Do you think we should add the necessary properties to the init of BTPayPalCheckoutRequest or BTPayPalVaultRequest, or make the properties in BTPayPalRequest public again and remove BTPayPalRequest.init?

That's a great catch, I don't think we want to make the properties on BTPayPalRequest public since that would allow merchants to use the dot syntax, which we would like to move away from. I think it makes sense to add the properties to BTPayPalCheckoutRequest or BTPayPalVaultRequest inits then pass them in the super.init? What do you think?

I think we can opt to use a Protocol, this approach would be similar to the one we use on Android, where they use an AbstractClass. The reason for using a protocol is to define a contract for the necessary properties in both models. Instead of inheritance, we would use conformance so that our BTPayPalClient functions the same. I've updated the PR, let me know if you have any questions or comments. I'm happy to address any concerns.

richherrera · 2025-01-09T18:07:58Z

Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift

        with configuration: BTConfiguration,
        universalLink: URL? = nil,
        isPayPalAppInstalled: Bool = false
    ) -> [String: Any] {
-        var baseParameters = super.parameters(with: configuration)
+        var experienceProfile: [String: Any] = [:]


Since we won't have inheritance, I had to move this code from BTPayPalRequest.parameters. In the next PR, this entire method will be removed because we are creating the PayPalCheckoutPOSTBody model, which conforms to the Encodable protocol.

richherrera · 2025-01-09T18:11:23Z

Sources/BraintreePayPal/BTPayPalRequest.swift

+
+    static var callbackURLHostAndPath: String { get }
+
+    func parameters(


This method is to mimic the inheritance we used to do

richherrera · 2025-01-09T18:34:08Z

Sources/BraintreePayPal/BTPayPalVaultRequest.swift

+        isShippingAddressEditable: Bool = false,
+        isShippingAddressRequired: Bool = false,
+        landingPageType: BTPayPalRequestLandingPageType = .none,
+        lineItems: [BTPayPalLineItem]? = nil,


Comparing the implementation with Android, on Android, lineItems is only used for the Checkout flow. What do you think about removing this property from the PayPalRequest protocol, so it's only visible in the Checkout flow? On Android, we would need to remove the property from the AbstractClass PayPalRequest, and it wouldn't be overridden in PayPalCheckoutRequest

Interesting, let's confirm with the pay with paypal team if line items can be passed on vault requests. If not, I agree with you we can remove it from the protocol and make it only available on PayPalCheckoutRequest

jaxdesmarais · 2025-01-10T14:26:18Z

Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift

+    let hermesPath: String
+    let paymentType: BTPayPalPaymentType


Do we have a preference on harding these here or below in the init? I lean slightly towards having them hardcoded at the top of the file, but am open to what we feel is best.

jaxdesmarais · 2025-01-10T14:28:21Z

Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift

+        experienceProfile["no_shipping"] = !isShippingAddressRequired
+        experienceProfile["brand_name"] = displayName != nil ? displayName : configuration.json?["paypal"]["displayName"].asString()
+
+        if landingPageType?.stringValue != nil {


Take it or leave it since I think this will change in the next PR, but we tend to prefer the if let syntax over != nil

Updated: e446f03

jaxdesmarais · 2025-01-10T14:30:07Z

Sources/BraintreePayPal/BTPayPalClient.swift

@@ -29,7 +29,7 @@ import BraintreeDataCollector
    var clientMetadataID: String?

    /// Exposed for testing the intent associated with this request
-    var payPalRequest: BTPayPalRequest?
+    var payPalRequest: PayPalRequest?


Should we keep the name as BTPayPalRequest for the protocol?

Since it's an internal protocol, I think we can omit the BT prefix. If we decide to keep the prefix, how about I change it in the next PR? Right now, there would be naming conflicts with the concrete class. Another option would be to choose a different name for the protocol 🤔

We can change it in the next PR - I feel like it makes sense to keep the prefix for now just based on the class names but can be in the follow up.

Updated: f90356b

scannillo · 2025-01-10T20:46:54Z

Sources/BraintreePayPal/BTPayPalLineItem.swift

+        if let unitTaxAmount, !unitTaxAmount.isEmpty {
+            try container.encode(unitAmount, forKey: .unitTaxAmount)
+        }


Do we need to implement a custom encode() method just for this case?

Does Encodable not by default just omit an optional value that is nil?

The reason is that on iOS, the value of unitAmount is used with the keys unit_amount

braintree_ios/Sources/BraintreePayPal/BTPayPalLineItem.swift

Line 124 in 652e696

"unit_amount": unitAmount,

and unit_tax_amount

braintree_ios/Sources/BraintreePayPal/BTPayPalLineItem.swift

Line 130 in 652e696

requestParameters["unit_tax_amount"] = unitAmount

but it seems this has always been an error. On Android it's different 🤔

I've updated the PR: e478437 let me know what do you think. Unfortunately, the enums need to implement a custom encode because they are of type Int, and we are encoding their string value.

jaxdesmarais

🚀

richherrera added 8 commits December 30, 2024 14:05

Add file with paypal request properties

d53299c

Add var to BTConfiguration extension

cf7ad1e

Add country iso code var

964d7e8

Add checkout properties to POST body model

7460d9d

Disable nesting lint

0a3dbf4

Address some lints

f5952e6

Add dot

3168bf5

Rename struct

623c681

richherrera requested a review from a team as a code owner January 2, 2025 18:21

richherrera commented Jan 2, 2025

View reviewed changes

richherrera self-assigned this Jan 3, 2025

richherrera added 10 commits January 6, 2025 12:38

Add Encodable protocol to BTPayPalLineItem class

09d964d

Add PayPalRequest protocol, BTPayPalRequest conforms PayPalRequest

1553b22

BTPayPalCheckoutRequest conforms PayPalRequest

f9c90ec

Move lineItems parameters on init

abe2c7e

BTPayPalVaultRequest conforms PayPalRequest

b0cea93

Update BTPayPalRequest UTs

4a3b49c

Update BTPayPalClient to use an interface instead of concrete class

49886c7

Merge branch 'v7' into paypal-checkout-encodable

47727f4

Disable cyclomatic_complexity lint

3c1701e

Disable function_body_length lint

b6ce34b

richherrera commented Jan 9, 2025

View reviewed changes

jaxdesmarais reviewed Jan 10, 2025

View reviewed changes

scannillo reviewed Jan 10, 2025

View reviewed changes

richherrera added 4 commits January 10, 2025 15:15

Use if let syntax

e446f03

Address PR comments

ac6f13e

Remove BTLineItems custom encode method

e478437

Revert unnecessary changes

6dfb488

jaxdesmarais approved these changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V7] Add `Encodable` protocol for PayPal Checkout #1491

[V7] Add `Encodable` protocol for PayPal Checkout #1491

richherrera commented Jan 2, 2025 •

edited

Loading

richherrera Jan 2, 2025

jaxdesmarais Jan 7, 2025

richherrera Jan 9, 2025 •

edited

Loading

richherrera Jan 9, 2025

richherrera Jan 9, 2025 •

edited

Loading

richherrera Jan 9, 2025

jaxdesmarais Jan 10, 2025

jaxdesmarais Jan 10, 2025

jaxdesmarais Jan 10, 2025

richherrera Jan 13, 2025

jaxdesmarais Jan 10, 2025

richherrera Jan 10, 2025

jaxdesmarais Jan 10, 2025

richherrera Jan 13, 2025

scannillo Jan 10, 2025

richherrera Jan 10, 2025

richherrera Jan 13, 2025

jaxdesmarais left a comment


		static var callbackURLHostAndPath: String { get }

		func parameters(

[V7] Add Encodable protocol for PayPal Checkout #1491

Are you sure you want to change the base?

[V7] Add Encodable protocol for PayPal Checkout #1491

Conversation

richherrera commented Jan 2, 2025 • edited Loading

Summary of changes

Checklist

Authors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richherrera Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richherrera Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxdesmarais left a comment

Choose a reason for hiding this comment

[V7] Add `Encodable` protocol for PayPal Checkout #1491

[V7] Add `Encodable` protocol for PayPal Checkout #1491

richherrera commented Jan 2, 2025 •

edited

Loading

richherrera Jan 9, 2025 •

edited

Loading

richherrera Jan 9, 2025 •

edited

Loading