Ignore payment step during checkout if the order total is equal to zero #307
Conversation
| @@ -282,6 +299,15 @@ def checkout_steps(request, form_class=OrderForm, extra_context=None): | |||
| # and send the order receipt email. | |||
| order = form.save(commit=False) | |||
| order.setup(request) | |||
|
|
|||
| if (not settings.SHOP_PAYMENT_STEP_ENABLED or | |||
| order.total == 0): | |||
There was a problem hiding this comment.
I'm confused. Doesn't the step+=1 part skip the payment step entirely? When would order.total == 0 at this point?
There was a problem hiding this comment.
step += 1 for some reason doesn't completely skip the payment. You can try with the master branch, activate a payment provider (like stripe) and SHOP_PAYMENT_STEP_ENABLED = False, you'll get an error from the payment processor...
By adding this line I just use the dummy payment processor when the order is zero.
There was a problem hiding this comment.
I haven't tried it but that makes sense. SHOP_PAYMENT_STEP_ENABLED seems to only effect the template context. But now that you've added step += 1, neither of these conditions should be possible, right?
There was a problem hiding this comment.
The first condition not settings.SHOP_PAYMENT_STEP_ENABLE is to handle the issue I mentioned, and the second order.total == 0 is to handle the original problem.
These conditions will be evaluated during the very last step of the checkout, so these are possible. step += 1 just skip the payment form page.
Sorry if it isn't clear, this problem isn't as easy to fix as I initially though it would be.
There was a problem hiding this comment.
Upon closer analysis I understand what you're doing here. This view was difficult to grok to begin with. I'd have to dig in with a debugger to understand it well enough to do any further review.
|
If this can help you understand better what I'm trying to do, here are the 4 things my changes are doing. If order total is 0 and payment step enabled:
|
|
Like Ryne I found the diff on this a bit confusing, but I get the general idea - just bump up the current step if the order total is zero. Unfortunately as I think you've discovered, I think it might be a bit difficult to implement this way, ending up with hacks and probably some unhandled edge cases (going back in the form steps comes to mind). I feel like the correct thing to do would be to refactor all the cartridge.checkout.CHECKOUT_FOO constants, so that they're returned by a function with takes a cart or request or something user/session specific, that can then return those vars for the given user and their cart at runtime (as opposed to import time as those vars are currently defined). |
Seems like the handlers are constants which could continue to be defined at import time, but the abstraction Stephen is suggesting means the Implementation details aside, I agree with Stephen that this view is in need of abstraction -- even without trying to implement this feature. @Remiz, if you want to make this happen my recommendation would be to start a new branch, refactor as Stephen suggested (or maybe you'll find a better/different abstraction), and then implement this feature. |
… free_order_skip_payment
… step. Use cart total instead.
…rtridge into free_order_skip_payment * 'free_order_skip_payment' of https://github.com/Remiz/cartridge: Remove email handling from checkout view. Now done within the post-checkout process. Only send email confirmation based on order type Don't rely on order.setup() otherwise it saves an empty order on each step. Use cart total instead. Replace dummy processor handler by dummy payment handler Ignore payment step during checkout if the order total is equal to zero
It seems to be working for me... I ran the tests and didn't get any issue either, but I would appreciate if anyone else gave their opinion.
Thanks.