search

solidusio-contrib / solidus_importer

Solidus importer extension to migrate data from other eCommerce systems
BSD 3-Clause "New" or "Revised" License
15 stars 30 forks source link

Do not allow OptionValues to be created without an OptionType #68

Open cesartalves opened 3 years ago

cesartalves commented 3 years ago

The OptionValue processor can create an OptionValue without an OptionType. This can lead to problems when querying Variants which are associated with it, resulting in 500 on both Solidus API and Admin (and I'm afraid, more concerning, the Frontend).

This problem has happened quite a few times on a project I'm working on; unfortunately I haven't been able to reproduce on the sandbox yet. There's an open pull request on Solidus addressing this problem

-- Maybe we could tweak https://github.com/solidusio-contrib/solidus_importer/blob/c89d279d81bd6c06a5b294163dc1a3824f7fd1aa/lib/solidus_importer/processors/option_values.rb#L23 so that it fails if the OptionType is nil ?

kennyadsl commented 3 years ago

👍 it should fail because it doesn't make sense to have an option value without a "parent" option type. Still not sure why we don't have that validation in Solidus...

cesartalves commented 3 years ago

👍 it should fail because it doesn't make sense to have an option value without a "parent" option type. Still not sure why we don't have that validation in Solidus...

We could perhaps add some sort of validation (even on a database level) to OptionValue option_type_id to make sure these are not allowed?

kennyadsl commented 3 years ago

We could, but it's hard to make this kind of things backward-compatible: if you have incorrect data in your database, you upgrade Solidus and run an update on an old, incorrect record (for example updating them in rake task for unrelated reasons), it will fail unexpectedly.

One way of solving this is creating a preference (like Spree::Config.validate_option_type_presence_on_option_values) that is true on new stores and false on existing ones. We can ask people with the false value to take actions on their inconsistent database and switch the preference to true because we are going to remove the support for it soon.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.