Skip to content

ncps - expand inline guest #2356

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 13, 2024
Merged

ncps - expand inline guest #2356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4700aa
ncps - expand inline guest
jshawl Mar 7, 2024
81bd599
redirect after onApprove
jshawl Mar 7, 2024
f661aa0
remove unused type
jshawl Mar 7, 2024
dda07c2
remove popup mock
jshawl Mar 7, 2024
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
9 changes: 9 additions & 0 deletions src/hosted-buttons/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getClientID,
getMerchantID as getSDKMerchantID,
} from "@paypal/sdk-client/src";
import { FUNDING } from "@paypal/sdk-constants/src";

import type {
ButtonVariables,
Expand Down Expand Up @@ -208,6 +209,14 @@ export const buildHostedButtonOnApprove = ({
merchant_id: merchantId,
context_id: data.orderID,
}),
}).then((response) => {
// The "Debit or Credit Card" button does not open a popup
Copy link
Member

Choose a reason for hiding this comment

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

It does sometimes open a popup though right? or are we saying in the ncps case it ALWAYS does the expand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does sometimes open a popup - e.g. clicking on the debit or credit card button before second render. I tested this scenario and made sure a second popup isn't opened though it's not obvious to me just yet why that isn't a bug 🤔

Copy link
Member

Choose a reason for hiding this comment

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

does it redirect inside the popup instead of the parent page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it closes the popup and then full page redirects from the merchant site to the thank you page

Copy link
Member

Choose a reason for hiding this comment

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

oh wow, so this handles both cases. That's great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! though I think what's happening under the hood is that this window.location = is forcing the popup to close in the beforeUnload event. Similar to:

  1. Click on paypal button (popup opens)
  2. open dev console and window.location = 'https://example.com/'
  3. Observe popup close

I think this is an OK tradeoff but wanted to be transparent about why this "works". An alternative would be only invoking window.location= if the popup window wasn't opened because of a prerender click (not sure what's involved in that but I bet it will be challenging / complex)

Copy link
Member

Choose a reason for hiding this comment

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

would there be enough time to close it through onApprove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! But I think the correct way would be to not close the popup - e.g. redirect from within the popup to the thank you page (identical to the PayPal button functionality)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g. I don't think there's a way to know that onApprove was invoked from a prerender-click context: I'll timebox it early next week and see if it's a quick win but otherwise I think we should go with what's in this PR - wdyt? I'm def open to other ideas

Copy link
Member

Choose a reason for hiding this comment

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

i'm down for the bare minimum that matches what product is asking for. Whatever that is lol but feel free to do as much exploration as you want

// so we need to redirect to the thank you page for buyers who complete
// a checkout via "Debit or Credit Card".
if (data.paymentSource === FUNDING.CARD) {
window.location = `${baseUrl}/ncp/payment/${hostedButtonId}/${data.orderID}`;
}
return response;
});
};
};
22 changes: 22 additions & 0 deletions src/hosted-buttons/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,28 @@ describe("buildHostedButtonOnApprove", () => {
);
expect.assertions(1);
});

describe("inline guest", () => {
const onApprove = buildHostedButtonOnApprove({
hostedButtonId,
merchantId,
});
// $FlowIssue
request.mockImplementation(() =>
// eslint-disable-next-line compat/compat
Promise.resolve({
body: {},
})
);

test("redirects from the merchant's site to a thank you page", async () => {
expect(window.location.href).toBe("http://localhost:3000/");
await onApprove({ orderID, paymentSource: "card" });
expect(window.location).toBe(
"https://example.com/ncp/payment/B1234567890/EC-1234567890"
);
});
});
});

/* eslint-enable no-restricted-globals, promise/no-native */
Loading