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

feat: add OPF integration #17250

Draft
wants to merge 310 commits into
base: develop
Choose a base branch
from
Draft

feat: add OPF integration #17250

wants to merge 310 commits into from

Conversation

Matejk00
Copy link
Contributor

No description provided.

@Matejk00 Matejk00 requested review from a team as code owners April 11, 2023 09:39
@Matejk00 Matejk00 requested a review from a team April 11, 2023 09:39
@github-actions github-actions bot marked this pull request as draft April 11, 2023 09:39
@cla-assistant
Copy link

cla-assistant bot commented Apr 11, 2023

CLA assistant check
All committers have signed the CLA.

FollowTheFlo
FollowTheFlo previously approved these changes Apr 11, 2023
@Matejk00 Matejk00 temporarily deployed to dev April 17, 2023 14:33 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 19, 2023 10:38 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 19, 2023 12:17 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 19, 2023 13:45 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 20, 2023 10:19 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 21, 2023 12:58 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 24, 2023 08:09 — with GitHub Actions Inactive
@FollowTheFlo FollowTheFlo temporarily deployed to dev April 24, 2023 14:08 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 24, 2023 15:20 — with GitHub Actions Inactive
@Matejk00 Matejk00 temporarily deployed to dev April 25, 2023 06:25 — with GitHub Actions Inactive
@rmch91 rmch91 temporarily deployed to dev April 25, 2023 08:39 — with GitHub Actions Inactive
@rmch91 rmch91 temporarily deployed to dev April 25, 2023 11:02 — with GitHub Actions Inactive
@FollowTheFlo FollowTheFlo temporarily deployed to dev April 25, 2023 16:45 — with GitHub Actions Inactive
@rmch91 rmch91 temporarily deployed to dev April 26, 2023 10:49 — with GitHub Actions Inactive
@rmch91 rmch91 temporarily deployed to dev April 28, 2023 09:37 — with GitHub Actions Inactive
@FollowTheFlo FollowTheFlo temporarily deployed to dev April 28, 2023 15:02 — with GitHub Actions Inactive
@FollowTheFlo FollowTheFlo temporarily deployed to dev May 1, 2023 17:25 — with GitHub Actions Inactive
@rmch91 rmch91 temporarily deployed to dev May 2, 2023 12:49 — with GitHub Actions Inactive
@Matejk00 Matejk00 changed the base branch from develop to develop-6.1.x May 5, 2023 09:48
Comment on lines 113 to 120
switchMap(([userId, cartId]: [string, string]) => {
this.activeCartId = cartId;
return this.cartAccessCodeService.getCartAccessCode(userId, cartId);
}),
filter((response) => Boolean(response?.accessCode)),
map(({ accessCode: otpKey }) =>
this.setPaymentInitiationConfig(otpKey, paymentOptionId)
),
Copy link
Contributor

Choose a reason for hiding this comment

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

[MEDIUM]

If possible, let's not mix static and reactive programming Here we're making a side-effect of setting a static property this.activeCartId in the Observable flow, and later in this flow we read the value from the static property.
Mixing static and reactive programming is harder to reason about and also prone to issues in async edge cases.

  1. I suggest to remove the property protected activeCartId, as not necessary.

  2. Then we can simply pass the cartId to the method this.setPaymentInitiationConfig(), e.g. as a first argument:

Suggested change
switchMap(([userId, cartId]: [string, string]) => {
this.activeCartId = cartId;
return this.cartAccessCodeService.getCartAccessCode(userId, cartId);
}),
filter((response) => Boolean(response?.accessCode)),
map(({ accessCode: otpKey }) =>
this.setPaymentInitiationConfig(otpKey, paymentOptionId)
),
switchMap(([userId, cartId]: [string, string]) =>
this.cartAccessCodeService.getCartAccessCode(userId, cartId).pipe(
filter((response) => Boolean(response?.accessCode)),
map(({ accessCode: otpKey }) =>
this.setPaymentInitiationConfig(cartId, otpKey, paymentOptionId)
)
)
),
    protected setPaymentInitiationConfig(
+     cartId: string,
      otpKey: string,
      paymentOptionId: number
    ) {
      return {
        otpKey,
        config: {
          configurationId: String(paymentOptionId),
-         cartId: this.activeCartId,
+         cartId

selector: 'cx-opf-verify-payment',
templateUrl: './opf-payment-verification.component.html',
})
export class OpfPaymentVerificationComponent implements OnInit, OnDestroy {
Copy link
Contributor

Choose a reason for hiding this comment

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

[MAJOR]

JS of this component (and its indirect dependencies like OpfPaymentVerificationService and OpfResourceLoaderService) is now eagerly loaded in main.js on every page. It unnecessarily slows down the load of SEO-critical pages like homepage, PDP, PLP.

Note: it happens only when the OPF feature is installed by the customer.

Instead, I suggest to move out as much a spossible of the logic of this component to a lazy-loadable service/facade. Thanks to that, only the component's definition (and a tiny HTML template) will be loaded in main.js, but the actual logic of this component (and its indirect dependencies) will be lazy-loaded only when entering the payment-verification page.

integration-libs/opf/cta/root/opf-cta-root.module.ts Outdated Show resolved Hide resolved
Comment on lines +43 to +48
constructor(
protected launchDialogService: LaunchDialogService,
protected el: ElementRef,
protected cd: ChangeDetectorRef,
protected opfErrorModalService: OpfErrorModalService
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[MEDIUM]

According to our Coding Guidelines, in newly written source code, let's use inject() function instead of constructors for Dependency Injection.

You might find this automated conversion tool useful: https://ngxtension.netlify.app/utilities/migrations/inject-migration/

integration-libs/opf/base/root/opf-base-root.module.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants