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

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

Closed
dimitriBouteille opened this issue Mar 27, 2024 · 5 comments · Fixed by #2583
Closed

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

dimitriBouteille opened this issue Mar 27, 2024 · 5 comments · Fixed by #2583
Assignees
Labels
Bug report Indicates that issue has been marked as a possible bug

Comments

@dimitriBouteille
Copy link
Contributor

dimitriBouteille commented Mar 27, 2024

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: 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)

  • Execute setPaymentMethodOnCart mutation
  • And execute placeOrder in second request
  • placeOrder returns Unable to place order: Error with payment method, please select a different payment method. 💥

Details

  • Magento version: 2.4.5
  • Adyen version: 9.4.0

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

@dimitriBouteille dimitriBouteille added the Bug report Indicates that issue has been marked as a possible bug label Mar 27, 2024
@candemiralp
Copy link
Member

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

@candemiralp candemiralp self-assigned this Mar 29, 2024
@dimitriBouteille
Copy link
Contributor Author

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?

  • setPaymentMethodOnCart
  • adyenSaveStateData
  • placeOrder

@candemiralp
Copy link
Member

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
Copy link
Contributor Author

dimitriBouteille commented Apr 3, 2024

Hu @candemiralp

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

#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 ❤️

@candemiralp
Copy link
Member

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

@candemiralp candemiralp changed the title 9.4.x break *DataAssignObserver and placeOrder [ECP-9112] 9.4.x break *DataAssignObserver and placeOrder Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug report Indicates that issue has been marked as a possible bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants