[Promotion] Fix js errors associated with actions/rules/scopes #13547
Conversation
GSadee
commented
Jan 26, 2022
GSadee
commented
| Q | A |
|---|---|
| Branch? | 1.11 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | fixes #13447 |
| License | MIT |
GSadee
added
Admin
| $('#sylius_catalog_promotion_scopes > a[data-form-collection="add"]').on('click', () => { | ||
| setTimeout(() => { | ||
| $(`select[name^="sylius_catalog_promotion[scopes]"][name$="[type]"]`).last().change(); | ||
| $('select[name^="sylius_promotion[rules]"][name$="[type]"]').last().change(); |
There was a problem hiding this comment.
This change from three different cases to one certainly doesn't look right.
Also, please use the direct ancestor selector (>), because this will allow adding collections in child forms without having to hack JS.
There was a problem hiding this comment.
Here, I've only removed the lines that do nothing in my opinion, they are needed for cart promotions, not catalog ones. But of course, I might have missed sth. However, the tests are green and I've checked both cart and catalog promotions manually also
There was a problem hiding this comment.
Don't catalog promotions use same JS?
And yeah, disregard that second part of my previous comment. It's irrelevant.
There was a problem hiding this comment.
Those lines allow to use nested collections inside the promotion rules and actions.
Unfortunately there's no test for that, because there's no such use case in Sylius by default.
There was a problem hiding this comment.
@vvasiloi no, it's just the > in the selector that allows nested collections. It doesn't mean that these lines were actually used.
And remember, we added behat scenarios for both catalog promotions and cart promotions. If all of them still succeed even without these lines, that means that either the tests are bad, or the lines aren't really used after all.
There was a problem hiding this comment.
Yes, I was referring to the > in the selector.
We tested that the form is displayed when a new action or rule is added and if it works with a single selector then it's even better.
There was a problem hiding this comment.
If I understood correctly, we are fine to have it merge. Thanks for your activity @rimas-kudelis and @vvasiloi ?
|
Thank you, Grzegorz! 🥇 |
