Refactor OptionFields / SHOP_OPTION_TYPE_CHOICES

[Kniyl]

I’m wondering why the option%s fields created in the ProductVariationMetaclass are OptionFields instead of ForeignKeys. What is the historical reason behind this?

It has always bothered me that changes on existing ProductOptions are not reflected in existing ProductVariations. I understand that ProductOptions are not really meant to be changed after creation but correcting typos or making one more precise when creating a similar one (e.g. green -> light green when creating dark green) are use cases where one would like the change to automatically reflect on existing ProductVariations.

A second drawback is that it triggers a modeltranslation issue when a ProductOption is not fully translated and is chosen for a variation. See deschler/django-modeltranslation#286.

Changing the OptionFields into ForeignKeys in the ProductVariationMetaclass would resolve both. But might be a mess to migrate (I’m not really into this kind of things, so I don't really know).

What are your views on this, is it worth trying to make the change?

[stephenmcd]

I vaguely recall the main reason being around the number of SQL queries performed when generating the dropdown lists of product options to choose from on the product page, but I couldn't say for certain - the design is 6 years old now. You'll also see in ProductVariation.option_fields its used as an identity check, so there's another (relatively minor) reason there.

I think what needs fixing here is somewhat broader - basically we shouldn't be adding a variable number of database columns based on SHOP_OPTION_TYPE_CHOICES. The fact a database migration is required each time this setting changes (in size at least) is a real problem in hindsight. It was discussed a while back a bit here but nothing's been done yet: https://groups.google.com/forum/#!searchin/mezzanine-users/SHOP_OPTION_TYPE_CHOICES|sort:date/mezzanine-users/ITOmtvEX8hQ/rdRH2FnhvGgJ

So I don't think trying to fix this for translations' sake is worth the effort, given the larger problem which should be addressed - perhaps if that was done, the translation problems would be resolved too, I'm really not sure.

[sjkingo]

I am presently working on support for dynamic product variations for a client, that eliminates SHOP_OPTION_TYPE_CHOICES, so when I get it working and packed up nicely I will send through a PR.

[stephenmcd]

Awesome Sam, thanks!

[sjkingo]

There are a few things that probably should be discussed in terms of maintaining backwards compatibility as migrating to the new dynamic choices would require dropping the option% columns and migrating them to the new table. That would make SHOP_OPTION_TYPE_CHOICES deprecated and safe to remove from settings (perhaps if it exists we don't use the new dynamic model, and mark it to remove in a future version).

Anyway - I have some thoughts as to how it should work - I will open an issue to track it once the time is right

[stephenmcd]

I just renamed this issue which we can use to track the broader task of refactoring here.

[sjkingo]

Well look at that, I thought this was on the mailing list, I never noticed it was an existing issue (have been email replying until now).

Thanks Stephen, will keep you all updated.

[twoblokeswithapostie]

I'm really interested in this as well. Sam, if you have made some progress on it I'd be willing to chip in.

[sjkingo]

I am still working on it - slowly. Will post more soon and we can see what else needs to be done. Thanks