-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Use repository to load order when manually creating an invoice #19727
Use repository to load order when manually creating an invoice #19727
Conversation
Hi @JeroenVanLeusden. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
*/ | ||
public function __construct( | ||
Action\Context $context, | ||
Registry $registry, | ||
PageFactory $resultPageFactory, | ||
InvoiceService $invoiceService | ||
InvoiceService $invoiceService, | ||
OrderRepositoryInterface $orderRepository |
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.
Please inject new dependencies in backward-compatible manner: https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter
$this->invoiceService = $invoiceService; | ||
$this->orderRepository = $orderRepository; | ||
|
||
parent::__construct($context); |
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.
Please call parent constructor first whenever possible.
8376b42
to
9653d67
Compare
@orlangur Updated the PR. |
Hi @orlangur, thank you for the review. |
|
||
try { | ||
/** @var \Magento\Sales\Model\Order $order */ | ||
$order = $this->_objectManager->create(\Magento\Sales\Model\Order::class)->load($orderId); | ||
$order = $this->orderRepository->get($orderId); | ||
if (!$order->getId()) { |
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.
This check doesn't make sense anymore because orderRepository
will throw an exception in case when order does not exist.
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.
@sidolov I've removed the check.
9546fc9
to
79ed11e
Compare
ENGCOM-3679: Static test fix. ENGCOM-3679: Unit test fix.
426d81f
to
3ee104d
Compare
Hi @sidolov, thank you for the review. |
@kandy can you please approve the performance degradation introduced by this PR:
|
@sivaschenko Degradation on Invoice Start Scenario is approved |
Hi @orlangur, thank you for the review. |
Hi @JeroenVanLeusden, thank you for your contribution! |
Description (*)
Use repository to load order when manually creating an invoice. This way extension attributes are set and can be used later in the flow (e.g. observers).
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)