Skip to content

Conversation

@ChetanGN
Copy link
Contributor

Note :- Please follow the below points while attaching test cases document link below:

- If label Tested is added then test cases document URL is mandatory.

- Link added should be a valid URL and accessible throughout the org.

- If the branch name contains hotfix / revert by default the BVT workflow check will pass.

Test Case Document URL
Please paste test case document link here....

ChetanGN and others added 29 commits August 29, 2024 16:08
Added COD fee support in magento order
Copy link

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

🔴 Brief Code Review shows huge gaps in the understanding of Magento maintainability and following best practices.

@@ -0,0 +1,26 @@
<?php

Choose a reason for hiding this comment

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

Add strict types declaration

use Magento\Sales\Block\Adminhtml\Order\Totals as MagentoOrderTotals;
use Magento\Framework\DataObject;

class Totals extends MagentoOrderTotals

Choose a reason for hiding this comment

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

You should avoid inheritance.
In this specific case, you should use Magento built-in composition for adding custom Totals.

@@ -0,0 +1,370 @@
<?php

Choose a reason for hiding this comment

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

Add strict types declaration


namespace Razorpay\Magento\Controller\OneClick;

use Magento\Framework\App\Action\Action;

Choose a reason for hiding this comment

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

Optimize imports

/**
* @var Http
*/
protected $request;

Choose a reason for hiding this comment

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

All non-public properties should be private

Comment on lines +56 to +58
const COD = 'cashondelivery';
const RAZORPAY = 'razorpay';
const STATE_PENDING_PAYMENT = 'pending_payment';

Choose a reason for hiding this comment

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

  1. const should be at the top of the class
  2. And have their visibility public / private


$orderPlacement = false;

try {

Choose a reason for hiding this comment

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

Nested try ... catch significantly increases the cyclomatic complexity, making it difficult to troubleshoot / debug

@@ -0,0 +1,98 @@
<!--

Choose a reason for hiding this comment

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

  1. Override in Payment Method module is really bad idea,
  2. Additionally -- You should not include outdated Magento copyright message.

@@ -0,0 +1,45 @@
<?php

Choose a reason for hiding this comment

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

You should never use ObjectManager directly.

Instead, inject ViewModel to the view.

@@ -0,0 +1,33 @@
<?php

$objectManager = getObjectManager();

Choose a reason for hiding this comment

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

You should never use ObjectManager directly.

Instead, use view model

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.

4 participants