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

[ncp] handle create order errors #2320

Merged
merged 2 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/hosted-buttons/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getButtonsComponent } from "../zoid/buttons";
import {
buildHostedButtonCreateOrder,
buildHostedButtonOnApprove,
buildOpenPopup,
getHostedButtonDetails,
renderForm,
getMerchantID,
Expand All @@ -24,14 +25,16 @@ export const getHostedButtonsComponent = (): HostedButtonsComponent => {
const merchantId = getMerchantID();

getHostedButtonDetails({ hostedButtonId }).then(
({ html, htmlScript, style }) => {
({ html, htmlScript, style, popupFallback }) => {
const { onInit, onClick } = renderForm({
hostedButtonId,
html,
htmlScript,
selector,
});

const openPopup = buildOpenPopup({ selector, popupFallback });

// $FlowFixMe
Buttons({
hostedButtonId,
Expand All @@ -45,6 +48,7 @@ export const getHostedButtonsComponent = (): HostedButtonsComponent => {
onApprove: buildHostedButtonOnApprove({
hostedButtonId,
merchantId,
openPopup,
}),
}).render(selector);
}
Expand Down
9 changes: 8 additions & 1 deletion src/hosted-buttons/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export type HostedButtonsComponentProps = {|
export type GetCallbackProps = {|
hostedButtonId: string,
merchantId?: string,
openPopup?: (url: string) => void,
|};

export type HostedButtonsInstance = {|
Expand All @@ -25,6 +26,7 @@ export type HostedButtonDetailsParams =
color: string,
label: string,
|},
popupFallback: string,
|}>;

export type ButtonVariables = $ReadOnlyArray<{|
Expand All @@ -34,7 +36,7 @@ export type ButtonVariables = $ReadOnlyArray<{|

export type CreateOrder = (data: {|
paymentSource: string,
|}) => ZalgoPromise<string>;
|}) => ZalgoPromise<string | void>;

export type OnApprove = (data: {|
orderID: string,
Expand All @@ -55,3 +57,8 @@ export type RenderForm = ({|
onInit: (data: mixed, actions: mixed) => void,
onClick: (data: mixed, actions: mixed) => void,
|};

export type BuildOpenPopup = ({|
popupFallback: string,
selector: string | HTMLElement,
|}) => (url: string) => void;
44 changes: 32 additions & 12 deletions src/hosted-buttons/utils.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* @flow */

import { request, memoize, popup, supportsPopups } from "@krakenjs/belter/src";
import { request, memoize, popup } from "@krakenjs/belter/src";
import {
getSDKHost,
getClientID,
Expand All @@ -11,6 +11,7 @@ import { FUNDING } from "@paypal/sdk-constants/src";
import { DEFAULT_POPUP_SIZE } from "../zoid/checkout";

import type {
BuildOpenPopup,
ButtonVariables,
CreateAccessToken,
CreateOrder,
Expand All @@ -23,6 +24,7 @@ import type {
const entryPoint = "SDK";
const baseUrl = `https://${getSDKHost()}`;
const apiUrl = baseUrl.replace("www", "api");
export const popupFallbackClassName = "paypal-popup-fallback";

const getHeaders = (accessToken?: string) => ({
...(accessToken && { Authorization: `Bearer ${accessToken}` }),
Expand Down Expand Up @@ -75,6 +77,7 @@ export const getHostedButtonDetails: HostedButtonDetailsParams = ({
},
html: body.html,
htmlScript: body.html_script,
popupFallback: body.popup_fallback,
};
});
};
Expand Down Expand Up @@ -115,6 +118,7 @@ export const buildHostedButtonCreateOrder = ({
return (data) => {
const userInputs =
window[`__pp_form_fields_${hostedButtonId}`]?.getUserInputs?.() || {};
const onError = window[`__pp_form_fields_${hostedButtonId}`]?.onError;
return createAccessToken(getClientID()).then((accessToken) => {
return request({
url: `${apiUrl}/v1/checkout/links/${hostedButtonId}/create-context`,
Expand All @@ -126,16 +130,37 @@ export const buildHostedButtonCreateOrder = ({
merchant_id: merchantId,
...userInputs,
}),
}).then(({ body }) => {
return body.context_id;
});
})
.then(({ body }) => body.context_id || onError(body.name))
.catch(() => onError("REQUEST_FAILED"));
});
};
};

export const buildOpenPopup: BuildOpenPopup = ({ popupFallback, selector }) => {
return (url) => {
try {
popup(url, {
width: DEFAULT_POPUP_SIZE.WIDTH,
height: DEFAULT_POPUP_SIZE.HEIGHT,
});
} catch (e) {
const div = document.createElement("div");
div.classList.add(popupFallbackClassName);
div.innerHTML = popupFallback.replace("#", url);
const container =
typeof selector === "string"
? document.querySelector(selector)
: selector;
container?.appendChild(div);
}
};
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I like this implementation. I did it so that I didn't have to pass popupFallback and selector down into onApprove. e.g. the alternative to this would be:

onApprove: buildHostedButtonOnApprove({
  merchantId,
  hostedButtonId,
  selector,
  popupFallback
})

and then inside of buildHostedButtonOnApprove would be the contents of this function. I did it this way so that I could easily unit-test it without managing passing around props but now that I'm looking at it in the GitHub diff it looks a little confusing. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ease of testing is a good reason in my book for going this route. Thanks for explaining this!


export const buildHostedButtonOnApprove = ({
hostedButtonId,
merchantId,
openPopup,
}: GetCallbackProps): OnApprove => {
return (data) => {
return createAccessToken(getClientID()).then((accessToken) => {
Expand All @@ -149,19 +174,14 @@ export const buildHostedButtonOnApprove = ({
context_id: data.orderID,
}),
}).then((response) => {
// remove the popup fallback message, if present
document.querySelector(`.${popupFallbackClassName}`)?.remove();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is another weird thing in this PR. I was really hoping to have all the dom manipulation contained within the popup handling code, but we need this here to handle a specific use case:

  1. buyer uses inline guest to checkout, has popup blocked, error message is rendered in the dom
  2. buyer uses a different payment method ("paypal") and when they click on the button, there should no longer be an error message.

// The "Debit or Credit Card" button does not open a popup
// so we need to open a new popup for buyers who complete
// a checkout via "Debit or Credit Card".
if (data.paymentSource === FUNDING.CARD) {
const url = `${baseUrl}/ncp/payment/${hostedButtonId}/${data.orderID}`;
if (supportsPopups()) {
popup(url, {
width: DEFAULT_POPUP_SIZE.WIDTH,
height: DEFAULT_POPUP_SIZE.HEIGHT,
});
} else {
window.location = url;
}
openPopup?.(url);
}
return response;
});
Expand Down
101 changes: 87 additions & 14 deletions src/hosted-buttons/utils.test.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
/* @flow */

import { test, expect, vi } from "vitest";
import { request, popup, supportsPopups } from "@krakenjs/belter/src";
import { request, popup } from "@krakenjs/belter/src";
import { ZalgoPromise } from "@krakenjs/zalgo-promise/src";

import {
buildHostedButtonCreateOrder,
buildHostedButtonOnApprove,
buildOpenPopup,
getHostedButtonDetails,
popupFallbackClassName,
} from "./utils";

vi.mock("@krakenjs/belter/src", async () => {
Expand Down Expand Up @@ -101,6 +103,31 @@ test("buildHostedButtonCreateOrder", async () => {
expect.assertions(1);
});

test("buildHostedButtonCreateOrder error handling", async () => {
const createOrder = buildHostedButtonCreateOrder({
hostedButtonId,
merchantId,
});

// $FlowIssue
request.mockImplementation(() =>
ZalgoPromise.resolve({
body: {
name: "RESOURCE_NOT_FOUND",
},
})
);

const onError = vi.fn();
window[`__pp_form_fields_${hostedButtonId}`] = {
onError,
};

await createOrder({ paymentSource: "paypal" });
expect(onError).toHaveBeenCalledWith("RESOURCE_NOT_FOUND");
expect.assertions(1);
});

describe("buildHostedButtonOnApprove", () => {
test("makes a request to the Hosted Buttons API", async () => {
const onApprove = buildHostedButtonOnApprove({
Expand All @@ -127,10 +154,20 @@ describe("buildHostedButtonOnApprove", () => {
expect.assertions(1);
});

test("provides its own popup for inline guest", async () => {
describe("inline guest", () => {
const url = "https://example.com/ncp/payment/B1234567890/EC-1234567890";
const selector = "buttons-container";
// $FlowIssue
document.body.innerHTML = `<div id="${selector}"></div>`; // eslint-disable-line compat/compat
const popupFallback = `<a href="#">See payment details</a>`;
const openPopup = buildOpenPopup({
popupFallback,
selector: `#${selector}`,
});
const onApprove = buildHostedButtonOnApprove({
hostedButtonId,
merchantId,
openPopup,
});
// $FlowIssue
request.mockImplementation(() =>
Expand All @@ -139,19 +176,55 @@ describe("buildHostedButtonOnApprove", () => {
})
);

// $FlowIssue
supportsPopups.mockImplementation(() => true);
await onApprove({ orderID, paymentSource: "card" });
expect(popup).toHaveBeenCalled();
test("provides its own popup", async () => {
await onApprove({ orderID, paymentSource: "card" });
expect(popup).toHaveBeenCalledWith(url, expect.anything());
expect.assertions(1);
});

// but redirects if popups are not supported
// $FlowIssue
supportsPopups.mockImplementation(() => false);
await onApprove({ orderID, paymentSource: "card" });
expect(window.location).toMatch(
`/ncp/payment/${hostedButtonId}/${orderID}`
);
test("appends a link if the popup is blocked", async () => {
// $FlowIssue
popup.mockImplementationOnce(() => {
throw new Error("popup_blocked");
});
await onApprove({ orderID, paymentSource: "card" });
const link = document.querySelector(`.${popupFallbackClassName} a`);
expect(link?.getAttribute("href")).toBe(url);
expect.assertions(1);
});

expect.assertions(2);
test("does not append a second link if the popup is blocked a second time", async () => {
// still present from the previous popup open failure
expect(
document.querySelectorAll(`.${popupFallbackClassName}`).length
).toBe(1);
// $FlowIssue
popup.mockImplementationOnce(() => {
throw new Error("popup_blocked");
});
await onApprove({ orderID, paymentSource: "card" });
expect(
document.querySelectorAll(`.${popupFallbackClassName}`).length
).toBe(1);
expect.assertions(2);
});

test("removes the fallback message if a different payment source is used", async () => {
// still present from the previous popup open failure
expect(
document.querySelectorAll(`.${popupFallbackClassName}`).length
).toBe(1);
// $FlowIssue
popup.mockImplementationOnce(() => {
throw new Error("popup_blocked");
});

// note new payment source
await onApprove({ orderID, paymentSource: "paypal" });
expect(
document.querySelectorAll(`.${popupFallbackClassName}`).length
).toBe(0);
expect.assertions(2);
});
});
});
Loading