-
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 for wildcard section reload being broken by section-config refacor #29081
Fix for wildcard section reload being broken by section-config refacor #29081
Conversation
Hi @complexthings. 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 |
@complexthings interesting... I see case with magento2/dev/tests/js/jasmine/tests/app/code/Magento/Customer/frontend/js/section-config.test.js Lines 99 to 130 in 5be754b
Related to #25764 @krzksz could you review this PR? |
@ihor-sviziev section-config does what it's supposed to, the refactor works, it just happened to break customer-data.js |
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 @complexthings,
Could you cover your change with js unit test? Example you can find here:
https://github.com/magento/magento2/blob/5be754b7d3de102cb9ec6ef2f43dbdbcb797540a/dev/tests/js/jasmine/tests/app/code/Magento/Customer/frontend/js/section-config.test.js
app/code/Magento/Customer/view/frontend/web/js/customer-data.js
Outdated
Show resolved
Hide resolved
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.
@ihor-sviziev suggestions look good to me. It would be great to have this module covered with unit tests.
5da0a04
to
4f10b5d
Compare
@ihor-sviziev @krzksz I am working on jasmine tests for customer-data.js but it will take some time. |
Ok, np
…On Wed, 15 Jul 2020 at 16:47, Greg Harvell ***@***.***> wrote:
@ihor-sviziev <https://github.com/ihor-sviziev> @krzksz
<https://github.com/krzksz> I am working on jasmine tests for
customer-data.js but it will take some time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29081 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOJOUKQAEACTQNPCHE7SC3R3WXNVANCNFSM4OX36JNA>
.
|
@ihor-sviziev @krzksz I added js unit test for customer-data.js |
71c6d14
to
1505a36
Compare
@magento run all tests |
@magento run Static Tests, Unit Tests |
@magento run Static Tests, Unit Tests |
@magento run all tests |
Hi @ihor-sviziev, thank you for the review. |
Hi @complexthings, thank you for your contribution! |
After apply the changes, one of my customer reported me 2 different related issues:
I created a new ticket: Hopefully @complexthings can fix this one as well. |
Description (*)
Since the refactor of section-config.js in Magento_Customer (976302d) reloading of all customer-data sections via wildcard
*
has caused requests tocustomer/section/load
to respond400
.This causes issues when you call reload directly from
customer-data
or you request wildcard section reloading via an action in sections.xmlYou can validate this yourself by executing the following code from your devtools console:
I've added an additional condition to the
getFromServer
method's parameters variable that checks to see if the wildcard section request is present, if it is, it makes sure parameters gets set to[]
instead of['*']
which fails.Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
This should return all customer-data sections.
2. Hitting any route with a request where the action expects to reload '*'.
Questions or comments
Contribution checklist (*)