Skip to content

Fatal error page occurs if use wrong shipment id ,invoice id ,creditmemo id passed in url #30410

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

Merged
merged 24 commits into from
Dec 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ff1a003
Fixed issue related with wrong invoice id ,creditmemo id and shipmen …
Oct 10, 2020
0e78fe4
Fixed issue related with wrong invoice id ,creditmemo id and shipmen …
Oct 10, 2020
ff5de32
Update ViewTest.php
Oct 10, 2020
1dab430
Merge branch '2.4-develop' into dcmm2020
Oct 10, 2020
6b1837b
Added new line to to mftf files, added words to csv and fixed compila…
Oct 12, 2020
589b1d2
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Oct 12, 2020
4ba689a
Fixed some test case issues
Oct 14, 2020
9e99f43
Fixed MFTF issue
Oct 14, 2020
6359ac5
Fixed MFTF issue
Oct 28, 2020
ade5168
updated test files
Oct 29, 2020
1f0bcbf
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Nov 6, 2020
937f9e5
fix mftf, static
engcom-Charlie Nov 6, 2020
2221a8b
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Nov 16, 2020
32ae2ff
fix static
engcom-Charlie Nov 16, 2020
3fb5db4
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Nov 18, 2020
e38122a
fix i18n
engcom-Charlie Nov 18, 2020
3b0e854
fix action group, revert i18n changes
engcom-Charlie Nov 27, 2020
7006a96
revert i18n shipping changes
engcom-Charlie Nov 27, 2020
beac203
remove redundant spaces
engcom-Charlie Nov 30, 2020
564a8a6
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Nov 30, 2020
ffc0ab7
update severity, testCaseId
engcom-Charlie Dec 1, 2020
247817a
update severity, testCaseId
engcom-Charlie Dec 1, 2020
5ec7bd4
Merge branch '2.4-develop' into dcmm2020
engcom-Charlie Dec 1, 2020
52b5270
Merge branch '2.4-develop' into dcmm2020
Dec 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
namespace Magento\Sales\Controller\Adminhtml\Order\Creditmemo;

use Magento\Backend\App\Action;
use Magento\Framework\App\Action\HttpGetActionInterface as HttpGetActionInterface;

