Skip to content

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dc1b659
[ECP-9482] Add the `ExternalPlatformHeaderDataBuilder` Interface and …
Oct 8, 2024
06aa3c6
[ECP-9482] Put magentoDetails method in data class
Oct 8, 2024
7cc3e2a
[ECP-9482] Put magentoDetails method in data class
Oct 9, 2024
cd9f4d0
[ECP-9482] Rename the interface and update payment detail helper, tes…
Oct 9, 2024
3b0ca36
[ECP-9482] Updating tests
Oct 9, 2024
ae841e0
[ECP-9482] Finalise HeaderDataBuilder tests
Oct 14, 2024
59dddd0
[ECP-9482] Finalise PaymentDetailsTest and Remove header test from Tr…
Oct 14, 2024
2333fff
[ECP-9482] Finalise TransactionCancelTest to use mocked header values…
Oct 14, 2024
5088018
[ECP-9482] Finalise All the transactionClient tests
Oct 14, 2024
01c741b
[ECP-9482] Remove unused property
Oct 14, 2024
eb63a59
[ECP-9482] Remove unused mock property
Oct 14, 2024
bad7cbc
[ECP-9482] Add BaseTransaction client, refactor all transaction clien…
Oct 15, 2024
86b406b
[ECP-9482] Correct Typo
Oct 15, 2024
0bfed34
Merge branch 'main' into ECP-9482
SushmitaThakur Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions Gateway/Http/Client/BaseTransaction.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

namespace Adyen\Payment\Gateway\Http\Client;

use Adyen\Payment\Helper\Data;
use Adyen\Payment\Gateway\Request\Header\HeaderDataBuilder;
use Magento\Payment\Gateway\Http\TransferInterface;
use Magento\Payment\Gateway\Http\ClientInterface;

abstract class BaseTransaction implements ClientInterface
{

/**
* @var Data
*/
protected Data $adyenHelper;

/**
* BaseTransactionClient constructor.
* @param Data $adyenHelper
*/
public function __construct(Data $adyenHelper)
{
$this->adyenHelper = $adyenHelper;
}

/**
* Builds the headers if they are not provided.
*
* @param TransferInterface $transferObject
* @return array
*/
protected function requestHeaders(TransferInterface $transferObject): array
{
$headers = $transferObject->getHeaders();

// If no headers exist, build them using HeaderDataBuilder
Copy link
Member

Choose a reason for hiding this comment

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

If any of the request builders add header H, this method will not be called. Thus, this will result in not adding desired headers. I would recommend adding header data builder to command chain of the transaction clients in di.xml. Let's have a catch-up if this is not clear.

if (empty($headers)) {
$headerBuilder = new HeaderDataBuilder($this->adyenHelper);
Copy link
Member

@candemiralp candemiralp Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

new notation is not a best practice carried on on Magento 2 as it uses dependency injection pattern. Here we can get rid of this usage via a design change.

However, if you strictly want to use a class' new instance created during the runtime, you can use factory design pattern.

$headers = $headerBuilder->buildRequestHeaders();
}

return $headers;
}
}
15 changes: 7 additions & 8 deletions Gateway/Http/Client/TransactionCancel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@
use Adyen\Payment\Helper\Data;
use Adyen\Payment\Helper\Idempotency;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;

