From 15a485153846617b9ca0fa88921847b723da6da3 Mon Sep 17 00:00:00 2001 From: Adam Dunkley Date: Sun, 15 Apr 2018 20:59:45 +0100 Subject: [PATCH] Variant property rules to optionally match all conditions 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. --- .../spree/admin/product_properties/index.html.erb | 8 ++++++++ .../spree/admin/products_controller_spec.rb | 6 ++++++ core/app/models/spree/variant_property_rule.rb | 6 +++++- core/config/locales/en.yml | 2 ++ ...3007_add_apply_to_all_to_variant_property_rule.rb | 12 ++++++++++++ core/spec/models/spree/variant_property_rule_spec.rb | 9 ++++++++- core/spec/models/spree/variant_spec.rb | 4 ++-- 7 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 core/db/migrate/20180416083007_add_apply_to_all_to_variant_property_rule.rb diff --git a/backend/app/views/spree/admin/product_properties/index.html.erb b/backend/app/views/spree/admin/product_properties/index.html.erb index 5c80e373840..fdb24c8801a 100644 --- a/backend/app/views/spree/admin/product_properties/index.html.erb +++ b/backend/app/views/spree/admin/product_properties/index.html.erb @@ -70,6 +70,14 @@ <%= f.fields_for :variant_property_rules, @variant_property_rule do |rule_form| %> <%= rule_form.hidden_field 'id', value: @variant_property_rule.id %> <%= rule_form.hidden_field 'option_value_ids', value: @option_value_ids.join(',') %> +
+
+ +
+
<% if @option_value_ids.present? %>
diff --git a/backend/spec/controllers/spree/admin/products_controller_spec.rb b/backend/spec/controllers/spree/admin/products_controller_spec.rb index a6aac25be6b..66849315d0a 100644 --- a/backend/spec/controllers/spree/admin/products_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/products_controller_spec.rb @@ -134,6 +134,7 @@ id: product.id, variant_property_rules_attributes: { "0" => { + apply_to_all: true, option_value_ids: option_value.id, values_attributes: { "0" => { @@ -157,6 +158,11 @@ expect { subject }.to change { product.variant_property_rules.count }.by(1) end + it "creates a variant property rule that applies to all" do + subject + expect(product.variant_property_rules.first.apply_to_all).to be_truthy + end + it "creates a variant property rule condition" do expect { subject }.to change { product.variant_property_rule_conditions.count }.by(1) end diff --git a/core/app/models/spree/variant_property_rule.rb b/core/app/models/spree/variant_property_rule.rb index ff6bb376a88..75e0208c6ff 100644 --- a/core/app/models/spree/variant_property_rule.rb +++ b/core/app/models/spree/variant_property_rule.rb @@ -38,7 +38,11 @@ def matches_option_value_ids?(option_value_ids) # @param variant [Spree::Variant] variant to check # @return [Boolean] def applies_to_variant?(variant) - (option_value_ids & variant.option_value_ids).present? + if apply_to_all + matches_option_value_ids?(variant.option_value_ids) + else + (option_value_ids & variant.option_value_ids).present? + end end end end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 038a12a79e4..852a1bc78c7 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -809,6 +809,8 @@ en: add_option_value: Add Option Value add_product: Add Product add_product_properties: Add Product Properties + add_variant_properties: Add Variant Properties + applies_to_all_variant_properties: Applies to all Variant Properties above add_rule_of_type: Add rule of type add_state: Add State add_stock: Add Stock diff --git a/core/db/migrate/20180416083007_add_apply_to_all_to_variant_property_rule.rb b/core/db/migrate/20180416083007_add_apply_to_all_to_variant_property_rule.rb new file mode 100644 index 00000000000..55e8b84d153 --- /dev/null +++ b/core/db/migrate/20180416083007_add_apply_to_all_to_variant_property_rule.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddApplyToAllToVariantPropertyRule < ActiveRecord::Migration[5.1] + def change + add_column :spree_variant_property_rules, :apply_to_all, :boolean, default: false + change_column :spree_variant_property_rules, :apply_to_all, :boolean, default: true + end + + def down + remove_column :spree_variant_property_rules, :apply_to_all + end +end diff --git a/core/spec/models/spree/variant_property_rule_spec.rb b/core/spec/models/spree/variant_property_rule_spec.rb index 921b56c07df..3f6df7af779 100644 --- a/core/spec/models/spree/variant_property_rule_spec.rb +++ b/core/spec/models/spree/variant_property_rule_spec.rb @@ -64,12 +64,19 @@ subject { rule.applies_to_variant?(variant) } - context "variant matches some of the rule's conditions" do + context "variant matches some of the rule's conditions when apply_to_all is false" do + let(:rule) { create(:variant_property_rule, option_value: rule_option_value, apply_to_all: false) } let(:option_values) { [variant_option_value_1, variant_option_value_2] } it { is_expected.to eq true } end + context "variant cannot match only some of the rule's conditions" do + let(:option_values) { [variant_option_value_1, variant_option_value_2] } + + it { is_expected.to eq false } + end + context "variant matches none of the rule's conditions" do let(:option_values) { [create(:option_value)] } diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index dc02f086515..93c84812feb 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -807,8 +807,8 @@ subject { variant.variant_properties } context "variant has properties" do - let!(:rule_1) { create(:variant_property_rule, product: variant.product, option_value: option_value_1) } - let!(:rule_2) { create(:variant_property_rule, product: variant.product, option_value: option_value_2) } + let!(:rule_1) { create(:variant_property_rule, product: variant.product, option_value: option_value_1, apply_to_all: false) } + let!(:rule_2) { create(:variant_property_rule, product: variant.product, option_value: option_value_2, apply_to_all: false) } it "returns the variant property rule's values" do expect(subject).to match_array rule_1.values + rule_2.values