Make option value to variant association unique #4146
Conversation
core/db/migrate/20210815004823_add_unique_index_to_option_values_variants.rb
Outdated
Show resolved
Hide resolved
core/db/migrate/20210815004823_add_unique_index_to_option_values_variants.rb
Outdated
Show resolved
Hide resolved
core/db/migrate/20210815004823_add_unique_index_to_option_values_variants.rb
Outdated
Show resolved
Hide resolved
|
I don't think any of the errors have anything to do with my change. 🤔 |
jarednorman
force-pushed
the
why-would-you-ever-want-the-same-option-value-on-a-variant-twice
branch
from
bb6f068
to
4af9993
Compare
|
Had to scroll back more. I used 6.2 as the migration version, but currently we have to write migrations for the lowest supported ActiveRecord version. |
jarednorman
force-pushed
the
why-would-you-ever-want-the-same-option-value-on-a-variant-twice
branch
2 times, most recently
from
6c77ecb
to
f38fa05
Compare
| def up | ||
| remove_index :spree_option_values_variants, [:variant_id, :option_value_id] | ||
| add_index :spree_option_values_variants, [:variant_id, :option_value_id], | ||
| name: "index_option_values_variants_on_variant_id_and_option_value_id", |
There was a problem hiding this comment.
out of curiosity, why the explicit index name?
There was a problem hiding this comment.
Rails will generate an index name that exceeds the 64 character limit. The name I've used just matches the existing index's name, so presumably whoever created the original index also ran into this.
kennyadsl
added
the
release:major
waiting-for-dev
added
type:enhancement
|
This is not to hold this back: We associate the same option value with multiple variants. We have a third field, an integer, that differentiates the variants from one another. We are a special case though as we sell a lot of stuff in differently sized bags. |
|
@mamhoff I don't think this will impact you then? You can still have multiple variants per option value, just not the the same option value on one variant multiple times. |
|
Ah true! I mistakenly thought we need to have different option value combos for each variant. 👍 |
|
Hey @jarednorman, can you please rebase this one? Thanks! 🙏 |
waiting-for-dev
force-pushed
the
why-would-you-ever-want-the-same-option-value-on-a-variant-twice
branch
from
f38fa05
to
a9aea0b
Compare
I did it myself and fixed the conflict |
|
This would be a good chance to mark the |
I can't think of a reason why someone would want to associate the same option value with a given variant multiple times. This makes the index on the variant/option_value_id columns of the spree_option_values_variants join table a unique index so we can't have a variant that has the same option value multiple times. There's a vaguely related issue over on solidus_importer where the option value processor attempts to do this if you try to reimport products. This will cause it to error out instead.
jarednorman
force-pushed
the
why-would-you-ever-want-the-same-option-value-on-a-variant-twice
branch
from
a9aea0b
to
672116e
Compare
We decided to skip this kind of changes for this major release. There are already a bunch of database changes and we don't want to be a nightmare to update. @BenMorganIO thanks a lot for the suggestion, I think we should design a process to make these changes safer. An idea is a script that runs at boot and checks the database (especially important in production) and gives some hints if you need to fix inconsistencies. BTW, we can discuss in Slack or in a separate issue/PR if you want. |
jarednorman
deleted the
why-would-you-ever-want-the-same-option-value-on-a-variant-twice
branch
Description
I can't think of a reason why someone would want to associate the same option value with a given variant multiple times. This makes the index on the
variant/option_value_idcolumns of thespree_option_values_variantsjoin table a unique index so we can't have a variant that has the same option value multiple times.Checklist: