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

PAYOSWXP-120: Adds new log feature: Notification Forward logs are now saved in payone_transaction_forward #288

Closed
wants to merge 2 commits into from

Conversation

amirinterlutions
Copy link
Contributor

@amirinterlutions amirinterlutions commented Jan 24, 2024

PAYOSWXP-120: Adds new log feature: Notification Forward logs are now saved in payone_transaction_forward
PAYOSWXP-121: Fixes Notification Forward: Now capable of sending notifications to a URL.

@amirinterlutions amirinterlutions force-pushed the task/PAYOSWXP-120_PAYOSWXP-121 branch from 1312097 to 213a189 Compare January 24, 2024 21:27
Copy link
Collaborator

@rommelfreddy rommelfreddy left a comment

Choose a reason for hiding this comment

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

just a few small changes

@@ -71,10 +81,10 @@ private function getNotificationForwards(array $ids, Context $context): EntitySe
}

private function updateResponses(
\CurlMultiHandle $multiHandle,
\CurlMultiHandle $multiHandle,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I used the Easy Code Standard from Payone. I should take another look to understand why it wasn't corrected.

{
$statusCode = $responseInfo['http_code'];

$response = new Response($responseContent, $statusCode, $responseInfo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

good idea to check with the Response-Object if the method is successfully & get the response text.

but it would make more sense to check if the HTTP status is greater than 200 & lower than 300.
All 20x status means that the response is successfully.

I like the idea to get the status text. But please use the status text from the http Header status.
I am not sure, but maybe you can get the status text also from the curl-info object.

Copy link
Contributor Author

@amirinterlutions amirinterlutions Jan 28, 2024

Choose a reason for hiding this comment

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

Thanks! Yes, I tried to structure it a bit. There's a method isSuccessful() that specifically checks if the HTTP code is between 200 and 300 (as you prefer). I used it to log the correct status ($loglevel) for all statuses.

All curl-info looks like this:

{
  "url": "https://payoswxp.ngrok.io/simple-my-webhook-notif.php",
  "content_type": "text/html; charset=UTF-8",
  "http_code": 200,
  "header_size": 197,
  "request_size": 163,
  "filetime": -1,
  "ssl_verify_result": 0,
  "redirect_count": 0,
  "total_time": 0.107368,
  "namelookup_time": 0.00117,
  "connect_time": 0.019795,
  "pretransfer_time": 0.046854,
  "size_upload": 469.0,
  "size_download": 28.0,
  "speed_download": 261.0,
  "speed_upload": 4383.0,
  "download_content_length": -1.0,
  "upload_content_length": 469.0,
  "starttransfer_time": 0.0469,
  "redirect_time": 0.0,
  "redirect_url": "",
  "primary_ip": "3.125.102.39",
  "certinfo": [],
  "primary_port": 443,
  "local_ip": "172.21.0.2",
  "local_port": 54576,
  "http_version": 3,
  "protocol": 2,
  "ssl_verifyresult": 0,
  "scheme": "HTTPS",
  "appconnect_time_us": 46726,
  "connect_time_us": 19795,
  "namelookup_time_us": 1170,
  "pretransfer_time_us": 46854,
  "redirect_time_us": 0,
  "starttransfer_time_us": 46900,
  "total_time_us": 107368
}

There are actually no status texts, so I used text from Response::$statusTexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I corrected the branch name, and further commits are here.
PAYOSWXP-120#290

@@ -136,7 +148,7 @@ private function getForwardRequests(\CurlMultiHandle $multiHandle, EntitySearchR

private function buildHeaders(
PayonePaymentNotificationForwardEntity $forward,
PayonePaymentNotificationTargetEntity $target
PayonePaymentNotificationTargetEntity $target
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this formatting

@rommelfreddy rommelfreddy changed the title PAYOSWXP-120 and PAYOSWXP-121: Fixes Notification Forward: Now capabl… PAYOSWXP-120: Adds new log feature: Notification Forward logs are now saved in payone_transaction_forward Jan 25, 2024
…e of sending notifications to a URL. Adds new log feature: Notification Forward logs are now saved in .
@rommelfreddy
Copy link
Collaborator

continue in #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants