Variant property rules to optionally match all conditions #2698
Conversation
adamdunkley
force-pushed
the
variant-property-rules-all
branch
from
0179184
to
8a51438
Compare
A new `apply_to_all` flag on `VariantPropertyRule` which, if true, only matches properties to variants if all of the conditions are met. Previously it would match any conditions which meant you could never apply a specific property to a variant that had more than one option type.
adamdunkley
force-pushed
the
variant-property-rules-all
branch
from
8a51438
to
983c614
Compare
There was a problem hiding this comment.
Thanks for contributing. I never used this feature so I'm not sure to fully get its purpose, anyway I cannot figure out a scenario where having the rule applied to any of the option values selected (the current solidus behavior) could be useful. I'm starting to think that it's an issue and it was thought and tested in real life scenarios only with variants with a single option type.
Anyway keeping backward compatibility is a good thing, but I think apply_to_all: true should be the default. We can maybe just make the checkbox default to true in the backend or even change the default value in the database and add some code to the migration so that old records take the false value. What do you think?
| @@ -0,0 +1,7 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class PropertyRulesApplyToAll < ActiveRecord::Migration[5.1] | |||
There was a problem hiding this comment.
this migration and file should be named add_apply_to_all_to_variant_property_rule to be consistent with the rest of migrations in the project
There was a problem hiding this comment.
Will update along with the defaulting changes
|
@kennyadsl Thanks I couldn't work out who would want it to be "any" either. I did also consider changing default values but didn't, I'll have a look at the best way of doing it and update the PR. |
|
@adamdunkley Hey Adam, this change looks good but needs a rebase so that we can include it in v2.11. |
I was expecting
applies_to_variant?onVariantPropertyRuleto only match when all rule conditions match.What's wrong with how it is?
When variant property rules match any of the rule's conditions it means you cannot link a property to a specific variant (if you have multiple option types)
Backward compatibility
Is kept as the flag defaults to false (so it'll continue to match to any of the rule's conditions)
Discussion
https://solidusio.slack.com/archives/C0JBKDF35/p1523712836000092
https://solidusio.slack.com/archives/C0JBKDF35/p1523833492000117