Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Codacy warning fixes #8949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a229c1a
Invalid css colors fixed
HansSchouten Mar 20, 2017
fb3b044
Fix expected LBRACE warning
HansSchouten Mar 20, 2017
170ced4
Reduce cyclomatic complexity
HansSchouten Mar 20, 2017
93be9bd
Cyclomatic complexity reduced
HansSchouten Mar 20, 2017
33236df
Missing @return added
HansSchouten Mar 20, 2017
4c47b96
Reduced cyclomatic complexity
HansSchouten Mar 21, 2017
00e1da7
Code refactored
HansSchouten Mar 21, 2017
fccf34b
Boolean case changed, long line split
HansSchouten Mar 21, 2017
eab05d4
Multi-line function call fixed
HansSchouten Mar 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 51 additions & 27 deletions app/code/Magento/Sales/Controller/Adminhtml/Order/Status/Save.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@

class Save extends \Magento\Sales\Controller\Adminhtml\Order\Status
{
/**
* @var \Magento\Backend\Model\View\Result\Redirect
*/
private $resultRedirect;

/**
* Save status form processing
*
Expand All @@ -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();
Copy link
Contributor

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?

if ($data) {
$statusCode = $this->getRequest()->getParam('status');

Expand All @@ -37,35 +41,55 @@ public function execute()
$label = $filterManager->stripTags($label);
}

$status = $this->_objectManager->create(\Magento\Sales\Model\Order\Status::class)->load($statusCode);
// check if status exist
if ($isNew && $status->getStatus()) {
$this->messageManager->addError(__('We found another order status with the same order status code.'));
$this->_getSession()->setFormData($data);
return $resultRedirect->setPath('sales/*/new');
}

$status->setData($data)->setStatus($statusCode);

try {
$status->save();
$this->messageManager->addSuccess(__('You saved the order status.'));
return $resultRedirect->setPath('sales/*/');
} catch (\Magento\Framework\Exception\LocalizedException $e) {
$this->messageManager->addError($e->getMessage());
} catch (\Exception $e) {
$this->messageManager->addException(
$e,
__('We can\'t add the order status right now.')
);
if ($this->updateStatus($isNew, $data, $statusCode) !== false) {
return $this->resultRedirect;
Copy link
Contributor

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.

}

$this->_getSession()->setFormData($data);
if ($isNew) {
return $resultRedirect->setPath('sales/*/new');
} else {
return $resultRedirect->setPath('sales/*/edit', ['status' => $this->getRequest()->getParam('status')]);
return $this->resultRedirect->setPath('sales/*/new');
}
return $this->resultRedirect->setPath(
Copy link
Contributor

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' ...

'sales/*/edit',
['status' => $this->getRequest()->getParam('status')]
);
}
return $this->resultRedirect->setPath('sales/*/');
Copy link
Contributor

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?

}

/**
* Update the order status
*
* @param bool $isNew
* @param array $data
* @param string $statusCode
* @return bool
*/
private function updateStatus($isNew, $data, $statusCode)
{
$status = $this->_objectManager->create(\Magento\Sales\Model\Order\Status::class)->load($statusCode);
// check if status exist
if ($isNew && $status->getStatus()) {
$this->messageManager->addError(__('We found another order status with the same order status code.'));
$this->_getSession()->setFormData($data);
return $this->resultRedirect->setPath('sales/*/new');
}

$status->setData($data)->setStatus($statusCode);

try {
$status->save();
$this->messageManager->addSuccess(__('You saved the order status.'));
return $this->resultRedirect->setPath('sales/*/');
} catch (\Magento\Framework\Exception\LocalizedException $e) {
$this->messageManager->addError($e->getMessage());
} catch (\Exception $e) {
$this->messageManager->addException(
$e,
__('We can\'t add the order status right now.')
);
}
return $resultRedirect->setPath('sales/*/');

return false;
}
}
41 changes: 27 additions & 14 deletions app/code/Magento/Theme/Model/Design/Config/Validator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,20 +75,33 @@ public function validate(DesignConfigInterface $designConfig)
}
$text = $template->getTemplateText();
$template->revertDesign();
// Check if template body has a reference to the same config path
if (preg_match_all(Template::CONSTRUCTION_TEMPLATE_PATTERN, $text, $constructions, PREG_SET_ORDER)) {
foreach ($constructions as $construction) {
$configPath = isset($construction[2]) ? $construction[2] : '';
$params = $this->getParameters($configPath);
if (isset($params['config_path']) && $params['config_path'] == $data['config_path']) {
throw new LocalizedException(
__(
"The %templateName contains an incorrect configuration. The template has " .
"a reference to itself. Either remove or change the reference.",
["templateName" => $name]
)
);
};
$this->checkTemplateConfiguration($data, $text, $name);
}
}

/**
* Check if template body has a reference to the same config path
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty logical change 👍

*
* @param array $data
* @param string $text
* @param string $name
* @throws LocalizedException
* @return void
*/
private function checkTemplateConfiguration($data, $text, $name)
{
if (preg_match_all(Template::CONSTRUCTION_TEMPLATE_PATTERN, $text, $constructions, PREG_SET_ORDER)) {
foreach ($constructions as $construction) {
$configPath = isset($construction[2]) ? $construction[2] : '';
$params = $this->getParameters($configPath);
if (isset($params['config_path']) && $params['config_path'] == $data['config_path']) {
throw new LocalizedException(
__(
"The %templateName contains an incorrect configuration. The template has " .
"a reference to itself. Either remove or change the reference.",
["templateName" => $name]
)
);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/web/css/docs/docs.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/web/jquery/fileUploader/css/jquery.fileupload-ui.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@
}

/* Fix for IE 6: */
*html .fileinput-button {
* html .fileinput-button {
line-height: 22px;
margin: 1px -3px 0 0;
}

/* Fix for IE 7: */
*+html .fileinput-button {
* + html .fileinput-button {
margin: 1px 0 0 0;
}

Expand Down