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

Make the default pricer country-aware #1125

Merged
merged 24 commits into from Jun 3, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ca59cfe
Add country_iso relation to prices
mamhoff May 9, 2016
02d2e69
Add reverse association Country#prices
mamhoff May 9, 2016
223b3ed
Add Spree::Tax::TaxLocation.country
mamhoff May 13, 2016
272a186
Add country_iso attribute to default pricing options
mamhoff May 9, 2016
ec7683c
Add country iso to pricing options factory .from_line_item
mamhoff May 9, 2016
2f10a22
Add country_iso to pricing options factory .from_price
mamhoff May 9, 2016
5343b53
Remove Default Rate Validator
mamhoff May 10, 2016
e04b076
Add Spree::Price#net_amount
mamhoff May 10, 2016
ca8324b
Migrate prices for default VAT zone to country-specific prices
mamhoff May 10, 2016
99815f1
Add specs for not creating existing prices
mamhoff May 18, 2016
1237156
Create all necessary prices for a variant before validation
mamhoff May 18, 2016
8266182
Add upgrade task for country-dependent prices
mamhoff May 18, 2016
c2aa4b3
Add Comments to PriceMigrator
mamhoff May 18, 2016
203c5dc
Add comments to PriceGenerator
mamhoff May 18, 2016
513d8a9
Add comments for Variant pricer
mamhoff May 18, 2016
f76ee2f
Add comments for Variant pricing options
mamhoff May 18, 2016
12a9d01
Add a for_country scope to TaxRate
mamhoff May 18, 2016
ec72bca
Variant: Move price setters to before_validation
mamhoff May 18, 2016
7b7a819
Add country fields to admin pricing interface
mamhoff May 23, 2016
192a056
Rename PriceGenerator to VatPriceGenerator
mamhoff May 23, 2016
26e751d
Improve UX on the prices overview page
mamhoff May 24, 2016
87d40b9
Only run the VatPriceGenerator when absolutely necessary
mamhoff May 24, 2016
2b16884
Allow setting a product's tax category on creation
mamhoff May 24, 2016
9af171f
Remove n+1 price query from product lists
mamhoff May 25, 2016
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
8 changes: 4 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
object. For now, this object (`Spree::Variant::PricingOptions`) only holds the
currency. It is used for caching instead of the deprecated `current_currency` helper.

Additionally, your pricing can be customized using a `VariantPricer` object, a default
implementation of which can be found in `Spree::Variant::Pricer`. It is responsible for
Additionally, your pricing can be customized using a `VariantPriceSelector` object, a default
implementation of which can be found in `Spree::Variant::PriceSelector`. It is responsible for
finding the right price for variant, be it for front-end display or for adding it to the
cart. You can set it through the new `Spree::Config.variant_pricer_class` setting. This
cart. You can set it through the new `Spree::Config.variant_price_selector_class` setting. This
class also knows which `PricingOptions` class it cooperates with.

