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

OWA-107: Fix broken SCA flow payment finalization #677

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

mirovladimitrovski
Copy link
Collaborator

@mirovladimitrovski mirovladimitrovski commented Mar 12, 2025

Description

This PR solves OWA-107

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

Copy link

github-actions bot commented Mar 12, 2025

Visit the preview URL for this PR (updated for commit 84f900a):

https://ottwebapp--pr677-owa-107-sca-ppv-flow-u6gyrnzp.web.app

(expires Wed, 16 Apr 2025 11:59:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@ChristiaanScheermeijer
Copy link
Collaborator

Hi @mirovladimitrovski, I believe this fix is for JWP integrations only, should we handle this from the JWP integration instead of making it a separate component/modal?

I think if we do something like this in the JWPAccountService.ts together with the reload subscription it should work basically the same, right?

  private async handlePPVPaymentIntent(url: string) {
    const searchParams = new URLSearchParams(url.split('?')[1]);
    const paymentIntent = searchParams.get('payment_intent');

    if (paymentIntent) {
      await this.apiService.post<CommonResponse>('/payments', { pi_id: paymentIntent }, { withAuthentication: true });
    }
  }

  initialize = async (config: Config, url: string, _logoutFn: () => Promise<void>) => {
    const jwpConfig = config.integrations?.jwp;

    await this.handlePPVPaymentIntent(url);

@mirovladimitrovski
Copy link
Collaborator Author

Hi @mirovladimitrovski, I believe this fix is for JWP integrations only, should we handle this from the JWP integration instead of making it a separate component/modal?

I think if we do something like this in the JWPAccountService.ts together with the reload subscription it should work basically the same, right?

  private async handlePPVPaymentIntent(url: string) {
    const searchParams = new URLSearchParams(url.split('?')[1]);
    const paymentIntent = searchParams.get('payment_intent');

    if (paymentIntent) {
      await this.apiService.post<CommonResponse>('/payments', { pi_id: paymentIntent }, { withAuthentication: true });
    }
  }

  initialize = async (config: Config, url: string, _logoutFn: () => Promise<void>) => {
    const jwpConfig = config.integrations?.jwp;

    await this.handlePPVPaymentIntent(url);

This is a good idea actually, much lighter in code. I'll have it done shortly.

@mirovladimitrovski
Copy link
Collaborator Author

@ChristiaanScheermeijer
In practice, your suggestion introduces a different problem. The problem is that the confirmation request is done very early during the app's boot-up, before we're subscribed to the notification websocket that is supposed to notify us that access has been granted. I still think the idea is good though, so I'm working on solving the problem.


const finalize = useCallback(async (paymentIntent: string) => {
try {
await checkoutController.finalizeStripePpvPayment(paymentIntent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try to reuse everything in FanalizeAdyenPayment except using a different method instead of a checkoutController.finalizeAdyenPayment one?

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.

3 participants