MUI controller lacks JSON response, instead returns status 200 with empty body #19859
Conversation
|
Hi @woutersamaey. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
…ont-bvba/magento2 into mui-controller-lacks-json-return # Conflicts: # app/code/Magento/Ui/Controller/Adminhtml/Index/Render.php
|
Forgot the return line at the end. Added it to the PR. Sorry. |
|
@magento-engcom-team give me 2.3-develop instance |
|
Hi @sidolov. Thank you for your request. I'm working on Magento 2.3-develop instance for you |
|
Hi @sidolov, here is your Magento instance. |
|
@magento-engcom-team give me test instance |
|
Hi @sivaschenko. Thank you for your request. I'm working on Magento instance for you |
|
Hi @sivaschenko, here is your new Magento instance. |
|
Hi @woutersamaey thanks for your contribution! Could you please fix the code style for static tests to pass. With your fix an admin with limited permissions will see the screen like: It would be nice to have an appropriate error (Not enough permissions), however, that's probably out of this pull request scope. |
|
@sivaschenko You're right about this error message and indeed, it is out of scope. This error can come up in a number of situations and it's always vague. A more thorough refactoring is needed. Also, the admin notifications should not try to load in the first place, as the user doesn't have the required rights, so dealing with the root cause is best. These could be 2 new separate issues. About the code style, this is just another one of those "i dont like else-statements" messages. We've ignored those in the past due to an overzealous Codacy. |
There was a problem hiding this comment.
Hi @woutersamaey thanks for your reply. We will not be able to merge the pull request to mainline without code style fixes. Please see my code suggestions. All you need to to do is to add these suggestions to batch and then commit them using github UI.
Co-Authored-By: woutersamaey <wouter.samaey@storefront.be>
Co-Authored-By: woutersamaey <wouter.samaey@storefront.be>
Co-Authored-By: woutersamaey <wouter.samaey@storefront.be>
|
@sivaschenko OK, now I see what you mean. Thanks for preparing this for me. I have commited your suggestions. Is everything OK now? Honestly I'm not 100% familiar with Github so please excuse me if I didn't understand correctly. |
|
Hi @sivaschenko, thank you for the review. |
p-bystritsky
force-pushed
the
mui-controller-lacks-json-return
branch
from
893ed7d to
25e04de
Compare
p-bystritsky
force-pushed
the
mui-controller-lacks-json-return
branch
2 times, most recently
from
7718593 to
481bd15
Compare
p-bystritsky
force-pushed
the
mui-controller-lacks-json-return
branch
from
481bd15 to
8b97f75
Compare
p-bystritsky
force-pushed
the
mui-controller-lacks-json-return
branch
from
8b97f75 to
94cb3f6
Compare
p-bystritsky
force-pushed
the
mui-controller-lacks-json-return
branch
from
94cb3f6 to
40d44c2
Compare
ghost
added
the
|
Hi @woutersamaey, thank you for your contribution! |
…us 200 with empty body #19859
Description (*)
Example URL http://hostname/admin/mui/index/render/key/xxx/?namespace=notification_area&sorting%5Bfield%5D=created_at&sorting%5Bdirection%5D=asc&isAjax=true
Can return status 200 with an empty body, when it should return JSON. This later causes a JavaScript error because data.items does not exist.
This happens when the admin user does not have enough ACL rights to this controller.
Fixed Issues (if relevant)
Manual testing scenarios (*)
data-storage.js:270 Uncaught TypeError: Cannot read property 'items' of null
at UiClass.onRequestComplete (data-storage.js:270)
at fire (jquery.js:3232)
at Object.fireWith [as resolveWith] (jquery.js:3362)
at done (jquery.js:9840)
at XMLHttpRequest.callback (jquery.js:10311)
Contribution checklist (*)