Skip to content

Commit

Permalink
Rework ReturnAuthorization-InventoryUnit relationship
Browse files Browse the repository at this point in the history 
* Create a join table: `ReturnAuthorizationInventoryUnit`
  * Allows inventory units to be part of multiple RMAs (e.g., if an initial RMA was cancelled) while still retaining historical data
  * Migrate existing data into the new table
  * Drop the `inventory_units.return_authorization_id` column
* Start using nested attributes for adding/removing InventoryUnits to ReturnAuthorizations
  * Remove the `API::ReturnAuthorizationsController#add` endpoint (use nested attributes instead)
* Pave the way for exchanges with an `exchange_variant_id` field
* Update the admin interface for `Admin::ReturnAuthorizationsController`:
  * No longer group inventory units by variant.  This will be important for allowing per-unit data, such as for exchanges

Created by @richardnuno and @jordan-brough

Fixes #4907
  • Loading branch information
@jordan-brough
jordan-brough authored and Jeff Dutil committed Jul 3, 2014
1 parent c1d44b1 commit a8b51af
Show file tree
Hide file tree
Showing 15 changed files with 293 additions and 167 deletions.
10 changes: 0 additions & 10 deletions api/app/controllers/spree/api/return_authorizations_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,6 @@ def update
end
end

def add
@return_authorization = order.return_authorizations.accessible_by(current_ability, :update).find(params[:id])
@return_authorization.add_variant params[:variant_id].to_i, params[:quantity].to_i
if @return_authorization.valid?
respond_with @return_authorization, default_template: :show
else
invalid_resource!(@return_authorization)
end
end

def receive
@return_authorization = order.return_authorizations.accessible_by(current_ability, :update).find(params[:id])
if @return_authorization.receive
Expand Down
29 changes: 0 additions & 29 deletions api/spec/controllers/spree/api/return_authorizations_controller_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,35 +105,6 @@ module Spree
json_response.should have_attributes(attributes)
end

it "can add an inventory unit to a return authorization on the order" do
FactoryGirl.create(:return_authorization, :order => order)
return_authorization = order.return_authorizations.first
inventory_unit = return_authorization.returnable_inventory.first
inventory_unit.should be
return_authorization.inventory_units.should be_empty
api_put :add, :id => return_authorization.id, variant_id: inventory_unit.variant.id, quantity: 1
response.status.should == 200
json_response.should have_attributes(attributes)
return_authorization.reload.inventory_units.should_not be_empty
end

it "can mark a return authorization as received on the order with an inventory unit" do
FactoryGirl.create(:new_return_authorization, :order => order, :stock_location_id => order.shipments.first.stock_location.id)
return_authorization = order.return_authorizations.first
return_authorization.state.should == "authorized"

# prep (use a rspec context or a factory instead?)
inventory_unit = return_authorization.returnable_inventory.first
inventory_unit.should be
return_authorization.inventory_units.should be_empty
api_put :add, :id => return_authorization.id, variant_id: inventory_unit.variant.id, quantity: 1
# end prep

api_delete :receive, :id => return_authorization.id
response.status.should == 200
return_authorization.reload.state.should == "received"
end

it "cannot mark a return authorization as received on the order with no inventory units" do
FactoryGirl.create(:new_return_authorization, :order => order)
return_authorization = order.return_authorizations.first
Expand Down
22 changes: 15 additions & 7 deletions backend/app/controllers/spree/admin/return_authorizations_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,28 @@ module Spree
module Admin
class ReturnAuthorizationsController < ResourceController
belongs_to 'spree/order', :find_by => :number

update.after :associate_inventory_units
create.after :associate_inventory_units
before_filter :load_return_authorization_inventory_units, except: [:fire, :destroy, :index]

def fire
@return_authorization.send("#{params[:e]}!")
flash[:success] = Spree.t(:return_authorization_updated)
redirect_to :back
end

protected
def associate_inventory_units
(params[:return_quantity] || []).each { |variant_id, qty| @return_authorization.add_variant(variant_id.to_i, qty.to_i) }
end
private

