Copy all prices from master over children variants #3994

waiting-for-dev · 2021-03-16T06:59:26Z

Description

This commit changes functionalities in three ways:

Backend: Before this change, when a user created a new variant the
form was pre-populated only with a single price field for the default
currency. In the case the user removed the amount field and submitted
the form, the created price had an amount equal to 0. Now, all the
prices created for a given product (for a given master variant, in
fact) get pre-populated in a nested form. In the case one of the
amounts is removed by the user, the form becomes invalid on submission
and forces the user to set an amount for every price.
API: Before this change, the user was able to create a variant 1)
manually assigning a single price in the default currency through the
price attribute, and 2) letting it inherit the default price from
the master variant when price attribute was not present. Now, when
not overridden all master prices get inherited. Otherwise, the user
can specify both through price (for the default currency) and
prices_attributes attributes.
Product duplicator: Before this change, only the default master price
was copied. Now, all prices are taken into account.

Internally, the set_price callback on Variant has been removed in
order to allow that flexibility. The behavior has been moved to a layer
one step closer than where it should actually be: A service layer. It
will help when we tackle this long-term goal. It also means that when we
manually create a variant we must specify the default price for it.

Having three rails associations between Variant and Price is a
source of inconsistencies, as when we add a Price in-memory to one of
them the others don't get updated. It should be changed as follow-up
work. For now, Variant#any_price? hack has been added. In the same
way, Variant#find_or_build_default_price didn't find it when it still
was in-memory. Variant#default_price_from_memory have been added to
fix it.

There are still two considerations that Variant#default_price_from_memory is not taking into account, but I'd appreciate some feedback. Take a look at the default_price definition:

solidus/core/app/models/concerns/spree/default_price.rb

Lines 8 to 14 in 3cc2d3e

    
             has_one :default_price, 
        
               -> { with_discarded.currently_valid.with_default_attributes }, 
        
               class_name: 'Spree::Price', 
        
               inverse_of: :variant, 
        
               dependent: :destroy, 
        
               autosave: true 
        
           end

currently_valid is showing prices with a country associated to it in the first place. This seems weird, as I'd think as the default price that one with no country associated to it:

solidus/core/app/models/spree/price.rb

Line 23 in 3cc2d3e

    
           scope :currently_valid, -> { order(Arel.sql("country_iso IS NULL")).order(updated_at: :DESC, id: :DESC) }

irb(main):009:0> Spree::Price.order(Arel.sql("country_iso IS NULL"))[0..1].map(&:country_iso)
  Spree::Price Load (0.3ms)  SELECT "spree_prices".* FROM "spree_prices" WHERE "spree_prices"."deleted_at" IS NULL ORDER BY country_iso IS NULL
=> ["AD", nil]

This change was introduced in 9af171f , but doesn't seem to be justified.

It doesn't seem legit to include deleted prices in the query... It was introduced to allow deleted variants to render their deleted default prices, but I'd say it's wrong for any other situation. This change was introduced in 87cef09

Demo

screencast-localhost_3000-2021.03.16-06_59_49.mp4

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)

backend/app/views/spree/admin/variants/_prices_form.html.erb

This commit changes functionalities in three ways: - Backend: Before this change, when a user created a new variant the form was pre-populated only with a single price field for the default currency. In the case the user removed the amount field and submitted the form, the created price had an amount equal to 0. Now, all the prices created for a given product (for a given master variant, in fact) get pre-populated in a nested form. In the case one of the amounts is removed by the user, the form becomes invalid on submission and forces the user to set an amount for every price. - API: Before this change, the user was able to create a variant 1) manually assigning a single price in the default currency through the `price` attribute, and 2) letting it inherit the default price from the master variant when `price` attribute was not present. Now, when not overridden all master prices get inherited. Otherwise, the user can specify both through `price` (for the default currency) and `prices_attributes` attributes. - Product duplicator: Before this change, only the default master price was copied. Now, all prices are taken into account. Internally, the `set_price` callback on `Variant` has been removed in order to allow that flexibility. The behavior has been moved to a layer one step closer than where it should actually be: A service layer. It will help when we tackle this long-term goal. It also means that when we manually create a variant we must specify the default price for it. Having three rails associations between `Variant` and `Price` is a source of inconsistencies, as when we add a `Price` in-memory to one of them the others don't get updated. It should be changed as follow-up work. For now, `Variant#any_price?` hack has been added. In the same way, `Variant#find_or_build_default_price` didn't find it when it still was in-memory. `Variant#default_price_from_memory` have been added to fix it.

Having `currently_valid_prices` as an association between `Variant` and `Price` made it a source of inconsistencies with the undecorated `prices` association: ``` Spree::Variant.new.tap do |v| v.currently_valid_prices << Spree::Price.new end.prices.any? # => false ``` We're adding tests to the method, making explicit that at this moment a price that belongs to a country is prioritized over a generic price when it comes to choose which one is showed as the default. Probably it's not desired behavior and we should change it in a follow-up work. We should also consider renaming it to something more communicative. The current name conveys the idea that we're filtering some records out, while in fact we're just ordering them.

