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

Improve ShippingRate#display_price with taxes #2306

Merged

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Oct 19, 2017

This removes about 4 unnecessary queries when calling ShippingRate#display_price.

Previously, the shipment->shipping_rates and shipping_rate->taxes associations were missing inverse of, causing ShippingRateTax#label to re-query the shipping_rate, shipment, and order.

The shipping_rate->taxes association was also queried twice in #display_price, once to determine if there are any taxes, and a second time to determine their labels.

Before

irb(main):002:0> rate.display_price
  Spree::Shipment Load (0.2ms)  SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Spree::Order Load (0.3ms)  SELECT  "spree_orders".* FROM "spree_orders" WHERE "spree_orders"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Spree::ShippingRateTax Exists (0.1ms)  SELECT  1 AS one FROM "spree_shipping_rate_taxes" WHERE "spree_shipping_rate_taxes"."shipping_rate_id" = ? LIMIT ?  [["shipping_rate_id", 1], ["LIMIT", 1]]
  Spree::ShippingRateTax Load (0.2ms)  SELECT "spree_shipping_rate_taxes".* FROM "spree_shipping_rate_taxes" WHERE "spree_shipping_rate_taxes"."shipping_rate_id" = ?  [["shipping_rate_id", 1]]
  Spree::TaxRate Load (0.2ms)  SELECT  "spree_tax_rates".* FROM "spree_tax_rates" WHERE "spree_tax_rates"."deleted_at" IS NULL AND "spree_tax_rates"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Spree::ShippingRate Load (0.2ms)  SELECT  "spree_shipping_rates".* FROM "spree_shipping_rates" WHERE "spree_shipping_rates"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Spree::Shipment Load (0.1ms)  SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
  Spree::Order Load (0.1ms)  SELECT  "spree_orders".* FROM "spree_orders" WHERE "spree_orders"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
=> "$5.00 (+ $0.25 North America)"

After

irb(main):002:0> rate.display_price
  Spree::ShippingRateTax Load (0.2ms)  SELECT "spree_shipping_rate_taxes".* FROM "spree_shipping_rate_taxes" WHERE "spree_shipping_rate_taxes"."shipping_rate_id" = ?  [["shipping_rate_id", 1]]
  Spree::TaxRate Load (0.2ms)  SELECT  "spree_tax_rates".* FROM "spree_tax_rates" WHERE "spree_tax_rates"."deleted_at" IS NULL AND "spree_tax_rates"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
=> "$5.00 (+ $0.25 North America)"

We were checking taxes.empty? and if it wasn't we were mapping over the
list of taxes. Instead, we can call taxes.to_a.empty?, if there are no
taxes this is just an empty array and if there are we load them and
tests if there are any with just the one query.
Determining the shipping_rate_tax's currency delegates all the way back
to the order, which was causing unnecessary selects of the
shipping_rate, shipment, and order because the inverse_of couldn't be
determined.
@@ -26,7 +27,7 @@ class ShippingRate < Spree::Base
def display_price
price = display_amount.to_s

return price if taxes.empty? || amount == 0
return price if taxes.to_a.empty? || amount == 0

Choose a reason for hiding this comment

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

Use amount.zero? instead of amount == 0.

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Nice work. I'm always impressed by these seemingly small fixes.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks.

@jhawthorn jhawthorn merged commit ec5eca5 into solidusio:master Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants