[Core][Promotion] Continuation of rules with amount per channel #6906
Conversation
|
Rebase needed 😉 |
|
@michalmarcinkowski done |
lchrusciel
force-pushed
the
rules-with-amount-per-channel-can-into-master
branch
from
575a4dd
to
6d9f77e
Compare
|
Damn it, I was already in the middle of this :/ |
| @@ -24,7 +24,7 @@ Feature: Adding a new promotion with rule | |||
| Given I want to create a new promotion | |||
| When I specify its code as "100_MUGS_PROMOTION" | |||
| And I name it "100 Mugs promotion" | |||
| And I add the "Total price of items from taxon" rule configured with 100 "Mugs" | |||
| And I add the "Total price of items from taxon" rule configured with "Mugs" taxon and €100 amount for "United States" channel | |||
There was a problem hiding this comment.
Should be $100 instead of €100
| /** | ||
| * @author Mateusz Zalewski <mateusz.zalewski@lakion.com> | ||
| */ | ||
| final class BuildChannelAwarePromotionRuleFormSubscriber extends BuildPromotionRuleFormSubscriber |
| return; | ||
| } | ||
|
|
||
| if (!$model instanceof ChannelAwareRuleCheckerInterface) { |
| $resolver | ||
| ->setDefined(['currency']) | ||
| ->setAllowedTypes('currency', 'string') | ||
| ->setDefault('currency', 'USD') |
There was a problem hiding this comment.
I think we shouldn't set USD as default currency.
There was a problem hiding this comment.
Do we have any better idea, what should be added by default? Or should we just skipped this?
There was a problem hiding this comment.
Skip the default value and make it required.
| $this->isConfigurationValid($configuration); | ||
| $channelCode = $subject->getChannel()->getCode(); | ||
| if (!isset($configuration[$channelCode])) { | ||
| return; |
There was a problem hiding this comment.
Change method signature and return false here.
There was a problem hiding this comment.
Should I adjust all early returns in this class? If so, should I change a logic of the rest of commands to return false, if nothing happened?
There was a problem hiding this comment.
We will try to work on that tomorrow, but definitely a separate PR.
michalmarcinkowski
added
BC Break
lchrusciel
changed the title
lchrusciel
force-pushed
the
rules-with-amount-per-channel-can-into-master
branch
2 times, most recently
from
d0b5e2c
to
e965fd4
Compare
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getName() |
There was a problem hiding this comment.
It should be getBlockPrefix - it also causes some errors, as there is no proper widget used to rendering action/rule configuration.
There was a problem hiding this comment.
Highly possible it's the only thing that crashes the build right now 😄 🐃
lchrusciel
force-pushed
the
rules-with-amount-per-channel-can-into-master
branch
from
621d767
to
4dd4914
Compare
…xedDiscountConfigurationTypeExtension
lchrusciel
force-pushed
the
rules-with-amount-per-channel-can-into-master
branch
from
4dd4914
to
7712774
Compare
|
Great! 👍 👍 🎉 |
…channel-can-into-master [Core][Promotion] Continuation of rules with amount per channel
…channel-can-into-master [Core][Promotion] Continuation of rules with amount per channel
Continuation of @Zales0123 work from #6895 with Symfony3 support