Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cesartalves opened this issue May 11, 2021 · 4 comments
Open

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

cesartalves opened this issue May 11, 2021 · 4 comments
Labels

Comments

@cesartalves
Copy link
Contributor

cesartalves commented May 11, 2021

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

so that it fails if the OptionType is nil ?

@kennyadsl
Copy link
Member

👍 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
Copy link
Contributor Author

👍 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
Copy link
Member

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
Copy link

stale bot commented Jul 10, 2021

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.

@stale stale bot added the stale label Jul 10, 2021
@kennyadsl kennyadsl added pinned and removed stale labels Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants