[PW-4180] Multi-Step / Custom checkouts (e.g. with headless) - Various issues

[AndreasA]

Describe the bug Not sure, if I have found them all yet but in one project, the checkout is setup like this:

• Step 1: Enter shipping method and addresses
• Step 2: Select payment method
• Technicall still step 2 but only after the payment method has been selected: Enter payment details
• Step 3: Summary plus entering additional data (one is required)

This results in:

• The adyen payment data is deleted as soon as one enters data in step 3 as you clear the context on all context changes which can also just be a currency change: https://github.com/Adyen/adyen-shopware6/blob/1.5.0/src/Subscriber/PaymentSubscriber.php#L370
• Saving the adyen data requires the payment method ID to be sent with it. However, as mentioned above, this can be done in a separate request and then it does not make sense to sent it again: https://github.com/Adyen/adyen-shopware6/blob/1.5.0/src/Subscriber/PaymentSubscriber.php#L341

Possible solutions:

• Use payment method ID from context, if the parameter paymentMethodId has not been provided as that is the current valid payment method ID then as it didn't change.
• Clear the adyen payment only for some context changes, e.g. if the payment method changes. Probably the only one it should be cleared actually.
• Add a scheduled task (like you already did for other stuff) that periodically checks if there are old/expired entries and remove them to avoid old payment data from incomplete checkouts filling up the database table. Maybe in combination with the context lifetime parameter: https://github.com/shopware/platform/blob/v6.3.5.1/src/Core/System/SalesChannel/Context/SalesChannelContextPersister.php#L35

[peterojo]

Hi @AndreasA, thanks for the suggestions. Indeed it does make sense to save/clear the payment state data only when the payment method is changed (i.e. when paymentMethodId exists in the request). However, I don't think it is necessary to add a scheduled task, the saveStateData method reliably cleans up old data whenever the PM changes and the new PM has no state data. Please let us know if you encounter a bug/edge-case in which old state data remains in the DB

[AndreasA]

@peterojo with the change regarding payment method ID now the data will not be cleared that often anymore. also if someone stops the payment altogether I don't think it is cleared at all now?

also shouldn't you also call save Data, if adyenState has been transmited and not only if payment method id has been transmited?

[peterojo]

Hey @AndreasA you make a good point about the flow when someone abandons the cart altogether after selecting a payment method. I will make a follow up internal ticket to deal with this. To your other point, we are accepting the state.data and paymentMethodId in the same request, this way we can better ensure that at most one state data record exists for the current context token, i.e. if the payment method changes, then we need to also remove the existing state data, if any, and/or save the current one. There's also the possibility to send the state data directly with the payment request to /handle-payment without saving it in the database. See here, at the moment we use this for payment components with a direct pay button, like Google pay

[AndreasA]

@peterojo

What about:

• Step 1: Select payment method: credit card.
• Step 2: Enter payment details
• Step 3: Summary
• Step 4: Submit.

I really don't want to send the existing payment method Id again in this scenario. I would prefer to just send the payment details in this case.

[peterojo]

Hey @AndreasA, It is possible to allow you to send the payment data as a separate request after selecting the payment method, we can get the saved payment method from the context. What do you think of the following flow, when responding to the SalesChannelContextSwitchEvent:

if `paymentMethodId` exists:
    save / clear state data for current context
else if `adyenStateData` exists AND current selected PM is Adyen PM:
    save / clear state data for current context
else:
    do nothing

[AndreasA]

