Prevent invoice cancelation multiple times

[osrecio]

Add Guard Clause to check if invoice has been canceled previously

Description

The invoice gets canceled twice, so order amounts are also being adjusted twice.

With same approach of Magento\Sales\Model\Order::cancel added guard clause to avoid recancel order.

Fixed Issues (if relevant)

1. Canceled invoice can be canceled again #9968

Manual testing scenarios

1. Load Invoice programmatically
2. Cancel Invoice
3. Try to Cancel the same invoice

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[fooman]

The changes look good to me. Would you be interested in adding a test that covers your scenario?

[osrecio]

@fooman In this case, I don't know how cover this scenario with tests. (I'm not an expert with PHPUnit 😔 ), in all cases cancel will return an invoice. If you could help me will be appreciated.

BTW I saw that InvoiceService.php not have a cancel method like OrderService.php. @vkublytskyi you think it might be interesting add this function for webapi? (maybe in other PR)

[vkublytskyi]

@osrecio Yes, it would be nice to have Web API for full invoice management and additional PR with such changes would be a great contribution. But be aware of specific backward compatibility policy that affects Magento\Sales\Api\InvoiceManagementInterface and does not allow simple addition of new methods to it. Please read Backward compatible development guide before implementation.

Regarding test coverage. I believe this fix is a good subject to start. Such changes can be covered with unit tests which are located in \Magento\Sales\Test\Unit\Model\Order\InvoiceTest.
As we can see cancel method is a pretty legacy and is not covered by any test method. As Invoice have only 3 possible states, methods testCancelOpenInvoice, testCancelCancelledInvoice, testCancelPaidInvoice should cover all scenarois. Cases with open and paid invoice may be tested by asserts on result of getState method. With already cancelled invoice we have to chek if some methods invoked or not. As a test class already have mock object we may use it:

$this->eventManagerMock->expects($this->never())->method('dispatch')->with('sales_order_invoice_cancel');

As well as we can ensure that for canceled order payment cancellation is not triggered:

$this->paymentMock->expects($this->never())->method('cancelInvoice');

For more safety order items collection may be mocked tu ensure that cancel method of order invoice items is not called.

---

In general this implementation consistent with Order implementation but it cannot be considered as the best approach:

1. The fact that invoice canceled twice is an error. If we silently handle this case it will hide a problem not fix it.
2. One of the most used mechanism for M2 customization and extending is a plugin. In case of current implementation all plugins on Magento\Sales\Model\Order\Invoice:cancel will be executed and that may lead to new bugs.

As a conclusion, I would like to ask you, in addition to the implemented solution, discover and fix why invoice cancelation is invoked twice.

[osrecio]

I got it, I will add new commits with test coverage. Thanks!

About your question: only can cancel invoice when you're making programmatically things. Actually this PR not is a fix, would be a improvement.

About API implementation I will work with Backward compatibility like we are made in PR: #9560 😄

[osrecio]

Added test, I hope are ok @vkublytskyi :)

[okorshenko]

@vkublytskyi here is a backport: #11261