Skip to content

Conversation

@omaxmo
Copy link
Contributor

@omaxmo omaxmo commented Nov 28, 2018

Description (*)

We can cancel invoice if invoice state == self::STATE_OPEN but we can't cancel invoice if invoice state == STATE_PAID.

Fixed Issues (if relevant)

  1. Can't cancelled invoice if invoice state == STATE_PAID #18509 Can't cancelled invoice if invoice state == STATE_PAID

Manual testing scenarios (*)

  1. Load an invoice programmatically where state == STATE_PAID
  2. Call $invoice->cancel()

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)

check if invoice already cancelled
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 28, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @omaxmo. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@sivaschenko
Copy link
Member

@omaxmo thanks for the contribution!

I see there is an open question in the issue #18509 (comment)

Can invoice be cancelled if it is paid? If yes then should cancellation create a credit memo ond refund the paid sum?

@omaxmo
Copy link
Contributor Author

omaxmo commented Nov 28, 2018

@omaxmo thanks for the contribution!

I see there is an open question in the issue #18509 (comment)

Can invoice be cancelled if it is paid? If yes then should cancellation create a credit memo ond refund the paid sum?

@sivaschenko Magento had opportunity to cancel invoice with state == STATE_PAID. The opportunity has been lost in #9968 #11261 .

Not sure about credit memo and refund.
Before we just

   if ($this->getState() == self::STATE_PAID) {
            $order->setTotalPaid($order->getTotalPaid() - $this->getGrandTotal());
            $order->setBaseTotalPaid($order->getBaseTotalPaid() - $this->getBaseGrandTotal());
        }

Anyway we have some code which will never be used if we not have opportunity to cancel invoice if invoice state == STATE_PAID

     
   if ($this->getState() == self::STATE_PAID) {
            $order->setTotalPaid($order->getTotalPaid() - $this->getGrandTotal());
            $order->setBaseTotalPaid($order->getBaseTotalPaid() - $this->getBaseGrandTotal());
        }


Copy link
Member

@larsroettig larsroettig left a comment

Choose a reason for hiding this comment

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

This PR will break the most PSP-Provider we should close.

For more Information see:
https://github.com/magento/magento2/pull/11261/files

@rodrigowebjump rodrigowebjump self-requested a review November 29, 2018 03:17
@rodrigowebjump
Copy link
Member

@omaxmo Could you double check if these changes break PR 11261?

@larsroettig From my perspective, this will not break the PR, since isCanceled will prevent invoice to be canceled again.

@VladimirZaets VladimirZaets self-assigned this Nov 29, 2018
@VladimirZaets
Copy link
Contributor

@omaxmo As I see the button to cancel invoice is also available only in the case when status is open

@orlangur
Copy link
Contributor

@omaxmo this is not a valid change, reported issue is closed as invalid, see #18509 (comment) for more details.

Thanks for collaboration!

@orlangur orlangur closed this Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants