Adyen / adyen-magento2

Adyen Payment plugin for Magento2
MIT License
150 stars 196 forks source link

[ECP-9112] 9.4.x break *DataAssignObserver and placeOrder #2573

Closed dimitriBouteille closed 2 months ago

dimitriBouteille commented 3 months ago

Hi

I see that in version 9.4 you have modified the state data logic, these changes cause some problems.

Indeed, if the mutations setPaymentMethodOnCart and placeOrder are executed in 2 requests I have the following error which is reported during the place order:

{
   "status" : 422,
   "errorCode" : "14_006",
   "message" : "Required object 'paymentMethod' is not provided.",
   "errorType" : "validation",
   "pspReference" : "HBMHNHDQP39ZDV65"
}

Looking at the changes, I see that you have deleted the following line: https://github.com/Adyen/adyen-magento2/compare/9.3.0...9.4.0#diff-abf006a34a9d14c62cfbbe463af7fdd13e6b0c2603aa5145d80872d6581a9e2fL120

// JSON decode state data from the frontend or fetch it from the DB entity with the quote ID
if (!empty($additionalData[self::STATE_DATA])) {
    $stateData = json_decode((string) $additionalData[self::STATE_DATA], true);
-} else {
-    $stateData = $this->stateDataCollection->getStateDataArrayWithQuoteId($paymentInfo->getData('quote_id'));
-}

Except that this line is important since in my case the state data is loaded from the database because I make 2 requests for technical reasons.

Is there a reason for changing this logic? If so, it is a major change that risks breaking more than one site:(

To Reproduce

Try place order in graphQl with adyen_cc (bug present with all Adyen methods)

Details


Currently I can't update Adyen to >= 9.4 :(

candemiralp commented 3 months ago

Hello @dimitriBouteille,

Thank you for creating this issue. Plugin does not store the state data on the DB by default, we changed that approach in the earlier versions of V8. It stores the state data temporarily via setting it by observes and consumes it on the request builders.

The reason of this removal is for supporting giftcard payments through graphql. If the payment contains two payment methods (giftcard + credit card), first giftcard is redeemed and then placeOrder mutation is executed with credit card state data. However, if you call setPaymentMethodOnCart and placeOrder in a combined mutation, AdyenCcDataAssignObserver is hit twice (for each of the mutations). Since, placeOrder mutation doesn't accept additional data, state data is empty on the second observer hit. Therefore, it tries to access to DB. DB holds giftcard state data at that moment and due to this fact observer passes the wrong state data to the request builder CheckoutDataBuilder for credit card payment.

By default, setPaymentMethodOnCart doesn't save the state data to the DB. Do you have a custom logic to save the state data? Could you please give more details about your implementation? So that we can try to investigate the potential solutions?

Best Regards, Can

dimitriBouteille commented 3 months ago

Hi @candemiralp

Thank you for this information, I better understand the problem.

Concerning myself, in my application I use for technical reasons a proxy that will automatically separate the mutations, so if this request:

mutation setPaymentMethodAndPlaceOrder(
  $cartId: String!
  $paymentMethod: PaymentMethodInput!
) {
  setPaymentMethodOnCart(
    input: { cart_id: $cartId, payment_method: $paymentMethod }
  ) {
    cart {
      selected_payment_method {
        ...CartSelectedPaymentMethodFragment
      }
    }
  }

  placeOrder(input: { cart_id: $cartId }) {
    order {
      order_number
    }
  }
}

will be executed in two queries:

mutation setPaymentMethodAndPlaceOrder(
  $cartId: String!
  $paymentMethod: PaymentMethodInput!
) {
  setPaymentMethodOnCart(
    input: { cart_id: $cartId, payment_method: $paymentMethod }
  ) {
    cart {
      selected_payment_method {
        ...CartSelectedPaymentMethodFragment
      }
    }
  }
}
mutation setPaymentMethodAndPlaceOrder(
  $cartId: String!
) {
  placeOrder(input: { cart_id: $cartId }) {
    order {
      order_number
    }
  }
}

Unfortunately I can not do otherwise:( I see that you added the adyenSaveStateData mutation, do you know if it can fix the problem? and thus execute the queries in this order?

candemiralp commented 3 months ago

Hello @dimitriBouteille,

Thank you for explaining the details of your use case. I don't think adyenSaveStateData will solve your issue with its current state.

Do you insert the state data to the database via that proxy?

Best Regards, Can

dimitriBouteille commented 3 months ago

Hu @candemiralp

I made a plugin to save the state because in <= 9.3 the adyenSaveStateData mutation does not exist.

https://github.com/Adyen/adyen-magento2/pull/1924

/**
 * @param \Adyen\Payment\Helper\StateData $subject
 * @param array $stateData
 * @param int $quoteId
 * @return array
 * @SuppressWarnings(PHPMD.UnusedFormalParameter)
 */
public function beforeSetStateData(
    \Adyen\Payment\Helper\StateData $subject,
    array $stateData,
    int $quoteId
): array {

    /**
     * Fix bug with graphQl
     * @see https://github.com/Adyen/adyen-magento2/pull/1924
     */
    try {
        $stateDataObject = $this->stateDataFactory->create();
        $stateDataObject->setData('state_data', json_encode($stateData));
        $stateDataObject->setData('quote_id', $quoteId);
        $this->stateDataResource->save($stateDataObject);
    } catch (\Exception $exception) {
        $this->adyenLogger->critical(
            sprintf(
                'Something when wrong durant save State Data: %s',
                $exception->getMessage()
            ),
            [
                'cart_id'  => $quoteId,
                'exception' => $exception,
            ]
        );
    }

    return [$stateData, $quoteId];
}
<type name="Adyen\Payment\Helper\StateData">
    <plugin name="Reflet_Adyen::save_stateDate_in_db"
            type="Reflet\Adyen\Plugin\Helper\StateData"/>
</type>

I see that you are already working on 9.5.0, would it be possible to prioritize this ticket ? so that I can make the updates :heart:

candemiralp commented 3 months ago

Hello @dimitriBouteille,

Thank you for explaining the details of your implementation. Now, the whole picture is clear for us.

As I mentioned earlier, we've decided not to store the stated data on the database since it contains card information (even though it's encrypted). But, while implementing new gift card flow, that became a blocker for us since one quote might have multiple state data (credit card + gift card). The main purpose of implementing adyenSaveStateData mutation is inserting the gift card state data to the DB. While designing headless gift card flow, we assumed gift card state data could be inserted via adyenSaveStateData mutation and credit card state data could be inserted as a part of setPaymentMethodOnCart mutation withouth persisting it to the database.

Your implementation shows a valid use case of storing the state data for all of the payment methods besides gift cards. So, I will create an internal case to discuss and fix this implementation. Unfortunately, it's not possible to make it part of 9.5.0 due to time concern. But, we are prioritising this task.

Best Regards, Can