-
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
Fix issue #30270 Product Reviews can not be sorted by Admin on the Product edit page #30477
Fix issue #30270 Product Reviews can not be sorted by Admin on the Product edit page #30477
Conversation
Hi @Chandresh22. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
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 @Chandresh22, thank you for your contribution. However, due to Magento Definition of Done the changes should be covered by tests. Could you please cover your fix by a MFTF test, that will pass through all the steps and then assert that the grid sorting works?
Thank you.
@eduard13 can you help me what is mean Auto Test not covered? and how to resolve this thanks. |
@Chandresh22 as mentioned in my previous comment, in order to deliver this fix, it should be covered by automated tests. I suggest adding a simple MFTF test, that will make sure that the grid sorting works as expected. |
@magento run Static Tests |
@magento run all tests |
1 similar comment
@magento run all tests |
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.
Please check my comments below. Additionally, as far there is already a Unit Test that covers this class, please update accordingly your test cases.
Thanks.
$params = $this->request->getParams(); | ||
if (isset($params['sorting'])) { | ||
$sorting = $this->request->getParam('sorting'); |
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.
Please check a more simplified version that should also work.
$params = $this->request->getParams(); | |
if (isset($params['sorting'])) { | |
$sorting = $this->request->getParam('sorting'); | |
if ($sorting = $this->request->getParam('sorting', null)) { |
@@ -79,7 +87,8 @@ public function getData() | |||
} | |||
|
|||
/** | |||
* {@inheritdoc} | |||
* @inheritdoc | |||
* @return mixed |
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.
As far as I can see it doesn't return anything. Am I missing something? This change should fix the Static Tests and Semantic Version Checker.
* @return mixed | |
* @return void |
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.
@eduard13 thanks for suggestion
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.
@eduard13 check the screenshot it shows error http://prntscr.com/uz4ukj
but i don't change in it. this function has as it is.
@magento run all tests |
@magento run Static Tests, Semantic Version Checker |
@Chandresh22 please fix the rest of static fails, and adjust the Unit Test that is currently failing. Unfortunately you're not using a branch for these changes, so I cannot help you updating the code 😞 . |
okay @eduard13 i will check |
@magento run all tests |
@magento run Static Tests, Unit Tests, Semantic Version Checker. @Chandresh22 any reasons of why have you reverted the changes I suggested? Also have you had any problems with squashing the commits into one commit, in order to have a clean history? Let me know if you have any questions on my feedback. |
@magento run Static Tests, Unit Tests |
Hi @eduard13, thank you for the review. |
…min on the Product edit page #30477
Hi @Chandresh22, thank you for your contribution! |
Description (*)
Product Reviews can not be sorted by Admin on the Product edit page so that resolve this issue.
after changes code Records of "Product Reviews" grid are sorted.
Related Pull Requests
Fixed Issues (if relevant)
Create a few reviews for the product
Create an object of ReviewDataProvider with request param with some sorting order and current_product_id
Assert that sorting order will be applied for review items by calling
Review/Ui/DataProvider/Product/ReviewDataProvider::getData()
Fixes “Product Reviews” can not be sorted by Admin on the Product page #30270 Product Reviews can not be sorted by Admin on the Product Edit page
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)