[Core] NthOrderRuleChecker should be more strict #1476
Conversation
|
Btw. won't this count carts as well? @umpirsky It should count only completed orders, right? |
|
@pjedrzejewski No, it can't get carts as only |
|
@pjedrzejewski @umpirsky I guess it's now more correct approach =) |
| return PaymentInterface::STATE_COMPLETED === $order->getPaymentState(); | ||
| }); | ||
|
|
||
| if ($orders->isEmpty()) { |
There was a problem hiding this comment.
Why? The case where orders is empty is covered by the count === 0, I don't see what this if is adding.
|
@pjedrzejewski What do you think about this? |
|
@pjedrzejewski @umpirsky Any feedback on this? |
| @@ -285,7 +285,18 @@ public function getPaymentState() | |||
| */ | |||
| public function setPaymentState($paymentState) | |||
| { | |||
| $this->paymentState = $paymentState; | |||
| if (BasePaymentInterface::STATE_COMPLETED === $paymentState) { | |||
There was a problem hiding this comment.
This is not code I would like to see in data model.
There was a problem hiding this comment.
Yeah, that's definitely not the right place.
There was a problem hiding this comment.
@umpirsky @pjedrzejewski Other place where it can be done is probably OrderPaymentCallback::updateOrderOnPayment() or OrderPaymentListener::updateOrderPayment() but I'm not sure which fits better.
|
@stloyd Looks good except comment I posted above. 👍 |
|
|
||
| if ($payments->count() === $this->payments->count()) { | ||
| $this->paymentState = BasePaymentInterface::STATE_COMPLETED; | ||
| } |
There was a problem hiding this comment.
@stloyd I don't think this is right. We might have failed payment linked to the order.
IMO we should let OrderPaymentCallback::updateOrderOnPayment() do all the work.
There was a problem hiding this comment.
Yes I have considered this, but in fact I noticed that we have job duplication in OrderPaymentCallback & OrderProcessing\StateResolver both handle similar case (check of amount in payment & in order, one marks order state, second call state machine).
There was a problem hiding this comment.
Agree, I think we can fix OrderPaymentCallback in another PR.
stloyd
force-pushed
the
bugfix/nth_order
branch
2 times, most recently
from
dbc891d
to
7b4256b
Compare
stloyd
force-pushed
the
bugfix/nth_order
branch
from
7b4256b
to
b293408
Compare
stloyd
force-pushed
the
bugfix/nth_order
branch
from
b293408
to
4bcdb98
Compare
stloyd
force-pushed
the
bugfix/nth_order
branch
from
4bcdb98
to
c1ba82a
Compare
|
@pjedrzejewski ping. |
[Core] NthOrderRuleChecker should be more strict
|
Thank you Joseph! 👍 |
[Core] NthOrderRuleChecker should be more strict
No description provided.