-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixed rating issue website wise #18888
Fixed rating issue website wise #18888
Conversation
updated for 2.3.x
Update repo
Hi @saphaljha. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@saphaljha get rid of useless merge commits, please. |
/** | ||
* @var \Magento\Framework\App\State | ||
*/ | ||
protected $_state; |
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.
Hi, @saphaljha, due to Magento Technical guidelines we can't introduce protected properties. All new properties should be private.
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.
Hello @VladimirZaets , Thank you for your guidance, I'll check and update it asap.
@@ -48,12 +54,14 @@ public function __construct( | |||
\Magento\Framework\Module\Manager $moduleManager, | |||
\Magento\Store\Model\StoreManagerInterface $storeManager, | |||
\Magento\Review\Model\ResourceModel\Review\Summary $reviewSummary, | |||
\Magento\Framework\App\State $state, |
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.
Due to Magento backward-compatible guide we can't add new required parameters to constructor.
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.
Hello @VladimirZaets , I'll check and update it asap.
@@ -425,9 +433,11 @@ public function getReviewSummary($object, $onlyForCurrentStore = true) | |||
|
|||
$data = $connection->fetchAll($select, [':review_id' => $object->getReviewId()]); | |||
|
|||
$currentStore = ($this->_state->getAreaCode() == "adminhtml") ? false : $this->_storeManager->getStore()->getId(); |
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.
I think we should use an existing constant for providing area name.
Also, please use NULL instead of false, we shouldn't mix types of variable. Store id is number or null type, but not false
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.
Hello @VladimirZaets , I'll check and update it asap.
Hi @sidolov, thank you for the review. |
@saphaljha thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
Hi @saphaljha. Thank you for your contribution. |
Description (*)
Fixed rating issue website wise.
Fixed Issues (if relevant)
Manual testing scenarios (*)
N/A
Contribution checklist (*)