Simplify `Variant#default_price` logic by waiting-for-dev · Pull Request #4076 · solidusio/solidus

waiting-for-dev · 2021-05-24T04:58:46Z

Before this work, #default_price was a has_one association between
Variant and Product. As we already have a has_many association
between both models, this redundancy was a source of inconsistencies.
E.g.:

include Spree
Variant.new(price: Price.new) # price delegates to default_price
Variant.prices # => []

From now on, #default_price is a regular method that searches within
#prices taking into account the criteria for a price to be considered
the default one.

We have renamed previous method #find_default_price_or_build to
default_price_or_build. The latter feels less standard according to
Rails conventions, but the intention here is, precisely, to communicate
that this is not a Rails association.

No longer being a Rails association makes it impossible to use the default
ransack conventions to build the sort-by-price link on the products
listing page. For this reason, we added
sort_by_master_default_price_amount_{asc, desc} scopes to the Price
model, which is automatically picked up by ransack. As it's now
explicit, these queries ignore the ORDER BY clauses implicit in the
currently_valid_prices method, but this was also happening in the
query built by ransack from the has_one association:

SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN
"spree_variants" ON "spree_variants"."is_master" = ? AND "spree
_variants"."product_id" = "spree_products"."id" LEFT OUTER JOIN
"spree_prices" ON "spree_prices"."currency" = ? AND "spree_pric
es"."country_iso" IS NULL AND "spree_prices"."variant_id" =
"spree_variants"."id" WHERE "spree_products"."deleted_at" IS NULL O
RDER BY "spree_prices"."amount" ASC, "spree_products"."id" ASC LIMIT ?
OFFSET ?  [["is_master", 1], ["currency", "USD"], ["LIMI  T", 10],
["OFFSET", 0]]

However, the new query doesn't include discarded prices on the result,
but I think it's something desirable:

SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN
"spree_variants" "master" ON "master"."is_master" = ? AND "mast
er"."product_id" = "spree_products"."id" LEFT OUTER JOIN "spree_prices"
"prices" ON "prices"."deleted_at" IS NULL AND "prices".  "variant_id" =
"master"."id" LEFT OUTER JOIN "spree_variants" "masters_spree_products"
ON "masters_spree_products"."is_master"   = ? AND
"masters_spree_products"."product_id" = "spree_products"."id" WHERE
"spree_products"."deleted_at" IS NULL AND "prices".  "currency" = ? AND
"prices"."country_iso" IS NULL AND "prices"."deleted_at" IS NULL ORDER
BY prices.amount DESC, "spree_product  s"."id" ASC LIMIT ? OFFSET ?
[["is_master", 1], ["is_master", 1], ["currency", "USD"], ["LIMIT", 10],
["OFFSET", 0]]

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
[] I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

Note: Extracted from #3994

kennyadsl

Left some questions, thanks Marc!

kennyadsl · 2021-05-24T10:23:35Z

          product.master.default_price.currency = 'JPY'
-          product.master.default_price.save!
          stub_spree_preferences(currency: 'JPY')
+          product.master.default_price.save!


Why do we need this change?

default_price was an association with autosave option set to true, same that master on product. Now we need to save it manually.

Thinking about it twice, maybe that could have backward compatibility issues. However, at least, not providing the price will result in a validation error when the app is configured to require a default price. I don't think there's a reliable way to detect the situation to emit a warning, though.

But the change is only moving the save a line above, isn't it?

I'm sorry, I don't understand what you mean. We're adding a new line to the test so that price gets persisted into the database.

This is what I see in the diff:

🤦‍♂️ 🤦‍♂️ 🤦‍♂️ sorry, you're right 🙈

Ok, the reason is that now the default_price is not cached and instead is fetched from memory at every execution. So with the new behavior when we did product.master.default_price.currency = 'JPY', then the referenced price was no longer returned on the next product.master.default_price just one line below, so save! was called on nil. If we stub the default currency before, then it becomes again the default price.

Following @kennyadsl suggestion, we updated it a bit to look less confusing:

product.master.default_price.update!(currency: 'JPY') stub_spree_preferences(currency: 'JPY')

kennyadsl · 2021-05-24T10:28:13Z

+    #
+    # @return [Spree::Price, nil]
+    # @see Spree::Price.default_pricing
+    def default_price


This method is a bit complex and hard to read to understand what it does. Any thoughts about moving it to a separate class that is responsible to select the default price only?

Yeah, I completely agree. I have some reserves because of:

It would be nice to have it be injectable for the end-user. However, we haven't decided how to do that yet. We could build on top of the old system, though.