#### Deprecated methods:
Expand All @@ -32,7 +32,7 @@
`Spree::Variant` to be patched with methods like `Spree::Variant#gift_wrap_price_modifier_in`
and is generally deemed a non-preferred way of modifying pricing.
This functionality has now been moved into a [Gem of its own](https://github.com/solidusio-contrib/solidus_price_modifier)
to ease the transition to the new `Variant::Pricer` system.
to ease the transition to the new `Variant::PriceSelector` system.

* Respect `Spree::Store#default_currency`

Expand Down
5 changes: 4 additions & 1 deletion backend/app/controllers/spree/admin/prices_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ def index
params[:q] ||= {}

@search = @product.prices.accessible_by(current_ability, :index).ransack(params[:q])
@prices = @search.result.page(params[:page]).per(10)
@prices = @search.result
.currently_valid
.order(:variant_id, :country_iso, :currency)
.page(params[:page]).per(Spree::Config.admin_variants_per_page)
end

def edit
Expand Down
4 changes: 4 additions & 0 deletions backend/app/helpers/spree/admin/products_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ def option_types_options_for(product)
end
safe_join(options)
end

def show_rebuild_vat_checkbox?
Spree::TaxRate.included_in_price.exists?
end
end
end
end
18 changes: 15 additions & 3 deletions backend/app/views/spree/admin/prices/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<% end %>
</div>

<div class="four columns alpha" data-hook="admin_product_price_form_currency">
<div class="three columns alpha" data-hook="admin_product_price_form_currency">
<%= f.field_container :currency do %>
<%= f.label :currency %>
<%= f.select :currency,
Expand All @@ -21,14 +21,26 @@
<% end %>
</div>

<div class="four columns alpha" data-hook="admin_product_price_form_amount">
<div class="three columns" data-hook="admin_product_price_form_country">
<%= f.field_container :country do %>
<%= f.label :country %>
<%= f.collection_select :country_iso, available_countries, :iso, :name,
{
include_blank: t(:any_country, scope: [:spree, :admin, :prices]),
selected: Spree::Config.admin_vat_country_iso
},
{ class: 'select2 fullwidth', disabled: !f.object.new_record? } %>
<% end %>
</div>

<div class="three columns" data-hook="admin_product_price_form_amount">
<%= f.field_container :price do %>
<%= f.label :price %>
<%= f.text_field :price, value: f.object.money.format, class: 'fullwidth title' %>
<% end %>
</div>

<div class="field four columns checkbox omega" data-hook="is_default_price">
<div class="field three columns checkbox omega" data-hook="is_default_price">
<%= f.label :is_default do %>
<%= f.check_box :is_default %>
<%= Spree::Price.human_attribute_name(:is_default) %>
Expand Down
20 changes: 6 additions & 14 deletions backend/app/views/spree/admin/prices/_table.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,20 @@
<thead data-hook="prices_header">
<tr>
<th><%= Spree::Variant.model_name.human %> </th>
<th><%= Spree::Price.human_attribute_name(:is_default) %></th>
<th><%= Spree::Price.human_attribute_name(:amount) %></th>
<th><%= Spree::Price.human_attribute_name(:country) %></th>
<th><%= Spree::Price.human_attribute_name(:currency) %></th>
<th><%= Spree::Price.human_attribute_name(:amount) %></th>
<th class="actions"></th>
</tr>
</thead>

<tbody>
<% prices.each do |price| %>
<tr id="<%= spree_dom_id price %>" data-hook="prices_row" class="<%= "deleted" if price.deleted? %> <%= cycle('odd', 'even')%>">
<td class="align-center">
<%= price.variant.descriptive_name %>
</td>
<td class="align-center">
<% if price.is_default? %>
<span class="state complete"> <%= Spree.t(:say_yes) %> </span>
<% else %>
<span class="state canceled"> <%= Spree.t(:say_no) %> </span>
<% end %>
</td>
<td class="align-center"><%= price.money.to_html %></td>
<td class="align-center"><%= price.currency %></td>
<td><%= price.variant.descriptive_name %></td>
<td><%= price.display_country %>
<td><%= price.currency %></td>
<td class="align-right"><%= price.money.to_html %></td>
<td class="actions">
<% if can?(:update, price) %>
<%= link_to_edit(price, :no_text => true) unless price.deleted? %>
Expand Down
22 changes: 16 additions & 6 deletions backend/app/views/spree/admin/prices/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
</div>
</div>

<div class="four columns">
<div class="field" data-hook="sku-select">
<div class="three columns">
<div class="field" data-hook="currency-select">
<%= label_tag :q_currency_eq, Spree::Price.human_attribute_name(:currency) %>
<%= f.select :currency_eq,
@prices.map(&:currency).uniq,
Expand All @@ -34,15 +34,25 @@
</div>
</div>

<div class="four columns">
<div class="field" data-hook="sku-select">
<div class="three columns">
<div class="field" data-hook="country-select">
<%= label_tag :q_country_iso_eq, Spree::Price.human_attribute_name(:country) %>
<%= f.select :country_iso_eq,
@prices.map(&:country).compact.uniq.map { |c| [c.name, c.iso]},
{include_blank: true},
class: "select2 fullwidth" %>
</div>
</div>

<div class="three columns">
<div class="field">
<%= label_tag :q_amount_gt, t(".amount_greater_than") %>
<%= f.text_field :amount_gt %>
</div>
</div>

<div class="omega four columns">
<div class="field" data-hook="sku-select">
<div class="omega three columns">
<div class="field">
<%= label_tag :q_amount_lt, t(".amount_less_than") %>
<%= f.text_field :amount_lt %>
</div>
Expand Down
17 changes: 11 additions & 6 deletions backend/app/views/spree/admin/products/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@
</div>

<div class="right four columns omega" data-hook="admin_product_form_right">
<div data-hook="admin_product_form_price">
<%= f.field_container :price do %>
<%= f.label :price, class: 'required' %>
<%= f.text_field :price, :value => number_to_currency(@product.price, :unit => ''), :required => true %>
<%= f.error_message_on :price %>
<% end %>
<div data-hook="admin_product_form_price" class="alpha omega four columns">
<%= f.field_container :price do %>
<%= f.label :price, class: 'required' %>
<%= f.text_field :price, :value => number_to_currency(@product.price, :unit => ''), :required => true %>
<%= f.error_message_on :price %>
<% end %>
</div>

<% if show_rebuild_vat_checkbox? %>
<%= render "spree/admin/shared/rebuild_vat_prices_checkbox", form: f, model_name: "product", wrapper_class: "alpha omega field four columns" %>
<div class="clearfix"></div>
<% end %>

<div data-hook="admin_product_form_cost_price" class="alpha two columns">
<%= f.field_container :cost_price do %>
<%= f.label :cost_price %>
Expand Down
10 changes: 9 additions & 1 deletion backend/app/views/spree/admin/products/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,22 @@
</div>

<div class='row'>
<div data-hook="new_product_shipping_category" class="alpha four columns">
<div data-hook="new_product_shipping_category" class="alpha field four columns">
<%= f.field_container :shipping_category do %>
<%= f.label :shipping_category_id, Spree::ShippingCategory.
model_name.human, class: 'required' %><br />
<%= f.collection_select(:shipping_category_id, @shipping_categories, :id, :name, { :include_blank => Spree.t('match_choices.none') }, { :class => 'select2 fullwidth' }) %>
<%= f.error_message_on :shipping_category_id %>
<% end %>
</div>

<div data-hook="new_product_tax_category" class="field four columns">
<%= f.field_container :tax_category do %>
<%= f.label :tax_category_id, Spree::TaxCategory.model_name.human %>
<%= f.collection_select(:tax_category_id, @tax_categories, :id, :name, { :include_blank => Spree.t('match_choices.none') }, { :class => 'select2 fullwidth' }) %>
<%= f.error_message_on :tax_category %>
<% end %>
</div>
</div>

<div class="clearfix" data-hook="product-from-prototype" id="product-from-prototype">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<div data-hook="admin_<%= model_name %>_form_generate_vat_prices" class="<%= wrapper_class %> checkbox">
<%= form.label :rebuild_vat_prices do %>
<%= form.check_box :rebuild_vat_prices, checked: form.object.prices.size <= 1 %>
<%= Spree::Variant.human_attribute_name(:rebuild_vat_prices) %>
<% end %>
</div>

<script type="text/javascript">
$('#<%= model_name %>_price').on('change', function(e) {
$('#<%= model_name %>_rebuild_vat_prices').prop('checked', true);
});
</script>
4 changes: 4 additions & 0 deletions backend/app/views/spree/admin/variants/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@
<%= f.text_field :price, :value => number_to_currency(@variant.price, :unit => ''), :class => 'fullwidth' %>
</div>

<% if show_rebuild_vat_checkbox? %>
<%= render "spree/admin/shared/rebuild_vat_prices_checkbox", form: f, model_name: "variant", wrapper_class: "field four columns" %>
<% end %>

<div class="field four columns" data-hook="cost_price">
<%= f.label :cost_price %>
<%= f.text_field :cost_price, :value => number_to_currency(@variant.cost_price, :unit => ''), :class => 'fullwidth' %>
Expand Down
7 changes: 5 additions & 2 deletions backend/spec/features/admin/products/pricing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
end

context "in the prices tab" do
let!(:country) { create :country, iso: "DE" }
let(:master_price) { product.master.default_price }
let!(:other_price) { product.master.prices.create(amount: 34.56, currency: "RUB") }
let!(:other_price) { product.master.prices.create(amount: 34.56, currency: "RUB", country_iso: "DE") }

before do
visit spree.admin_product_prices_path(product)
Expand All @@ -35,6 +36,8 @@
expect(page).to have_content("34.56 ₽")
expect(page).to have_content("RUB")
expect(page).to have_content("Master")
expect(page).to have_content("Any Country")
expect(page).to have_content("Germany")
end
end

Expand Down Expand Up @@ -63,7 +66,7 @@
context "deleting", js: true do
let(:product) { create(:product, price: 65.43) }
let!(:variant) { product.master }
let!(:other_price) { product.master.prices.create(amount: 34.56, is_default: false) }
let!(:other_price) { product.master.prices.create(amount: 34.56, currency: "EUR") }

it "will delete the non-default price" do
within "#spree_price_#{other_price.id}" do
Expand Down
18 changes: 9 additions & 9 deletions core/app/models/spree/app_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -281,22 +281,22 @@ def searcher_class
end

# Allows implementing custom pricing for variants
# @!attribute [rw] variant_pricer_class
# @see Spree::Variant::Pricer
# @!attribute [rw] variant_price_selector_class
# @see Spree::Variant::PriceSelector
# @return [Class] an object that conforms to the API of
# the standard variant pricer class Spree::Variant::Pricer.
attr_writer :variant_pricer_class
def variant_pricer_class
@variant_pricer_class ||= Spree::Variant::Pricer
# the standard variant price selector class Spree::Variant::PriceSelector.
attr_writer :variant_price_selector_class
def variant_price_selector_class
@variant_price_selector_class ||= Spree::Variant::PriceSelector
end

# Shortcut for getting the variant pricer's pricing options class
# Shortcut for getting the variant price selector's pricing options class
#
# @return [Class] The pricing options class to be used
delegate :pricing_options_class, to: :variant_pricer_class
delegate :pricing_options_class, to: :variant_price_selector_class

# Shortcut for the default pricing options
# @return [variant_pricer_class] An instance of the pricing options class with default desired
# @return [variant_price_selector_class] An instance of the pricing options class with default desired
# attributes
def default_pricing_options
@default_pricing_options ||= pricing_options_class.new
Expand Down
1 change: 1 addition & 0 deletions core/app/models/spree/country.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module Spree
class Country < Spree::Base
has_many :states, -> { order(:name) }, dependent: :destroy
has_many :addresses, dependent: :nullify
has_many :prices, class_name: "Spree::Price", foreign_key: "country_iso", primary_key: "iso"

validates :name, :iso_name, presence: true

Expand Down
33 changes: 29 additions & 4 deletions core/app/models/spree/price.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,29 @@ class Price < Spree::Base
MAXIMUM_AMOUNT = BigDecimal('99_999_999.99')

belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant', touch: true
has_one :product, class_name: 'Spree::Product', through: :variant
belongs_to :country, class_name: "Spree::Country", foreign_key: "country_iso", primary_key: "iso"

delegate :product, to: :variant
delegate :tax_rates, to: :variant

validate :check_price
validates :amount, allow_nil: true, numericality: {
greater_than_or_equal_to: 0,
less_than_or_equal_to: MAXIMUM_AMOUNT
}

validates :currency, inclusion: { in: ::Money::Currency.all.map(&:iso_code), message: :invalid_code }
validates :country, presence: true, unless: -> { for_any_country? }

scope :currently_valid, -> { where(is_default: true) }
scope :currently_valid, -> { where(is_default: true).order("country_iso IS NULL") }
scope :for_any_country, -> { where(country: nil) }
scope :with_default_attributes, -> { where(Spree::Config.default_pricing_options.desired_attributes) }
after_save :set_default_price

extend DisplayMoney
money_methods :amount, :price
alias_method :money, :display_amount

self.whitelisted_ransackable_attributes = %w( amount variant_id currency )
self.whitelisted_ransackable_attributes = %w( amount variant_id currency country_iso )

# An alias for #amount
def price
Expand All @@ -38,8 +42,29 @@ def price=(price)
self[:amount] = Spree::LocalizedNumber.parse(price)
end

def net_amount
amount / (1 + sum_of_vat_amounts)
end

def for_any_country?
country_iso.nil?
end

def display_country
if country_iso
"#{country_iso} (#{country.name})"
else
I18n.t(:any_country, scope: [:spree, :admin, :prices])
end
end

private

def sum_of_vat_amounts
return 0 unless variant.tax_category
tax_rates.included_in_price.for_country(country).sum(:amount)
end

def check_price
self.currency ||= Spree::Config[:currency]
end
Expand Down
5 changes: 4 additions & 1 deletion core/app/models/spree/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ def find_or_build_master
master || build_master
end

MASTER_ATTRIBUTES = [:sku, :price, :currency, :display_amount, :display_price, :weight, :height, :width, :depth, :cost_currency, :price_in, :price_for, :amount_in, :cost_price]
MASTER_ATTRIBUTES = [
:rebuild_vat_prices, :sku, :price, :currency, :display_amount, :display_price, :weight,
:height, :width, :depth, :cost_currency, :price_in, :price_for, :amount_in, :cost_price
]
MASTER_ATTRIBUTES.each do |attr|
delegate :"#{attr}", :"#{attr}=", to: :find_or_build_master
end
Expand Down
4 changes: 4 additions & 0 deletions core/app/models/spree/tax/tax_location.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def ==(other)
state_id == other.state_id && country_id == other.country_id
end

def country
Spree::Country.find_by(id: country_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider memoizing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do it. The Spree::Tax::TaxLocation is usually a pretty throwaway object. The big reason why we need this method is that countries are referenced by ID rather that ISO code (in which case I could just use that and not use a DB lookup).

Also: This query can return nil, in which case memoizing won't help.

end

def empty?
country_id.nil? && state_id.nil?
end
Expand Down
Loading