Promotions #754
Promotions #754
Conversation
|
@umpirsky you can transform the #751 issue into a PR using http://stackoverflow.com/a/18815173/244058 |
| $order->setUser($this->getUser()); | ||
| $orderManager = $this->get('sylius.manager.order'); | ||
| $orderManager->persist($order); | ||
| $orderManager->flush(); |
There was a problem hiding this comment.
Why do we need this here? The user should be assigned after logging in in security step or right away if he is already signed in. I'd be in favor of doing this through events. (IIRC in one of PRs I did that...)
There was a problem hiding this comment.
I wanted to set user asap in order to use it in promotion checkers so I moved this from finalize step here. Doing it through events is a good idea.
There was a problem hiding this comment.
@pjedrzejewski You are right, we should set user on order after login. Can you help me with choosing right event for this? I tried to find login event, but looks like nothing is dispatched in profiler. Also, what is the right way of getting order/cart in listener, cart provider?
|
Basic Behat scenarios for these would be nice, we have promotion steps already so it should be relatively easy to write. Thanks for your work Sasha! 👍 |
|
Thanks, but it is still wip. Thanks for early feedback. |
| use Symfony\Component\Form\AbstractType; | ||
| use Symfony\Component\Form\FormBuilderInterface; | ||
| use Symfony\Component\Validator\Constraints\NotBlank; | ||
| use Symfony\Component\Validator\Constraints\Type; |
There was a problem hiding this comment.
Both validator use are never called.
|
@pjedrzejewski Country transformers and Behat scenarios left. Anything else? |
|
@pjedrzejewski I think this PR is pretty finished, except I need to find a way to set user on order/cart after user authentication. Any help regarding this is appreciated. |
|
@pjedrzejewski When can I expect this to be merged? Its pretty urgent because we need it for internal project asap. |
|
The travis is broken completely because of some missing variable. I'll review it and hopefully merge today. Awesome work! |
| @@ -57,6 +61,8 @@ public function forwardAction(ProcessContextInterface $context) | |||
|
|
|||
| $this->dispatchCheckoutEvent(SyliusCheckoutEvents::ADDRESSING_COMPLETE, $order); | |||
|
|
|||
| $this->getManager()->flush($order); | |||
There was a problem hiding this comment.
Maybe also use $this->get('sylius.manager.order'); to be consistent with code before?
There was a problem hiding this comment.
I don't get why you added another flush? The flush is supposed to be between INITIALIZE and COMPLETE events, not after each one if I'm not wrong.
There was a problem hiding this comment.
@winzou Ah, you are right, I can listen to SyliusCheckoutEvents::ADDRESSING_PRE_COMPLETE.
| } | ||
|
|
||
| foreach ($subject->getInventoryUnits() as $unit) { | ||
| foreach ($unit->getStockable()->getProduct()->getTaxons() as $taxon) { |
There was a problem hiding this comment.
TBH. this will introduce quite a lot of not needed queries... maybe use some custom repository call here?
|
@pjedrzejewski Argh, I cant fix specs. Tried with https://gist.github.com/umpirsky/ab6b19bb86617c558c18 but no luck https://gist.github.com/umpirsky/73350c45308d6e4cd4d3. Looks like $payment
->execute(Argument::type('Sylius\Bundle\PayumBundle\Payum\Request\StatusRequest'))
->will(function ($args) use ($order, $paymentModel) {
$args[0]->markSuccess();
$args[0]->setModel($order);
}
);closure is not called after I added $cartProvider->getCart()->shouldBeCalled()->willReturn($order);Any ideas? |
|
@pjedrzejewski @winzou @stloyd Anyone to give some help on fixing this specs? |
|
Actually if you flush from the |
|
I'll checkout your branch and see what I can do. |
|
@pjedrzejewski Thanks Paweł! |
|
@umpirsky So far I figured out that the execute() method is not called properly, thus the payment state is not changed (events are not dispatched). I'll give it another try later today to see if I can fix it and send you a PR to your repo. |
|
@pjedrzejewski Yes, I noticed same, but cant figure out why. I just noticed it happens after I added: $cartProvider->getCart()->shouldBeCalled()->willReturn($order);Thanks. |
|
@pjedrzejewski Don't ask me how I fixed the build :) |
|
@umpirsky Wish you good luck this time LOL |
|
@pjedrzejewski @kayue GREEEN! |
|
@umpirsky requires a rebase. (sorry) I can do this for ya if you'll hate me. :D |
|
@pjedrzejewski Rebased :) |
|
Niiice... thanks for your work Sasha! These are some cool new features! I'll include them in the coming blog post. |
#751