@peterojo I think that would work, However, maybe make some minor adjustments, see below:

    // If a payment method ID has been provided.
    $oldPaymentMethodId = $context->getPaymentMethod()->getId(); // The context object is not updated by SW before this hook is called, so it still contains the old data.
    $newPaymentMethodId = $requestData->get('paymentMethodId');

    // We oly need to clear the data, if there is a payment method provided and if it differs from the previously set one.
    if (null !== $newPaymentMethodId && $newPaymentMethodId  !==$oldPaymentMethodId) {  
          // DELETE adyen payment data. Even if we provide an adyen payment method ID, we can remove it here because, if the adyen data has been provided, it will be set below and has to be adjusted anyway.
    }

    // If adyen state has been provided and current payment method is adyen payment.
    if ($requestData->has('adyenStateData')) {
         $adyenStateData = $requestData->get('adyenStateData');
         if (null === $adyenStateData) {
              // Delete stored adyen state data. This way the user can send `adyenStateData: null` and it will remove the currently stored data.
         } elseif ($this->isAdyenPayment($context->getPaymentMethod()) {
              // Save adyen data, if the current payment method is an adyen payment method.
         }
    }

EDIT: I have made some adjustments to it.

I think now it should cover all use cases. One can send paymentMethodId and adyen state data, one can only send the adyen state data for a selected adyen payment and one can delete the adyen state manually if necessary.

[AndreasA]

@peterojo If you adjust to the change above, you also need to add a fallback to the context's payment method Id here, if there was no paymentMethodId provided during switching the context. https://github.com/Adyen/adyen-shopware6/blob/develop/src/Subscriber/PaymentSubscriber.php#L402

[peterojo]

@AndreasA great suggestion, I would only change one thing: if the paymentMethodId is provided, then delete the payment data; even if it is the same as the current selected one, because a user of the plugin (storefront) can decide to select the same payment method and change the card/issuer details for example. EDIT: Also, if you send adyenStateData: null this is the same thing as not sending adyenStateData in the request, because in both cases $event->getRequestDataBag()->get('adyenStateData') will return null. So the best way to delete the current state data will be to send the paymentMethodId without the adyenStateData. May I ask what is the use case where you need to manually delete the state data without sending the paymentMethodId?

[AndreasA]

@peterojo Regarding deleting payment data upon using the same payment method ID, that is fine.

However, that was way I added the the null option to explicitely delete the payment state data, so it does not happen unexpectely, e.g. if I just click on the same payment method.

Regarding adyenStateData: null: That is why I checked with has and not get because has is not the same as get.
Has checks if the value was sent at all. It is NOT the same as using 😄 You can easily verify by debugging a switch context request. has uses array_key_exists internally. That just check for the existence.

Regarding use case: we have a button on the payment form that is caleld update payment data, this basically deletes the existing data and refreshes the info, if payment data is stored, in a mobx store.

Especially, this ensures that if the user refreshes the page, the state does not switch to payment data has already been entered.

Currently I actually do this by sending the same payment method ID again which deletes the data but I think that is a bit hacky and I would prefer to do this cleaner.

[peterojo]

@AndreasA in the storefront plugin we are always sending the adyenStateData field in the form for selecting a payment method. For the payment methods without state data, the adyenStateData will be empty, that is why we are using $event->getRequestDataBag()->get('adyenStateData') to check for state data. If we use $event->getRequestDataBag()->has('adyenStateData') it will always be true. For your use case, what do you think about if we create an API endpoint for deleting payment state data, instead of hooking that into the SalesChannelContextSwitchEvent event, would you be able to work with that?

[AndreasA]

@peterojo Separate API request is fine too. So you can keep using get here.

[peterojo]

Great! I created an internal ticket for the new API endpoint.

[AndreasA]

@peterojo Just noticed that the paymentMethodId validation should actually be removed as you now fallback to the default payment method in case non was provided. Which is what might be needed as dicussed above for multi-step / PWA setups. So the validation, is probably wrong.

If you want to add a check regarding invalid payment method, you could add it as else to the condition if ($paymentMethod->getPluginId() === $this->adyenPluginId) { because then something was wrong with the request as adyen data was provided for a non adyen payment method.

[AndreasA]

@peterojo Just to let you know, I did the update to 1.6.0 today and it works great. I was able to remove a lot of workarounds for these things. Thanks.

[peterojo]

@AndreasA thanks for the feedback :)