Skip to content

Commit

Permalink
Promote Order#all_adjustments to association.
Browse files Browse the repository at this point in the history
  • Loading branch information
mbj authored and Jeff Dutil committed Jun 10, 2015
1 parent d83733d commit eef16ba
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 17 deletions.
2 changes: 1 addition & 1 deletion core/app/models/spree/adjustment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module Spree
class Adjustment < Spree::Base
belongs_to :adjustable, polymorphic: true, touch: true
belongs_to :source, polymorphic: true
belongs_to :order, class_name: "Spree::Order"
belongs_to :order, class_name: 'Spree::Order', inverse_of: :all_adjustments

validates :adjustable, presence: true
validates :order, presence: true
Expand Down
10 changes: 5 additions & 5 deletions core/app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ def generate_number(options = {})
has_many :products, through: :variants
has_many :variants, through: :line_items
has_many :refunds, through: :payments
has_many :all_adjustments,
class_name: 'Spree::Adjustment',
foreign_key: :order_id,
dependent: :destroy,
inverse_of: :order

has_and_belongs_to_many :promotions, join_table: 'spree_orders_promotions'

Expand Down Expand Up @@ -132,11 +137,6 @@ def self.register_line_item_comparison_hook(hook)
self.line_item_comparison_hooks.add(hook)
end

def all_adjustments
Adjustment.where("order_id = :order_id OR (adjustable_id = :order_id AND adjustable_type = 'Spree::Order')",
order_id: self.id)
end

# For compatiblity with Calculator::PriceSack
def amount
line_items.inject(0.0) { |sum, li| sum + li.amount }
Expand Down
70 changes: 70 additions & 0 deletions core/db/migrate/20150515211137_fix_adjustment_order_id.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
class FixAdjustmentOrderId < ActiveRecord::Migration
def change
say 'Populate order_id from adjustable_id where appropriate'
execute(<<-SQL.squish)
UPDATE
spree_adjustments
SET
order_id = adjustable_id
WHERE
adjustable_type = 'Spree::Order'
;
SQL

# Submitter of change does not care about MySQL, as it is not officially supported.
# Still spree officials decided to provide a working code path for MySQL users, hence
# submitter made a AR code path he could validate on PostgreSQL.
#
# Whoever runs a big enough MySQL installation where the AR solution hurts:
# Will have to write a better MySQL specific equivalent.
if Spree::Order.connection.adapter_name.eql?('MySQL')
Spree::Adjustment.where(adjustable_type: 'Spree::LineItem').find_each do |adjustment|
adjustment.update_columns(order_id: Spree::LineItem.find(adjustment.adjustable_id).order_id)
end
else
execute(<<-SQL.squish)
UPDATE
spree_adjustments
SET
order_id =
(SELECT order_id FROM spree_line_items WHERE spree_line_items.id = spree_adjustments.adjustable_id)
WHERE
adjustable_type = 'Spree::LineItem'
SQL
end

say 'Fix schema for spree_adjustments order_id column'
change_table :spree_adjustments do |t|
t.change :order_id, :integer, null: false
end

# Improved schema for postgresql, uncomment if you like it:
#
# # Negated Logical implication.
# #
# # When adjustable_type is 'Spree::Order' (p) the adjustable_id must be order_id (q).
# #
# # When adjustable_type is NOT 'Spree::Order' the adjustable id allowed to be any value (including of order_id in
# # case foreign keys match). XOR does not work here.
# #
# # Postgresql does not have an operator for logical implication. So we need to build the following truth table
# # via AND with OR:
# #
# # p q | CHECK = !(p -> q)
# # -----------
# # t t | t
# # t f | f
# # f t | t
# # f f | t
# #
# # According to de-morgans law the logical implication q -> p is equivalent to !p || q
# #
# execute(<<-SQL.squish)
# ALTER TABLE ONLY spree_adjustments
# ADD CONSTRAINT fk_spree_adjustments FOREIGN KEY (order_id)
# REFERENCES spree_orders(id) ON UPDATE RESTRICT ON DELETE RESTRICT,
# ADD CONSTRAINT check_spree_adjustments_order_id CHECK
# (adjustable_type <> 'Spree::Order' OR order_id = adjustable_id);
# SQL
end
end
12 changes: 1 addition & 11 deletions core/spec/models/spree/order/adjustments_spec.rb
Original file line number Diff line number Diff line change
@@ -1,16 +1,6 @@
require 'spec_helper'

describe Spree::Order, :type => :model do
context "#all_adjustments" do
# Regression test for #4537
it "does not show adjustments from other, non-order adjustables" do
order = Spree::Order.new(:id => 1)
where_sql = order.all_adjustments.where_values.to_s
expect(where_sql).to include("(adjustable_id = 1 AND adjustable_type = 'Spree::Order')")
end
end

# Regression test for #2191
describe Spree::Order do
context "when an order has an adjustment that zeroes the total, but another adjustment for shipping that raises it above zero" do
let!(:persisted_order) { create(:order) }
let!(:line_item) { create(:line_item) }
Expand Down

0 comments on commit eef16ba

Please sign in to comment.