-
Notifications
You must be signed in to change notification settings - Fork 14
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
PW-1364: review comments #2
Conversation
Aleffio
commented
Jul 16, 2019
- Set applicationinfo and externalPlatform
- add urlprefix
- loop on payment collection
- added error template page on refused payment
- show error on template if originkey is missing
- added correct amount calculation per currency
show error if originKey is missing, added live endpoint url prefix
controllers/front/payment.php
Outdated
@@ -59,7 +57,7 @@ public function postProcess() | |||
$response = $service->payments($request); | |||
} catch (\Adyen\AdyenException $e) { | |||
$response['error'] = $e->getMessage(); | |||
die('There was an error with the payment method.'); | |||
// die('There was an error with the payment method.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason of keeping this line ?
controllers/front/payment.php
Outdated
$payment[0]->save(); | ||
$paymentCollection = $new_order->getOrderPaymentCollection(); | ||
foreach($paymentCollection as $payment) { | ||
if (isset($payment)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would the paymentCollection not have a payment object ? think this isset check is not needed.
$payment->card_holder = pSQL($response['additionalData']['cardHolderName']); | ||
$payment->save(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better if we change this big if to all small ifs with the separate setters. so:
if(!empty($response['additionalData']['cardBin']) {
..
}
if(!empty($response['additionalData']['expiryDate'])) {
..
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @param $currency | ||
* @return string | ||
*/ | ||
public function formatAmount($amount, $currency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we not add this into our UTIL class of the API library and use it there. Same we can do for M2 so we have consistency if there are changes made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please check Adyen/adyen-php-api-library#113
will remove this and add the library reference in the next release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)