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

Defining ThreeDomainSecureClient component interface #2447

Merged
merged 10 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
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
3 changes: 3 additions & 0 deletions __sdk__.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,7 @@ module.exports = {
"shopper-insights": {
entry: "./src/shopper-insights/interface",
},
"three-domain-secure": {
entry: "./src/three-domain-secure/interface",
},
};
2 changes: 1 addition & 1 deletion dist/button.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/test/button.js

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@
"puppeteer": "^1.20.0",
"serve": "^14.0.0",
"vite": "^3.2.4",
"vitest": "^1.3.1"
"vitest": "^1.3.1",
"flow-remove-types": "2.246.0",
"hermes-parser": "0.23.1"
},
"dependencies": {
"@krakenjs/beaver-logger": "^5.7.0",
Expand Down
70 changes: 46 additions & 24 deletions src/three-domain-secure/component.jsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,52 @@
/* @flow */
import { getLogger, getSDKToken } from "@paypal/sdk-client/src";
import { type LoggerType } from "@krakenjs/beaver-logger/src";
import { FPTI_KEY } from "@paypal/sdk-constants/src";

import { ValidationError } from "../lib";

export const getThreeDomainSecure = (): Function => {
const sdkToken = getSDKToken();
const ThreeDomainSecureAuth = () => {
if (sdkToken) {
// eslint-disable-next-line no-console
console.log("Three Domain Secure Called");
// Make a Zoid component and introduce methods here
// onSuccess
// onCancel
// onClose
getLogger()
.info("three domain secure v2 invoked")
.track({
[FPTI_KEY.TRANSITION]: "three_DS_auth_v2",
});
} else {
throw new ValidationError(
`script data attribute sdk-client-token is required but was not passed`
);
}
};

return ThreeDomainSecureAuth;
type SdkConfig = {|
sdkToken: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can probably remove the ? here. It is required to be a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this creates a flow error. I believe that it is possible for null or undefined to be returned by getSDKToken which is why it should be optional

|};

const parseSdkConfig = ({ sdkConfig, logger }): SdkConfig => {
if (!sdkConfig.sdkToken) {
throw new ValidationError(
`script data attribute sdk-client-token is required but was not passed`
);
}

logger.info("three domain secure v2 invoked").track({
[FPTI_KEY.TRANSITION]: "three_DS_auth_v2",
});

return sdkConfig;
};
export interface ThreeDomainSecureComponentInterface {
isEligible(): void;
mchoun marked this conversation as resolved.
Show resolved Hide resolved
show(): void;
}
export class ThreeDomainSecureComponent {
logger: LoggerType;
sdkConfig: SdkConfig;

constructor({
logger,
sdkConfig,
}: {|
logger: LoggerType,
sdkConfig: SdkConfig,
|}) {
this.logger = logger;
this.sdkConfig = parseSdkConfig({ sdkConfig, logger });
}

isEligible() {
// eslint-disable-next-line no-console
console.log("eligible");
}

show() {
// eslint-disable-next-line no-console
console.log("show");
}
}
83 changes: 50 additions & 33 deletions src/three-domain-secure/component.test.js
Original file line number Diff line number Diff line change
@@ -1,44 +1,61 @@
/* @flow */
import { describe, expect, it, vi } from "vitest";
import { getSDKToken } from "@paypal/sdk-client/src";
import { describe, expect, vi } from "vitest";

import { ValidationError } from "../lib";
import { ThreeDomainSecureComponent } from "./component";

import { getThreeDomainSecure } from "./component";
const defaultSdkConfig = {
sdkToken: "sdk-client-token",
};

vi.mock("@paypal/sdk-client/src", () => ({
getSDKToken: vi.fn(),
getLogger: vi.fn(() => ({
const createThreeDomainSecureComponent = ({
sdkConfig = defaultSdkConfig,
logger = {
info: vi.fn().mockReturnThis(),
warn: vi.fn().mockReturnThis(),
error: vi.fn().mockReturnThis(),
track: vi.fn().mockReturnThis(),
flush: vi.fn().mockReturnThis(),
})),
}));
vi.mock("../lib", () => ({
ValidationError: vi.fn(),
}));
describe("getThreeDomainSecure returns ThreeDomainSecureComponent", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want to keep this test?

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 moved this test further down on Line 46 as a general initialization test. Instead of mocking the ValidationError I'm just asserting that it would throw an error. Is this OK or does it change the test? I wasn't entirely sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yeah that should work too. My idea with that test was to capture validationError specifically since it will be one of the trace events in the future.
This is not a blocker for the PR to be merged

it("should throw an error if sdkToken is not present", () => {
// $FlowFixMe prop missing error
getSDKToken.mockReturnValue(undefined);
const ThreeDomainSecureComponent = getThreeDomainSecure();
expect(() => ThreeDomainSecureComponent()).toThrowError(ValidationError);
expect(ValidationError).toHaveBeenCalledWith(
`script data attribute sdk-client-token is required but was not passed`
);
metricCounter: vi.fn().mockReturnThis(),
},
} = {}) =>
new ThreeDomainSecureComponent({
sdkConfig,
// $FlowIssue
logger,
});
it("should return the ThreeDomainSecure component and log the correct message", async () => {
// eslint-disable-next-line no-empty-function
const consoleSpy = vi.spyOn(console, "log").mockImplementation(() => {});
// $FlowFixMe prop missing error
getSDKToken.mockReturnValue("84ghb8984");
const ThreeDomainSecureComponent = getThreeDomainSecure();
expect(typeof ThreeDomainSecureComponent).toBe("function");

// Call the returned component and check the console log
await ThreeDomainSecureComponent();
expect(consoleSpy).toHaveBeenCalledWith("Three Domain Secure Called");
afterEach(() => {
vi.clearAllMocks();
});

describe("three domain secure component - isEligible method", () => {
test("should console log eligible", () => {
const consoleSpy = vi.spyOn(console, "log");
const threeDomainSecuretClient = createThreeDomainSecureComponent();
threeDomainSecuretClient.isEligible();
expect(consoleSpy).toHaveBeenCalledWith("eligible");
});
});

consoleSpy.mockRestore();
describe("three domain descure component - show method", () => {
test("should console log show", () => {
const consoleSpy = vi.spyOn(console, "log");
const threeDomainSecuretClient = createThreeDomainSecureComponent();
threeDomainSecuretClient.show();
expect(consoleSpy).toHaveBeenCalledWith("show");
});
});

describe("three domain secure component - initialization", () => {
test("should throw an error if sdkToken is not present", () => {
expect(() =>
createThreeDomainSecureComponent({
sdkConfig: {
...defaultSdkConfig,
sdkToken: "",
},
})
).toThrowError(
`script data attribute sdk-client-token is required but was not passed`
);
});
});
27 changes: 20 additions & 7 deletions src/three-domain-secure/interface.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,26 @@
/* @flow */
import { type ZoidComponent } from "@krakenjs/zoid/src";
import { getLogger, getSDKToken } from "@paypal/sdk-client/src";

import type { LazyExport } from "../types";
import { protectedExport } from "../lib";

import { getThreeDomainSecure } from "./component";
import {
ThreeDomainSecureComponent,
type ThreeDomainSecureComponentInterface,
} from "./component";

type ThreeDomainSecureAuth = ZoidComponent<void>;

export const ThreeDomainSecureComponent: LazyExport<ThreeDomainSecureAuth> = {
__get__: () => protectedExport(getThreeDomainSecure()),
};
export const ThreeDomainSecureClient: LazyExport<ThreeDomainSecureComponentInterface> =
{
__get__: () => {
const threeDomainSecureInstance = new ThreeDomainSecureComponent({
logger: getLogger(),
sdkConfig: {
sdkToken: getSDKToken(),
},
});
return protectedExport({
isEligible: () => threeDomainSecureInstance.isEligible(),
show: () => threeDomainSecureInstance.show(),
});
},
};
22 changes: 12 additions & 10 deletions src/zoid/card-fields/component.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ const CARD_FIELD_TYPE = {
};

type InstallmentsConfiguration = {|
financingCountryCode : string,
currencyCode : string,
billingCountryCode : string,
amount : string,
includeBuyerInstallments ? : boolean
financingCountryCode: string,
currencyCode: string,
billingCountryCode: string,
amount: string,
includeBuyerInstallments?: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we want to remove the lint updates out of this PR for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I can create a separate PR for the lint commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave it in for this one. But also the comment, so it is easier to trace back that it is simply a lint update and not actual code changes.

|};

type CardFieldsProps = {|
Expand Down Expand Up @@ -109,10 +109,12 @@ type CardFieldsProps = {|
hcfSessionID: string,
partnerAttributionID: string,
merchantID: $ReadOnlyArray<string>,
installments? : {|
onInstallmentsRequested : () => InstallmentsConfiguration | ZalgoPromise<InstallmentsConfiguration>,
onInstallmentsAvailable : (Object) => void,
onInstallmentsError? : (Object) => void
installments?: {|
onInstallmentsRequested: () =>
| InstallmentsConfiguration
| ZalgoPromise<InstallmentsConfiguration>,
onInstallmentsAvailable: (Object) => void,
onInstallmentsError?: (Object) => void,
|},
|};

Expand Down Expand Up @@ -445,7 +447,7 @@ export const getCardFieldsComponent: () => CardFieldsComponent = memoize(
installments: {
type: "object",
required: false,
value: ({ props }) => props.parent.props.installments
value: ({ props }) => props.parent.props.installments,
},
},
});
Expand Down
Loading