-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Codacy warning fixes #8949
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
Codacy warning fixes #8949
Conversation
@@ -17,8 +22,7 @@ public function execute() | |||
{ | |||
$data = $this->getRequest()->getPostValue(); | |||
$isNew = $this->getRequest()->getParam('is_new'); | |||
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */ | |||
$resultRedirect = $this->resultRedirectFactory->create(); | |||
$this->resultRedirect = $this->resultRedirectFactory->create(); |
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.
Why need this instead of local variable?
__('We can\'t add the order status right now.') | ||
); | ||
if ($this->updateStatus($isNew, $data, $statusCode) !== false) { | ||
return $this->resultRedirect; |
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.
Such logic seems quite confusing to me.
Random piece of code is moved to a separate method which may return boolean or object :)
Please change method name to isStatusUpdated
and return true
instead of object there.
} | ||
return $this->resultRedirect->setPath( |
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.
Two returns is not a good practice. Simply change to
return $isNew
? $this->resultRedirect->setPath('sales/*/new')
: $this->resultRedirect->setPath('sales/*/edit' ...
['status' => $this->getRequest()->getParam('status')] | ||
); | ||
} | ||
return $this->resultRedirect->setPath('sales/*/'); |
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.
Where this new return comes from?
} | ||
|
||
/** | ||
* Check if template body has a reference to the same config path |
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.
Pretty logical change 👍
Closing this PR for now. Feel free to reopen if you would like to continue at any time. |
[Bengals Team] Test Fixes and Automation Tasks
Description
This pull request fixes a number of errors and warnings reported by Codacy.
Contribution checklist