profile
viewpoint
Nate Bierdeman nbierdeman Software Engineer @paypal
PullRequestReviewEvent
PullRequestReviewEvent

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export interface PayPalButtonsComponentOptions {     onInit?: (data: Record<string, unknown>, actions: OnInitActions) => void;     /**      * Called when the buyer changes their shipping address on PayPal.+     * @deprecated Use `onShippingAddressChange` or `onShippingOptionsChange` instead.      */     onShippingChange?: (         data: OnShippingChangeData,         actions: OnShippingChangeActions,     ) => Promise<void>;+    /**+     * Called when the buyer selects a new shipping option on PayPal.+     */+    onShippingOptionsChange?: (+        data: OnShippingOptionsChangeData,+        actions: OnShippingOptionsChangeActions,+    ) => Promise<void>;+    /**+     * Called when the buyer updates their shipping address on PayPal.

Oh man, I don't know the difference. A buyer can either add one to begin with or select a different one that's already on file. Does that help? Probably not. 🤣

jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export type OnShippingChangeActions = {     }; }; +type CurrencyCodeAndValue = {+    currencyCode: string;+    value: string;+};++type ShippingOption = {+    amount: CurrencyCodeAndValue;+    id?: string;+    label: string;+    selected: boolean;+    type: "SHIPPING" | "PICKUP";+};++export type OnShippingOptionsChangeData = {+    orderID?: string;+    paymentID?: string;+    paymentToken?: string;+    selectedShippingOption?: ShippingOption;+};++type BuildOrderPatchPayloadOptions = {+    discount?: string;+    handling?: string;+    insurance?: string;+    itemTotal?: string;+    option?: ShippingOption;+    shippingOptions?: ShippingOption[];+    shippingDiscount?: string;+    taxTotal?: string;+};++export type OnShippingOptionsChangeActions = {+    buildOrderPatchPayload: ({+        discount,+        handling,+        insurance,+        itemTotal,+        shippingOptions,+        shippingDiscount,+        taxTotal,+    }: BuildOrderPatchPayloadOptions) => UpdateOrderRequestBody;+    reject: () => Promise<void>;+};++export type OnShippingAddressChangeData = {+    amount: CurrencyCodeAndValue;+    orderID?: string;+    paymentID?: string;+    paymentToken?: string;+    shippingAddress: {+        city: string;+        state: string;+        /** The [two-character ISO 3166-1 code](/docs/integration/direct/rest/country-codes/) that identifies the country or region. */+        countryCode: string;+        /**+         * The postal code, which is the zip code or equivalent.+         * Typically required for countries with a postal code or an equivalent.+         */+        postalCode: string;+    };+};++export type OnShippingAddressChangeActions = {+    buildOrderPatchPayload: ({+        discount,+        handling,+        insurance,+        itemTotal,+        shippingOptions,+        shippingDiscount,+        taxTotal,+    }: BuildOrderPatchPayloadOptions) => UpdateOrderRequestBody;+    reject: () => Promise<void>;+};

I'm with you on that, though I left at least one export in SPB even though it wasn't being imported anywhere. 😄

jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export type OnShippingChangeActions = {     }; }; +type CurrencyCodeAndValue = {+    currencyCode: string;+    value: string;+};

I think you're good here on defining a separate type, since it's not intended to be extended or used as a class contract like with an interface.

jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export interface PayPalButtonsComponentOptions {     onInit?: (data: Record<string, unknown>, actions: OnInitActions) => void;     /**      * Called when the buyer changes their shipping address on PayPal.+     * @deprecated Use `onShippingAddressChange` or `onShippingOptionsChange` instead.      */     onShippingChange?: (         data: OnShippingChangeData,         actions: OnShippingChangeActions,     ) => Promise<void>;+    /**+     * Called when the buyer selects a new shipping option on PayPal.+     */+    onShippingOptionsChange?: (+        data: OnShippingOptionsChangeData,+        actions: OnShippingOptionsChangeActions,+    ) => Promise<void>;+    /**+     * Called when the buyer their shipping address on PayPal.
     * Called when the buyer updates their shipping address on PayPal.
jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export type OnShippingChangeActions = {     }; }; +type CurrencyCodeAndValue = {+    currencyCode: string;+    value: string;+};++type ShippingOption = {+    amount: CurrencyCodeAndValue;+    id?: string;+    label: string;+    selected: boolean;+    type: "SHIPPING" | "PICKUP";+};++export type OnShippingOptionsChangeData = {+    orderID?: string;+    paymentID?: string;+    paymentToken?: string;+    selectedShippingOption?: ShippingOption;+};++type BuildOrderPatchPayloadOptions = {+    discount?: string;+    handling?: string;+    insurance?: string;+    itemTotal?: string;+    option?: ShippingOption;+    shippingOptions?: ShippingOption[];+    shippingDiscount?: string;+    taxTotal?: string;+};

How about using BuildOrderPatchPayloadArgs like in SPB?

jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export type OnShippingChangeActions = {     }; }; +type CurrencyCodeAndValue = {+    currencyCode: string;+    value: string;+};++type ShippingOption = {+    amount: CurrencyCodeAndValue;+    id?: string;+    label: string;+    selected: boolean;+    type: "SHIPPING" | "PICKUP";+};++export type OnShippingOptionsChangeData = {+    orderID?: string;+    paymentID?: string;+    paymentToken?: string;+    selectedShippingOption?: ShippingOption;+};++type BuildOrderPatchPayloadOptions = {+    discount?: string;+    handling?: string;+    insurance?: string;+    itemTotal?: string;+    option?: ShippingOption;+    shippingOptions?: ShippingOption[];+    shippingDiscount?: string;+    taxTotal?: string;+};++export type OnShippingOptionsChangeActions = {+    buildOrderPatchPayload: ({+        discount,+        handling,+        insurance,+        itemTotal,+        shippingOptions,

I'm thinking this should be singular, shippingOption, like here.

jshawl

comment created time in 6 days

Pull request review commentpaypal/paypal-js

feat: add type definitions for two new callbacks

 export type OnShippingChangeActions = {     }; }; +type CurrencyCodeAndValue = {+    currencyCode: string;+    value: string;+};++type ShippingOption = {+    amount: CurrencyCodeAndValue;+    id?: string;+    label: string;+    selected: boolean;+    type: "SHIPPING" | "PICKUP";+};

How would you feel about naming this CheckoutShippingOption to match the equivalent type in SPB?

jshawl

comment created time in 6 days

PullRequestReviewEvent
PullRequestReviewEvent

push eventpaypal/paypal-smart-payment-buttons

Nate Bierdeman

commit sha 0fbeaf7f9987a971cf0085bc53d8496df981ee05

Consolidate new shipping callback helper methods (#679) * Add multifunctional method * Remove unused file * Add tests for onShippingAddressChange actions * Refactor onShippingAddressChange buildPatchPayload * Refactor onShippingOptionsChange buildPatchPayload * Remove irrelevant tests * Refactor oSAC vitests Co-authored-by: Jesse Shawl <jshawl@paypal.com> * Fix oSAC bugs * Refactor oSOC vitests * Fix oSOC bugs * Fix lint errors * Shorten logic * Define newAmount inside of buildPatchPayload * Add more oSAC vitests * Add more oSOC vitests * selectedShippingOptionAmount refactor Co-authored-by: Shane Brunson <wbrunson@paypal.com> * Add more fields to oSAC buildPatchPayload * Add note * Add more fields to oSOC buildPatchPayload * Fix updateShippingOptions bug * Rename helper methods and types * Improve readability & maintainability * Fix shipping options patch operations bugs --------- Co-authored-by: Jesse Shawl <jshawl@paypal.com> Co-authored-by: Shane Brunson <wbrunson@paypal.com>

view details

push time in 6 days

PR merged paypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

Description

This PR consolidates the new shipping callback helper methods into a single method, buildOrderPatchPayload() (subject to change), with the exception of reject().

Dependent Changes

These are breaking changes. Checkout/RYP will need complementary changes.

❤️ Thank you!

+1174 -261

2 comments

8 changed files

nbierdeman

pr closed time in 6 days

delete branch paypal/paypal-smart-payment-buttons

delete branch : test-on-shipping

delete time in 6 days

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 describe("onShippingOptionsChange", () => {           },         })       ).toEqual([+        {+          op: "replace",

Yeah, here was my initial thinking:

  • we explicitly have add coverage for oSAC, which implicitly tests the replace logic
  • we explicitly have replace coverage for oSOC, which implicitly tests the add logic

If you think we should be explicit about both add and replace coverage for each callback, I'm down to add more tests!

nbierdeman

comment created time in 6 days

PullRequestReviewEvent

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 export function buildXOnShippingAddressChangeActions({ data, actions: passedActi                 throw new Error(`Missing reject action callback`);             }, -        updateTax: ({ tax }) => {-            breakdown = buildBreakdown({ breakdown, updatedAmounts: { tax_total: tax } });-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { tax_total: tax } });-        -            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };--            return actions;-        },--        updateShippingOptions: ({ options }) => {-            const ordersV2Options = optionsKeyChanges(options);-            const selectedShippingOption = options.filter(option => option.selected === true);+        buildPatchPayload: ({tax, options, discount} = {}) => {+            const selectedShippingOption = options?.filter(option => option.selected === true);             const selectedShippingOptionAmount = selectedShippingOption && selectedShippingOption.length > 0                 ? selectedShippingOption[0]?.amount?.value                 : '0.00'; -            breakdown = buildBreakdown({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });-        -            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };--            patchQueries[ON_SHIPPING_CHANGE_PATHS.OPTIONS] = {-                op:    data?.event || 'replace', // or 'add' if there are none.-                path:  ON_SHIPPING_CHANGE_PATHS.OPTIONS,-                value: ordersV2Options-            };--            return actions;-        },--        updateShippingDiscount: ({ discount }) => {-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { shipping_discount: discount } });-            breakdown = buildBreakdown({ breakdown, updatedAmounts: { shipping_discount: discount } });--            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };+            const updatedAmounts = {}; -            return actions;-        },+            if (tax) {+                updatedAmounts.tax_total = tax;+            }++            if (selectedShippingOption) {+                updatedAmounts.shipping = selectedShippingOptionAmount;+            }++            if (discount) {+                updatedAmounts.shipping_discount = discount+            }++            breakdown = buildBreakdown({ breakdown, updatedAmounts });+            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts });+            +            if (tax || (options && options.length > 0) || discount) {+                patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {+                    op:       'replace',+                    path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,+                    value: {+                        value:         `${ newAmount }`,+                        currency_code: data?.amount?.currencyCode,+                        breakdown+                    }+                };+            }++            if (options && options.length > 0) {+                const ordersV2Options = optionsKeyChanges(options);++                patchQueries[ON_SHIPPING_CHANGE_PATHS.OPTIONS] = {+                    op:    data?.event || 'replace', // or 'add' if there are none.

Just pushed ^ these changes!

nbierdeman

comment created time in 6 days

PullRequestReviewEvent

push eventpaypal/paypal-smart-payment-buttons

Nate Bierdeman

commit sha 946621b0f72abf1e9b265fab8e9b9b6e5a81dcbe

Fix shipping options patch operations bugs

view details

push time in 6 days

PullRequestReviewEvent

push eventpaypal/paypal-smart-payment-buttons

Nate Bierdeman

commit sha f11bad5b71794951b6a03780c6ca6c60b7c4f8eb

Improve readability & maintainability

view details

push time in 6 days

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 export function buildXOnShippingAddressChangeActions({ data, actions: passedActi                 throw new Error(`Missing reject action callback`);             }, -        updateTax: ({ tax }) => {-            breakdown = buildBreakdown({ breakdown, updatedAmounts: { tax_total: tax } });-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { tax_total: tax } });-        -            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };--            return actions;-        },--        updateShippingOptions: ({ options }) => {-            const ordersV2Options = optionsKeyChanges(options);-            const selectedShippingOption = options.filter(option => option.selected === true);+        buildPatchPayload: ({tax, options, discount} = {}) => {+            const selectedShippingOption = options?.filter(option => option.selected === true);             const selectedShippingOptionAmount = selectedShippingOption && selectedShippingOption.length > 0                 ? selectedShippingOption[0]?.amount?.value                 : '0.00'; -            breakdown = buildBreakdown({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });-        -            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };--            patchQueries[ON_SHIPPING_CHANGE_PATHS.OPTIONS] = {-                op:    data?.event || 'replace', // or 'add' if there are none.-                path:  ON_SHIPPING_CHANGE_PATHS.OPTIONS,-                value: ordersV2Options-            };--            return actions;-        },--        updateShippingDiscount: ({ discount }) => {-            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { shipping_discount: discount } });-            breakdown = buildBreakdown({ breakdown, updatedAmounts: { shipping_discount: discount } });--            patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {-                op:       'replace',-                path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,-                value: {-                    value:         `${ newAmount }`,-                    currency_code: data?.amount?.currencyCode,-                    breakdown-                }-            };+            const updatedAmounts = {}; -            return actions;-        },+            if (tax) {+                updatedAmounts.tax_total = tax;+            }++            if (selectedShippingOption) {+                updatedAmounts.shipping = selectedShippingOptionAmount;+            }++            if (discount) {+                updatedAmounts.shipping_discount = discount+            }++            breakdown = buildBreakdown({ breakdown, updatedAmounts });+            newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts });+            +            if (tax || (options && options.length > 0) || discount) {+                patchQueries[ON_SHIPPING_CHANGE_PATHS.AMOUNT] = {+                    op:       'replace',+                    path:     ON_SHIPPING_CHANGE_PATHS.AMOUNT,+                    value: {+                        value:         `${ newAmount }`,+                        currency_code: data?.amount?.currencyCode,+                        breakdown+                    }+                };+            }++            if (options && options.length > 0) {+                const ordersV2Options = optionsKeyChanges(options);++                patchQueries[ON_SHIPPING_CHANGE_PATHS.OPTIONS] = {+                    op:    data?.event || 'replace', // or 'add' if there are none.

Alright, I just synced with @mnicpt, and he said we should remove data?.event and have shippingMethods drive whether this ends up being replace or add. Will push up this change asap.

nbierdeman

comment created time in 9 days

PullRequestReviewEvent

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 describe('onShippingAddressChange', () => {                     })                 }); -                const query = await actions.query();+                const query = await actions.buildOrderPatchPayload();

Follow up: if we pass in taxTotal, then buildOrderPatchPayload() returns an array with a query object (expected), and the error on line 243 is thrown (also expected).

nbierdeman

comment created time in 9 days

PullRequestReviewEvent

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 describe('onShippingAddressChange', () => {                     })                 }); -                const query = await actions.query();+                const query = await actions.buildOrderPatchPayload();

I think it's working because first we expect the GQL call to not update the shipping (as if there's an error), and then we expect buildOrderPatchPayload() to return an empty array as a result of no shipping options (or anything else) being passed in (that's the long way of saying it's passing because it's invoked with no arguments).

nbierdeman

comment created time in 9 days

PullRequestReviewEvent

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 export function buildXOnShippingOptionsChangeActions({ data, actions: passedActi                 throw new Error(`Missing reject action callback`);             }, -        updateShippingOption: ({ option }) => {-            if (option && data.options) {-                const selectedShippingOptionAmount = option?.amount?.value;-                const options = optionsKeyChanges(updateShippingOptions({ option, options: data.options }));+        buildOrderPatchPayload: ({discount, handling, insurance, itemTotal, shippingOption, shippingDiscount, taxTotal} = {}) => {+            const selectedShippingOptionAmount = shippingOption?.amount?.value;+            const options = shippingOption && data.options ? optionsKeyChanges(updateShippingOptions({ option: shippingOption, options: data.options })) : undefined; -                newAmount = calculateTotalFromShippingBreakdownAmounts({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });-                breakdown = buildBreakdown({ breakdown, updatedAmounts: { shipping: selectedShippingOptionAmount } });+            const updatedAmounts = {}; -                if (options && options.length > 0) {-                    patchQueries[ON_SHIPPING_CHANGE_PATHS.OPTIONS] = {-                        op:    data?.event || 'replace', // or 'add' if there are none.-                        path:  ON_SHIPPING_CHANGE_PATHS.OPTIONS,-                        value: options-                    };-                }+            if (discount) {+                updatedAmounts.discount = discount;+            }++            if (handling) {+                updatedAmounts.handling = handling;+            }++            if (insurance) {+                updatedAmounts.insurance = insurance;+            }++            if (itemTotal) {+                updatedAmounts.item_total = itemTotal;+            }++            if (selectedShippingOptionAmount) {+                updatedAmounts.shipping = selectedShippingOptionAmount;+            }++            if (shippingDiscount) {+                updatedAmounts.shipping_discount = shippingDiscount;+            }++            if (taxTotal) {+                updatedAmounts.tax_total = taxTotal;+            }

Yeah, I hear you on sharing code/DRYing it up. 🤔

Another idea for these conditionals would be to grab the entire buildOrderPatchPayload() argument object and loop through it for mapping properties to updatedAmounts, but it'd probably be harder to read and reason about.

nbierdeman

comment created time in 9 days

PullRequestReviewEvent

Pull request review commentpaypal/paypal-smart-payment-buttons

Consolidate new shipping callback helper methods

 export function buildXOnShippingOptionsChangeActions({ data, actions: passedActi                 throw new Error(`Missing reject action callback`);             }, -        updateShippingOption: ({ option }) => {-            if (option && data.options) {-                const selectedShippingOptionAmount = option?.amount?.value;-                const options = optionsKeyChanges(updateShippingOptions({ option, options: data.options }));+        buildOrderPatchPayload: ({discount, handling, insurance, itemTotal, shippingOption, shippingDiscount, taxTotal} = {}) => {

I'm amazed you notice! 🤣

nbierdeman

comment created time in 9 days

more