Skip to content

Conversation

@coderimus
Copy link
Contributor

@coderimus coderimus commented Mar 14, 2018

Description

Hello,
In this PR I fixed several not used class imports, remove legacy code and not used variables.

Fixed Issues (if relevant)

N/A

Manual testing scenarios

N/A

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)

{
return false;
$canVoid = false;
if ($this->getState() == self::STATE_REFUNDED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really nice to analyze from git history when this was hardcoded to return false, maybe a part of functionality is broken now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur
image
As I can see this was from the very beginning.

* Check order grand total and invoice amounts
*/
if ($invoice->isLast()) {
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern as https://github.com/magento/magento2/pull/14106/files#r174779579 - prior to removal we should understand the intention of code author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur
image
As I can see this code was added to the repo with the first push :)

use Magento\Framework\Exception\NoSuchEntityException;
use Magento\Tax\Api\Data\OrderTaxDetailsAppliedTaxInterfaceFactory as TaxDetailsDataObjectFactory;
use Magento\Tax\Api\Data\OrderTaxDetailsAppliedTaxInterface as AppliedTax;
use Magento\Tax\Model\Sales\Order\Tax;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid removal of unused imports, someday we will remove them all at once with php-cs-fixer. Just uncheck this inspection in PhpStorm for now or whatever tool you use for code analysis.

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.

4 participants