# To satisfy how nested attributes works we want to create placeholder ReturnAuthorizationInventoryUnits for
# any InventoryUnits that have not already been added to the ReturnAuthorization.
def load_return_authorization_inventory_units
all_inventory_unit_ids = @return_authorization.order.inventory_units.map(&:id)
rma_inventory_unit_ids = @return_authorization.return_authorization_inventory_units.map(&:inventory_unit_id)

new_ids = all_inventory_unit_ids - rma_inventory_unit_ids
new_units = new_ids.map { |new_id| Spree::ReturnAuthorizationInventoryUnit.new(inventory_unit_id: new_id) }

@form_return_authorization_inventory_units = (@return_authorization.return_authorization_inventory_units + new_units).sort_by(&:inventory_unit_id)
end

end
end
end
69 changes: 34 additions & 35 deletions backend/app/views/spree/admin/return_authorizations/_form.html.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,41 @@
<table class="index">
<thead>
<tr data-hook="rma_header">
<th><%= check_box_tag 'select-all' %></th>
<th><%= Spree.t(:product) %></th>
<th><%= Spree.t(:total_per_item) %></th>
<th><%= Spree.t(:quantity_shipped) %></th>
<th><%= Spree.t(:quantity_returned) %></th>
<th><%= Spree.t(:return_quantity) %></th>
<th><%= Spree.t(:state) %></th>
<th><%= Spree.t(:price) %></th>
</tr>
</thead>
<tbody>
<% @return_authorization.order.line_items.each do |line_item| %>
<% next if line_item.inventory_units.shipped.none? %>
<tr id="<%= dom_id(line_item) %>" data-hook="rma_row" class="<%= cycle('odd', 'even')%>">
<td>
<div class="variant-name"><%= line_item.variant.name %></div>
<div class="variant-options"><%= line_item.variant.options_text %></div>
</td>
<td class="align-center"><%= line_item.display_rounded_total_per_item %></td>
<td class="align-center"><%= line_item.inventory_units.shipped.size %></td>
<td class="align-center"><%= line_item.inventory_units.returned.size %></td>
<td class="return_quantity align-center">
<% if @return_authorization.received? %>
<%= @return_authorization.inventory_units.group_by(&:variant)[line_item.variant].try(:size) || 0 %>
<% else %>
<% returning_count = @return_authorization.inventory_units.group_by(&:variant)[line_item.variant].try(:size) || 0 %>
<%= number_field_tag "return_quantity[#{line_item.variant.id}]", returning_count, { :style => 'width:100px;',
:min => 0, :max => line_item.inventory_units.shipped.size, "data-line-item-id" => line_item.id} %>
<%= f.fields_for :return_authorization_inventory_units, @form_return_authorization_inventory_units do |unit_fields| %>
<% return_authorization_inventory_unit = unit_fields.object %>
<% inventory_unit = return_authorization_inventory_unit.inventory_unit %>
<tr>
<td class="align-center" class="inventory-unit-checkbox">
<% if inventory_unit.shipped? %>
<%= unit_fields.hidden_field :inventory_unit_id %>
<%= unit_fields.check_box :_destroy, {checked: return_authorization_inventory_unit.persisted?, class: 'add-item', "data-price" => inventory_unit.line_item.rounded_total_per_item}, '0', '1' %>
<% end %>
</td>
<td>
<div class="variant-name"><%= inventory_unit.variant.name %></div>
<div class="variant-options"><%= inventory_unit.variant.options_text %></div>
</td>
<td class="align-center"><%= inventory_unit.state.humanize %></td>
<td class="align-center"><%= inventory_unit.line_item.display_rounded_total_per_item %></td>
</tr>
<% end %>
</tbody>
</table>

<%= f.field_container :amount do %>
<%= f.label :amount, Spree.t(:amount) %> <span class="required">*</span><br />
<% if @return_authorization.updatable? %>
<% if @return_authorization.received? %>
<%= @return_authorization.display_amount %>
<% else %>
<%= f.text_field :amount, {:style => 'width:80px;'} %> <%= Spree.t(:rma_value) %>: <span id="rma_value">0.00</span>
<%= f.error_message_on :amount %>
<% else %>
<%= @return_authorization.display_amount %>
<% end %>
<% end %>

Expand All @@ -58,23 +54,26 @@
</div>

<script>
var line_item_amounts = {};
<% @return_authorization.order.line_items.each do |line_item| %>
line_item_amounts[<%= line_item.id.to_s %>] = <%= line_item.rounded_total_per_item %>;
<% end %>

$(document).ready(function(){
var rma_amount = 0;
$("td.return_quantity input").on('change', function() {
function updateSuggestedAmount() {
$("span#rma_value").html()
var rma_amount = 0;
$.each($("td.return_quantity input"), function(i, input) {
var line_item_id = $(input).data('line-item-id');
rma_amount += line_item_amounts[line_item_id] * $(input).val();
$.each($("input.add-item:checked"), function(i, checkbox) {
rma_amount += $(checkbox).data("price");
});

if(!isNaN(rma_amount)){
$("span#rma_value").html(rma_amount.toFixed(2));
}
})
}

updateSuggestedAmount();

$("input#select-all").on("change", function() {
$("input.add-item").prop('checked', this.checked);
updateSuggestedAmount();
});

$("input.add-item").on("change", updateSuggestedAmount);
});
</script>
2 changes: 1 addition & 1 deletion backend/app/views/spree/admin/return_authorizations/new.html.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<%= render :partial => 'form', :locals => { :f => f } %>

<div class="form-buttons filter-actions actions" data-hook="buttons">
<%= button Spree.t(:continue), 'arrow-right' %>
<%= button Spree.t(:create), 'ok' %>
<span class="or"><%= Spree.t(:or) %></span>
<%= button_link_to Spree.t('actions.cancel'), admin_order_return_authorizations_url(@order), :icon => 'remove' %>
</div>
Expand Down
76 changes: 73 additions & 3 deletions backend/spec/controllers/spree/admin/return_authorizations_controller_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,79 @@
stub_authorization!

# Regression test for #1370 #3
let!(:order) { create(:order) }
let!(:order) { create(:shipped_order, line_items_count: 3) }
let(:inventory_unit_1) { order.inventory_units.order('id asc')[0] }
let(:inventory_unit_2) { order.inventory_units.order('id asc')[1] }
let(:inventory_unit_3) { order.inventory_units.order('id asc')[2] }

let(:params) do
{
order_id: order.to_param,
return_authorization: {amount: 0.0, reason: ""},
}
end

it "can create a return authorization" do
spree_post :create, :order_id => order.to_param, :return_authorization => { :amount => 0.0, :reason => "" }
response.should be_success
spree_post :create, params
response.should redirect_to spree.admin_order_return_authorizations_path(order)
end

context 'update' do
let(:return_authorization) { create(:return_authorization, order: order) }
let(:params) do
super().merge({
id: return_authorization.to_param,
return_authorization: {amount: 0.0, reason: ""}.merge(inventory_units_params),
})
end

subject { spree_put :update, params }

context "adding an item" do
let(:inventory_units_params) do
{
return_authorization_inventory_units_attributes: {
'0' => {inventory_unit_id: inventory_unit_1.to_param},
}
}
end

context 'without existing items' do
it 'creates a new unit' do
expect { subject }.to change { Spree::ReturnAuthorizationInventoryUnit.count }.by(1)
end
end

context 'with existing items' do
let!(:return_authorization_inventory_unit) {
create(:return_authorization_inventory_unit, return_authorization: return_authorization, inventory_unit: inventory_unit_1)
}

it 'does not create new units' do
expect { subject }.to_not change { Spree::ReturnAuthorizationInventoryUnit.count }
expect(assigns[:return_authorization].errors['return_authorization_inventory_units.inventory_unit']).to eq ["has already been taken"]
end
end
end

context "removing an item" do
let!(:return_authorization_inventory_unit) {
create(:return_authorization_inventory_unit, return_authorization: return_authorization, inventory_unit: inventory_unit_1)
}

let(:inventory_units_params) do
{
return_authorization_inventory_units_attributes: {
'0' => {id: return_authorization_inventory_unit.to_param, _destroy: '1'},
}
}
end

context 'with existing items' do
it 'removes the unit' do
expect { subject }.to change { Spree::ReturnAuthorizationInventoryUnit.count }.by(-1)
end
end
end
end
end
2 changes: 2 additions & 0 deletions core/app/models/spree/inventory_unit.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ class InventoryUnit < Spree::Base
belongs_to :return_authorization, class_name: "Spree::ReturnAuthorization"
belongs_to :line_item, class_name: "Spree::LineItem", inverse_of: :inventory_units

has_many :return_authorization_inventory_units, inverse_of: :inventory_unit

scope :backordered, -> { where state: 'backordered' }
scope :on_hand, -> { where state: 'on_hand' }
scope :shipped, -> { where state: 'shipped' }
Expand Down
51 changes: 10 additions & 41 deletions core/app/models/spree/return_authorization.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@ module Spree
class ReturnAuthorization < Spree::Base
belongs_to :order, class_name: 'Spree::Order'

has_many :inventory_units
has_many :return_authorization_inventory_units, inverse_of: :return_authorization, dependent: :destroy
has_many :inventory_units, through: :return_authorization_inventory_units
has_many :refunds
belongs_to :stock_location
before_create :generate_number
before_validation :force_positive_amount

accepts_nested_attributes_for :return_authorization_inventory_units, allow_destroy: true

validates :order, presence: true
validates :amount, numericality: { greater_than_or_equal_to: 0 }
validate :must_have_shipped_units
validate :must_have_shipped_units, on: :create

state_machine initial: :authorized do
after_transition to: :received, do: :process_return
Expand Down Expand Up @@ -49,36 +52,8 @@ def display_amount
Spree::Money.new(amount, { currency: currency })
end

def add_variant(variant_id, quantity)
order_units = returnable_inventory.group_by(&:variant_id)
returned_units = inventory_units.group_by(&:variant_id)
return false if order_units.empty?

count = 0

if returned_units[variant_id].nil? || returned_units[variant_id].size < quantity
count = returned_units[variant_id].nil? ? 0 : returned_units[variant_id].size

order_units[variant_id].each do |inventory_unit|
next unless inventory_unit.return_authorization.nil? && count < quantity

inventory_unit.return_authorization = self
inventory_unit.save!

count += 1
end
elsif returned_units[variant_id].size > quantity
(returned_units[variant_id].size - quantity).times do |i|
returned_units[variant_id][i].return_authorization_id = nil
returned_units[variant_id][i].save!
end
end

order.authorize_return! if inventory_units.reload.size > 0 && !order.awaiting_return?
end

def returnable_inventory
order.shipped_shipments.collect{|s| s.inventory_units.to_a}.flatten
order.inventory_units.shipped
end

# Used when Adjustment#update! wants to update the related adjustmenrt
Expand Down Expand Up @@ -111,7 +86,9 @@ def process_refund
private

def must_have_shipped_units
errors.add(:order, Spree.t(:has_no_shipped_units)) if order.nil? || !order.shipped_shipments.any?
if order.nil? || order.inventory_units.shipped.none?
errors.add(:order, Spree.t(:has_no_shipped_units))
end
end

def generate_number
Expand All @@ -122,15 +99,7 @@ def generate_number
end

def process_return
inventory_units(include: :variant).each do |iu|
iu.return!

if iu.variant.should_track_inventory?
if stock_item = Spree::StockItem.find_by(variant_id: iu.variant_id, stock_location_id: stock_location_id)
Spree::StockMovement.create!(stock_item_id: stock_item.id, quantity: 1)
end
end
end
return_authorization_inventory_units.includes(:inventory_unit).each(&:receive!)

order.return if inventory_units.all?(&:returned?)
end
Expand Down
Loading

0 comments on commit a8b51af

Please sign in to comment.