core/spec/models/spree/variant_spec.rb

core/app/models/concerns/spree/default_price.rb

waiting-for-dev · 2021-03-22T07:05:37Z

I added two commits addressing the concerns about redundant associations Variant <-> Product. Please, tell me if you'd prefer to have it in separated PRs. I copy their description here for more clarity:

Move currently_valid_prices to a method

Having currently_valid_prices as an association between Variant and
Price made it a source of inconsistencies with the undecorated
prices association:

Spree::Variant.new.tap do |v|
  v.currently_valid_prices << Spree::Price.new
end.prices.any? # => false

We're adding tests to the method, making explicit that at this moment a
price that belongs to a country is prioritized over a generic price when
it comes to choose which one is showed as the default. Probably it's not
desired behavior and we should change it in a follow-up work.

We should also consider renaming it to something more communicative. The
current name conveys the idea that we're filtering some records out,
while in fact we're just ordering them.

Simplify Variant#default_price logic

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.
I.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]]

To study this logic made me realize that the #default_price logic is
kind of weird. The fact that it includes discarded prices makes little
sense in my view (other than being able to get the default price that a
discarded variant had, but as far as I can see there's no GUI for
discarded models). The order criteria neither make sense. Taking the
updated_at attribute to prioritize could lead to undesired behavior:

Create a product with a default price.
Add another price in the same currency to override the previous one.
Update the previous price for some reason.
Now, the previous price becomes the default.

On top of that, probably it doesn't make sense to allow having different
prices for the same country & currency tuple.

As follow-up work I'd propose:

Stop including discarded prices.
Validate that they can't coexist two prices with the same currency/country
tuple, and remove the ordering criteria for default prices. We can
provide more flexibility adding an active flag and scoping the
validation to the case it's true.

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. I.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]] ``` To study this logic made me realize that the `#default_price` logic is kind of weird. The fact that it includes discarded prices makes little sense in my view (other than being able to get the default price that a discarded variant had, but as far as I can see there's no GUI for discarded models). The order criteria neither make sense. Taking the `updated_at` attribute to prioritize could lead to undesired behavior: - Create a product with a default price. - Add another price in the same currency to override the previous one. - Update the previous price for some reason. - Now, the previous price becomes the default. On top of that, probably it doesn't make sense to allow having different prices for the same country & currency tuple. As follow-up work I'd propose: - Stop including discarded prices. - Validate that they can't coexist two prices with the same currency/country tuple, and remove the ordering criteria for default prices. We can provide more flexibility adding an `active` flag and scoping the validation to the case it's `true`.

kennyadsl

Thanks Marc, I left some comments after a first pass.

probably it doesn't make sense to allow having different prices for the same country & currency tuple.

I think this is done intentionally. There are other combinations of things that may determine the price in the same country/currency. For example if someone builds their own pricing strategy to allow different prices based on the role (eg. B2B vs B2C), so I guess we should keep that constraint.

The other considerations feel quite right, can't imagine a reason to keep discarded prices in the query and the default set by the updated_at is weird.

kennyadsl · 2021-03-22T15:44:43Z

core/app/models/concerns/spree/default_price.rb

@@ -23,6 +23,13 @@ def self.default_pricing
      end
    end

+    # Returns `#prices` prioritized for being considered as default price
+    #
+    # @return [Array<Spree::Price>]


currently_valid is a regular scope so it returns an ActiveRecord::Relation, isn't it?

Right! Fixed.

kennyadsl · 2021-03-22T15:50:06Z

core/spec/models/spree/price_spec.rb

+        result = described_class.currently_valid
+
+        expect(
+          result.index(price_1) < result.index(price_2)


I think

expect(described_class.currently_valid).to eq [price_1, price_2]

has the same effect also taking the order into account. Do you prefer this version because it's more explicit?

Yeah, it was my first try. The problem is that the fixtures are not very well optimized and other Price records get being created, so the result would contain other items. I think it should be food for another PR...

Fixes bug where discarded prices were taken into account in order to tell the default price for a variant. The rationale behind the previous behavior was to be able to render a price when discarded products are included in the backend products listing page (see 'Show deleted' checkbox near the top-right edge). It was introduced in commit 87cef09. However, this logic was incorrect in two ways: - It affected non-discarded variants, where we don't want to consider discarded prices. - Even for discarded variants, it could lead to display the wrong information. As all prices get discarded when a variant is discarded, there's no guarantee that the one that is rendered was the previously considered as the default. The list of products on the backend will render the price field for discarded products as blank. In case we want to track default prices, we should add a flag to uniquely identify them.

Until now the time of update was instead taken into account. This could lead to an incorrect result: - Create price 1 - Try to supersede price 1 creating price 2 - Update price 1 for some reason - At this point, price 1 was considered as the default

core/app/models/spree/variant/price_selector.rb

It conveys better what it's doing, as the query only consists of `ORDER` clauses and no record is filtered out.

waiting-for-dev · 2021-03-23T07:06:22Z

I think this is done intentionally. There are other combinations of things that may determine the price in the same country/currency. For example if someone builds their own pricing strategy to allow different prices based on the role (eg. B2B vs B2C), so I guess we should keep that constraint.

I see... However it makes the default price concept as it's right now quite vague. I think we should revisit it at some point.

The other considerations feel quite right, can't imagine a reason to keep discarded prices in the query and the default set by the updated_at is weird.

I added three more commits. One to ignore discarded prices, another one changing the prioritization to take creation time instead of update time, and a third one renaming currently_valid to prioritize_for_default_price:

Ignore discarded prices on Variant#default_price

Fixes bug where discarded prices were taken into account in order to
tell the default price for a variant.

The rationale behind the previous behavior was to be able to render a
price when discarded products are included in the backend products
listing page (see 'Show deleted' checkbox near the top-right edge). It
was introduced in commit 87cef09.

However, this logic was incorrect in two ways:

It affected non-discarded variants, where we don't want to consider
discarded prices.
Even for discarded variants, it could lead to display the wrong
information. As all prices get discarded when a variant is discarded,
there's no guarantee that the one that is rendered was the previously
considered as the default.

The list of products on the backend will render the price field for
discarded products as blank. In case we want to track default prices, we
should add a flag to uniquely identify them.

Prioritize by creation date on default_price

Until now the time of update was instead taken into account. This could
lead to an incorrect result:

Create price 1
Try to supersede price 1 creating price 2
Update price 1 for some reason
At this point, price 1 was considered as the default

** Rename currently_valid to prioritized_for_default **

It conveys better what it's doing, as the query only consists of ORDER
clauses and no record is filtered out.

waiting-for-dev · 2021-08-05T07:29:55Z

We're closing it as we'll be revisiting this stuff in the context of services objects.

houndci-bot reviewed Mar 16, 2021

View reviewed changes

backend/app/views/spree/admin/variants/_prices_form.html.erb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/copy_all_prices_from_master_over_variants branch 2 times, most recently from c6bb075 to 7a54da9 Compare March 16, 2021 07:27

waiting-for-dev force-pushed the waiting-for-dev/copy_all_prices_from_master_over_variants branch from 7a54da9 to aba70db Compare March 16, 2021 09:05

houndci-bot reviewed Mar 22, 2021

View reviewed changes

core/spec/models/spree/variant_spec.rb Outdated Show resolved Hide resolved

core/app/models/concerns/spree/default_price.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/copy_all_prices_from_master_over_variants branch 4 times, most recently from 6fea3f0 to d5df6e3 Compare March 22, 2021 15:22

waiting-for-dev force-pushed the waiting-for-dev/copy_all_prices_from_master_over_variants branch from d5df6e3 to 0e4b484 Compare March 22, 2021 15:50

kennyadsl reviewed Mar 22, 2021

View reviewed changes

waiting-for-dev added 2 commits March 23, 2021 07:45

houndci-bot reviewed Mar 23, 2021

View reviewed changes

core/app/models/spree/variant/price_selector.rb Show resolved Hide resolved

Rename currently_valid to prioritized_for_default

db31154

It conveys better what it's doing, as the query only consists of `ORDER` clauses and no record is filtered out.

waiting-for-dev force-pushed the waiting-for-dev/copy_all_prices_from_master_over_variants branch from 7ad44f4 to db31154 Compare March 23, 2021 06:54

This was referenced May 21, 2021

Move currently_valid_prices to a method #4073

Merged

Simplify Variant#default_price logic #4076

Merged

Deprecate callback setting variant price from master #4078

Merged

This was referenced Jun 21, 2021

Ignore discarded prices on Variant#default_price #4108

Closed

Rename currently_valid to prioritized_for_default #4109

Closed

waiting-for-dev closed this Aug 5, 2021

waiting-for-dev deleted the waiting-for-dev/copy_all_prices_from_master_over_variants branch September 4, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy all prices from master over children variants #3994

Copy all prices from master over children variants #3994

waiting-for-dev commented Mar 16, 2021

waiting-for-dev commented Mar 22, 2021

kennyadsl left a comment

kennyadsl Mar 22, 2021

waiting-for-dev Mar 23, 2021

kennyadsl Mar 22, 2021

waiting-for-dev Mar 23, 2021

waiting-for-dev commented Mar 23, 2021

waiting-for-dev commented Aug 5, 2021

	has_one :default_price,
	-> { with_discarded.currently_valid.with_default_attributes },
	class_name: 'Spree::Price',
	inverse_of: :variant,
	dependent: :destroy,
	autosave: true
	end

Copy all prices from master over children variants #3994

Copy all prices from master over children variants #3994

Conversation

waiting-for-dev commented Mar 16, 2021

waiting-for-dev commented Mar 22, 2021

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Mar 22, 2021

Choose a reason for hiding this comment

waiting-for-dev Mar 23, 2021

Choose a reason for hiding this comment

kennyadsl Mar 22, 2021

Choose a reason for hiding this comment

waiting-for-dev Mar 23, 2021

Choose a reason for hiding this comment

waiting-for-dev commented Mar 23, 2021

waiting-for-dev commented Aug 5, 2021