Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Revision of 4c4daa1, to remove the change to the shipping

calculator interface (instead, creates a dummy shipment just before doing the calcs)

Fix for 463. Revised the checkout process so that creditcard info is held entirely in memory and only persisted (with number/cvv blanked) when the order succeeds; also clarified the get/post behaviour

Highlights:

* logic for GET/POST/PUT cleaned up, drawing on Ethan Rowe's useful comments in
  http://groups.google.com/group/spree-user/browse_thread/thread/1500ac8d7d7b2ec3

* creditcard info is stored entirely as an object attribute and not persisted. It is
  only saved by after being connected to a payment - which is done by authorize()
  when the request succeeds.

* creditcards association removed from order side - need to go through payments now
  (this is also in preparation for generalising payments to non-cc forms)

* the (single) shipment is only created when the order is completed
  (there's no good reason to have it before, and it did complicate things)

* interface to shipping calculator IS UNCHANGED

* order.save! now used - silent failure not really an option

* relevant admin code, checkout views, and javascript code updated correspondingly
  • Loading branch information...
commit d777c031a1df9ffd526620d4a30ff54ccc1a1295 1 parent 0b0a258
Paul Callaghan paulcc authored schof committed
2  app/controllers/admin/orders_controller.rb
@@ -43,7 +43,7 @@ def collection
43 43 #set results per page to default or form result
44 44 @search.per_page = Spree::Config[:orders_per_page]
45 45
46   - @collection = @search.find(:all, :include => [:user, :shipments, {:creditcards => :address}] )
  46 + @collection = @search.find(:all, :include => [:user, :shipments, {:creditcard_payments => {:creditcard => :address}}] )
47 47 end
48 48
49 49 # Allows extensions to add new forms of payment to provide their own display of transactions
10 app/controllers/orders_controller.rb
@@ -91,9 +91,11 @@ def load_data
91 91 end
92 92
93 93 def rate_hash
94   - shipment = @order.shipments.last
95   - @order.shipping_methods.collect { |ship_method| {:id => ship_method.id,
96   - :name => ship_method.name,
97   - :rate => number_to_currency(ship_method.calculate_shipping(shipment)) } }
  94 + fake_shipment = Shipment.new :order => @order, :address => @order.ship_address
  95 + @order.shipping_methods.collect do |ship_method|
  96 + { :id => ship_method.id,
  97 + :name => ship_method.name,
  98 + :rate => number_to_currency(ship_method.calculate_shipping(fake_shipment)) }
  99 + end
98 100 end
99 101 end
2  app/models/creditcard.rb
... ... @@ -1,5 +1,5 @@
1 1 class Creditcard < ActiveRecord::Base
2   - before_create :filter_sensitive
  2 + before_save :filter_sensitive
3 3 belongs_to :order
4 4 belongs_to :address
5 5 has_many :creditcard_payments
13 app/models/order.rb
@@ -8,12 +8,10 @@ class Order < ActiveRecord::Base
8 8 has_many :state_events
9 9 has_many :payments
10 10 has_many :creditcard_payments
11   - has_many :creditcards
12 11 belongs_to :user
13 12 has_many :shipments, :dependent => :destroy
14 13 belongs_to :bill_address, :foreign_key => "bill_address_id", :class_name => "Address"
15 14 belongs_to :ship_address, :foreign_key => "ship_address_id", :class_name => "Address"
16   - accepts_nested_attributes_for :creditcards, :reject_if => proc { |attributes| attributes['number'].blank? }
17 15 accepts_nested_attributes_for :ship_address, :bill_address
18 16
19 17 validates_associated :line_items, :message => "are not valid"
@@ -31,7 +29,13 @@ class Order < ActiveRecord::Base
31 29
32 30 # attr_accessible is a nightmare with attachment_fu, so use attr_protected instead.
33 31 attr_protected :ship_amount, :tax_amount, :item_total, :total, :user, :number, :ip_address, :checkout_complete, :state, :token
34   -
  32 +
  33 + # for memory-only storage of creditcard details
  34 + attr_accessor :creditcard
  35 +
  36 + # for storage of shipping method before it is saved in a shipment
  37 + attr_accessor :initial_shipping_method
  38 +
35 39 def to_param
36 40 self.number if self.number
37 41 generate_order_number unless self.number
@@ -181,10 +185,11 @@ def update_totals
181 185
182 186 private
183 187 def complete_order
  188 + shipments.build(:address => ship_address, :shipping_method => initial_shipping_method)
184 189 self.update_attribute(:checkout_complete, true)
185 190 InventoryUnit.sell_units(self)
186 191 update_totals
187   - save_result = save
  192 + save_result = save!
188 193 if user && user.email
189 194 OrderMailer.deliver_confirm(self)
190 195 end
4 app/views/admin/orders/index.html.erb
@@ -63,8 +63,8 @@
63 63 <%= user.text_field :email, :size=>25 %>
64 64 </p>
65 65 <% end %>
66   - <% orders.fields_for orders.object.creditcards do |cc| %>
67   - <% cc.fields_for cc.object.address do |address| %>
  66 + <% orders.fields_for orders.object.creditcard_payments do |cc| %>
  67 + <% cc.fields_for cc.object.creditcard.address do |address| %>
68 68 <p>
69 69 <label><%= t("first_name") %></label><br />
70 70 <%= address.text_field :lower_of_firstname_contains, :size=>25 %>
10 app/views/orders/_payment.html.erb
... ... @@ -1,4 +1,4 @@
1   -<% order_form.fields_for :creditcards do |creditcard_form| %>
  1 +<% order_form.fields_for :creditcard do |creditcard_form| %>
2 2 <h2><%= t("payment_information") %></h2>
3 3 <div class="inner">
4 4 <p>
@@ -20,13 +20,13 @@
20 20 <%= creditcard_form.text_field :issue_number, {:style => 'width:40px', :title => t('enter_exactly_as_shown_on_card') + ', e.g. 2, 01.' } -%>
21 21 &nbsp;<strong>OR</strong>&nbsp;
22 22 <%= t('start_date') %>
23   - <%= select_month(@date, :prefix => 'order[creditcards_attributes][0]', :field_name => 'start_month', :use_month_numbers => true, :include_blank => true) -%>
24   - <%= select_year(@date, :prefix => 'order[creditcards_attributes][0]', :field_name => 'start_year', :start_year => Date.today.year - 15, :end_year => Date.today.year, :include_blank => true) -%>
  23 + <%= select_month(@date, :prefix => 'order[creditcard]', :field_name => 'start_month', :use_month_numbers => true, :include_blank => true) -%>
  24 + <%= select_year(@date, :prefix => 'order[creditcard]', :field_name => 'start_year', :start_year => Date.today.year - 15, :end_year => Date.today.year, :include_blank => true) -%>
25 25 </p>
26 26 <p>
27 27 <label for=""><%= t("expiration") %></label>
28   - <%= select_month(Date.today, :prefix => 'order[creditcards_attributes][0]', :field_name => 'month', :use_month_numbers => true, :class => 'required') -%>
29   - <%= select_year(Date.today, :prefix => 'order[creditcards_attributes][0]', :field_name => 'year', :start_year => Date.today.year, :end_year => Date.today.year + 15, :class => 'required') -%>
  28 + <%= select_month(Date.today, :prefix => 'order[creditcard]', :field_name => 'month', :use_month_numbers => true, :class => 'required') -%>
  29 + <%= select_year(Date.today, :prefix => 'order[creditcard]', :field_name => 'year', :start_year => Date.today.year, :end_year => Date.today.year + 15, :class => 'required') -%>
30 30 <span class="req">*</span>
31 31 </p>
32 32 <p>
53 lib/spree/checkout.rb
@@ -8,34 +8,35 @@ def checkout
8 8 load_data
9 9 load_checkout_steps
10 10
11   - # push the current record ids into the incoming params to allow nested_attribs to do update-in-place
12   - # assumes that if a record is set, it was set from the nested hash and so update will work
13   - if params[:order]
14   - params[:order][:bill_address_attributes][:id] = @order.bill_address.id if @order.bill_address
15   - params[:order][:ship_address_attributes][:id] = @order.ship_address.id if @order.ship_address
  11 + if request.get? # default values needed for GET / first pass
  12 + @order.bill_address ||= Address.new(:country => @default_country)
  13 + @order.ship_address ||= Address.new(:country => @default_country)
  14 +
  15 + if @order.creditcard.nil?
  16 + @order.creditcard = Creditcard.new(:month => Date.today.month, :year => Date.today.year)
  17 + end
16 18 end
17 19
18   - # and now do the over-write, saving any new changes as we go
19   - @order.update_attributes(params[:order])
  20 + unless request.get? # the proper processing
  21 + @order.initial_shipping_method = ShippingMethod.find_by_id(params[:method_id]) if params[:method_id]
  22 + @method_id = params[:method_id]
20 23
21   - # default values needed for GET / first pass
22   - @order.bill_address ||= Address.new(:country => @default_country)
23   - @order.ship_address ||= Address.new(:country => @default_country)
24   - if @order.creditcards.empty?
25   - @order.creditcards.build(:month => Date.today.month, :year => Date.today.year)
26   - end
27   - @shipping_method = ShippingMethod.find_by_id(params[:method_id]) if params[:method_id]
28   - @shipping_method ||= @order.shipping_methods.first
29   - @order.shipments.build(:address => @order.ship_address, :shipping_method => @shipping_method) if @order.shipments.empty?
  24 + # push the current record ids into the incoming params to allow nested_attribs to do update-in-place
  25 + if @order.bill_address && params[:order][:bill_address_attributes]
  26 + params[:order][:bill_address_attributes][:id] = @order.bill_address.id
  27 + end
  28 + if @order.ship_address && params[:order][:ship_address_attributes]
  29 + params[:order][:ship_address_attributes][:id] = @order.ship_address.id
  30 + end
30 31
31   - if request.post?
32   - # @order.creditcards.clear
33   - # @order.attributes = params[:order]
34   - @order.creditcards[0].address = @order.bill_address if @order.creditcards.present?
  32 + # and now do the over-write, saving any new changes as we go
  33 + @order.update_attributes(params[:order])
  34 +
  35 + # set some derived information
35 36 @order.user = current_user
36   - @order.ip_address = request.env['REMOTE_ADDR']
37   - @order.update_totals
38 37 @order.email = current_user.email if @order.email.blank? && current_user
  38 + @order.ip_address = request.env['REMOTE_ADDR']
  39 + @order.update_totals
39 40
40 41 begin
41 42 # need to check valid b/c we dump the creditcard info while saving
@@ -43,9 +44,13 @@ def checkout
43 44 if params[:final_answer].blank?
44 45 @order.save
45 46 else
46   - @order.creditcards[0].authorize(@order.total)
  47 + # now fetch the CC info and do the authorization
  48 + @order.creditcard = Creditcard.new params[:order][:creditcard]
  49 + @order.creditcard.address = @order.bill_address
  50 + @order.creditcard.order = @order;
  51 + @order.creditcard.authorize(@order.total)
  52 +
47 53 @order.complete
48   - # remove the order from the session
49 54 session[:order_id] = nil
50 55 end
51 56 else
6 public/javascripts/checkout.js
@@ -4,7 +4,7 @@ $(function() {
4 4 $('span#bcountry select').change(function() { update_state('b'); });
5 5 $('span#scountry select').change(function() { update_state('s'); });
6 6 get_states();
7   - $('input#order_creditcards_attributes_0_number').blur(set_card_validation);
  7 + $('input#order_creditcard_number').blur(set_card_validation);
8 8
9 9 // hook up the continue buttons for each section
10 10 for(var i=0; i < regions.length; i++) {
@@ -451,11 +451,11 @@ var card_type = function(number) {
451 451 };
452 452
453 453 var set_card_validation = function () {
454   - if ($("#order_creditcards_attributes_0_number").val().match(/^\s*$/)) {
  454 + if ($("#order_creditcard_number").val().match(/^\s*$/)) {
455 455 $('#card_type').hide();
456 456 return;
457 457 }
458   - current_card_type = card_type($("#order_creditcards_attributes_0_number").val());
  458 + current_card_type = card_type($("#order_creditcard_number").val());
459 459 $('#card_type').show();
460 460 $('#card_type #looks_like').hide();
461 461 $('#card_type #unrecognized').hide();
20 spec/models/shipping_method_spec.rb
... ... @@ -1,16 +1,16 @@
1 1 require File.dirname(__FILE__) + '/../spec_helper'
2 2
3 3 class MockCalculator
4   - def available?(order)
  4 + def available?(shipment)
5 5 true
6 6 end
7   - def calculate_shipping(order)
  7 + def calculate_shipping(shipment)
8 8 2.5
9 9 end
10 10 end
11 11
12 12 class MockUnavailableCalculator
13   - def available?(order)
  13 + def available?(shipment)
14 14 false
15 15 end
16 16 end
@@ -22,23 +22,23 @@ def available?(order)
22 22 @zone = mock_model(Zone)
23 23 @address = mock_model(Address)
24 24 @shipping_method = ShippingMethod.new(:zone => @zone, :shipping_calculator => "MockCalculator")
25   - @order = mock_model(Order, :address => @address)
  25 + @shipment = mock_model(Shipment, :address => @address)
26 26 end
27 27
28 28 describe "available?" do
29 29 it "should return true if the calculator indicates method is supported" do
30   - @shipping_method.available?(@order).should be_true
  30 + @shipping_method.available?(@shipment).should be_true
31 31 end
32 32 it "should return false if the calculator indicates method is not supported" do
33 33 @shipping_method = ShippingMethod.new(:zone => @zone, :shipping_calculator => "MockUnavailableCalculator")
34   - @shipping_method.available?(@order).should be_false
  34 + @shipping_method.available?(@shipment).should be_false
35 35 end
36 36 end
37 37
38 38 describe "calculate_shipping" do
39 39 it "should be 0 if the shipping address does not fall within the method's zone" do
40 40 @zone.stub!(:include?).with(@address).and_return(false)
41   - @shipping_method.calculate_shipping(@order).should == 0
  41 + @shipping_method.calculate_shipping(@shipment).should == 0
42 42 end
43 43 describe "when the shipping address is included within the method's zone" do
44 44 before :each do
@@ -48,11 +48,11 @@ def available?(order)
48 48 it "should use the calculate_shipping method of the specified calculator" do
49 49 @calculator = MockCalculator.new
50 50 MockCalculator.stub!(:new, :return => @calculator)
51   - @calculator.should_receive(:calculate_shipping).with(@order)
52   - @shipping_method.calculate_shipping(@order)
  51 + @calculator.should_receive(:calculate_shipping).with(@shipment)
  52 + @shipping_method.calculate_shipping(@shipment)
53 53 end
54 54 it "should return the correct amount" do
55   - @shipping_method.calculate_shipping(@order).should == 2.5
  55 + @shipping_method.calculate_shipping(@shipment).should == 2.5
56 56 end
57 57 end
58 58 end
2  test/functional/orders_controller_test.rb
@@ -23,4 +23,4 @@ class OrdersControllerTest < ActionController::TestCase
23 23 should_change "Address.count", :by => 2
24 24 end
25 25 end
26   -end
  26 +end

0 comments on commit d777c03

Please sign in to comment.
Something went wrong with that request. Please try again.