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

Discount system #4296

Closed
wants to merge 38 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
dd6d245
Add LineItemProduct Rule
mamhoff Mar 27, 2022
44920c9
Rename Product rule
mamhoff Mar 30, 2022
e0ad236
Use "Order Product(s)" rule in promotion admin feature spec
mamhoff Mar 30, 2022
5a828ee
Fix promotion adjustments spec to not use Product rule
mamhoff Mar 30, 2022
0aeaf81
Add promotion integration spec
mamhoff Mar 31, 2022
3f3d202
Prevent ineligible line item adjustments
mamhoff Mar 31, 2022
47f4297
Simplify Promotion Rule Option Value
mamhoff Mar 31, 2022
9192a7d
Add Promotion Rule LineItemOptionValue
mamhoff Mar 31, 2022
908bbbc
Add Promotion Rule "LineItemTaxon"
mamhoff Apr 1, 2022
80dbd5c
Add migration to add necessary line item rules to existing promotions
mamhoff Apr 1, 2022
77b01d2
Remove "actionable?" from Product rule
mamhoff Apr 1, 2022
f50ba52
Option Value rule: Remove #actionable?
mamhoff Apr 1, 2022
53659f1
Remove Rules::Taxon#actionable?
mamhoff Apr 1, 2022
c1ed25e
Deprecate Promotion#line_item_actionable and PromotionRule#actionable?
mamhoff Apr 1, 2022
24509e9
Fix DistributedAmounts spec
mamhoff Apr 1, 2022
7d5863e
Fix CreateItemAdjustments spec
mamhoff Apr 1, 2022
0aaead2
Fix for Promotion#line_item_eligible spec
mamhoff Apr 1, 2022
c142e66
Fix spec for Promotion#line_item_actionable?
mamhoff Apr 1, 2022
884c2e0
User Guides: Update Promotion Rules page
mamhoff Apr 1, 2022
5edffa8
Update Developer Guides for new Promotion API
mamhoff Apr 1, 2022
5b78a43
Deprecate Promotion#match_policy == any
mamhoff Mar 17, 2022
8c0b9b5
Update migrations with unnecessary "any" match_policy
mamhoff Mar 17, 2022
88299ce
Add Changelog entry
mamhoff Mar 17, 2022
127c197
Add RSpec metadata helper "deprecated_examples"
mamhoff Mar 18, 2022
4a61e8c
Deprecate Promotion specs that test "any" match policy
mamhoff Mar 18, 2022
665f38c
Refactor Promotion#usage_count
mamhoff Mar 19, 2022
1d68443
Add line item discounts
mamhoff Mar 14, 2022
5a87382
Add shipment discounts
mamhoff Mar 14, 2022
9622cd7
Introduce Feature Flag for the new promotion system
mamhoff Mar 15, 2022
bc19751
Disable legacy promotion system in order contents
mamhoff Mar 15, 2022
064b274
Add Promotion#activatable?
mamhoff Mar 15, 2022
1164ee0
CreateItemAdjustments: Add #discount
mamhoff Mar 16, 2022
9e4e492
Add FreeShipping#discount
mamhoff Mar 16, 2022
b2f6c71
Introduce Spree::Discounts
mamhoff Mar 15, 2022
f7d78ab
Adapt Spree::Promotion#discounted_orders
mamhoff Mar 18, 2022
b9858de
Fix Spree::Promotion#usage_count specs or new discount system
mamhoff Mar 19, 2022
ad122e4
Add Priority Column to Spree::Promotions
mamhoff Mar 29, 2022
408315d
Add Priority to Promotion Actions
mamhoff Mar 29, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
16 changes: 9 additions & 7 deletions backend/app/views/spree/admin/promotions/_rules.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

<%= form_for @promotion, url: object_url, method: :put do |f| %>
<fieldset class="no-border-top">
<div id="promotion-policy-select" class="align-center row">
<% Spree::Promotion::MATCH_POLICIES.each do |policy| %>
<div class="col-6">
<label><%= f.radio_button :match_policy, policy %> <%= t "spree.promotion_form.match_policies.#{policy}" %></label>
</div>
<% end %>
</div>
<% if Spree::Config.allow_promotions_any_match_policy %>
<div id="promotion-policy-select" class="align-center row">
<% Spree::Promotion::MATCH_POLICIES.each do |policy| %>
<div class="col-6">
<label><%= f.radio_button :match_policy, policy %> <%= t "spree.promotion_form.match_policies.#{policy}" %></label>
</div>
<% end %>
</div>
<% end %>

<div id="rules" class="filter_list">
<% if @promotion.rules.any? %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<div class="field promo-rule-option-values">
<div class="param-prefix hidden" data-param-prefix="<%= param_prefix %>"></div>
<div class="row">
<div class="col-6"><%= label_tag nil, Spree::Product.model_name.human %></div>
<div class="col-6"><%= label_tag nil, plural_resource_name(Spree::OptionValue) %></div>
</div>

<%= content_tag :div, nil, class: "js-promo-rule-option-values",
data: { :'original-option-values' => promotion_rule.preferred_eligible_values } %>

<button class="button js-add-promo-rule-option-value"><%= t("spree.actions.add") %></button>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<div class="row">
<div class="col-12">
<div class="field products_rule_products">
<%= 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" %>
</div>
</div>
<div class="col-12">
<div class="field">
<%= 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"
) %>
</div>
</div>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<div class="field taxons_rule_taxons">
<%= 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" %>
</div>
<div class="field">
<div class="field">
<%= 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",
) %>
</div>
</div>
4 changes: 2 additions & 2 deletions backend/spec/features/admin/promotion_adjustments_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions core/app/models/spree/calculator/distributed_amount.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions core/app/models/spree/discounts/chooser.rb
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions core/app/models/spree/discounts/line_item_discounter.rb
Original file line number Diff line number Diff line change
@@ -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
61 changes: 61 additions & 0 deletions core/app/models/spree/discounts/order_discounter.rb
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions core/app/models/spree/discounts/shipment_discounter.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions core/app/models/spree/line_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 8 additions & 0 deletions core/app/models/spree/line_item_discount.rb
Original file line number Diff line number Diff line change
@@ -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
20 changes: 13 additions & 7 deletions core/app/models/spree/order_contents.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 11 additions & 3 deletions core/app/models/spree/order_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
Loading