class TransactionCancel implements ClientInterface
class TransactionCancel extends BaseTransaction
{
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var Idempotency
Expand All @@ -40,7 +39,7 @@ public function __construct(
Data $adyenHelper,
Idempotency $idempotencyHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->idempotencyHelper = $idempotencyHelper;
}

Expand All @@ -53,20 +52,20 @@ public function __construct(
public function placeRequest(TransferInterface $transferObject): array
{
$requests = $transferObject->getBody();
$headers = $transferObject->getHeaders();
$clientConfig = $transferObject->getClientConfig();
$requestOptions['headers'] = $this->requestHeaders($transferObject);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally $transferObject->getHeaders() should be sufficient to fetch the headers of the requests. Implementing an abstract class for an abstract method would increase the complexity of the design. Let's discuss this approach.


$clientConfig = $transferObject->getClientConfig();
$client = $this->adyenHelper->initializeAdyenClientWithClientConfig($clientConfig);
$service = $this->adyenHelper->initializeModificationsApi($client);
$responseData = [];

foreach ($requests as $request) {
$idempotencyKey = $this->idempotencyHelper->generateIdempotencyKey(
$request,
$headers['idempotencyExtraData'] ?? null
$requestOptions['headers']['idempotencyExtraData'] ?? null
);

$requestOptions['idempotencyKey'] = $idempotencyKey;
$requestOptions['headers'] = $this->adyenHelper->buildRequestHeaders();
$this->adyenHelper->logRequest($request, Client::API_CHECKOUT_VERSION, '/cancels');
$request['applicationInfo'] = $this->adyenHelper->buildApplicationInfo($client);
$paymentCancelRequest = new PaymentCancelRequest($request);
Expand Down
16 changes: 7 additions & 9 deletions Gateway/Http/Client/TransactionCapture.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@
use Adyen\Payment\Logger\AdyenLogger;
use Adyen\Service\Checkout\ModificationsApi;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;

class TransactionCapture implements ClientInterface
class TransactionCapture extends BaseTransaction
{
const MULTIPLE_AUTHORIZATIONS = 'multiple_authorizations';
const FORMATTED_CAPTURE_AMOUNT = 'formatted_capture_amount';
Expand All @@ -35,7 +34,7 @@ class TransactionCapture implements ClientInterface
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var AdyenLogger
Expand All @@ -57,7 +56,7 @@ public function __construct(
AdyenLogger $adyenLogger,
Idempotency $idempotencyHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->adyenLogger = $adyenLogger;
$this->idempotencyHelper = $idempotencyHelper;
}
Expand All @@ -71,13 +70,12 @@ public function __construct(
public function placeRequest(TransferInterface $transferObject): array
{
$request = $transferObject->getBody();
$headers = $transferObject->getHeaders();
$clientConfig = $transferObject->getClientConfig();
$requestOptions['headers'] = $this->requestHeaders($transferObject);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally $transferObject->getHeaders() should be sufficient to fetch the headers of the requests. Implementing an abstract class for an abstract method would increase the complexity of the design. Let's discuss this approach.


$clientConfig = $transferObject->getClientConfig();
$client = $this->adyenHelper->initializeAdyenClientWithClientConfig($clientConfig);
$service = $this->adyenHelper->initializeModificationsApi($client);

$requestOptions['headers'] = $this->adyenHelper->buildRequestHeaders();
$request['applicationInfo'] = $this->adyenHelper->buildApplicationInfo($client);

if (array_key_exists(self::MULTIPLE_AUTHORIZATIONS, $request)) {
Expand All @@ -86,10 +84,10 @@ public function placeRequest(TransferInterface $transferObject): array

$idempotencyKey = $this->idempotencyHelper->generateIdempotencyKey(
$request,
$headers['idempotencyExtraData'] ?? null
$requestOptions['headers']['idempotencyExtraData'] ?? null
);
$requestOptions['idempotencyKey'] = $idempotencyKey;

$requestOptions['idempotencyKey'] = $idempotencyKey;
$this->adyenHelper->logRequest($request, Client::API_CHECKOUT_VERSION, '/captures');
$paymentCaptureRequest = new PaymentCaptureRequest($request);
$responseData = [];
Expand Down
14 changes: 5 additions & 9 deletions Gateway/Http/Client/TransactionDonate.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Could you please cover all the lines in the unit tests? It looks like some coverage is missing in this report.

Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@
use Adyen\Payment\Helper\Idempotency;
use Adyen\Service\Checkout\DonationsApi;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;

class TransactionDonate implements ClientInterface
class TransactionDonate extends BaseTransaction
{
/**
* @var Client
Expand All @@ -32,7 +31,7 @@ class TransactionDonate implements ClientInterface
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var Idempotency
Expand All @@ -49,9 +48,8 @@ public function __construct(
Data $adyenHelper,
Idempotency $idempotencyHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->idempotencyHelper = $idempotencyHelper;

$this->client = $this->adyenHelper->initializeAdyenClient();
}

Expand All @@ -63,17 +61,15 @@ public function __construct(
public function placeRequest(TransferInterface $transferObject): array
{
$request = $transferObject->getBody();
$headers = $transferObject->getHeaders();

$requestOptions['headers'] = $this->requestHeaders($transferObject);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally $transferObject->getHeaders() should be sufficient to fetch the headers of the requests. Implementing an abstract class for an abstract method would increase the complexity of the design. Let's discuss this approach.

$service = new DonationsApi($this->client);

$idempotencyKey = $this->idempotencyHelper->generateIdempotencyKey(
$request,
$headers['idempotencyExtraData'] ?? null
$requestOptions['headers']['idempotencyExtraData'] ?? null
);

$requestOptions['idempotencyKey'] = $idempotencyKey;
$requestOptions['headers'] = $this->adyenHelper->buildRequestHeaders();
$request['applicationInfo'] = $this->adyenHelper->buildApplicationInfo($this->client);

$this->adyenHelper->logRequest($request, Client::API_CHECKOUT_VERSION, 'donations');
Expand Down
7 changes: 3 additions & 4 deletions Gateway/Http/Client/TransactionPayment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,15 @@
use Adyen\Service\Checkout\PaymentsApi;
use Magento\Framework\Exception\AlreadyExistsException;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;
use Magento\Store\Model\StoreManagerInterface;

class TransactionPayment implements ClientInterface
class TransactionPayment extends BaseTransaction
Copy link
Member

Choose a reason for hiding this comment

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

Abstract requestHeaders() method has not been called in this class. Do we still need to extend BaseTransaction?

{
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var PaymentResponseFactory
Expand Down Expand Up @@ -87,7 +86,7 @@ public function __construct(
StoreManagerInterface $storeManager,
GiftcardPayment $giftcardPaymentHelper,
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->paymentResponseFactory = $paymentResponseFactory;
$this->paymentResponseResourceModel = $paymentResponseResourceModel;
$this->idempotencyHelper = $idempotencyHelper;
Expand Down
15 changes: 7 additions & 8 deletions Gateway/Http/Client/TransactionPaymentLinks.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Could you please cover all the lines in the unit tests? It looks like some coverage is missing in this report.

Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@
use Adyen\Payment\Helper\Idempotency;
use Adyen\Service\Checkout\PaymentLinksApi;
use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;

class TransactionPaymentLinks implements ClientInterface
class TransactionPaymentLinks extends BaseTransaction
{
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var Idempotency
Expand All @@ -41,7 +40,7 @@ public function __construct(
Data $adyenHelper,
Idempotency $idempotencyHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->idempotencyHelper = $idempotencyHelper;
}

Expand All @@ -54,9 +53,9 @@ public function __construct(
public function placeRequest(TransferInterface $transferObject): array
{
$request = $transferObject->getBody();
$headers = $transferObject->getHeaders();
$clientConfig = $transferObject->getClientConfig();
$requestOptions['headers'] = $this->requestHeaders($transferObject);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally $transferObject->getHeaders() should be sufficient to fetch the headers of the requests. Implementing an abstract class for an abstract method would increase the complexity of the design. Let's discuss this approach.


$clientConfig = $transferObject->getClientConfig();
$client = $this->adyenHelper->initializeAdyenClientWithClientConfig($clientConfig);
$service = new PaymentLinksApi($client);

Expand All @@ -68,11 +67,11 @@ public function placeRequest(TransferInterface $transferObject): array

$idempotencyKey = $this->idempotencyHelper->generateIdempotencyKey(
$request,
$headers['idempotencyExtraData'] ?? null
$requestOptions['headers']['idempotencyExtraData'] ?? null
);

$requestOptions['idempotencyKey'] = $idempotencyKey;
$requestOptions['headers'] = $this->adyenHelper->buildRequestHeaders();

$request['applicationInfo'] = $this->adyenHelper->buildApplicationInfo($client);

$this->adyenHelper->logRequest($request, Client::API_CHECKOUT_VERSION, '/paymentLinks');
Expand Down
5 changes: 2 additions & 3 deletions Gateway/Http/Client/TransactionPosCloudSync.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Could you please cover all the lines in the unit tests? It looks like some coverage is missing in this report.

Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@
use Adyen\Payment\Helper\Config;
use Adyen\Payment\Helper\Data;
use Adyen\Payment\Logger\AdyenLogger;
use Magento\Payment\Gateway\Http\ClientInterface;
use Magento\Payment\Gateway\Http\TransferInterface;
use Magento\Store\Model\StoreManagerInterface;

class TransactionPosCloudSync implements ClientInterface
class TransactionPosCloudSync extends BaseTransaction
Copy link
Member

Choose a reason for hiding this comment

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

Abstract requestHeaders() method has not been called in this class. Do we still need to extend BaseTransaction?

{
protected int $storeId;
protected mixed $timeout;
Expand All @@ -36,7 +35,7 @@ public function __construct(
StoreManagerInterface $storeManager,
Config $configHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->adyenLogger = $adyenLogger;
$this->configHelper = $configHelper;

Expand Down
14 changes: 7 additions & 7 deletions Gateway/Http/Client/TransactionRefund.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@
/**
* Class TransactionSale
*/
class TransactionRefund implements TransactionRefundInterface
class TransactionRefund extends BaseTransaction implements TransactionRefundInterface
{
/**
* @var Data
*/
private Data $adyenHelper;
protected Data $adyenHelper;

/**
* @var Idempotency
Expand All @@ -42,7 +42,7 @@ public function __construct(
Data $adyenHelper,
Idempotency $idempotencyHelper
) {
$this->adyenHelper = $adyenHelper;
parent::__construct($adyenHelper);
$this->idempotencyHelper = $idempotencyHelper;
}

Expand All @@ -55,9 +55,9 @@ public function __construct(
public function placeRequest(TransferInterface $transferObject): array
{
$requests = $transferObject->getBody();
$headers = $transferObject->getHeaders();
$clientConfig = $transferObject->getClientConfig();
$requestOptions['headers'] = $this->requestHeaders($transferObject);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally $transferObject->getHeaders() should be sufficient to fetch the headers of the requests. Implementing an abstract class for an abstract method would increase the complexity of the design. Let's discuss this approach.


$clientConfig = $transferObject->getClientConfig();
$client = $this->adyenHelper->initializeAdyenClientWithClientConfig($clientConfig);
$service = $this->adyenHelper->initializeModificationsApi($client);
$responses = [];
Expand All @@ -66,10 +66,10 @@ public function placeRequest(TransferInterface $transferObject): array
$responseData = [];
$idempotencyKey = $this->idempotencyHelper->generateIdempotencyKey(
$request,
$headers['idempotencyExtraData'] ?? null
$requestOptions['headers']['idempotencyExtraData'] ?? null
);

$requestOptions['idempotencyKey'] = $idempotencyKey;
$requestOptions['headers'] = $this->adyenHelper->buildRequestHeaders();
$this->adyenHelper->logRequest($request, Client::API_CHECKOUT_VERSION, '/refunds');
$request['applicationInfo'] = $this->adyenHelper->buildApplicationInfo($client);
$paymentRefundRequest = new PaymentRefundRequest($request);
Expand Down
Loading
Loading