Bugfix: display full cart promotion rule form when adding a new rule #13464
Conversation
rimas-kudelis
force-pushed
the
bugfix/open-full-add-promotion-rule-form
branch
2 times, most recently
from
32e59d0
to
e5762bb
Compare
rimas-kudelis
force-pushed
the
bugfix/open-full-add-promotion-rule-form
branch
3 times, most recently
from
d19a14c
to
fd14449
Compare
Fixes Sylius#13463. Adds tests for all four forms: - cart promotion rules and actions - catalog promotion scopes and actions
rimas-kudelis
force-pushed
the
bugfix/open-full-add-promotion-rule-form
branch
from
fd14449
to
865a680
Compare
|
I really want to thank @vvasiloi for guiding me over the process of writing these behat scenarios, so – thank you! |
|
Thank you, Rimas! 🥇 |
| @@ -131,3 +131,11 @@ Feature: Creating a catalog promotion | |||
| Then there should be 1 new catalog promotion on the list | |||
| And it should have "winter_sale" code and "Winter sale" name | |||
| And it should have priority equal to 10 | |||
|
|
|||
| @ui @javascript | |||
There was a problem hiding this comment.
I don't thinks so. API scenarios should have the @api tag and you either run Behat with --tags="@api" to include API scenarios or with --tags="~@api" to exclude them.
There was a problem hiding this comment.
@no-api is an already existing tag though. Perhaps it would make sense to remove it everywhere, if ~@api is to be used instead.
There was a problem hiding this comment.
Then I'm curious why it exists...
There was a problem hiding this comment.
By the looks of it, Lukasz is the one who introduced the tag and it was added to about 80 scenarios, but I don't see it actually being used. Perhaps it's obsolete. Let's see what Lukasz has to say in his defense.
There was a problem hiding this comment.
The @no-api tag was created to mark scenarios that we don't want to cover in API. The main reason was to be aware of how many features and scenarios still need to be covered
There was a problem hiding this comment.
Thanks for the clarification. It make a bit more sense now.
At this point, isn't it already easier to explicitly mark the scenarios that we want to cover in the API and assume that the rest are "@no-api"?
I'm just concerned that this is going to be confusing for users and contributors and probably even become redundant once all current features are covered in the new API.
|
@lchrusciel so should I add the tag in a new PR? 😀 |
Fixes #13463
Additionally to the bugfix, I made the jquery selectors more explicit by using direct ancestors. In fact, that's how I discovered this bug, because we were patching app.js to make these selectors more explicit, which allowed us embed additional collection forms inside action/rule collections forms.