Skip to content

Commit

Permalink
Variant property rules to optionally match all conditions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
adamdunkley authored and filippoliverani committed Jun 12, 2020
1 parent 239ce9f commit bf5cd59
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(',') %>
<div class="col-12">
<div class="field checkbox">
<label>
<%= rule_form.check_box 'apply_to_all', value: @variant_property_rule.apply_to_all %>
<%= t('spree.applies_to_all_variant_properties') %>
</label>
</div>
</div>
<% if @option_value_ids.present? %>
<fieldset class='no-border-top'>
<table class="index sortable" data-hook data-sortable-link="<%= update_positions_admin_product_variant_property_rule_values_url %>">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" => {
Expand All @@ -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
Expand Down
6 changes: 5 additions & 1 deletion core/app/models/spree/variant_property_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 2 additions & 0 deletions core/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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, null: 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
9 changes: 8 additions & 1 deletion core/spec/models/spree/variant_property_rule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 when apply_to_all is true" 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)] }

Expand Down
4 changes: 2 additions & 2 deletions core/spec/models/spree/variant_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit bf5cd59

Please sign in to comment.