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

Feature/payment provider gateway #636

Closed
wants to merge 1,180 commits into from

Conversation

LucianTuriacArnia
Copy link
Contributor

No description provided.

$this->getOrder()->getStore()
);

$availableCards = $this->getPayment()->getAdditionalInformation('giftcard_method') ?? $availableCards . ',ideal';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need to display iDeal in giftcards list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

app/code/Buckaroo/Magento2/Model/Method/Giftcards.php:209 was added for this ticket https://buckaroonl.atlassian.net/browse/BP-185 by Sergey Saranchuk

return 'B2C';
}

protected function getGender(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add extra gender values here according to BP-1933.

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 change these files:
app/code/Buckaroo/Magento2/Gateway/Request/PayPerEmailDataBuilder.php:37
app/code/Buckaroo/Magento2/Gateway/Request/Recipient/BillinkDataBuilder.php:56

{
$data = [
'title' => $this->getFirstname(),
'gender' => $this->getGender(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Gender is no longer required according to BP-1933.

{
return [
'category' => $this->getCategory(),
'gender' => $this->getGender(),
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add the extra values to the parent getGender method, we should add a new one here to contain only male/female values.

$customerAccountName = $paymentInfo->getAdditionalInformation('customer_account_name');

$data = [
'iban' => $customerIban,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add collectdate / mandateDate parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, in develop branch we don't add it, but I will check if the old version send it somehow.

{
parent::initialize($buildSubject);
return [
'dateDue' => $this->transferConfig->getDueDateFormated($this->getOrder()->getStore()),
Copy link
Contributor

Choose a reason for hiding this comment

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

email, country, customer missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Are added by another builders

if (!empty($paylink)) {
$this->messageManager->addSuccess(
__(
'You PayLink <a href="%1">%1</a>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typo here to 'Your'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"You Bad" :))
I copied from this app/code/Buckaroo/Magento2/Controller/Adminhtml/PayLink/Index.php:147

/**
* {@inheritdoc}
*/
public function getWsdlTestWeb($store = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this class and references since we don't use SOAP anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"You Good", I will remove it

Copy link
Contributor

@serpentscode serpentscode left a comment

Choose a reason for hiding this comment

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

  • Buckaroo SDK is not present in composer.json, needs to be added.
  • BuckarooSDK doesn't work with Magento 2.4.3 (monolog conflict).
  • Fix for addLog function, doesn't work with Magento 2.4.3 .
  • Not sure if we still need Console/Command/PushSend.php.
  • Move updateShippingAddressCommonMappingV2 method from AbstractAddressBuilder to MyParcelAddressHandler (it's used only in this class).
  • Implement SDK's credentials checker method instead of current solution from Controller/CredentialChecker/Index.php->testJson(..).
  • I don't think we need a certificate to be uploaded anymore. Maybe we should remove this.

@@ -151,10 +152,9 @@ private function getTotalsExceptCode($totals, $code)
private function isCreditmemoTotalSelected($creditTotals, $saleTotal)
{
foreach ($creditTotals as $creditTotal) {
if (
isset($creditTotal['code']) && $creditTotal['code'] === $saleTotal['code']
if ( isset($creditTotal['code']) && $creditTotal['code'] === $saleTotal['code']
Copy link

Choose a reason for hiding this comment

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

some unnecessary spaces inside the parentheses

string $cart_id = null
);
public function execute(
string $paypal_order_id,
Copy link

Choose a reason for hiding this comment

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

variable name should be camelCase

\Magento\Backend\App\Action\Context $context,
\Magento\Backend\Model\View\Result\ForwardFactory $resultForwardFactory
) {
$this->resultForwardFactory = $resultForwardFactory;
Copy link

Choose a reason for hiding this comment

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

you don't need the resultForwardFactory dependency.
if this class extends \Magento\Backend\App\Action (and it does) you can create a forward response with

 $this->resultFactory->create(\Magento\Framework\Controller\ResultFactory::TYPE_FORWARD)

$resultFactory is already added as a dependency in a parent class Magento\Framework\App\Action\AbstractAction

@@ -22,6 +23,11 @@

class Delete extends \Buckaroo\Magento2\Controller\Adminhtml\Giftcard\Index
Copy link

Choose a reason for hiding this comment

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

Controller action should implement a specific interface depending on the methods that are allowed for this action.

  • \Magento\Framework\App\Action\HttpGetActionInterface - for GET requests
  • \Magento\Framework\App\Action\HttpPostActionInterface - for POST requests
  • ....
    This prevents destructive actions (like delete for example) to be executed on GET requests.

@@ -20,10 +21,34 @@

namespace Buckaroo\Magento2\Controller\Adminhtml\Giftcard;

class NewAction extends \Buckaroo\Magento2\Controller\Adminhtml\Giftcard\Index
class NewAction extends \Magento\Backend\App\Action
Copy link

Choose a reason for hiding this comment

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

Same as above, should implement an action interface (use Magento\Framework\App\Action\HttpGetActionInterface in this case)

Helper/Data.php Outdated Show resolved Hide resolved
@@ -157,7 +174,7 @@ public function getGroupTransactionAmount($order_id)

public function getGroupTransactionOriginalTransactionKey($order_id)
{
if($order_id === null) {
if ($order_id === null) {
Copy link

Choose a reason for hiding this comment

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

var name should be camelCase


$this->order = $order;
$this->dateTime = $dateTime;
$this->order = $order;
Copy link

Choose a reason for hiding this comment

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

most likele $ordershould not be a dependency of the calss. A repository should be used to retrieve the order. Not even sure if it is used in the class at all

\Magento\Customer\Model\Session $customerSession
) {
$this->logFactory = $logFactory;
$this->checkoutSession = $checkoutSession;
Copy link

Choose a reason for hiding this comment

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

checkoutSession, session and customer session are not used in the class. There is no need for the dependencies here.

/**
* @param ModuleDataSetupInterface $setup
*
* @return $this
*
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
*/
Copy link

Choose a reason for hiding this comment

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

this is just a remarkt, not sure if it;s worth fixing at this point. module should not add columns to the order related tables. all module related data should reside in a separate table with a FK to the orders (quote, items, ...) tables. If this module gets installed on a database with 100k+ orders for example the install script will take a lot of time and might even crash. Doing this properly at this point will require a lot of time and a lot of code changes. not sure if it;s worth changing, but it's sure worth investigating.

harli91
harli91 previously approved these changes Mar 24, 2023
Buckaroo-Rene
Buckaroo-Rene previously approved these changes Jun 20, 2023
Buckaroo-Rene
Buckaroo-Rene previously approved these changes Aug 17, 2023
Rinor12010 and others added 9 commits November 10, 2023 16:02
…n-canceled-in-magento' into BP-2748-giftcard-grouptransaction-canceled-in-magento
…o feature/payment_provider_gateway

# Conflicts:
#	Controller/Applepay/Add.php
#	Controller/Applepay/Common.php
#	Controller/Applepay/GetShippingMethods.php
#	Controller/Applepay/SaveOrder.php
#	Controller/Applepay/UpdateShippingMethods.php
#	Model/Method/AbstractMethod.php
#	Model/Method/Capayable.php
#	Service/Applepay/Add.php
LucianTuriacArnia and others added 25 commits April 12, 2024 11:41
…d-tinka-magento-2-refactor

BP-3502 Remove payment method: Tinka
…ency-refactor

BP-3451 - Missing dependency - refactor
…b2b-mode-subtext-and-tooltip-information

BP-3391 improve payperemail b2b mode subtext and tooltip information
…o feature/payment_provider_gateway

# Conflicts:
#	Model/Config/Source/PaymentMethods/AfterExpiry.php
#	Model/Config/Source/PaymentMethods/PayLink.php
#	Model/Config/Source/PaymentMethods/PayPerEmail.php
#	Model/ConfigProvider/Method/Voucher.php
#	etc/adminhtml/system/payment_methods.xml
#	etc/adminhtml/system/payment_methods/voucher.xml
#	etc/config.xml
#	etc/di.xml
#	etc/frontend/di.xml
#	view/frontend/layout/checkout_index_index.xml
#	view/frontend/web/js/view/payment/buckaroo-payments.js
#	view/frontend/web/js/view/payment/method-renderer/giftcards.js
…g-of-payment-methods-on-the-configuration-settings

BP-3533 incorrect positioning of payment methods on the configuration settings
…-approvals-refactor

BP-3510 partial refunds with approvals refactor
…g-provider

Fix Creditcard config provider
…data-refactor

BP-3540 creditcard company data missing
…/buckaroo-it/Magento2 into sonarqube-refactor-fixes

# Conflicts:
#	Gateway/Request/Articles/ArticlesHandler/AfterpayHandler.php
#	Gateway/Validator/IssuerValidator.php
#	Gateway/Validator/ResponseCodeSDKValidator.php
#	Model/ConfigProvider/Method/Applepay.php
#	Model/Push/DefaultProcessor.php
#	Model/Service/CreateInvoice.php
@LucianTuriacArnia
Copy link
Contributor Author

We created another branch: "refactor"

@LucianTuriacArnia LucianTuriacArnia deleted the feature/payment_provider_gateway branch April 24, 2024 15:49
@LucianTuriacArnia LucianTuriacArnia restored the feature/payment_provider_gateway branch April 24, 2024 15:50
@LucianTuriacArnia LucianTuriacArnia deleted the feature/payment_provider_gateway branch April 24, 2024 15:50
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.

9 participants