[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

SushmitaThakur · 2024-10-08T19:48:23Z

Description
This PR -

Refactors HeaderDatabuilder and moves buildHeaderData method from Data helper class to the builder.
It adds a HeaderDataBuiderInterface that declares all the header constants.
Adds a BaseTransaction client class that accommodates common methods used by all transactions clients
Updates all the TransactionClients and PaymentDetails class accordingly
Updates all the unit tests related to TransactionClients and PaymentDetails
Updates the path to header builder in di.xml
Updates relevant observers to use correct frontendType constant
Synchronises the frontendType value to luma in JS renderers

Tested Scenarios

TransactionPayment
TransactionCancel
TransactionCapture
PaymentDetails Call

…Builder class, Refactor all the usage of HeaderDataBuilder, Remover header builder from helper class and all its usage, Define header constants, Update dependency injection with new builder name, Synchronise the value of `frontendtype` to `luma` in relevant js files

Gateway/Request/Header/ExternalPlatformHeaderDataBuilderInterface.php

…ts and tests, update header data builder tests, remove defaulting frontend type to headless

sonarcloud · 2024-11-11T09:46:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

peterojo · 2024-11-11T11:39:55Z

Gateway/Request/Header/HeaderDataBuilderInterface.php

+    const ADDITIONAL_DATA_FRONTEND_TYPE_KEY = 'frontendType';
+    const FRONTEND_TYPE_HEADLESS_VALUE = 'headless';


candemiralp · 2024-11-11T12:21:45Z

Gateway/Http/Client/BaseTransaction.php

+
+        // If no headers exist, build them using HeaderDataBuilder
+        if (empty($headers)) {
+            $headerBuilder = new HeaderDataBuilder($this->adyenHelper);


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.

candemiralp · 2024-11-11T12:23:19Z

Gateway/Http/Client/BaseTransaction.php

+    {
+        $headers = $transferObject->getHeaders();
+
+        // If no headers exist, build them using HeaderDataBuilder


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.

candemiralp · 2024-11-11T12:24:31Z

Gateway/Http/Client/TransactionCancel.php

@@ -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);


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.

candemiralp · 2024-11-11T12:24:43Z

Gateway/Http/Client/TransactionCapture.php

@@ -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);


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.

candemiralp · 2024-11-11T12:24:48Z

Gateway/Http/Client/TransactionDonate.php

@@ -63,17 +61,15 @@ public function __construct(
    public function placeRequest(TransferInterface $transferObject): array
    {
        $request = $transferObject->getBody();
-        $headers = $transferObject->getHeaders();
-
+        $requestOptions['headers'] = $this->requestHeaders($transferObject);


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.

candemiralp · 2024-11-11T12:26:13Z

Gateway/Http/Client/TransactionRefund.php

@@ -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);


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.

candemiralp · 2024-11-11T12:26:43Z

Gateway/Request/Header/HeaderDataBuilder.php

+        return ['headers' => $headers];
+    }
+
+    public function buildRequestHeaders($payment = null): array


What would you think about converting this class into a data builder?

candemiralp · 2024-11-11T12:28:30Z

Gateway/Http/Client/TransactionDonate.php

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

candemiralp · 2024-11-11T12:28:50Z

Gateway/Http/Client/TransactionPaymentLinks.php

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

candemiralp · 2024-11-11T12:29:12Z

Gateway/Http/Client/TransactionPosCloudSync.php

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

sushmita added 2 commits October 8, 2024 15:28

[ECP-9482] Put magentoDetails method in data class

06aa3c6

SushmitaThakur requested a review from a team as a code owner October 8, 2024 19:48

SushmitaThakur had a problem deploying to internal October 8, 2024 19:48 — with GitHub Actions Error

SushmitaThakur had a problem deploying to internal October 8, 2024 19:48 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 8, 2024 19:48 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 8, 2024 19:48 — with GitHub Actions Failure

peterojo reviewed Oct 8, 2024

View reviewed changes

Gateway/Request/Header/ExternalPlatformHeaderDataBuilderInterface.php Outdated Show resolved Hide resolved

peterojo reviewed Oct 8, 2024

View reviewed changes

Gateway/Request/Header/ExternalPlatformHeaderDataBuilderInterface.php Outdated Show resolved Hide resolved

[ECP-9482] Put magentoDetails method in data class

7cc3e2a

SushmitaThakur had a problem deploying to internal October 9, 2024 09:29 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 9, 2024 09:29 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 9, 2024 09:29 — with GitHub Actions Failure

SushmitaThakur had a problem deploying to internal October 9, 2024 09:29 — with GitHub Actions Error

SushmitaThakur closed this Oct 9, 2024

SushmitaThakur reopened this Oct 9, 2024

SushmitaThakur had a problem deploying to internal October 9, 2024 09:35 — with GitHub Actions Failure

SushmitaThakur had a problem deploying to internal October 9, 2024 09:35 — with GitHub Actions Error

SushmitaThakur had a problem deploying to internal October 9, 2024 09:35 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 9, 2024 09:35 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 9, 2024 09:35 — with GitHub Actions Failure

SushmitaThakur marked this pull request as draft October 9, 2024 09:36

[ECP-9482] Add BaseTransaction client, refactor all transaction clien…

bad7cbc

…ts and tests, update header data builder tests, remove defaulting frontend type to headless

SushmitaThakur had a problem deploying to internal October 15, 2024 13:00 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 15, 2024 13:00 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 15, 2024 13:00 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 15, 2024 13:00 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 15, 2024 13:00 — with GitHub Actions Failure

[ECP-9482] Correct Typo

86b406b

SushmitaThakur temporarily deployed to internal October 15, 2024 13:10 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 15, 2024 13:10 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 15, 2024 13:10 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal October 15, 2024 13:10 — with GitHub Actions Failure

SushmitaThakur temporarily deployed to internal October 15, 2024 13:10 — with GitHub Actions Inactive

SushmitaThakur changed the title ~~[ECP-9482] Refactor header data builder~~ [ECP-9482] Refactor HeaderDataBuilder Oct 15, 2024

SushmitaThakur changed the title ~~[ECP-9482] Refactor HeaderDataBuilder~~ [ECP-9482] Refactor HeaderDataBuilder and TransactionClient Oct 15, 2024

SushmitaThakur marked this pull request as ready for review October 15, 2024 13:45

Merge branch 'main' into ECP-9482

0bfed34

SushmitaThakur temporarily deployed to internal November 11, 2024 09:40 — with GitHub Actions Inactive

SushmitaThakur had a problem deploying to internal November 11, 2024 09:40 — with GitHub Actions Failure

peterojo reviewed Nov 11, 2024

View reviewed changes

peterojo approved these changes Nov 11, 2024

View reviewed changes

candemiralp requested changes Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

SushmitaThakur commented Oct 8, 2024 •

edited

Loading

sonarcloud bot commented Nov 11, 2024

peterojo Nov 11, 2024

candemiralp Nov 11, 2024 •

edited

Loading

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

candemiralp Nov 11, 2024

		const ADDITIONAL_DATA_FRONTEND_TYPE_KEY = 'frontendType';
		const FRONTEND_TYPE_HEADLESS_VALUE = 'headless';

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

Are you sure you want to change the base?

[ECP-9482] Refactor HeaderDataBuilder and TransactionClient #2769

Conversation

SushmitaThakur commented Oct 8, 2024 • edited Loading

sonarcloud bot commented Nov 11, 2024

Quality Gate passed

Choose a reason for hiding this comment

candemiralp Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SushmitaThakur commented Oct 8, 2024 •

edited

Loading

candemiralp Nov 11, 2024 •

edited

Loading