The logic around prices is very complex, and I'd like to simplify it further. However, probably it's not a preference at this point.

WDYT? We can do it now if you think it's better to leave it clean at this point.

As we discussed IRL yesterday. Maybe a good tradeoff could be trying to use more semantic variable names and/or extracting something to a method in this same class to help reading the code.

I tried to make everything clear. I think that the result is much more readable. Thanks for pointing it out. Please, take a look when you have a second.

I think this is perfect, thanks Marc!

mamhoff

I like this.

mamhoff · 2021-05-27T07:09:17Z

+      delegate :price=, to: :default_price_or_build
+
+      # @see Spree::Variant::PricingOptions.default_price_attributes
+      def self.default_pricing


naming nit: Should this be called self.default_price_attributes?

mamhoff · 2021-05-27T07:13:00Z

+    }
+    scope :with_default_price, -> {
+      left_joins(master: :prices)
+        .where(master: { spree_prices: Spree::Config.default_pricing_options.desired_attributes })


This didn't work with some database adapters in the past. Can you confirm it does with pg, mysql and sqlite? Because if this works, the cursed Spree::Variant.with_prices scope can go.

It seems it works. That's the SQL generated for each adapter:

sqlite:

=> "SELECT \"spree_products\".* FROM \"spree_products\" LEFT OUTER JOIN \"spree_variants\" \"master\" ON \"master\".\"is_master\" = 1 AND \"master\".\"product_id\" = \"spree_products\".\"id\" LEFT OUTER JOIN \"spree_prices\" ON \"spree_prices\".\"deleted_at\" IS NULL AND \"spree_prices\".\"variant_id\" = \"master\".\"id\" WHERE \"spree_products\".\"deleted_at\" IS NULL AND \"spree_prices\".\"currency\" = 'USD' AND \"spree_prices\".\"country_iso\" IS NULL"

Postgres:

"SELECT \"spree_products\".* FROM \"spree_products\" LEFT OUTER JOIN \"spree_variants\" \"master\" ON \"master\".\"is_master\" = TRUE AND \"master\".\"product_id\" = \"spree_products\".\"id\" LEFT OUTER JOIN \"spree_prices\" ON \"spree_prices\".\"deleted_at\" IS NULL AND \"spree_prices\".\"variant_id\" = \"master\".\"id\" WHERE \"spree_products\".\"deleted_at\" IS NULL AND \"spree_prices\".\"currency\" = 'USD' AND \"spree_prices\".\"country_iso\" IS NULL"

MySQL:

"SELECT `spree_products`.* FROM `spree_products` LEFT OUTER JOIN `spree_variants` `master` ON `master`.`is_master` = TRUE AND `master`.`product_id` = `spree_products`.`id` LEFT OUTER JOIN `spree_prices` ON `spree_prices`.`deleted_at` IS NULL AND `spree_prices`.`variant_id` = `master`.`id` WHERE `spree_products`.`deleted_at` IS NULL AND `spree_prices`.`currency` = 'USD' AND `spree_prices`.`country_iso` IS NULL"

While the tests using that method work in the three adapters:

https://github.com/nebulab/solidus/blob/cb87e1666ef7d9dc07d0e0df0b96ffd05e8bab0a/core/spec/models/spree/product_spec.rb#L623

https://github.com/nebulab/solidus/blob/cb87e1666ef7d9dc07d0e0df0b96ffd05e8bab0a/core/spec/models/spree/product_spec.rb#L634

We could tackle that scope in a separate PR.

Before this work, `#default_price` was a `has_one` association between `Variant` and `Product`. As we already have a `has_many` association between both models, this redundancy was a source of inconsistencies. E.g.: ```ruby include Spree Variant.new(price: Price.new) # price delegates to default_price Variant.prices # => [] ``` From now on, `#default_price` is a regular method that searches within `#prices` taking into account the criteria for a price to be considered the default one. We have renamed previous method `#find_default_price_or_build` to `default_price_or_build`. The latter feels less standard according to Rails conventions, but the intention here is, precisely, to communicate that this is not a Rails association. No longer being a Rails association makes it impossible to use the default ransack conventions to build the sort-by-price link on the products listing page. For this reason, we added `sort_by_master_default_price_amount_{asc, desc}` scopes to the `Price` model, which is automatically picked up by ransack. As it's now explicit, these queries ignore the `ORDER BY` clauses implicit in the `currently_valid_prices` method, but this was also happening in the query built by ransack from the `has_one` association: ```SQL SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN "spree_variants" ON "spree_variants"."is_master" = ? AND "spree _variants"."product_id" = "spree_products"."id" LEFT OUTER JOIN "spree_prices" ON "spree_prices"."currency" = ? AND "spree_pric es"."country_iso" IS NULL AND "spree_prices"."variant_id" = "spree_variants"."id" WHERE "spree_products"."deleted_at" IS NULL O RDER BY "spree_prices"."amount" ASC, "spree_products"."id" ASC LIMIT ? OFFSET ? [["is_master", 1], ["currency", "USD"], ["LIMI T", 10], ["OFFSET", 0]] ``` However, the new query doesn't include discarded prices on the result, but I think it's something desirable: ```SQL SELECT "spree_products".* FROM "spree_products" LEFT OUTER JOIN "spree_variants" "master" ON "master"."is_master" = ? AND "mast er"."product_id" = "spree_products"."id" LEFT OUTER JOIN "spree_prices" "prices" ON "prices"."deleted_at" IS NULL AND "prices". "variant_id" = "master"."id" LEFT OUTER JOIN "spree_variants" "masters_spree_products" ON "masters_spree_products"."is_master" = ? AND "masters_spree_products"."product_id" = "spree_products"."id" WHERE "spree_products"."deleted_at" IS NULL AND "prices". "currency" = ? AND "prices"."country_iso" IS NULL AND "prices"."deleted_at" IS NULL ORDER BY prices.amount DESC, "spree_product s"."id" ASC LIMIT ? OFFSET ? [["is_master", 1], ["is_master", 1], ["currency", "USD"], ["LIMIT", 10], ["OFFSET", 0]] ```

kennyadsl

Thanks @waiting-for-dev!

A breaking change was introduced in PR#[4076](solidusio/solidus#4076) which modifies the the behavior on Variant#default_price and removes the has_one relationship. This extension was built based on this relationship and the `DefaultPricingQuery` model will need to be modified when 3.0 is no longer supported. This patch prevents a nil `reflection` in the `batch_loader#for`.

Due to a relationship and behavior change in variant pricing made between Solidus versions (see PR[4076](solidusio/solidus#4076)), the former method utilizing BatchLoader to pull details on "default_pricing" no longer works as intended. However, returning variant#default_price works for both versions and improves the performance of the call as can be seen [here](https://user-images.githubusercontent.com/68167430/123831394-849b7380-d8c1-11eb-8c0b-9b18c3dbf02c.png). The call method no longer returns a BatchLoader::GraphQl object, and instead returns a Spree::Price object so #sync was removed from the spec.

Due to a relationship and behavior change in variant pricing made between Solidus versions (see PR solidusio/solidus#4076), the former method utilizing BatchLoader to pull details on "default_pricing" no longer works as intended. However, returning variant#default_price works for both versions and improves the performance of the call as can be seen here: (https://user-images.githubusercontent.com/68167430/123831394-849b7380-d8c1-11eb-8c0b-9b18c3dbf02c.png) The call method no longer returns a BatchLoader::GraphQl object, and instead returns a Spree::Price object so #sync was removed from the spec.

houndci-bot reviewed May 24, 2021

View reviewed changes

Comment thread core/app/models/concerns/spree/default_price.rb Outdated

waiting-for-dev force-pushed the waiting-for-dev/simplify_default_price branch 2 times, most recently from 2081017 to cb87e16 Compare May 24, 2021 08:27

kennyadsl reviewed May 24, 2021

View reviewed changes

mamhoff approved these changes May 27, 2021

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/simplify_default_price branch 2 times, most recently from 647b799 to fed6d00 Compare June 1, 2021 08:30

waiting-for-dev force-pushed the waiting-for-dev/simplify_default_price branch from fed6d00 to 6420b10 Compare June 1, 2021 08:54

kennyadsl approved these changes Jun 10, 2021

View reviewed changes

kennyadsl requested a review from a team June 10, 2021 09:35

jarednorman approved these changes Jun 17, 2021

View reviewed changes

kennyadsl added Important Change changelog:solidus_core Changes to the solidus_core gem labels Jun 18, 2021

kennyadsl merged commit 2479b24 into solidusio:master Jun 18, 2021

kennyadsl deleted the waiting-for-dev/simplify_default_price branch June 18, 2021 07:13

mamhoff mentioned this pull request Sep 23, 2022

Fix variant price performance regressions #4639

Merged

7 tasks

Uh oh!

Conversation

waiting-for-dev commented May 24, 2021

Uh oh!

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamhoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants