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

Variant property rules to optionally match all conditions #2698

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,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 @@ -62,6 +62,7 @@
id: product.id,
variant_property_rules_attributes: {
"0" => {
apply_to_all: true,
option_value_ids: option_value.id,
values_attributes: {
"0" => {
Expand All @@ -85,6 +86,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
1 change: 1 addition & 0 deletions core/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,7 @@ en:
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
7 changes: 7 additions & 0 deletions core/db/migrate/20180416083007_property_rules_apply_to_all.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class PropertyRulesApplyToAll < ActiveRecord::Migration[5.1]
jarednorman marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update along with the defaulting changes

def change
add_column :spree_variant_property_rules, :apply_to_all, :boolean, default: false
end
end
7 changes: 7 additions & 0 deletions core/spec/models/spree/variant_property_rule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@
it { is_expected.to eq true }
end

context "variant cannot match only some of the rule's conditions when applies to all" do
let(:rule) { create(:variant_property_rule, option_value: rule_option_value, apply_to_all: true) }
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