Skip to content
This repository

override issue #229

Closed
jerzyk opened this Issue July 08, 2012 · 7 comments

2 participants

jerzyk David Winterbottom
jerzyk
jerzyk commented July 08, 2012

I was trying to override oscar.apps.checkout.views.OrderPlacementMixin but as far as I can see, there is no way that this can be done on the actual code - please tell me that I'm wrong! :)

What I've came up is to push mixins to the seaprate module, and then call get_class from the main module.

Let me know what do you think, is so will anybody will be intersted in such a patch?

David Winterbottom
Owner

I think you're right there. It's not possible to override that mixin as it's defined in the same place as where it's used. Don't worry though - we can do what you suggest and put it in its own module so it can be loaded dynamically.

I'm happy to do it, but patches welcome too as I probably won't get to it for a day or so.

jerzyk
jerzyk commented July 09, 2012

ok, done - I've updated my pull request, there are 2 additonal commits - one related to this issue, second - foxes on translations - misspeling, omitions, missing calls etc. lt me know what do you think

David Winterbottom
Owner

Thanks for that. It would have been cleaner to put the checkout change in a separate pull request but it doesn't matter now. I'll merge both the commits into master so they can be part of the 0.3 release.

jerzyk
jerzyk commented July 16, 2012

I've had all of those changes in one branch, so there were no possibilities to create separate pull requests, but you can extract each change as a separate patch.

David Winterbottom
Owner

I've merged this in - but moved the two mixins to a single module:
https://github.com/tangentlabs/django-oscar/blob/master/oscar/apps/checkout/mixins.py

David Winterbottom codeinthehole closed this July 16, 2012
jerzyk
jerzyk commented July 16, 2012

Unfortunatelly this will not work. Initally I had a same idea, but If you could look closely, you will find, that wen you will be overrining CheckoutSessionMixin then this override will not work for the classes that use directly OrderPlacementMixin e.g. PaymentDetailsView.

This is problemmatic, I spend over an hour to find this simple 'bug'.

I was thinking to create a package mixins and then two file under it, but then, the override mechanism need to be changed ad this additional level will mix things up.

I had another idea - to have both classes in the one file but create something like:


OverriderCheckoutSessionMixin = get_class('checkout.mixins', 'CheckoutSessionMixin')

class OrderPlacementMixin(OverriderCheckoutSessionMixin):
....

but never tested this idea (it may not work as system will be doing circular imports).

That's why it was much simplier (but uglier) to have this one additional file and no additional changes and possible problems in other places.

David Winterbottom
Owner

Yes, good point. I've move the session mixing in the a new session.py module to allow them to load separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.