-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Simplify Variant#default_price
logic
#4076
Simplify Variant#default_price
logic
#4076
Conversation
2081017
to
cb87e16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions, thanks Marc!
stub_spree_preferences(currency: 'JPY') | ||
product.master.default_price.save! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default_price
was an association with autosave
option set to true
, same that master
on product
. Now we need to save it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the change is only moving the save a line above, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ 🤦♂️ 🤦♂️ 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following @kennyadsl suggestion, we updated it a bit to look less confusing:
product.master.default_price.update!(currency: 'JPY')
stub_spree_preferences(currency: 'JPY')
# | ||
# @return [Spree::Price, nil] | ||
# @see Spree::Price.default_pricing | ||
def default_price |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is perfect, thanks Marc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this.
delegate :price=, to: :default_price_or_build | ||
|
||
# @see Spree::Variant::PricingOptions.default_price_attributes | ||
def self.default_pricing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nit: Should this be called self.default_price_attributes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
} | ||
scope :with_default_price, -> { | ||
left_joins(master: :prices) | ||
.where(master: { spree_prices: Spree::Config.default_pricing_options.desired_attributes }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
647b799
to
fed6d00
Compare
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]] ```
fed6d00
to
6420b10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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[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.
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.
Before this work,
#default_price
was ahas_one
association betweenVariant
andProduct
. As we already have ahas_many
associationbetween both models, this redundancy was a source of inconsistencies.
E.g.:
From now on,
#default_price
is a regular method that searches within#prices
taking into account the criteria for a price to be consideredthe default one.
We have renamed previous method
#find_default_price_or_build
todefault_price_or_build
. The latter feels less standard according toRails 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 thePrice
model, which is automatically picked up by ransack. As it's now
explicit, these queries ignore the
ORDER BY
clauses implicit in thecurrently_valid_prices
method, but this was also happening in thequery built by ransack from the
has_one
association:However, the new query doesn't include discarded prices on the result,
but I think it's something desirable:
Checklist:
Note: Extracted from #3994