Create cancel invoice method on API #12049
Conversation
crissanclick
force-pushed
the
WebApiCancelInvoice
branch
from
4a72b97 to
5e7ed2b
Compare
enriquei4
added
mm17es
Release Line: 2.1
Release Line: 2.2
and removed
Release Line: 2.1
labels
| namespace Magento\Sales\Api; | ||
|
|
||
| /** | ||
| * Interface InvoiceCancelInterface |
There was a problem hiding this comment.
Please provide description for the interface in the doc block.
It may be some information regarding it's responsibility and reasons for introduction.
| /** | ||
| * Interface InvoiceCancelInterface | ||
| * | ||
| * @package Magento\Sales\Api |
There was a problem hiding this comment.
@package annotation is not used in Magento
| * | ||
| * @package Magento\Sales\Api | ||
| */ | ||
| interface InvoiceCancelInterface |
There was a problem hiding this comment.
It may be a good idea to mark this with @api. This would also require providing PRs for other branches.
| * @param int $invoiceId | ||
| * @return bool | ||
| */ | ||
| public function cancel($invoiceId); |
There was a problem hiding this comment.
Since this PR is targeting 2.2-develop it is possible to declare scalar type hint and return type for the method.
| public function cancel($invoiceId) | ||
| { | ||
| /** @var \Magento\Sales\Api\Data\InvoiceInterface $invoice */ | ||
| $invoice = $this->invoiceRepository->get($invoiceId); |
There was a problem hiding this comment.
This method may throw an exception, which has to be either caught or declared in the interface.
| { | ||
| /** @var \Magento\Sales\Api\Data\InvoiceInterface $invoice */ | ||
| $invoice = $this->invoiceRepository->get($invoiceId); | ||
| if ($invoice && $invoice->canCancel()) { |
There was a problem hiding this comment.
I believe there is no reason to check for $invoice variable, as repository would throw an exception or return an invoice, this variable would not be empty at this point.
Additionally, canCancel check is already performed in the cancel method implementation, is there ay particular reason for this?
| /** | ||
| * @var InvoiceRepositoryInterface | ||
| */ | ||
| protected $invoiceRepository; |
There was a problem hiding this comment.
Please avoid creating protected properties in the core codebase, as usage of inheritance is discouraged.
| if ($invoice && $invoice->canCancel()) { | ||
| $invoice->cancel(); | ||
| $this->invoiceRepository->save($invoice); | ||
| return true; |
There was a problem hiding this comment.
Is it possible to return value of $invoice->getState() here? This would allow to get rid of the if statement.
|
@crissanclick I am closing this PR now due to inactivity. |
I have worked on this PR at #MM17ES
Description
Magento have not implemented cancel invoice method on API.
I have added the cancel method and created the tests.
Fixed Issues (if relevant)
Contribution checklist