diff --git a/CHANGELOG.md b/CHANGELOG.md index ad3fa43f154..8b99db8ecc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Solidus 3.2.0.alpha (master, unreleased) +- Promotions with a `match_policy` of `any` are deprecated. - Monkey patch Authentication Bypass by CSRF Weakness vulnerability on solidus_auth_devise for extra security [GHSA-5629-8855-gf4g](https://github.com/solidusio/solidus/security/advisories/GHSA-5629-8855-gf4g) - Fix ReDos vulnerability on Spree::EmailValidator::EMAIL_REGEXP [GHSA-qxmr-qxh6-2cc9](https://github.com/solidusio/solidus/security/advisories/GHSA-qxmr-qxh6-2cc9) diff --git a/backend/app/views/spree/admin/promotions/_rules.html.erb b/backend/app/views/spree/admin/promotions/_rules.html.erb index 4520aa3e09e..978b2d7c32d 100644 --- a/backend/app/views/spree/admin/promotions/_rules.html.erb +++ b/backend/app/views/spree/admin/promotions/_rules.html.erb @@ -19,13 +19,15 @@ <%= form_for @promotion, url: object_url, method: :put do |f| %>
-
- <% Spree::Promotion::MATCH_POLICIES.each do |policy| %> -
- -
- <% end %> -
+ <% if Spree::Config.allow_promotions_any_match_policy %> +
+ <% Spree::Promotion::MATCH_POLICIES.each do |policy| %> +
+ +
+ <% end %> +
+ <% end %>
<% if @promotion.rules.any? %> diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb new file mode 100644 index 00000000000..99ca90d34af --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb @@ -0,0 +1,12 @@ +
+ +
+
<%= label_tag nil, Spree::Product.model_name.human %>
+
<%= label_tag nil, plural_resource_name(Spree::OptionValue) %>
+
+ + <%= content_tag :div, nil, class: "js-promo-rule-option-values", + data: { :'original-option-values' => promotion_rule.preferred_eligible_values } %> + + +
diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb new file mode 100644 index 00000000000..972486873b6 --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb @@ -0,0 +1,18 @@ +
+
+
+ <%= label_tag "#{param_prefix}_product_ids_string", t('spree.product_rule.choose_products') %> + <%= hidden_field_tag "#{param_prefix}[product_ids_string]", promotion_rule.product_ids.join(","), class: "product_picker fullwidth" %> +
+
+
+
+ <%= label_tag("#{param_prefix}_preferred_match_policy", promotion_rule.class.human_attribute_name(:preferred_match_policy)) %> + <%= select_tag( + "#{param_prefix}[preferred_match_policy]", + options_for_select(I18n.t("spree.promotion_rules.line_item_product.match_policies").to_a.map(&:reverse), promotion_rule.preferred_match_policy), + class: "custom-select fullwidth" + ) %> +
+
+
diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb new file mode 100644 index 00000000000..c06d03c79d8 --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb @@ -0,0 +1,14 @@ +
+ <%= label_tag "#{param_prefix}_taxon_ids_string", t("spree.taxon_rule.choose_taxons") %> + <%= hidden_field_tag "#{param_prefix}[taxon_ids_string]", promotion_rule.taxon_ids.join(","), class: "taxon_picker fullwidth", id: "product_taxon_ids" %> +
+
+
+ <%= label_tag("#{param_prefix}_preferred_match_policy", promotion_rule.class.human_attribute_name(:preferred_match_policy)) %> + <%= select_tag( + "#{param_prefix}[preferred_match_policy]", + options_for_select(I18n.t("spree.promotion_rules.line_item_taxon.match_policies").to_a.map(&:reverse), promotion_rule.preferred_match_policy), + class: "custom-select fullwidth", + ) %> +
+
diff --git a/backend/spec/features/admin/promotion_adjustments_spec.rb b/backend/spec/features/admin/promotion_adjustments_spec.rb index 383a4a70f29..342143bcd77 100644 --- a/backend/spec/features/admin/promotion_adjustments_spec.rb +++ b/backend/spec/features/admin/promotion_adjustments_spec.rb @@ -118,7 +118,7 @@ click_button "Create" expect(page).to have_title("SAVE SAVE SAVE - Promotions") - select "Product(s)", from: "Discount Rules" + select "Line Item Product(s)", from: "Discount Rules" within("#rule_fields") { click_button "Add" } select2_search "RoR Mug", from: "Choose products" within('#rule_fields') { click_button "Update" } @@ -136,7 +136,7 @@ expect(promotion.codes.first).to be_nil first_rule = promotion.rules.first - expect(first_rule.class).to eq(Spree::Promotion::Rules::Product) + expect(first_rule.class).to eq(Spree::Promotion::Rules::LineItemProduct) expect(first_rule.products.map(&:name)).to include("RoR Mug") first_action = promotion.actions.first diff --git a/backend/spec/features/admin/promotions/product_rule_spec.rb b/backend/spec/features/admin/promotions/product_rule_spec.rb index 2b6d909c352..11b1ec081fa 100644 --- a/backend/spec/features/admin/promotions/product_rule_spec.rb +++ b/backend/spec/features/admin/promotions/product_rule_spec.rb @@ -18,7 +18,7 @@ def add_promotion_rule_of_type(type) background do visit spree.edit_admin_promotion_path(promotion) - add_promotion_rule_of_type("Product(s)") + add_promotion_rule_of_type("Order Product(s)") end it "can select by product sku" do diff --git a/core/app/models/spree/calculator/distributed_amount.rb b/core/app/models/spree/calculator/distributed_amount.rb index a472a737591..445a0771f49 100644 --- a/core/app/models/spree/calculator/distributed_amount.rb +++ b/core/app/models/spree/calculator/distributed_amount.rb @@ -15,18 +15,18 @@ class Calculator::DistributedAmount < Calculator def compute_line_item(line_item) return 0 unless line_item return 0 unless preferred_currency.casecmp(line_item.currency).zero? - return 0 unless calculable.promotion.line_item_actionable?(line_item.order, line_item) + return 0 unless calculable.promotion.line_item_eligible?(line_item) Spree::DistributedAmountsHandler.new( - actionable_line_items(line_item.order), + eligible_line_items(line_item.order), preferred_amount ).amount(line_item) end private - def actionable_line_items(order) + def eligible_line_items(order) order.line_items.select do |line_item| - calculable.promotion.line_item_actionable?(order, line_item) + calculable.promotion.line_item_eligible?(line_item) end end end diff --git a/core/app/models/spree/discounts/chooser.rb b/core/app/models/spree/discounts/chooser.rb new file mode 100644 index 00000000000..5aaa9af9b7f --- /dev/null +++ b/core/app/models/spree/discounts/chooser.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class Chooser + def initialize(_discountable) + # This signature is here to provide context in case + # this needs to be customized + end + + def call(discounts) + [discounts.min_by(&:amount)].compact + end + end + end +end diff --git a/core/app/models/spree/discounts/line_item_discounter.rb b/core/app/models/spree/discounts/line_item_discounter.rb new file mode 100644 index 00000000000..11808930f6a --- /dev/null +++ b/core/app/models/spree/discounts/line_item_discounter.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class LineItemDiscounter + attr_reader :promotions + + def initialize(promotions:) + @promotions = promotions + end + + def call(line_item) + discounts = promotions.select do |promotion| + promotion.eligible_rules(line_item) + end.flat_map do |promotion| + promotion.actions.select do |action| + action.can_discount? Spree::LineItem + end.map do |action| + action.discount(line_item) + end + end + + chosen_discounts = Spree::Config.discount_chooser_class.new(line_item).call(discounts) + line_item.discounts = chosen_discounts + end + end + end +end diff --git a/core/app/models/spree/discounts/order_discounter.rb b/core/app/models/spree/discounts/order_discounter.rb new file mode 100644 index 00000000000..fa22675a2df --- /dev/null +++ b/core/app/models/spree/discounts/order_discounter.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class OrderDiscounter + attr_reader :order + + def initialize(order) + @order = order + end + + def call + discount_line_items + discount_shipments + end + + private + + def discount_line_items + line_item_discounter = LineItemDiscounter.new(promotions: promotions) + order.line_items.each { |line_item| line_item_discounter.call(line_item) } + end + + def discount_shipments + shipment_discounter = ShipmentDiscounter.new(promotions: promotions) + order.shipments.each { |shipment| shipment_discounter.call(shipment) } + end + + def promotions + @_promotions ||= begin + preloader = ActiveRecord::Associations::Preloader.new + (connected_order_promotions | sale_promotions).select do |promotion| + promotion.activatable?(order) + end.map do |promotion| + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Product" }, :products) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Store" }, :stores) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Taxon" }, :taxons) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::User" }, :users) + preloader.preload(promotion.actions.select { |a| a.respond_to?(:calculator) }, :calculator) + promotion + end + end.select { |promotion| promotion.eligible_rules(order) } + end + + def connected_order_promotions + order.promotions.includes(promotion_includes).select(&:active?) + end + + def sale_promotions + Spree::Promotion.where(apply_automatically: true).active.includes(promotion_includes) + end + + def promotion_includes + [ + :promotion_rules, + :promotion_actions, + ] + end + end + end +end diff --git a/core/app/models/spree/discounts/shipment_discounter.rb b/core/app/models/spree/discounts/shipment_discounter.rb new file mode 100644 index 00000000000..0f6c704fe74 --- /dev/null +++ b/core/app/models/spree/discounts/shipment_discounter.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class ShipmentDiscounter + attr_reader :promotions + + def initialize(promotions:) + @promotions = promotions + end + + def call(shipment) + discounts = promotions.select do |promotion| + promotion.eligible_rules(shipment) + end.flat_map do |promotion| + promotion.actions.select do |action| + action.can_discount? Spree::Shipment + end.map do |action| + action.discount(shipment) + end + end + + chosen_discounts = Spree::Config.discount_chooser_class.new(shipment).call(discounts) + shipment.discounts = chosen_discounts + end + end + end +end diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index 719a40ed8ec..9ae84a07b31 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -16,6 +16,7 @@ class LineItem < Spree::Base has_one :product, through: :variant + has_many :discounts, class_name: "Spree::LineItemDiscount", inverse_of: :line_item, dependent: :destroy, autosave: true has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :destroy has_many :inventory_units, inverse_of: :line_item diff --git a/core/app/models/spree/line_item_discount.rb b/core/app/models/spree/line_item_discount.rb new file mode 100644 index 00000000000..452b0a409e6 --- /dev/null +++ b/core/app/models/spree/line_item_discount.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Spree + class LineItemDiscount < Spree::Base + belongs_to :line_item, inverse_of: :discounts + belongs_to :promotion_action, -> { with_discarded }, inverse_of: :line_item_discounts + end +end diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index bc5962b6e6d..27ec9ac6f86 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -38,11 +38,13 @@ def update_cart(params) if order.update(params) unless order.completed? order.line_items = order.line_items.select { |li| li.quantity > 0 } - # Update totals, then check if the order is eligible for any cart promotions. - # If we do not update first, then the item total will be wrong and ItemTotal - # promotion rules would not be triggered. - reload_totals - PromotionHandler::Cart.new(order).activate + if legacy_promotion_system? + # Update totals, then check if the order is eligible for any cart promotions. + # If we do not update first, then the item total will be wrong and ItemTotal + # promotion rules would not be triggered. + reload_totals + PromotionHandler::Cart.new(order).activate + end order.ensure_updated_shipments end reload_totals @@ -71,14 +73,18 @@ def approve(user: nil, name: nil) private def after_add_or_remove(line_item, options = {}) - reload_totals + reload_totals if legacy_promotion_system? shipment = options[:shipment] shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments - PromotionHandler::Cart.new(order, line_item).activate + PromotionHandler::Cart.new(order, line_item).activate if legacy_promotion_system? reload_totals line_item end + def legacy_promotion_system? + Spree::Config.promotion_system == :adjustments + end + def reload_totals @order.recalculate end diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 444cc665229..56012107a52 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -110,8 +110,12 @@ def recalculate_adjustments # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ - update_item_promotions - update_order_promotions + if Spree::Config.promotion_system == :adjustments + update_item_promotions + update_order_promotions + elsif Spree::Promotion.order_activatable?(order) + Spree::Config.discounter_class.new(order).call + end update_taxes update_cancellations update_item_totals @@ -246,7 +250,11 @@ def update_item_totals item.adjustment_total = item.adjustments. select(&:eligible?). reject(&:included?). - sum(&:amount) + sum(&:amount) + item.discounts.sum(&:amount) + item.promo_total = item.adjustments. + select(&:promotion?). + select(&:eligible?). + sum(&:amount) + item.discounts.sum(&:amount) if item.changed? item.update_columns( diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index e48589dadbe..149058de587 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -3,10 +3,13 @@ module Spree class Promotion < Spree::Base MATCH_POLICIES = %w(all any) + UNACTIVATABLE_ORDER_STATES = ["complete", "awaiting_return", "returned"] attr_reader :eligibility_errors + acts_as_list column: :priority + belongs_to :promotion_category, optional: true has_many :promotion_rules, autosave: true, dependent: :destroy, inverse_of: :promotion @@ -40,18 +43,18 @@ class Promotion < Spree::Base scope :advertised, -> { where(advertise: true) } scope :active, -> { has_actions.started_and_unexpired } scope :started_and_unexpired, -> do - table = arel_table - time = Time.current + table = arel_table + time = Time.current - where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))). - where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time))) - end + where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))). + where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time))) + end scope :has_actions, -> do - joins(:promotion_actions).distinct - end + joins(:promotion_actions).distinct + end scope :applied, -> { joins(:order_promotions).distinct } - self.whitelisted_ransackable_associations = ['codes'] + self.whitelisted_ransackable_associations = ["codes"] self.whitelisted_ransackable_attributes = %w[name path promotion_category_id] def self.ransackable_scopes(*) %i(active) @@ -67,17 +70,25 @@ def self.with_coupon_code(val) ).first end - # All orders that have been discounted using this promotion + # All orders that have been discounted using this promotionp def discounted_orders - Spree::Order. - joins(:all_adjustments). - where( + if Spree::Config.promotion_system == :adjustments + Spree::Order. + joins(:all_adjustments). + where( spree_adjustments: { source_type: "Spree::PromotionAction", source_id: actions.map(&:id), - eligible: true - } + eligible: true, + }, ).distinct + else + Spree::Order.where.not(spree_line_item_discounts: { id: nil }). + where(spree_line_item_discounts: { promotion_action_id: actions.map(&:id) }).or( + Spree::Order.where.not(spree_shipment_discounts: { id: nil }). + where(spree_shipment_discounts: { promotion_action_id: actions.map(&:id) }) + ).left_outer_joins(line_items: :discounts, shipments: :discounts).distinct + end end def as_json(options = {}) @@ -118,7 +129,7 @@ def activate(order:, line_item: nil, user: nil, path: nil, promotion_code: nil) line_item: line_item, user: user, path: path, - promotion_code: promotion_code + promotion_code: promotion_code, } # Track results from actions to see if any action has been taken. @@ -176,6 +187,13 @@ def eligible_rules(promotable, options = {}) end specific_rules else + Spree::Deprecation.warn( + <<~WARN + Your promotion "#{name}" with ID #{id} has a match_policy of 'any'. + This is deprecated, please split the promotion into separate promotions for each rule. + WARN + +) unless specific_rules.any?(&eligible) @eligibility_errors = specific_rules.map(&:eligibility_errors).detect(&:present?) return nil @@ -211,20 +229,21 @@ def usage_count(excluded_orders: []) end def line_item_actionable?(order, line_item, promotion_code: nil) - return false if blacklisted?(line_item) + line_item_eligible?(line_item, promotion_code: promotion_code) + end + deprecate line_item_actionable?: :line_item_eligible?, deprecator: Spree::Deprecation - if eligible?(order, promotion_code: promotion_code) - rules = eligible_rules(order) - if rules.blank? - true - else - rules.send(match_all? ? :all? : :any?) do |rule| - rule.actionable? line_item - end - end - else - false - end + def line_item_eligible?(line_item, promotion_code: nil) + !blacklisted?(line_item) && + !!eligible_rules(line_item) && + deprecated_line_item_actionable?(line_item, promotion_code: promotion_code) + end + + def activatable?(order) + promotion_code = order.order_promotions.detect { |op| op.promotion_id == id }&.promotion_code + active? && + !usage_limit_exceeded?(excluded_orders: [order]) && + !promotion_code&.usage_limit_exceeded?(excluded_orders: [order]) end def used_by?(user, excluded_orders = []) @@ -252,6 +271,31 @@ def remove_from(order) private + def deprecated_line_item_actionable?(line_item, promotion_code: {}) + if eligible?(line_item.order, promotion_code: promotion_code) + rules = eligible_rules(line_item.order) + if rules.blank? + true + else + rules.send(match_all? ? :all? : :any?) do |rule| + if rule.respond_to?(:actionable?) + Spree::Deprecation.warn( + <<~WARN + The API of promotion rules has changed. Rather than specifying "actionable?" on your rule, create a new rule + that is applicable to line items and move the logic in your `actionable?` method to that rule's `eligible?` method. + WARN + ) + rule.actionable? line_item + else + true + end + end + end + else + false + end + end + def blacklisted?(promotable) case promotable when Spree::LineItem diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb index 19a3bf5b10f..6e418f61ecb 100644 --- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb @@ -19,6 +19,19 @@ def preload_relations [:calculator] end + def can_discount?(klass) + klass == Spree::LineItem + end + + def discount(line_item) + discount = line_item.discounts.detect do |discount| + discount.promotion_action == self + end || line_item.discounts.build(promotion_action: self) + discount.label = I18n.t('spree.adjustment_labels.line_item', promotion: Spree::Promotion.model_name.human, promotion_name: promotion.name) + discount.amount = compute_amount(line_item) + discount + end + def perform(payload = {}) order = payload[:order] promotion = payload[:promotion] @@ -35,7 +48,7 @@ def perform(payload = {}) # item_total and ship_total def compute_amount(adjustable) order = adjustable.is_a?(Order) ? adjustable : adjustable.order - return 0 unless promotion.line_item_actionable?(order, adjustable) + return 0 unless promotion.line_item_eligible?(adjustable) promotion_amount = calculator.compute(adjustable) promotion_amount ||= BigDecimal(0) promotion_amount = promotion_amount.abs @@ -89,7 +102,7 @@ def ensure_action_has_calculator def line_items_to_adjust(promotion, order) order.line_items.select do |line_item| line_item.adjustments.none? { |adjustment| adjustment.source == self } && - promotion.line_item_actionable?(order, line_item) + promotion.line_item_eligible?(line_item) end end end diff --git a/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb b/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb index 063dbd33478..60ff65e8348 100644 --- a/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb @@ -61,7 +61,7 @@ def compute_amount(line_item) adjustment_amount = adjustment_amount.abs order = line_item.order - line_items = actionable_line_items(order) + line_items = eligible_line_items(order) actioned_line_items = order.line_item_adjustments.reload. select { |adjustment| adjustment.source == self && adjustment.amount < 0 }. @@ -83,9 +83,9 @@ def compute_amount(line_item) private - def actionable_line_items(order) + def eligible_line_items(order) order.line_items.select do |item| - promotion.line_item_actionable? order, item + promotion.line_item_eligible? item end end diff --git a/core/app/models/spree/promotion/actions/free_shipping.rb b/core/app/models/spree/promotion/actions/free_shipping.rb index 628f3b4cede..9281a565e3b 100644 --- a/core/app/models/spree/promotion/actions/free_shipping.rb +++ b/core/app/models/spree/promotion/actions/free_shipping.rb @@ -4,6 +4,19 @@ module Spree class Promotion < Spree::Base module Actions class FreeShipping < Spree::PromotionAction + def can_discount?(klass) + klass == Spree::Shipment + end + + def discount(shipment) + discount = shipment.discounts.detect do |discount| + discount.promotion_action == self + end || shipment.discounts.build(promotion_action: self) + discount.label = I18n.t('spree.adjustment_labels.shipment', promotion: Spree::Promotion.model_name.human, promotion_name: promotion.name) + discount.amount = compute_amount(shipment) + discount + end + def perform(payload = {}) order = payload[:order] promotion_code = payload[:promotion_code] diff --git a/core/app/models/spree/promotion/rules/line_item_option_value.rb b/core/app/models/spree/promotion/rules/line_item_option_value.rb new file mode 100644 index 00000000000..b639c804f5c --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_option_value.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + class LineItemOptionValue < PromotionRule + preference :eligible_values, :hash + + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + pid = line_item.product.id + ovids = line_item.variant.option_values.pluck(:id) + + product_ids.include?(pid) && (value_ids(pid) & ovids).present? + end + + def preferred_eligible_values + values = preferences[:eligible_values] || {} + Hash[values.keys.map(&:to_i).zip( + values.values.map do |value| + (value.is_a?(Array) ? value : value.split(",")).map(&:to_i) + end + )] + end + + private + + def product_ids + preferred_eligible_values.keys + end + + def value_ids(product_id) + preferred_eligible_values[product_id] + end + end + end + end +end diff --git a/core/app/models/spree/promotion/rules/line_item_product.rb b/core/app/models/spree/promotion/rules/line_item_product.rb new file mode 100644 index 00000000000..daafcfb5096 --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_product.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + # A rule to apply a promotion only to line items with or without a chosen product + class LineItemProduct < Spree::PromotionRule + MATCH_POLICIES = %w(normal inverse) + + has_many :product_promotion_rules, + dependent: :destroy, + foreign_key: :promotion_rule_id, + class_name: "Spree::ProductPromotionRule" + has_many :products, + class_name: "Spree::Product", + through: :product_promotion_rules + + preference :match_policy, :string, default: MATCH_POLICIES.first + + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + if inverse? + !product_ids.include?(line_item.variant.product_id) + else + product_ids.include?(line_item.variant.product_id) + end + end + + def product_ids_string + product_ids.join(",") + end + + def product_ids_string=(product_ids) + self.product_ids = product_ids.to_s.split(",").map(&:strip) + end + + private + + def inverse? + preferred_match_policy == "inverse" + end + end + end + end +end diff --git a/core/app/models/spree/promotion/rules/line_item_taxon.rb b/core/app/models/spree/promotion/rules/line_item_taxon.rb new file mode 100644 index 00000000000..5f5f9075034 --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_taxon.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + class LineItemTaxon < PromotionRule + has_many :promotion_rule_taxons, class_name: 'Spree::PromotionRuleTaxon', foreign_key: :promotion_rule_id, + dependent: :destroy + has_many :taxons, through: :promotion_rule_taxons, class_name: 'Spree::Taxon' + + MATCH_POLICIES = %w(normal inverse) + + validates_inclusion_of :preferred_match_policy, in: MATCH_POLICIES + + preference :match_policy, :string, default: MATCH_POLICIES.first + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + found = Spree::Classification.where( + product_id: line_item.variant.product_id, + taxon_id: rule_taxon_ids_with_children + ).exists? + + case preferred_match_policy + when 'normal' + found + when 'inverse' + !found + else + raise "unexpected match policy: #{preferred_match_policy.inspect}" + end + end + + def taxon_ids_string + taxons.pluck(:id).join(',') + end + + def taxon_ids_string=(taxon_ids) + taxon_ids = taxon_ids.to_s.split(',').map(&:strip) + self.taxons = Spree::Taxon.find(taxon_ids) + end + + private + + # ids of taxons rules and taxons rules children + def rule_taxon_ids_with_children + taxons.flat_map { |taxon| taxon.self_and_descendants.ids }.uniq + end + end + end + end +end diff --git a/core/app/models/spree/promotion/rules/option_value.rb b/core/app/models/spree/promotion/rules/option_value.rb index 04de936957a..030151b2076 100644 --- a/core/app/models/spree/promotion/rules/option_value.rb +++ b/core/app/models/spree/promotion/rules/option_value.rb @@ -4,8 +4,6 @@ module Spree class Promotion < Spree::Base module Rules class OptionValue < PromotionRule - MATCH_POLICIES = %w(any) - preference :match_policy, :string, default: MATCH_POLICIES.first preference :eligible_values, :hash def applicable?(promotable) @@ -13,19 +11,11 @@ def applicable?(promotable) end def eligible?(promotable, _options = {}) - case preferred_match_policy - when 'any' - promotable.line_items.any? { |item| actionable?(item) } + promotable.line_items.any? do |item| + LineItemOptionValue.new(preferred_eligible_values: preferred_eligible_values).eligible?(item) end end - def actionable?(line_item) - pid = line_item.product.id - ovids = line_item.variant.option_values.pluck(:id) - - product_ids.include?(pid) && (value_ids(pid) & ovids).present? - end - def preferred_eligible_values values = preferences[:eligible_values] || {} Hash[values.keys.map(&:to_i).zip( @@ -34,16 +24,6 @@ def preferred_eligible_values end )] end - - private - - def product_ids - preferred_eligible_values.keys - end - - def value_ids(product_id) - preferred_eligible_values[product_id] - end end end end diff --git a/core/app/models/spree/promotion/rules/product.rb b/core/app/models/spree/promotion/rules/product.rb index c8888bc71ad..8b0294b8eec 100644 --- a/core/app/models/spree/promotion/rules/product.rb +++ b/core/app/models/spree/promotion/rules/product.rb @@ -56,17 +56,6 @@ def eligible?(order, _options = {}) eligibility_errors.empty? end - def actionable?(line_item) - case preferred_match_policy - when 'any', 'all' - product_ids.include? line_item.variant.product_id - when 'none' - product_ids.exclude? line_item.variant.product_id - else - raise "unexpected match policy: #{preferred_match_policy.inspect}" - end - end - def product_ids_string product_ids.join(',') end diff --git a/core/app/models/spree/promotion/rules/taxon.rb b/core/app/models/spree/promotion/rules/taxon.rb index 16b459636d5..e88c0fbd657 100644 --- a/core/app/models/spree/promotion/rules/taxon.rb +++ b/core/app/models/spree/promotion/rules/taxon.rb @@ -48,22 +48,6 @@ def eligible?(order, _options = {}) eligibility_errors.empty? end - def actionable?(line_item) - found = Spree::Classification.where( - product_id: line_item.variant.product_id, - taxon_id: rule_taxon_ids_with_children - ).exists? - - case preferred_match_policy - when 'any', 'all' - found - when 'none' - !found - else - raise "unexpected match policy: #{preferred_match_policy.inspect}" - end - end - def taxon_ids_string taxons.pluck(:id).join(',') end diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb index 9ddf9153a2d..989b53e3d76 100644 --- a/core/app/models/spree/promotion_action.rb +++ b/core/app/models/spree/promotion_action.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'spree/preferences/persistable' +require "spree/preferences/persistable" module Spree # Base class for all types of promotion action. @@ -11,7 +11,12 @@ class PromotionAction < Spree::Base include Spree::Preferences::Persistable include Spree::SoftDeletable - belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_actions, optional: true + acts_as_list column: :priority, scope: :promotion_id + + belongs_to :promotion, class_name: "Spree::Promotion", inverse_of: :promotion_actions, optional: true + + has_many :line_item_discounts, class_name: "Spree::LineItemDiscount", inverse_of: :promotion_action + has_many :shipment_discounts, class_name: "Spree::ShipmentDiscount", inverse_of: :promotion_action scope :of_type, ->(type) { where(type: Array.wrap(type).map(&:to_s)) } scope :shipping, -> { of_type(Spree::Config.environment.promotions.shipping_actions.to_a) } @@ -27,7 +32,7 @@ def preload_relations # # @note This method should be overriden in subclassses. def perform(_options = {}) - raise 'perform should be implemented in a sub-class of PromotionAction' + raise "perform should be implemented in a sub-class of PromotionAction" end # Removes the action from an order @@ -37,7 +42,7 @@ def perform(_options = {}) # @param order [Spree::Order] the order to remove the action from # @return [void] def remove_from(_order) - raise 'remove_from should be implemented in a sub-class of PromotionAction' + raise "remove_from should be implemented in a sub-class of PromotionAction" end def to_partial_path diff --git a/core/app/models/spree/promotion_rule.rb b/core/app/models/spree/promotion_rule.rb index 5fe201b7955..79439c573a6 100644 --- a/core/app/models/spree/promotion_rule.rb +++ b/core/app/models/spree/promotion_rule.rb @@ -31,12 +31,6 @@ def eligible?(_promotable, _options = {}) raise NotImplementedError, "eligible? should be implemented in a sub-class of Spree::PromotionRule" end - # This states if a promotion can be applied to the specified line item - # It is true by default, but can be overridden by promotion rules to provide conditions - def actionable?(_line_item) - true - end - def eligibility_errors @eligibility_errors ||= ActiveModel::Errors.new(self) end diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 9d6604adeae..274d4a5887d 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -7,6 +7,12 @@ class Shipment < Spree::Base belongs_to :order, class_name: 'Spree::Order', touch: true, inverse_of: :shipments, optional: true belongs_to :stock_location, class_name: 'Spree::StockLocation', optional: true + has_many :discounts, + class_name: "Spree::ShipmentDiscount", + foreign_key: :shipment_id, + dependent: :destroy, + inverse_of: :shipment, + autosave: true has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :delete_all has_many :inventory_units, dependent: :destroy, inverse_of: :shipment has_many :shipping_rates, -> { order(:cost) }, dependent: :destroy, inverse_of: :shipment diff --git a/core/app/models/spree/shipment_discount.rb b/core/app/models/spree/shipment_discount.rb new file mode 100644 index 00000000000..deb00bfbd39 --- /dev/null +++ b/core/app/models/spree/shipment_discount.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Spree + class ShipmentDiscount < Spree::Base + belongs_to :shipment, inverse_of: :discounts + belongs_to :promotion_action, -> { with_discarded }, inverse_of: :shipment_discounts + end +end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 3e1cec69b05..4841bb42689 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -226,12 +226,20 @@ en: description: Only one use per user spree/promotion/rules/option_value: description: Order includes specified product(s) with matching option value(s) + spree/promotion/rules/line_item_option_value: + description: Line Item has specified product with matching option value spree/promotion/rules/product: description: Order includes specified product(s) + spree/promotion/rules/line_item_product: + description: Line item matches specified product(s) + preferred_match_policy: Match Policy spree/promotion/rules/store: description: Available only to the specified stores spree/promotion/rules/taxon: description: Order includes products with specified taxon(s) + spree/promotion/rules/line_item_taxon: + description: Line Item has product with specified taxon(s) + preferred_match_policy: Match Policy spree/promotion/rules/user: description: Available only to the specified users spree/promotion/rules/user_logged_in: @@ -655,8 +663,11 @@ en: spree/promotion/rules/nth_order: Nth Order spree/promotion/rules/one_use_per_user: One Use Per User spree/promotion/rules/option_value: Option Value(s) - spree/promotion/rules/product: Product(s) - spree/promotion/rules/taxon: Taxon(s) + spree/promotion/rules/line_item_option_value: Line Item Option Value(s) + spree/promotion/rules/product: Order Product(s) + spree/promotion/rules/line_item_product: Line Item Product(s) + spree/promotion/rules/taxon: Order Taxon(s) + spree/promotion/rules/line_item_taxon: Line Item Taxon(s) spree/promotion/rules/user: User spree/promotion/rules/user_logged_in: User Logged In spree/promotion/rules/user_role: User Role(s) @@ -825,6 +836,7 @@ en: adjustment_labels: line_item: "%{promotion} (%{promotion_name})" order: "%{promotion} (%{promotion_name})" + shipment: "%{promotion} (%{promotion_name})" tax_rates: sales_tax: "%{name}" sales_tax_with_rate: "%{name} %{amount}" @@ -1867,6 +1879,15 @@ en: all: Match all of these rules any: Match any of these rules promotion_rule: Promotion Rule + promotion_rules: + line_item_product: + match_policies: + normal: Line item's product is one of the chosen products + inverse: Line item's product is NOT one of the chosen products + line_item_taxon: + match_policies: + normal: Line item's product has one of the chosen taxons + inverse: Line item's product does not have one of the chosen taxons promotion_successfully_created: Promotion has been successfully created! promotion_total_changed_before_complete: One or more of the promotions on your order have become ineligible and were removed. Please check the new order amounts diff --git a/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb b/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb new file mode 100644 index 00000000000..11ea4043f38 --- /dev/null +++ b/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateSpreeLineItemDiscounts < ActiveRecord::Migration[5.2] + def change + create_table :spree_line_item_discounts do |t| + t.references :line_item, null: false + t.references :promotion_action, null: false + t.decimal :amount, precision: 10, scale: 2, null: false + t.string :label, null: false + + t.timestamps + end + end +end diff --git a/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb b/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb new file mode 100644 index 00000000000..d9a73021f35 --- /dev/null +++ b/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateSpreeShipmentDiscounts < ActiveRecord::Migration[5.2] + def change + create_table :spree_shipment_discounts do |t| + t.references :shipment, null: false + t.references :promotion_action, null: false + t.decimal :amount, precision: 10, scale: 2, null: false + t.string :label, null: false + + t.timestamps + end + end +end diff --git a/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb b/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb new file mode 100644 index 00000000000..631d9cfd6fe --- /dev/null +++ b/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class SetPromotionsWithAnyPolicyToAllIfPossible < ActiveRecord::Migration[5.2] + def up + Spree::Promotion.where(match_policy: :any).includes(:promotion_rules).all.each do |promotion| + if promotion.promotion_rules.length <= 1 + promotion.update(match_policy: :all) + end + end + end + + def down + # No-Op + end +end diff --git a/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb b/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb new file mode 100644 index 00000000000..412cc8505e5 --- /dev/null +++ b/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddPriorityToSpreePromotions < ActiveRecord::Migration[5.2] + def up + add_column :spree_promotions, :priority, :integer + sql = <<~SQL + UPDATE spree_promotions + SET priority = id + SQL + execute(sql) + change_column :spree_promotions, :priority, :integer, null: false + end + + def down + remove_column :spree_promotions, :priority, :integer, null: false + end +end diff --git a/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb b/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb new file mode 100644 index 00000000000..78c6a1af3fa --- /dev/null +++ b/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddPriorityToSpreePromotionActions < ActiveRecord::Migration[5.2] + def up + add_column :spree_promotion_actions, :priority, :integer + sql = <<~SQL + UPDATE spree_promotion_actions + SET priority = id + SQL + execute(sql) + change_column :spree_promotion_actions, :priority, :integer, null: false + end + + def down + remove_column :spree_promotion_actions, :priority, :integer, null: false + end +end diff --git a/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb b/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb new file mode 100644 index 00000000000..89d483775af --- /dev/null +++ b/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class AddLineItemPromotionRulesToExistingPromotions < ActiveRecord::Migration[5.2] + def up + Spree::Promotion::Rules::Product.all.each do |promotion_rule| + match_policy = promotion_rule.preferred_match_policy == "none" ? "inverse" : "normal" + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemProduct.new( + preferred_match_policy: match_policy, + products: promotion_rule.products + ) + end + Spree::Promotion::Rules::Taxon.all.each do |promotion_rule| + match_policy = promotion_rule.preferred_match_policy == "none" ? "inverse" : "normal" + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemTaxon.new( + preferred_match_policy: match_policy, + taxons: promotion_rule.taxons + ) + end + Spree::Promotion::Rules::OptionValue.all.each do |promotion_rule| + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemOptionValue.new( + preferred_eligible_values: promotion_rule.preferred_eligible_values + ) + end + end + + def down + Spree::Promotion::Rules::LineItemOptionValue.destroy_all + Spree::Promotion::Rules::LineItemTaxon.destroy_all + Spree::Promotion::Rules::LineItemProduct.destroy_all + end +end diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index d167d0c83f2..00dbdc48767 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -60,6 +60,11 @@ class AppConfiguration < Preferences::Configuration # @return [Boolean] When false, customers must create an account to complete an order (default: +true+) preference :allow_guest_checkout, :boolean, default: true + # @!attribute [rw] allow_promotions_any_match_policy + # @return [Boolean] When false, admins cannot create promotions with an "any" match policy (default: +false+) + # Create individual, separate promotions for each of your rules instead. + preference :allow_promotions_any_match_policy, :boolean, default: false + # @!attribute [rw] guest_token_cookie_options # @return [Hash] Add additional guest_token cookie options here (ie. domain or path) preference :guest_token_cookie_options, :hash, default: {} @@ -202,6 +207,13 @@ class AppConfiguration < Preferences::Configuration # @return [Integer] Promotions to show per-page in the admin (default: +15+) preference :promotions_per_page, :integer, default: 15 + # @!attribute [rw] promotion_system + # + # Defines whether to use adjustment- or discount-based promotions. + # + # @return [Symbol] (can be :adjustments or :discounts) + preference :promotion_system, :symbol, default: :adjustments + # @!attribute [rw] require_master_price # @return [Boolean] Require a price on the master variant of a product (default: +true+) preference :require_master_price, :boolean, default: true @@ -309,6 +321,12 @@ def default_pricing_options # promotion_chooser_class allows extensions to provide their own PromotionChooser class_name_attribute :promotion_chooser_class, default: 'Spree::PromotionChooser' + # promotion_handler_class allows extensions to provide their own Discount Order Updater + class_name_attribute :discounter_class, default: 'Spree::Discounts::OrderDiscounter' + + # discount_chooser_class allows extensions to provide their own discount chooser + class_name_attribute :discount_chooser_class, default: 'Spree::Discounts::Chooser' + class_name_attribute :allocator_class, default: 'Spree::Stock::Allocator::OnHandFirst' class_name_attribute :shipping_rate_sorter_class, default: 'Spree::Stock::ShippingRateSorter' @@ -623,10 +641,13 @@ def environment Spree::Promotion::Rules::User Spree::Promotion::Rules::FirstOrder Spree::Promotion::Rules::UserLoggedIn + Spree::Promotion::Rules::LineItemProduct Spree::Promotion::Rules::OneUsePerUser Spree::Promotion::Rules::Taxon + Spree::Promotion::Rules::LineItemTaxon Spree::Promotion::Rules::NthOrder Spree::Promotion::Rules::OptionValue + Spree::Promotion::Rules::LineItemOptionValue Spree::Promotion::Rules::FirstRepeatPurchaseSince Spree::Promotion::Rules::UserRole Spree::Promotion::Rules::Store diff --git a/core/lib/spree/testing_support/deprecated_examples.rb b/core/lib/spree/testing_support/deprecated_examples.rb new file mode 100644 index 00000000000..650c338d373 --- /dev/null +++ b/core/lib/spree/testing_support/deprecated_examples.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +RSpec.configure do |config| + config.around(:each, deprecated_examples: true) do |example| + Spree::Deprecation.silence do + example.run + end + end +end diff --git a/core/lib/spree/testing_support/factories/line_item_discount_factory.rb b/core/lib/spree/testing_support/factories/line_item_discount_factory.rb new file mode 100644 index 00000000000..d2bf7b169ab --- /dev/null +++ b/core/lib/spree/testing_support/factories/line_item_discount_factory.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spree/testing_support/factory_bot' +Spree::TestingSupport::FactoryBot.when_cherry_picked do + Spree::TestingSupport::FactoryBot.deprecate_cherry_picking + + require 'spree/testing_support/factories/line_item_factory' +end + +FactoryBot.define do + factory :line_item_discount, class: 'Spree::LineItemDiscount' do + amount { BigDecimal("-2.00") } + line_item + promotion_action { Spree::Promotion::Actions::CreateItemAdjustments.new } + label { "Cheap because promotion" } + end +end diff --git a/core/lib/spree/testing_support/factories/shipment_discount_factory.rb b/core/lib/spree/testing_support/factories/shipment_discount_factory.rb new file mode 100644 index 00000000000..c09a5fbdeb1 --- /dev/null +++ b/core/lib/spree/testing_support/factories/shipment_discount_factory.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spree/testing_support/factory_bot' +Spree::TestingSupport::FactoryBot.when_cherry_picked do + Spree::TestingSupport::FactoryBot.deprecate_cherry_picking + + require 'spree/testing_support/factories/shipment_factory' +end + +FactoryBot.define do + factory :shipment_discount, class: 'Spree::ShipmentDiscount' do + amount { BigDecimal("-4.00") } + shipment + promotion_action { Spree::Promotion::Actions::CreateAdjustment.new } + label { "10% off" } + end +end diff --git a/core/spec/lib/spree/app_configuration_spec.rb b/core/spec/lib/spree/app_configuration_spec.rb index 5296419e6fc..d3eb6c33535 100644 --- a/core/spec/lib/spree/app_configuration_spec.rb +++ b/core/spec/lib/spree/app_configuration_spec.rb @@ -54,6 +54,12 @@ end end + describe '@promotion_system' do + it 'is `:adjustments` by default' do + expect(prefs[:promotion_system]).to eq(:adjustments) + end + end + describe '#environment' do class DummyClass; end; diff --git a/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb b/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb new file mode 100644 index 00000000000..8df1022a983 --- /dev/null +++ b/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'line item discount factory' do + let(:factory_class) { Spree::LineItemDiscount } + + let(:factory) { :line_item_discount } + + it_behaves_like 'a working factory' +end diff --git a/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb b/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb new file mode 100644 index 00000000000..5a36b5773fe --- /dev/null +++ b/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'shipment discount factory' do + let(:factory_class) { Spree::ShipmentDiscount } + + let(:factory) { :shipment_discount } + + it_behaves_like 'a working factory' +end diff --git a/core/spec/models/spree/calculator/distributed_amount_spec.rb b/core/spec/models/spree/calculator/distributed_amount_spec.rb index 17f9eee82fc..86d62d7caef 100644 --- a/core/spec/models/spree/calculator/distributed_amount_spec.rb +++ b/core/spec/models/spree/calculator/distributed_amount_spec.rb @@ -31,14 +31,13 @@ let(:first_product) { order.line_items.first.product } before do - rule = Spree::Promotion::Rules::Product.create!( - promotion: promotion, - product_promotion_rules: [ - Spree::ProductPromotionRule.new(product: first_product), - ], + order_rule = Spree::Promotion::Rules::Product.new( + products: [first_product], ) - promotion.rules << rule - promotion.save! + line_item_rule = Spree::Promotion::Rules::LineItemProduct.new( + products: [first_product], + ) + promotion.rules << order_rule << line_item_rule order.recalculate end diff --git a/core/spec/models/spree/line_item_discount_spec.rb b/core/spec/models/spree/line_item_discount_spec.rb new file mode 100644 index 00000000000..99db47fe6e7 --- /dev/null +++ b/core/spec/models/spree/line_item_discount_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::LineItemDiscount do + subject(:line_item_discount) { build(:line_item_discount) } + + it { is_expected.to respond_to(:line_item) } + it { is_expected.to respond_to(:promotion_action) } + it { is_expected.to respond_to(:amount) } + it { is_expected.to respond_to(:label) } +end diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index dc18f2030e0..7a3dffa8ee6 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -112,6 +112,27 @@ include_context "discount changes order total" end + + context "with new discount-based promotion system" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + context "one active line item promotion" do + let!(:action) { Spree::Promotion::Actions::CreateItemAdjustments.create(promotion: promotion, calculator: calculator) } + + it "creates valid discount on order" do + subject.add(variant, 1) + expect(subject.order.line_items.flat_map(&:discounts).to_a.sum(&:amount)).not_to eq 0 + end + + include_context "discount changes order total" + end + end end describe 'tax calculations' do diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb index ee842f535f2..68fb65ec387 100644 --- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb +++ b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb @@ -10,10 +10,41 @@ module Spree let(:action) { promotion.actions.first! } let(:line_item) { order.line_items.to_a.first } let(:payload) { { order: order, promotion: promotion } } + let(:rules) { [] } before do - allow(action).to receive(:promotion).and_return(promotion) promotion.promotion_actions = [action] + promotion.promotion_rules = rules + end + + + context "#discount" do + subject { action.discount(line_item) } + + before do + expect(action).to receive(:compute_amount).and_return(10) + action.promotion = promotion + end + + it "builds an unpersisted discount" do + expect(subject).to be_a(Spree::LineItemDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + end + + context "if the line item already has a discount" do + before do + line_item.discounts << build(:line_item_discount, line_item: line_item, promotion_action: action) + end + + it "changes the discounts label and price" do + expect(subject).to be_a(Spree::LineItemDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + expect(subject).to be_persisted + expect(subject).to be_changed + end + end end context "#perform" do @@ -47,12 +78,10 @@ module Spree end context "with products rules" do - let(:rule) { double Spree::Promotion::Rules::Product } - - before { allow(promotion).to receive(:eligible_rules) { [rule] } } - - context "when the rule is actionable" do - before { allow(rule).to receive(:actionable?).and_return(true) } + let(:rule) { Spree::Promotion::Rules::LineItemProduct.new } + let(:rules) { [rule] } + context "when the rule is eligible" do + before { allow(rule).to receive(:eligible?).and_return(true) } it "creates an adjustment" do expect { @@ -65,8 +94,8 @@ module Spree end end - context "when the rule is not actionable" do - before { allow(rule).to receive(:actionable?).and_return(false) } + context "when the rule is not eligible" do + before { allow(rule).to receive(:eligible?).and_return(false) } it "does not create an adjustment" do expect { @@ -95,7 +124,7 @@ module Spree context "#compute_amount" do before { promotion.promotion_actions = [action] } - context "when the adjustable is actionable" do + context "when the adjustable is eligible" do it "calls compute on the calculator" do expect(action.calculator).to receive(:compute).with(line_item).and_call_original action.compute_amount(line_item) @@ -113,8 +142,8 @@ module Spree end end - context "when the adjustable is not actionable" do - before { allow(promotion).to receive(:line_item_actionable?) { false } } + context "when the adjustable is not eligible" do + before { allow(promotion).to receive(:line_item_eligible?) { false } } it 'returns 0' do expect(action.compute_amount(line_item)).to eql(0) diff --git a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb b/core/spec/models/spree/promotion/actions/free_shipping_spec.rb index 0b9a14c1ca3..b8956f1bead 100644 --- a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb +++ b/core/spec/models/spree/promotion/actions/free_shipping_spec.rb @@ -10,6 +10,34 @@ let(:payload) { { order: order, promotion_code: promotion_code } } let(:promotion_code) { promotion.codes.first! } + context "#discount" do + subject { action.discount(shipment) } + + before do + expect(action).to receive(:compute_amount).and_return(10) + action.promotion = promotion + end + + it "builds an unpersisted discount" do + expect(subject).to be_a(Spree::ShipmentDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + end + + context "if the shipment already has a discount" do + before do + shipment.discounts << build(:shipment_discount, shipment: shipment, promotion_action: action) + end + + it "changes the discounts label and price" do + expect(subject).to be_a(Spree::ShipmentDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + expect(subject).to be_persisted + expect(subject).to be_changed + end + end + end # From promotion spec: describe "#perform" do before do diff --git a/core/spec/models/spree/promotion/integration_spec.rb b/core/spec/models/spree/promotion/integration_spec.rb new file mode 100644 index 00000000000..275b0270d94 --- /dev/null +++ b/core/spec/models/spree/promotion/integration_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "Promotion System" do + context "A promotion that creates line item adjustments" do + let(:shirt) { create(:product) } + let(:pants) { create(:product) } + let(:promotion) { create(:promotion, name: "20% off Shirts", apply_automatically: true) } + let(:order) { create(:order) } + + before do + promotion.rules << rule + promotion.actions << action + order.contents.add(shirt.master, 1) + order.contents.add(pants.master, 1) + end + + context "with an order-level rule" do + let(:rule) { Spree::Promotion::Rules::Product.new(products: [shirt]) } + + context "with an order level action" do + let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateAdjustment.new(calculator: calculator) } + + it "creates one order-level adjustment" do + expect(order.adjustments.length).to eq(1) + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + # This is wrong! But order level adjustments can't work any other way + expect(order.item_total_before_tax).to eq(39.98) + expect(order.line_items.flat_map(&:adjustments)).to be_empty + end + end + + context "with an line item level action" do + let(:calculator) { Spree::Calculator::PercentOnLineItem.new(preferred_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateItemAdjustments.new(calculator: calculator) } + + it "creates one order-level adjustment" do + expect(order.adjustments).to be_empty + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + expect(order.item_total_before_tax).to eq(31.98) + expect(order.line_items.flat_map(&:adjustments).length).to eq(2) + end + end + end + + context "with a line-item level rule" do + let(:rule) { Spree::Promotion::Rules::LineItemProduct.new(products: [shirt]) } + + context "with an order level action" do + let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateAdjustment.new(calculator: calculator) } + + it "creates one order-level adjustment" do + # Whoops - this works because line item level rules don't affect order-level actions :( + expect(order.adjustments.length).to eq(1) + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + # This is wrong! But order level adjustments can't work any other way + expect(order.item_total_before_tax).to eq(39.98) + expect(order.line_items.flat_map(&:adjustments)).to be_empty + end + end + + context "with an line item level action" do + let(:calculator) { Spree::Calculator::PercentOnLineItem.new(preferred_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateItemAdjustments.new(calculator: calculator) } + + it "creates one line item level adjustment" do + expect(order.adjustments).to be_empty + expect(order.total).to eq(35.98) + expect(order.item_total).to eq(39.98) + expect(order.item_total_before_tax).to eq(35.98) + expect(order.line_items.flat_map(&:adjustments).length).to eq(1) + end + end + end + end +end diff --git a/core/spec/models/spree/promotion/rules/line_item_product_spec.rb b/core/spec/models/spree/promotion/rules/line_item_product_spec.rb new file mode 100644 index 00000000000..de68cbe93b4 --- /dev/null +++ b/core/spec/models/spree/promotion/rules/line_item_product_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::Promotion::Rules::LineItemProduct, type: :model do + let(:rule) { described_class.new(rule_options) } + let(:rule_options) { {} } + + context "#eligible?(line_item)" do + let(:rule_line_item) { Spree::LineItem.new(product: rule_product) } + let(:other_line_item) { Spree::LineItem.new(product: other_product) } + + let(:rule_options) { super().merge(products: [rule_product]) } + let(:rule_product) { mock_model(Spree::Product) } + let(:other_product) { mock_model(Spree::Product) } + + it "should be eligible if there are no products" do + expect(rule).to be_eligible(rule_line_item) + end + + subject { rule.eligible?(line_item, {}) } + + context "for product in rule" do + let(:line_item) { rule_line_item } + it { is_expected.to be_truthy } + end + + context "for product not in rule" do + let(:line_item) { other_line_item } + it { is_expected.to be_falsey } + end + + context "if match policy is inverse" do + let(:rule_options) { super().merge(preferred_match_policy: "inverse") } + + context "for product in rule" do + let(:line_item) { rule_line_item } + it { is_expected.to be_falsey } + end + + context "for product not in rule" do + let(:line_item) { other_line_item } + it { is_expected.to be_truthy } + end + end + end +end diff --git a/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb b/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb new file mode 100644 index 00000000000..25cc37625b8 --- /dev/null +++ b/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::Promotion::Rules::LineItemTaxon, type: :model do + let(:taxon) { create :taxon, name: 'first' } + let(:taxon2) { create :taxon, name: 'second' } + let(:order) { create :order_with_line_items } + let(:product) { order.products.first } + + let(:rule) do + described_class.create!(promotion: create(:promotion)) + end + + describe '#eligible?' do + let(:line_item) { order.line_items.first! } + let(:order) { create :order_with_line_items } + let(:taxon) { create :taxon, name: 'first' } + + context 'with an invalid match policy' do + before do + rule.preferred_match_policy = 'invalid' + rule.save!(validate: false) + line_item.product.taxons << taxon + rule.taxons << taxon + end + + it 'raises' do + expect { + rule.eligible?(line_item) + }.to raise_error('unexpected match policy: "invalid"') + end + end + + context 'when a product has a taxon of a taxon rule' do + before do + product.taxons << taxon + rule.taxons << taxon + rule.save! + end + + it 'is eligible' do + expect(rule).to be_eligible(line_item) + end + end + + context 'when a product has a taxon child of a taxon rule' do + before do + taxon.children << taxon2 + product.taxons << taxon2 + rule.taxons << taxon + rule.save! + end + + it 'is eligible' do + expect(rule).to be_eligible(line_item) + end + + context "with inverse match policy" do + before do + rule.update(preferred_match_policy: :inverse) + end + + it "is not eligible" do + expect(rule).not_to be_eligible(line_item) + end + end + end + + context 'when a product does not have taxon or child taxon of a taxon rule' do + before do + product.taxons << taxon2 + rule.taxons << taxon + rule.save! + end + + it 'is not eligible' do + expect(rule).not_to be_eligible(line_item) + end + + context "with inverse match policy" do + before do + rule.update(preferred_match_policy: :inverse) + end + + it "is not eligible" do + expect(rule).to be_eligible(line_item) + end + end + end + end +end diff --git a/core/spec/models/spree/promotion/rules/option_value_spec.rb b/core/spec/models/spree/promotion/rules/option_value_spec.rb index 26eef3780db..db01051c8fb 100644 --- a/core/spec/models/spree/promotion/rules/option_value_spec.rb +++ b/core/spec/models/spree/promotion/rules/option_value_spec.rb @@ -45,52 +45,4 @@ it { is_expected.to be false } end end - - describe "#actionable?" do - let(:line_item) { create :line_item } - let(:option_value_blue) do - create( - :option_value, - name: 'Blue', - presentation: 'Blue', - option_type: create( - :option_type, - name: 'foo-colour', - presentation: 'Colour' - ) - ) - end - let(:option_value_medium) do - create( - :option_value, - name: 'Medium', - presentation: 'M' - ) - end - before do - line_item.variant.option_values << option_value_blue - rule.preferred_eligible_values = Hash[product_id => option_value_ids] - end - subject { rule.actionable?(line_item) } - context "when the line item has the correct product" do - let(:product_id) { line_item.product.id } - context "when all of the option values match" do - let(:option_value_ids) { [option_value_blue.id] } - it { is_expected.to be true } - end - context "when any of the option values match" do - let(:option_value_ids) { [option_value_blue.id, option_value_medium.id] } - it { is_expected.to be true } - end - context "when none of the option values match" do - let(:option_value_ids) { [option_value_medium.id] } - it { is_expected.to be false } - end - end - context "when the line item's product doesn't match" do - let(:product_id) { 99 } - let(:option_value_ids) { [99] } - it { is_expected.to be false } - end - end end diff --git a/core/spec/models/spree/promotion/rules/product_spec.rb b/core/spec/models/spree/promotion/rules/product_spec.rb index afe73223aa0..7e400baa287 100644 --- a/core/spec/models/spree/promotion/rules/product_spec.rb +++ b/core/spec/models/spree/promotion/rules/product_spec.rb @@ -124,70 +124,4 @@ end end end - - describe '#actionable?' do - subject do - rule.actionable?(line_item) - end - - let(:rule_line_item) { Spree::LineItem.new(product: rule_product) } - let(:other_line_item) { Spree::LineItem.new(product: other_product) } - - let(:rule_options) { super().merge(products: [rule_product]) } - let(:rule_product) { mock_model(Spree::Product) } - let(:other_product) { mock_model(Spree::Product) } - - context "with 'any' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'any') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_truthy } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_falsey } - end - end - - context "with 'all' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'all') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_truthy } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_falsey } - end - end - - context "with 'none' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'none') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_falsey } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_truthy } - end - end - - context 'with an invalid match policy' do - let(:rule_options) { super().merge(preferred_match_policy: 'invalid') } - let(:line_item) { rule_line_item } - - it 'raises' do - expect { - rule.actionable?(line_item) - }.to raise_error('unexpected match policy: "invalid"') - end - end - end end diff --git a/core/spec/models/spree/promotion/rules/taxon_spec.rb b/core/spec/models/spree/promotion/rules/taxon_spec.rb index 322be092b4e..4c866595076 100644 --- a/core/spec/models/spree/promotion/rules/taxon_spec.rb +++ b/core/spec/models/spree/promotion/rules/taxon_spec.rb @@ -24,22 +24,6 @@ expect(rule).to be_eligible(order) end - context 'when order contains items from different taxons' do - before do - product.taxons << taxon - rule.taxons << taxon - end - - it 'should act on a product within the eligible taxon' do - expect(rule).to be_actionable(order.line_items.last) - end - - it 'should not act on a product in another taxon' do - order.line_items << create(:line_item, product: create(:product, taxons: [taxon2])) - expect(rule).not_to be_actionable(order.line_items.last) - end - end - context "when order does not have any prefered taxon" do before { rule.taxons << taxon2 } it { expect(rule).not_to be_eligible(order) } @@ -157,62 +141,4 @@ end end end - - describe '#actionable?' do - let(:line_item) { order.line_items.first! } - let(:order) { create :order_with_line_items } - let(:taxon) { create :taxon, name: 'first' } - - context 'with an invalid match policy' do - before do - rule.preferred_match_policy = 'invalid' - rule.save!(validate: false) - line_item.product.taxons << taxon - rule.taxons << taxon - end - - it 'raises' do - expect { - rule.eligible?(order) - }.to raise_error('unexpected match policy: "invalid"') - end - end - - context 'when a product has a taxon of a taxon rule' do - before do - product.taxons << taxon - rule.taxons << taxon - rule.save! - end - - it 'is actionable' do - expect(rule).to be_actionable(line_item) - end - end - - context 'when a product has a taxon child of a taxon rule' do - before do - taxon.children << taxon2 - product.taxons << taxon2 - rule.taxons << taxon - rule.save! - end - - it 'is actionable' do - expect(rule).to be_actionable(line_item) - end - end - - context 'when a product does not have taxon or child taxon of a taxon rule' do - before do - product.taxons << taxon2 - rule.taxons << taxon - rule.save! - end - - it 'is not actionable' do - expect(rule).not_to be_actionable(line_item) - end - end - end end diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 2b2294d572b..7b29a68c638 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -357,39 +357,90 @@ end describe "#usage_count" do - let(:promotion) do - FactoryBot.create( - :promotion, - :with_order_adjustment, - code: "discount" - ) - end + context "with legacy promotion system" do + let(:promotion) do + FactoryBot.create( + :promotion, + :with_order_adjustment, + code: "discount" + ) + end - subject { promotion.usage_count } + subject { promotion.usage_count } - context "when the code is applied to a non-complete order" do - let(:order) { FactoryBot.create(:order_with_line_items) } - before { promotion.activate(order: order, promotion_code: promotion.codes.first) } - it { is_expected.to eq 0 } + context "when the code is applied to a non-complete order" do + let(:order) { FactoryBot.create(:order_with_line_items) } + before { promotion.activate(order: order, promotion_code: promotion.codes.first) } + it { is_expected.to eq 0 } + end + context "when the code is applied to a complete order" do + let!(:order) do + FactoryBot.create( + :completed_order_with_promotion, + promotion: promotion + ) + end + context "and the promo is eligible" do + it { is_expected.to eq 1 } + end + context "and the promo is ineligible" do + before { order.adjustments.promotion.update_all(eligible: false) } + it { is_expected.to eq 0 } + end + context "and the order is canceled" do + before { order.cancel! } + it { is_expected.to eq 0 } + it { expect(order.state).to eq 'canceled' } + end + end end - context "when the code is applied to a complete order" do - let!(:order) do + + context "with discount system" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + let(:promotion) do FactoryBot.create( - :completed_order_with_promotion, - promotion: promotion + :promotion, + :with_line_item_adjustment, ) end - context "and the promo is eligible" do - it { is_expected.to eq 1 } - end - context "and the promo is ineligible" do - before { order.adjustments.promotion.update_all(eligible: false) } + + subject { promotion.usage_count } + + context "when the code is applied to a non-complete order" do + let(:order) { FactoryBot.create(:order_with_line_items) } + before do + order.order_promotions << Spree::OrderPromotion.new(promotion_code: promotion.codes.first, promotion: promotion) + order.recalculate + end + it { is_expected.to eq 0 } end - context "and the order is canceled" do - before { order.cancel! } - it { is_expected.to eq 0 } - it { expect(order.state).to eq 'canceled' } + + context "when the code is applied to a complete order" do + let!(:order) { FactoryBot.create(:order_ready_to_complete) } + + before do + order.order_promotions << Spree::OrderPromotion.new(promotion_code: promotion.codes.first, promotion: promotion) + order.recalculate + order.complete! + end + + context "and the promo is eligible" do + it { is_expected.to eq 1 } + end + + context "and the order is canceled" do + before { order.cancel! } + it { is_expected.to eq 0 } + it { expect(order.state).to eq 'canceled' } + end end end end @@ -795,7 +846,7 @@ end end - context "with 'any' match policy" do + context "with 'any' match policy", :deprecated_examples do let(:promotable) { double('Promotable') } before do @@ -827,79 +878,100 @@ end end - describe '#line_item_actionable?' do - let(:order) { double Spree::Order } - let(:line_item) { double Spree::LineItem } - let(:true_rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true, actionable?: true } - let(:false_rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true, actionable?: false } - let(:rules) { [] } + describe "activatable?" do + let(:order) { create(:order) } + let(:promotion) { create(:promotion, :with_order_adjustment) } - before do - promotion.promotion_rules = rules - promotion.promotion_actions = [Spree::PromotionAction.new] - end + subject { promotion.activatable?(order) } - subject { promotion.line_item_actionable? order, line_item } + context "when promotion is expired" do + before { promotion.expires_at = Time.current - 10.days } - context 'when the order is eligible for promotion' do - context 'when there are no rules' do - it { is_expected.to be } - end + it { is_expected.to be false } + end - context 'when there are rules' do - context 'when the match policy is all' do - before { promotion.match_policy = 'all' } + context "when promotion's usage limit is exceeded" do + before do + promotion.usage_limit = 1 + create(:completed_order_with_promotion, promotion: promotion) + end - context 'when all rules allow action on the line item' do - let(:rules) { [true_rule] } - it { is_expected.to be } - end + it { is_expected.to be false } + end - context 'when at least one rule does not allow action on the line item' do - let(:rules) { [true_rule, false_rule] } - it { is_expected.not_to be } - end - end + context "when promotion code's usage limit is exceeded" do + let(:promotion_code) { create(:promotion_code, promotion: promotion) } - context 'when the match policy is any' do - before { promotion.match_policy = 'any' } + before do + promotion.per_code_usage_limit = 1 + order.order_promotions.create!(promotion: promotion, promotion_code: promotion_code) + create(:completed_order_with_promotion, promotion: promotion) + promotion.codes.first.adjustments.update_all(eligible: true) + end - context 'when at least one rule allows action on the line item' do - let(:rules) { [true_rule, false_rule] } - it { is_expected.to be } - end + it { is_expected.to be false } + end - context 'when no rules allow action on the line item' do - let(:rules) { [false_rule] } - it { is_expected.not_to be } - end - end + context "when promotion is at last usage on the same order" do + let(:order) { create(:completed_order_with_promotion, promotion: promotion) } - context 'when the line item has an non-promotionable product' do - let(:rules) { [true_rule] } - let(:line_item) { build(:line_item) { |li| li.variant.product.promotionable = false } } - it { is_expected.not_to be } - end + before do + promotion.usage_limit = 1 end + + it { is_expected.to be true } end - context 'when the order is not eligible for the promotion' do - context "due to promotion expiration" do - before { promotion.starts_at = Time.current + 2.days } - it { is_expected.not_to be } + context "when promotion code is at last usage on the same order" do + let(:order) { create(:completed_order_with_promotion, promotion: promotion) } + + before do + promotion.per_code_usage_limit = 1 end - context "due to promotion code not being eligible" do - let(:order) { create(:order) } - let(:promotion) { create(:promotion, per_code_usage_limit: 0) } - let(:promotion_code) { create(:promotion_code, promotion: promotion) } + it { is_expected.to be true } + end + end - subject { promotion.line_item_actionable? order, line_item, promotion_code: promotion_code } + describe "#line_item_eligible?" do + let(:line_item) { build(:line_item) } + let(:promotion) { create(:promotion, :with_action) } + let(:rules) { [] } - it "returns false" do - expect(subject).to eq false - end - end + subject { promotion.line_item_eligible?(line_item) } + + before do + promotion.promotion_rules = rules + end + + it { is_expected.to be true } + + context "if the line item's variant is not safelisted" do + let(:product) { build(:product, promotionable: false) } + let(:line_item) { build(:line_item, variant: product.master) } + + it { is_expected.to be false } + end + + context "if the promotion has inapplicable rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: false, applicable?: false } + let(:rules) { [rule] } + + it { is_expected.to be true } + end + + context "if the promotion has applicable rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: false, applicable?: true } + let(:rules) { [rule] } + + it { is_expected.to be false } + end + + context "if the promotion has applicable and eligible rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true } + let(:rules) { [rule] } + + it { is_expected.to be true } end end @@ -993,4 +1065,52 @@ expect(order.adjustment_total).to eq(-10) end end + + describe "#discounted_orders" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + let(:promotion) { create(:promotion, :with_action) } + let(:other_promotion) { create(:promotion, :with_action) } + let(:other_action) { other_promotion.actions.first } + let(:action) { promotion.actions.first } + + let!(:order_with_line_item_discount) do + create(:order_with_line_items).tap do |order| + order.line_items.first.discounts << build(:line_item_discount, line_item: nil, promotion_action: action) + end + end + + let!(:order_with_shipment_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: action) + end + end + + let!(:order_with_line_item_and_shipment_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: action) + order.line_items.first.discounts << build(:line_item_discount, line_item: nil, promotion_action: action) + end + end + + let!(:order_without_discount) { create(:order_with_line_items) } + + let!(:order_with_other_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: other_action) + end + end + + subject { promotion.discounted_orders } + + it { is_expected.to be_a(ActiveRecord::Relation) } + + it { is_expected.to match_array([order_with_line_item_and_shipment_discount, order_with_shipment_discount, order_with_line_item_discount]) } + end end diff --git a/core/spec/models/spree/shipment_discount_spec.rb b/core/spec/models/spree/shipment_discount_spec.rb new file mode 100644 index 00000000000..eedc32d122d --- /dev/null +++ b/core/spec/models/spree/shipment_discount_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::ShipmentDiscount do + subject(:shipment_discount) { build(:shipment_discount) } + + it { is_expected.to respond_to(:shipment) } + it { is_expected.to respond_to(:promotion_action) } + it { is_expected.to respond_to(:amount) } + it { is_expected.to respond_to(:label) } +end diff --git a/core/spec/spec_helper.rb b/core/spec/spec_helper.rb index 803b5deb1be..0bcb1b77a5c 100644 --- a/core/spec/spec_helper.rb +++ b/core/spec/spec_helper.rb @@ -8,6 +8,7 @@ require 'rspec/core' require 'spree/testing_support/partial_double_verification' +require 'spree/testing_support/deprecated_examples' require 'spree/testing_support/preferences' require 'spree/config' require 'with_model' diff --git a/guides/source/developers/promotions/overview.html.md b/guides/source/developers/promotions/overview.html.md index 06740935e71..45c8279c559 100644 --- a/guides/source/developers/promotions/overview.html.md +++ b/guides/source/developers/promotions/overview.html.md @@ -49,6 +49,8 @@ Take note of the following promotion attributes: - `match_policy`: When set to `all`, all promotion rules must be met in order for the promotion to be eligible. When set to `any`, just one of the [promotion rules](#promotion-rules) must be met. + Using "any" is deprecated and will be removed in future Solidus versions. + You can create separate promotions for each rule you have in mind instead. - `path`: If the promotion is activated when the customer visits a URL, this value is the path for the URL. - `per_code_usage_limit`: Specifies how many times each code can be used before @@ -86,8 +88,13 @@ By default, `Spree::Promotion`s have a `match_policy` value of `all`, meaning that all of the promotion rules on a promotion must be met before the promotion is eligible. However, this can be changed to `any`. +Using "any" is deprecated and will be removed in future Solidus versions. +You can create separate promotions for each rule you have in mind instead. + An example of a typical promotion rule would be a minimum order total of $75 -USD or that a specific product is in the cart at checkout. +USD or that a specific product is in the cart at checkout. In this case, +create one promotion with the order total rule, and a separate one with the +product rule. For a list of available rule types and more information, see the [Promotion rules][promotion-rules] article. diff --git a/guides/source/developers/promotions/promotion-rules.html.md b/guides/source/developers/promotions/promotion-rules.html.md index 6c3221a955b..e467e67f706 100644 --- a/guides/source/developers/promotions/promotion-rules.html.md +++ b/guides/source/developers/promotions/promotion-rules.html.md @@ -21,9 +21,14 @@ model][promotion-rules]: - `ItemTotal`: Eligible if the order total (before any adjustments) is less than or greater than a specified amount. - `OneUsePerUser`: Eligible for use one time for each user. -- `Product`: Eligible for specified products only. -- `OptionValue`: Eligible for specified variants (product option values) only. -- `Taxon`: Eligible for products with specified taxons. +- `Product`: Order is eligible if it contains specified products. +- `LineItemProduct`: Line Item is eligible if it has one of the specified products. +- `OptionValue`: Order is eligible if it contains specified variants + (product option values) only. +- `LineItemOptionValue`: Line Item is eligible if it has one of the + specified variants (product option values). +- `Taxon`: Order is eligible if any product has the specified taxons. +- `LineItemTaxon`: Line item is eligible if item's product has the specified taxons. - `User`: Eligible for specified users. - `UserRole`: Eligible for users with the specified user role. - `UserLoggedIn`: Eligible for users who are logged in. @@ -54,6 +59,9 @@ Administrators can change the match policy when adding or editing a promotion. By default, promotions use the "Match all of these rules" setting, but they can be changed to use "Match any of these rules". +Using "any" is deprecated and will be removed in future Solidus versions. +You can create separate promotions for each rule you have in mind instead. + ## Register a custom promotion rule You can create a custom promotion rule by creating a new class that inherits @@ -72,10 +80,6 @@ module Spree def eligible?(order, options = {}) ... end - - def actionable?(line_item) - ... - end ... ``` @@ -84,14 +88,16 @@ Note that the `applicable?` and `eligible?` are required: - `eligible?` should return `true` or `false` to indicate if the promotion is eligible for an order. - If your promotion supports discounts for some line items but not others, - define `actionable?` to return `true` when the specified line item meets the - criteria for the promotion. It should return `true` or `false` to indicate if - this line item can have a line item adjustment carried out on it. + define a separate promotion rule that is `applicable?` for line items: + + ```ruby + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + ``` + + This rule should contain the line item eligibility logic in its `eligible?` method. -For example, if you are giving a promotion on specific products only, -`eligible?` should return true if the order contains one of the products -eligible for promotion, and `actionable?` should return true when the line item -specified is one of the specific products for this promotion. Note that you can retrieve the associated `Spree::Promotion` information by calling the `promotion` method. @@ -122,7 +128,7 @@ en: models: # The presentation name of the promotion rule spree/promotion/rules/my_promotion_rule: My Promotion Rule - + # If you used a custom error message spree: eligibility_errors: @@ -131,4 +137,3 @@ en: ``` After a server restart, the new rule will be available from the Solidus admin promotion interface. - diff --git a/guides/source/users/promotions/promotion-rules.html.md b/guides/source/users/promotions/promotion-rules.html.md index 19020aa4ae2..4398cc43574 100644 --- a/guides/source/users/promotions/promotion-rules.html.md +++ b/guides/source/users/promotions/promotion-rules.html.md @@ -18,10 +18,17 @@ creating one. - **Item Total**: Eligible if the order total (before any adjustments) is less than or greater than a specified amount. - **One Use Per User**: Eligible for use one time per user. -- **Product(s)**: Eligible for specified products only. -- **Option Value(s)**: Eligible for specified variants (product option values) - only. -- **Taxon**: Eligible for products with specified taxons. +- **Order Product(s)**: Order is eligible if it has (or does not have) + the specified products. +- **Line Item Product(s)**: Line Items are eligible if they have (or do not have) + the specified products. +- **Order Option Value(s)**: Order is eligible if it has specified variants + (product option values) only. +- **Line Item Option Value(s)**: Line Items are eligible if they have specified + variants (product option values) only. +- **Order Taxon**: Order is eligible if products with specified taxons are in the order. +- **Line Item Taxon**: Line Items are eligible if they have products with specified + taxons. - **User**: Eligible for specified users. - **User Role**: Eligible for users with the specified user role. - **User Logged In**: Eligible for users who are logged in. @@ -36,6 +43,7 @@ Every time the promotion rules are re-checked, any promotional discounts are recalculated as well. For example, if the customer added more items to the cart, those new items could now be calculated against the promotion rules. +Note that line-item level rules do not apply to order-level discounts. ## Rule matching There are two types of rule matching in Solidus' promotion system: **Match all @@ -48,4 +56,3 @@ of these rules** or **Match any of these rules**. - **Match any of these rules**: This setting allows you to create more flexible promotions. For example, if you wanted to give a discount to customers who order your Tote Bag product *or* if it's their 5th order from your store. -