This article is a continuation of Test Driven Development in Practice – Part 1 article.
In order to start refactoring we need to be sure our method is covered with Unit Tests. There are 5 Payment methods covered out-of-the-box in the 0.74.0-beta3 release. None of tests covers Payment::registerPaymentReviewAction() and it seems like we need to cover the method with tests.
Tip: In case you want to be 99% sure that the refactoring exercise does not break existing behavior and everything works as expected all cases in class/method should be covered with tests.
First Step in Testing…
As we may notice the registerPaymentReviewAction() method has 2 input arguments $action and $isOnline. Let’s create a basic test with a data provider to be sure we have set everything correct. Here is the Magento\Sales\Test\Model\Order\PaymentTest::testRegisterPaymentReviewAction() implementation:
[php]
namespace Magento\Sales\Test\Model\Order\PaymentTest;
class PaymentTest extends \PHPUnit_Framework_TestCase
{
//… other test methods
/**
* Test for registerPaymentOnlineAction() method
*
* @dataProvider registerPaymentReviewActionProvider
* @param string $dataSetName
* @param string $action
* @param bool $isOnline
*/
public function testRegisterPaymentReviewAction($dataSetName, $action, $isOnline)
{
$this->assertEquals(‘data_set_1’, $dataSetName);
$this->assertEquals(Payment::REVIEW_ACTION_ACCEPT, $action);
$this->assertTrue($isOnline);
}
public function registerPaymentReviewActionProvider()
{
return [
[
‘data_set_1’,
Payment::REVIEW_ACTION_ACCEPT,
true
]
];
}
}
[/php]
The test method is a simple example test which uses data provider method. Read below, my dear Reader to see refactored test in action.
Everything looks good, until I run this test and notice the following message:
[php]
Expectation failed for method name is equal to <string:getMethodInstance> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
[/php]
Actually it fails due to expectation set for the Magento\Payment\Helper\Data::getMethodInstance() method in PaymentTest::setUp(). It means that all test methods expect invocation of the getMethodInstance() method. Which is actually wrong for our newly implemented test.
[php]
class PaymentTest extends \PHPUnit_Framework_TestCase
{
// … other code
protected function setUp()
{
// … other code
$this->helperMock->expects($this->once())
->method(‘getMethodInstance’)
->will($this->returnValue($this->paymentMethodMock));
// … other code
}
}
[/php]
Tip: Try to avoid setting expectation for all mocked dependencies in the Test::setUp() method. In majority of cases, class methods use a different set of dependencies and method calls. It is correct to have expectations set in the appropriate test method.
I’ve moved mocking of the getMethodInstance() method to methods testCancel(), testPlace(), testAuthorize(), testAuthorizeFraudDetected() and testAuthorizeTransactionPending() and re-run the PaymentTest class again. The new testRegisterPaymentReviewAction() test passes.
Pic. 1. PaymentTest execution result with new simple test added.
After few hours spent on tests creation for the Payment::registerPaymentReviewAction() method finally all cases are covered. You may see that the method requires 23 new tests with 115 assertions. This is one more point to TDD approach usage. The method logic would be as simpler if tests were created prior to the method implementation.
Details of new tests created in the Magento\Sales\Test\Model\Order\PaymentTest class you may find at https://github.com/mcspronko/magento2 – Step 1.
Pic. 2. PaymentTest execution result with the Payment::registerPaymentReviewAction() method coverage.
Let’s go back to the Payment::registerPaymentReviewAction() method. The method processes different behavior based on the $action and $isOnline arguments passed directly to the method. There are 3 states (true, false and –1) of the $result variable that might be set during the switch case execution. Payment is processed based on the $result variable. We are going to use the Extract Method refactoring to move the logic that processes payment into separate methods.
[php]
public function registerPaymentReviewAction($action, $isOnline)
{
// … big switch statement
if (-1 === $result) {
$this->paymentReviewNegative($order, $message, $transactionId);
} elseif (true === $result) {
$this->paymentReviewTrue($invoice, $order, $message);
} elseif (false === $result) {
$this->paymentReviewFalse($invoice, $order, $message);
}
return $this;
}
protected function paymentReviewTrue($invoice, \Magento\Sales\Model\Order $order, $message)
{
if ($invoice) {
$invoice->pay();
$this->_updateTotals([‘base_amount_paid_online’ => $invoice->getBaseGrandTotal()]);
$order->addRelatedObject($invoice);
}
$order->setState(\Magento\Sales\Model\Order::STATE_PROCESSING, true, $message);
}
protected function paymentReviewFalse($invoice, \Magento\Sales\Model\Order $order, $message)
{
if ($invoice) {
$invoice->cancel();
$order->addRelatedObject($invoice);
}
$order->registerCancellation($message, false);
}
protected function paymentReviewNegative(\Magento\Sales\Model\Order $order, $message, $transactionId)
{
if ($order->getState() != \Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW) {
$status = $this->getIsFraudDetected() ? \Magento\Sales\Model\Order::STATUS_FRAUD : false;
$order->setState(\Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW, $status, $message);
if ($transactionId) {
$this->setLastTransId($transactionId);
}
} else {
$order->addStatusHistoryComment($message);
}
}
[/php]
Execution of PaymentTest related tests shows that everything works after our changes.
This allows us to move the payment processing logic to appropriate switch cases and remove the $result variable. However, there are the _Payment::prependMessage() and _Payment::appendTransactionToMessage() methods which are used to add transaction ID and additional comment in case it is specified in payment.
[php]
$message = $this->_prependMessage($message);
if ($transactionId) {
$message = $this->_appendTransactionToMessage($transactionId, $message);
}
[/php]
Since the _Payment::appendTransactionToMessage() method checks whether the $transactionId variable is set we simply remove condition duplication.
[php]
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage($message)
);
[/php]
Changes might be found at https://github.com/mcspronko/magento2 – Step 2.
Next step of refactoring is to eliminate the offline processing logic out of the Payment::registerPaymentReviewAction() method. There is no real value of having the logic in the method that is never used. 5 tests which verify the offline logic processing are skipped. We may use these tests in case offline processing should be added. For the $isOnline argument elimination we are going to use the Remove Parameter refactoring.
[php]
public function registerPaymentReviewAction($action)
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
// invoke the payment method to determine what to do with the transaction
$result = null;
$message = null;
switch ($action) {
case self::REVIEW_ACTION_ACCEPT:
if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
$result = true;
$message = __(‘Approved the payment online.’);
} else {
$result = -1;
$message = __(‘There is no need to approve this payment.’);
}
break;
case self::REVIEW_ACTION_DENY:
if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
$result = false;
$message = __(‘Denied the payment online’);
} else {
$result = -1;
$message = __(‘There is no need to deny this payment.’);
}
break;
case self::REVIEW_ACTION_UPDATE:
$this->getMethodInstance()->setStore(
$order->getStoreId()
)->fetchTransactionInfo(
$this,
$transactionId
);
if ($this->getIsTransactionApproved()) {
$result = true;
$message = __(‘Registered update about approved payment.’);
} elseif ($this->getIsTransactionDenied()) {
$result = false;
$message = __(‘Registered update about denied payment.’);
} else {
$result = -1;
$message = __(‘There is no update for the payment.’);
}
break;
default:
throw new \Exception(‘Not implemented.’);
}
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
if (-1 === $result) {
$this->paymentReviewNegative($order, $message, $transactionId);
} elseif (true === $result) {
$this->paymentReviewTrue($invoice, $order, $message);
} elseif (false === $result) {
$this->paymentReviewFalse($invoice, $order, $message);
}
return $this;
}
[/php]
After this exercise we may move the payment processing methods calls into appropriate switch cases.
[php]
public function registerPaymentReviewAction($action)
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
$result = null;
$message = null;
switch ($action) {
case self::REVIEW_ACTION_ACCEPT:
if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
$result = true;
$message = __(‘Approved the payment online.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewTrue($invoice, $order, $message);
} else {
$result = -1;
$message = __(‘There is no need to approve this payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
case self::REVIEW_ACTION_DENY:
if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
$result = false;
$message = __(‘Denied the payment online’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$result = -1;
$message = __(‘There is no need to deny this payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
case self::REVIEW_ACTION_UPDATE:
$this->getMethodInstance()->setStore(
$order->getStoreId()
)->fetchTransactionInfo(
$this,
$transactionId
);
if ($this->getIsTransactionApproved()) {
$result = true;
$message = __(‘Registered update about approved payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewTrue($invoice, $order, $message);
} elseif ($this->getIsTransactionDenied()) {
$result = false;
$message = __(‘Registered update about denied payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$result = -1;
$message = __(‘There is no update for the payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
default:
throw new \Exception(‘Not implemented.’);
}
return $this;
}
[/php]
All Payment::registerPaymentReviewAction() method usages in code should no longer use the $isOnline argument. As a result the local $result variable became useless as well as the $message variable initialization and we can remove it.
[php]
public function registerPaymentReviewAction($action)
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
switch ($action) {
case self::REVIEW_ACTION_ACCEPT:
if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
$message = __(‘Approved the payment online.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewTrue($invoice, $order, $message);
} else {
$message = __(‘There is no need to approve this payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
case self::REVIEW_ACTION_DENY:
if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
$message = __(‘Denied the payment online’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$message = __(‘There is no need to deny this payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
case self::REVIEW_ACTION_UPDATE:
$this->getMethodInstance()->setStore(
$order->getStoreId()
)->fetchTransactionInfo(
$this,
$transactionId
);
if ($this->getIsTransactionApproved()) {
$message = __(‘Registered update about approved payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewTrue($invoice, $order, $message);
} elseif ($this->getIsTransactionDenied()) {
$message = __(‘Registered update about denied payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$message = __(‘There is no update for the payment.’);
$message = $this->_appendTransactionToMessage($transactionId, $this->_prependMessage($message));
$this->paymentReviewNegative($order, $message, $transactionId);
}
break;
default:
throw new \Exception(‘Not implemented.’);
}
return $this;
}
[/php]
Now, we have switch statement with 3 separate action cases. We use the Extract Method refactoring to move each case into a separate class method. The Payment::accept() and Payment::deny() methods are proxy methods for the logic defined in the registerPaymentReviewAction() method. It makes more sense to move the logic from registerPaymentReviewAction() to these methods.
[php]
public function accept()
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Approved the payment online.’))
);
$this->paymentReviewTrue($invoice, $order, $message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no need to approve this payment.’))
);
$this->paymentReviewNegative($order, $message, $transactionId);
}
return $this;
}
public function deny()
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Denied the payment online’))
);
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no need to deny this payment.’))
);
$this->paymentReviewNegative($order, $message, $transactionId);
}
return $this;
}
[/php]
And the new Payment::update() method with the logic related to payment update action is:
[php]
public function update()
{
$order = $this->getOrder();
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
$this->getMethodInstance()->setStore($order->getStoreId())->fetchTransactionInfo($this, $transactionId);
if ($this->getIsTransactionApproved()) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Registered update about approved payment.’))
);
$this->paymentReviewTrue($invoice, $order, $message);
} elseif ($this->getIsTransactionDenied()) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Registered update about denied payment.’))
);
$this->paymentReviewFalse($invoice, $order, $message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no update for the payment.’))
);
$this->paymentReviewNegative($order, $message, $transactionId);
}
}
[/php]
The Payment::registerPaymentReviewAction() method looks simpler, isn’t it?
[php]
public function registerPaymentReviewAction($action)
{
switch ($action) {
case self::REVIEW_ACTION_ACCEPT:
$this->accept();
break;
case self::REVIEW_ACTION_DENY:
$this->deny();
break;
case self::REVIEW_ACTION_UPDATE:
$this->update();
break;
default:
throw new \Exception(‘Not implemented.’);
}
return $this;
}
[/php]
All tests are passed except testRegisterPaymentReviewActionOnlineException() with the message:
[php]
Expectation failed for method name is equal to <string:getTransactionId> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
[/php]
You may find the updated PaymentTest class with tests that verify the allow(), deny(), and update() methods at https://github.com/mcspronko/magento2 – Step 3.
It is reasonable to modify the test by removing 3 lines of code which set transaction_id to the Payment~( instance and mock the Magento\Sales\Model\Order\Invoice instance. After this change all tests pass successfully.
Pic. 3. PaymentTest execution result without offline tests coverage.
You may check the https://github.com/mcspronko/magento2 – Step 4 commit with the updated Payment::registerPaymentReviewAction() method.
As a result of test refactoring and offline logic tests elimination we have 15 tests with 85 assertions related to the payment review functionality.
With the methods introduced for each of the payment review actions we may update the tests so that the registerPaymentReviewAction() method is removed as duplicate. Before we remove the method we should check and update the registerPaymentReviewAction() method usages to appropriate new methods.
The Payment::registerPaymentReviewAction() method is only used in \Magento\Sales\Controller\Adminhtml\Order\ReviewPayment::execute().
[php]
namespace Magento\Sales\Controller\Adminhtml\Order;
class ReviewPayment extends \Magento\Sales\Controller\Adminhtml\Order
{
public function execute()
{
//more code
switch ($action) {
// more cases
case ‘update’:
$order->getPayment()->registerPaymentReviewAction(
\Magento\Sales\Model\Order\Payment::REVIEW_ACTION_UPDATE,
true
);
$message = __(‘The payment update has been made.’);
break;
}
// more code
}
}
[/php]
Before we update code to use the Payment::update() method we are going to cover ReviewPayment::execute() with Unit Tests. It appeared that there is no ReviewPaymentTest class implemented, so let’s create it.
The ReviewPaymentTest::testExecuteUpdateAction() method test verifies that registerPaymentReviewAction() is called with the appropriate method arguments.
[php]
public function testExecuteUpdateAction()
{
$orderId = 30;
$action = ‘update’;
$this->requestMock->expects($this->at(0))->method(‘getParam’)->with(‘order_id’)->willReturn($orderId);
$this->requestMock->expects($this->at(1))->method(‘getParam’)->with(‘action’)->willReturn($action);
$this->resultRedirectFactoryMock->expects($this->once())->method(‘create’)
->willReturn($this->resultRedirectMock);
$this->objectManagerMock->expects($this->once())->method(‘create’)->with(‘Magento\Sales\Model\Order’)
->willReturn($this->orderMock);
$this->orderMock->expects($this->once())->method(‘load’)->with($orderId)->willReturn($this->orderMock);
$this->orderMock->expects($this->any())->method(‘getId’)->willReturn($orderId);
$this->orderMock->expects($this->once())->method(‘getPayment’)->willReturn($this->paymentMock);
$this->orderMock->expects($this->once())->method(‘save’)->willReturnSelf();
$this->messageManagerMock->expects($this->once())->method(‘addSuccess’)
->with(‘The payment update has been made.’);
$this->resultRedirectMock->expects($this->once())->method(‘setPath’)->with(‘sales/*/’)->willReturnSelf();
$this->paymentMock->expects($this->once())->method(‘registerPaymentReviewAction’)
->with(\Magento\Sales\Model\Order\Payment::REVIEW_ACTION_UPDATE, true);
$result = $this->reviewPayment->execute();
$this->assertEquals($this->resultRedirectMock, $result);
}
[/php]
You may find the full ReviewPaymentTest class implementation at https://github.com/mcspronko/magento2 – Step 5.
In order to use the Payment::update() method instead of the Payment::registerPaymentReviewAction() method, the testExecuteUpdateAction() test should expect the Payment::update() method call.
Instead of this line of code:
[php]
$this->paymentMock->expects($this->once())->method(‘registerPaymentReviewAction’)
->with(\Magento\Sales\Model\Order\Payment::REVIEW_ACTION_UPDATE, true);
[/php]
We expect the update() method to be called:
[php]
$this->paymentMock->expects($this->once())->method(‘update’);
[/php]
and execute the test which should fail:
[php]
Expectation failed for method name is equal to <string:update> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
[/php]
Let’s now refactor the ReviewPayment::execute() method to use the Payment::update() method.
Instead of using the registerPaymentReviewAction() method:
[php]
$order->getPayment()->registerPaymentReviewAction(
\Magento\Sales\Model\Order\Payment::REVIEW_ACTION_UPDATE,
true
);
[/php]
We use the update() method:
[php]
$order->getPayment()->update();
[/php]
And execute tests again. And, here we are! Our tests are green now.
The additional search for the registerPaymentReviewAction() method usage shows no results, so we may safely remove the method from the Payment class.
Going forward, the paymentReviewNegative(), paymentReviewTrue() and paymentReviewFalse() methods should have self-explainable method names. The paymentReviewNegative() method is responsible for setting an order state to ‘payment_review’ with appropriate status and message. It makes sense to rename the method to setOrderStatePaymentReview() and update all usages of old method name. Also, since Order instance might be used from same Payment instance let’s use Remove Parameter refactoring for this.
Current implementation:
[php]
protected function paymentReviewNegative(\Magento\Sales\Model\Order $order, $message, $transactionId)
{
if ($order->getState() != \Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW) {
$status = $this->getIsFraudDetected() ? \Magento\Sales\Model\Order::STATUS_FRAUD : false;
$order->setState(\Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW, $status, $message);
if ($transactionId) {
$this->setLastTransId($transactionId);
}
} else {
$order->addStatusHistoryComment($message);
}
}
[/php]
Updated implementation:
[php]
protected function setOrderStatePaymentReview($message, $transactionId)
{
if ($this->getOrder()->getState() != Order::STATE_PAYMENT_REVIEW) {
$status = $this->getIsFraudDetected() ? Order::STATUS_FRAUD : false;
$this->getOrder()->setState(Order::STATE_PAYMENT_REVIEW, $status, $message);
if ($transactionId) {
$this->setLastTransId($transactionId);
}
} else {
$this->getOrder()->addStatusHistoryComment($message);
}
}
[/php]
Running tests to verify the changes. All tests pass.
The paymentReviewTrue() method updates the base_amount_paid_online total of the Payment object and sets the Order state to ‘processing’. It looks like setting the Order state is additional method’s responsibility which should be moved out. The new method name is going to be updated to match the new behavior.
Current implementation:
[php]
protected function paymentReviewTrue($invoice, \Magento\Sales\Model\Order $order, $message)
{
if ($invoice) {
$invoice->pay();
$this->_updateTotals([‘base_amount_paid_online’ => $invoice->getBaseGrandTotal()]);
$order->addRelatedObject($invoice);
}
$order->setState(\Magento\Sales\Model\Order::STATE_PROCESSING, true, $message);
}
[/php]
Updated implementation:
[php]
protected function updateBaseAmountPaidOnlineTotal($invoice)
{
if ($invoice instanceof Invoice) {
$invoice->pay();
$this->_updateTotals([‘base_amount_paid_online’ => $invoice->getBaseGrandTotal()]);
$this->getOrder()->addRelatedObject($invoice);
}
}
protected function setOrderStateProcessing($message)
{
$this->getOrder()->setState(Order::STATE_PROCESSING, true, $message);
}
[/php]
The paymentReviewFalse() method is responsible for initiating Invoice cancellation and registering cancellation in the current Order. The new method is going to be responsible for the Invoice cancellation logic.
Current implementation:
[php]
protected function paymentReviewFalse($invoice, \Magento\Sales\Model\Order $order, $message)
{
if ($invoice) {
$invoice->cancel();
$order->addRelatedObject($invoice);
}
$order->registerCancellation($message, false);
}
[/php]
Updated implementation:
[php]
protected function cancelInvoiceAndRegisterCancellation($invoice, $message)
{
if ($invoice instanceof Invoice) {
$invoice->cancel();
$this->getOrder()->addRelatedObject($invoice);
}
$this->getOrder()->registerCancellation($message, false);
}
[/php]
With the method changes we have additionally updated usages of these methods and here what we have:
[php]
public function accept()
{
$transactionId = $this->getLastTransId();
/** @var \Magento\Payment\Model\Method\AbstractMethod $method */
$method = $this->getMethodInstance()->setStore($this->getOrder()->getStoreId());
if ($method->acceptPayment($this)) {
$invoice = $this->_getInvoiceForTransactionId($transactionId);
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Approved the payment online.’))
);
$this->updateBaseAmountPaidOnlineTotal($invoice);
$this->setOrderStateProcessing($message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no need to approve this payment.’))
);
$this->setOrderStatePaymentReview($message, $transactionId);
}
return $this;
}
public function deny()
{
$transactionId = $this->getLastTransId();
/** @var \Magento\Payment\Model\Method\AbstractMethod $method */
$method = $this->getMethodInstance()->setStore($this->getOrder()->getStoreId());
if ($method->denyPayment($this)) {
$invoice = $this->_getInvoiceForTransactionId($transactionId);
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Denied the payment online’))
);
$this->cancelInvoiceAndRegisterCancellation($invoice, $message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no need to deny this payment.’))
);
$this->setOrderStatePaymentReview($message, $transactionId);
}
return $this;
}
public function update()
{
$transactionId = $this->getLastTransId();
$invoice = $this->_getInvoiceForTransactionId($transactionId);
$this->getMethodInstance()->setStore($this->getOrder()->getStoreId())
->fetchTransactionInfo($this, $transactionId);
if ($this->getIsTransactionApproved()) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Registered update about approved payment.’))
);
$this->updateBaseAmountPaidOnlineTotal($invoice);
$this->setOrderStateProcessing($message);
} elseif ($this->getIsTransactionDenied()) {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘Registered update about denied payment.’))
);
$this->cancelInvoiceAndRegisterCancellation($invoice, $message);
} else {
$message = $this->_appendTransactionToMessage(
$transactionId,
$this->_prependMessage(__(‘There is no update for the payment.’))
);
$this->setOrderStatePaymentReview($message, $transactionId);
}
return $this;
}
[/php]
You may find all changes at https://github.com/mcspronko/magento2/commits/order_payment.
Final Thoughts
In this small article I’ve shared some of the refactoring techniques which I use on day-to-day basis. Refactoring is a very powerful exercise which helps to achieve loosely coupled and highly cohesive code. In case we combine refactoring with the Test Driven Development approach product bugs defect rate can be low. Small “Red, Green, Refactor” iterations allow moving faster and having no stress at work.
The Magento\Sales\Model\Order\Payment class now has 2620 lines of code. On one hand, this is bad because I added 25 new lines. On the other hand the class now has 3 self-explainable public methods. Moving forward, we may use the Move Method refactoring in order to move these and other methods to the new OrderReview class. Additionally, the logic and code duplication of service related methods (such as registerCaptureNotification(), capture(), place(), registerRefundNotification() etc.) in the class are a subject to refactoring and further modifications.
There is a great Refactoring Catalog prepared by Martin Fowler. You may use it as a starting point for your further refactoring or as a reference with implementation examples.
Community Credits
Many thanks to ihor-sviziev who checks and raises issues in the Magento 2 project. Every day large Magento Community brings both Magento 2 and Magento 1 to the new level. This is very engaging and motivating. Thank you guys.
Thanks to http://www.richardbagshaw.co.uk/ for image.
Looking forward for your feedback and comments.
Leave a Reply