class View extends \Magento\Backend\App\Action
class View extends \Magento\Backend\App\Action implements HttpGetActionInterface
{
/**
* Authorization level of a basic admin session
Expand Down Expand Up @@ -75,9 +76,9 @@ public function execute()
}
return $resultPage;
} else {
$resultForward = $this->resultForwardFactory->create();
$resultForward->forward('noroute');
return $resultForward;
$resultRedirect = $this->resultRedirectFactory->create();
$resultRedirect->setPath('sales/creditmemo');
return $resultRedirect;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@

use Magento\Framework\DataObject;
use Magento\Sales\Api\CreditmemoRepositoryInterface;
use \Magento\Sales\Model\Order\CreditmemoFactory;
use Magento\Sales\Model\Order;
use Magento\Sales\Model\Order\CreditmemoFactory;

/**
* Class CreditmemoLoader
* Loader for creditmemo
*
* @package Magento\Sales\Controller\Adminhtml\Order
* @method CreditmemoLoader setCreditmemoId($id)
* @method CreditmemoLoader setCreditmemo($creditMemo)
* @method CreditmemoLoader setInvoiceId($id)
Expand All @@ -22,6 +22,7 @@
* @method string getCreditmemo()
* @method int getInvoiceId()
* @method int getOrderId()
* @SuppressWarnings(PHPMD.CookieAndSessionMisuse)
*/
class CreditmemoLoader extends DataObject
{
Expand Down Expand Up @@ -129,7 +130,8 @@ protected function _getItemData()

/**
* Check if creditmeno can be created for order
* @param \Magento\Sales\Model\Order $order
*
* @param Order $order
* @return bool
*/
protected function _canCreditmemo($order)
Expand All @@ -153,7 +155,9 @@ protected function _canCreditmemo($order)
}

/**
* @param \Magento\Sales\Model\Order $order
* Inits invoice
*
* @param Order $order
* @return $this|bool
*/
protected function _initInvoice($order)
Expand Down Expand Up @@ -181,7 +185,12 @@ public function load()
$creditmemoId = $this->getCreditmemoId();
$orderId = $this->getOrderId();
if ($creditmemoId) {
$creditmemo = $this->creditmemoRepository->get($creditmemoId);
try {
$creditmemo = $this->creditmemoRepository->get($creditmemoId);
} catch (\Exception $e) {
$this->messageManager->addErrorMessage(__('This creditmemo no longer exists.'));
return false;
}
} elseif ($orderId) {
$data = $this->getCreditmemo();
$order = $this->orderFactory->create()->load($orderId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ public function execute()
{
$invoice = $this->getInvoice();
if (!$invoice) {
/** @var \Magento\Framework\Controller\Result\Forward $resultForward */
$resultForward = $this->resultForwardFactory->create();
return $resultForward->forward('noroute');
/** @var \Magento\Framework\Controller\Result\RedirectFactory $resultRedirect */
$resultRedirect = $this->resultRedirectFactory->create();
$resultRedirect->setPath('sales/invoice');
return $resultRedirect;
}

/** @var \Magento\Backend\Model\View\Result\Page $resultPage */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-->

<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="AdminGoToCreditmemoViewActionGroup">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest renaming this action group to something like AdminOpenCreditMemoByEntityIdActionGroup. Just to use more or less the same naming as, for example, in the following implementation

<actionGroup name="AdminOpenOrderByEntityIdActionGroup">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @rogyar .Thaks for your review. But can you please let me know what is the need of renaming the action group name? i have used this name based on other action group name.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is not critical, but you may find the example of another action group in the Sales module in my previous comment. My recommendation is more about keeping the names transparent, i.e.

  • AdminOpenCreditMemoByEntityIdActionGroup (new)
  • AdminOpenOrderByEntityIdActionGroup (existing)

<annotations>
<description>Goes to the Order Creditmemo View Page.</description>
</annotations>
<arguments>
<argument name="identifier" type="string"/>
</arguments>

<amOnPage url="{{AdminCreditMemoViewPage.url}}{{identifier}}" stepKey="amOnCreditmemoViewPage"/>
<waitForPageLoad stepKey="waitForPageLoad"/>
</actionGroup>
</actionGroups>
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<pages xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Page/etc/PageObject.xsd">
<page name="AdminCreditMemoViewPage" url="sales/order_creditmemo/view/creditmemo_id/{{memoId}}/" parameterized="true" area="admin" module="Magento_Sales">
<page name="AdminCreditMemoViewPage" url="sales/creditmemo/view/creditmemo_id/" area="admin" module="Magento_Sales">
<section name="AdminCreditMemoViewItemsSection"/>
<section name="AdminCreditMemoViewTotalSection"/>
</page>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-->

<tests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/testSchema.xsd">
<test name="AdminOpenCreditmemoViewPageWithWrongCreditmemoIdTest">
<annotations>
<stories value="Creditmemo Page With Wrong Creditmemo Id"/>
<title value="Open Creditmemo View Page with Wrong Creditmemo Id"/>
<description value="Open Creditmemo View Page with Wrong Creditmemo Id."/>
<severity value="BLOCKER"/>
<testCaseId value="MC-39500"/>
<group value="sales"/>
</annotations>
<before>
<actionGroup ref="AdminLoginActionGroup" stepKey="LoginAsAdmin"/>
</before>
<after>
<actionGroup ref="AdminLogoutActionGroup" stepKey="logout"/>
</after>

<actionGroup ref="AdminGoToCreditmemoViewActionGroup" stepKey="navigateOpenCreditmemoViewPage">
<argument name="identifier" value="test"/>
</actionGroup>

<waitForPageLoad stepKey="waitForPageLoad"/>
<seeInCurrentUrl url="{{AdminCreditMemosGridPage.url}}" stepKey="redirectToCreditMemosGridPage"/>
<see selector="{{AdminMessagesSection.error}}" userInput='This creditmemo no longer exists.' stepKey="seeErrorMessage"/>
</test>
</tests>
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
use Magento\Backend\Model\View\Result\Forward;
use Magento\Backend\Model\View\Result\ForwardFactory;
use Magento\Backend\Model\View\Result\Page;
use Magento\Backend\Model\View\Result\Redirect;
use Magento\Backend\Model\View\Result\RedirectFactory;
use Magento\Framework\App\ActionFlag;
use Magento\Framework\App\Request\Http;
use Magento\Framework\Message\Manager;
Expand Down Expand Up @@ -105,6 +107,17 @@ class ViewTest extends TestCase
*/
protected $pageTitleMock;

/**
* @var \Magento\Sales\Controller\Adminhtml\Order\Creditmemo\View
* @var RedirectFactory|MockObject
*/
protected $resultRedirectFactoryMock;

/**
* @var Redirect|MockObject
*/
protected $resultRedirectMock;

/**
* @var PageFactory|MockObject
*/
Expand All @@ -130,9 +143,6 @@ class ViewTest extends TestCase
*/
protected function setUp(): void
{
$titleMock = $this->getMockBuilder(\Magento\Framework\App\Action\Title::class)
->disableOriginalConstructor()
->getMock();
$this->invoiceMock = $this->getMockBuilder(Invoice::class)
->disableOriginalConstructor()
->getMock();
Expand Down Expand Up @@ -203,7 +213,13 @@ protected function setUp(): void
$this->resultForwardMock = $this->getMockBuilder(Forward::class)
->disableOriginalConstructor()
->getMock();

$this->resultRedirectFactoryMock = $this->getMockBuilder(RedirectFactory::class)
->disableOriginalConstructor()
->setMethods(['create'])
->getMock();
$this->resultRedirectMock = $this->getMockBuilder(Redirect::class)
->disableOriginalConstructor()
->getMock();
$this->contextMock->expects($this->any())
->method('getSession')
->willReturn($this->sessionMock);
Expand All @@ -219,9 +235,6 @@ protected function setUp(): void
$this->contextMock->expects($this->any())
->method('getObjectManager')
->willReturn($this->objectManagerMock);
$this->contextMock->expects($this->any())
->method('getTitle')
->willReturn($titleMock);
$this->contextMock->expects($this->any())
->method('getMessageManager')
->willReturn($this->messageManagerMock);
Expand All @@ -239,7 +252,8 @@ protected function setUp(): void
'context' => $this->contextMock,
'creditmemoLoader' => $this->loaderMock,
'resultPageFactory' => $this->resultPageFactoryMock,
'resultForwardFactory' => $this->resultForwardFactoryMock
'resultForwardFactory' => $this->resultForwardFactoryMock,
'resultRedirectFactory' => $this->resultRedirectFactoryMock
]
);
}
Expand All @@ -252,16 +266,11 @@ public function testExecuteNoCreditMemo()
$this->loaderMock->expects($this->once())
->method('load')
->willReturn(false);
$this->resultForwardFactoryMock->expects($this->once())
->method('create')
->willReturn($this->resultForwardMock);
$this->resultForwardMock->expects($this->once())
->method('forward')
->with('noroute')
->willReturnSelf();

$this->prepareRedirect();
$this->setPath('sales/creditmemo');
$this->assertInstanceOf(
Forward::class,
Redirect::class,
$this->controller->execute()
);
}
Expand Down Expand Up @@ -322,4 +331,25 @@ public function executeDataProvider()
[$this->invoiceMock]
];
}

/**
* prepareRedirect
*/
protected function prepareRedirect()
{
$this->resultRedirectFactoryMock->expects($this->once())
->method('create')
->willReturn($this->resultRedirectMock);
}

/**
* @param string $path
* @param array $params
*/
protected function setPath($path, $params = [])
{
$this->resultRedirectMock->expects($this->once())
->method('setPath')
->with($path, $params);
}
}
Loading