WIP Superseding historical prices #987

mamhoff · 2016-03-16T00:32:17Z

This is to replace the is_default boolean on prices.

That boolean governed what price would be currently valid. I'd like to replace that with logic that can tell you what price a variant was at any given point. In order to do that, I introduce a valid_from field, which defaults to the time the price is created and indefinitely valid afterwards. There is no valid_until field because the product has available_until and we don't want to run into a product ever not having a price.

@gmacdougall mentioned that this might have caching implications. @jhawthorn Please also have a look at this.

houndci-bot · 2016-03-16T07:59:38Z

core/app/models/spree/price.rb

@@ -12,6 +13,13 @@ class Price < Spree::Base
      less_than_or_equal_to: MAXIMUM_AMOUNT
    }

+    scope :valid_before, -> (date) { where("#{self.table_name}.valid_from <= ?", date).order(valid_from: :desc) }


Redundant self detected.

gmacdougall · 2016-03-17T13:28:25Z

An example of where we have caching that will be affected by this:

solidus/frontend/app/views/spree/products/show.html.erb

Line 1 in 96f186c

<% cache [I18n.locale, current_currency, @product] do %>

The product show page is currently cached on Locale, Currency, and Product and we rely upon Price touching Variant and Variant touching Product to bust the cache.

mamhoff · 2016-03-17T15:14:10Z

That should work, though: Whenever I update, create or destroy a price belonging to a variant, that variant and in turn the associated product will be touched.

Alternatively, we could create a more sophisticated cache key for products; something along the lines of

class Product
  def cache_key
    associated_object_timestamps = [updated_at] + variants.map(&:updated_at) + prices.map(&:updated_at) 
    hashed_timestamps = Digest::MD5.hexdigest associated_object_timestamps.map(&:to_s).join(",")
    super + hashed_timestamps
  end
end

Right now, that should bust the cache quite reliably whenever we update, add or remove any variants or prices.

In the future, we'll also have to look at modifying the *cache_key* methods in the core helpers to take into account the current order's tax location. But that is outside the scope of this PR.

mamhoff · 2016-03-17T18:45:49Z

@gmacdougall and I talked IRL (well, on the interwebs) and he explained that time itself should expire the cache key. I've cooked something up by looking up prices.currently_valid (which uses Time.current) to figure out which prices are currently possibly valid, and that correctly expires the cache when a new price becomes relevant. I've also added a spec to check for changing of the current price (which does the touching chain price -> variant -> product, and also correctly busts the cache. Would this work?

mamhoff · 2016-03-18T21:03:38Z

@gmacdougall @jhawthorn Please have a look at this, there is a caching strategy and a migration.

gmacdougall · 2016-03-21T13:57:19Z

core/app/models/spree/product.rb

@@ -274,6 +274,10 @@ def find_variant_property_rule(option_value_ids)
      end
    end

+    def cache_key
+      super + "/prices/" + prices.currently_valid.pluck(:id).join("/")


I think it would be best to digest the prices. I don't know if we'll encounter an issue with key length if we have thousands of prices, but it's better to err on the side of safety.

I thought about this but went for "the simplest thing that could possibly work". I do agree with digesting we'd be safe from key length issues.

Something that's crossed my mind though: Should we maybe ask the currently used pricer make sure the correct cache key is used?

Yes, I think that's makes sense. I'm not sure which PR you want to do that in.

Can absolutely be this one. I'm thinking after #994 is in, which gives us a first go at how a pricer would be structured, it could be quite neatly tucked into a new pricer that's less stupid than the ConservativeLineItemPricer.

Nope, coming to think of it: This PR should really just introduce this feature. Variant#prices.currently_valid will return all prices (all currencies and so on), and custom pricers will just add more attributes to select prices from - this caching strategy would still work in most cases. I'll work on your suggestions (use timecop and use the change matcher).

mamhoff · 2016-03-23T13:38:28Z

Wow, this is much more of a pain than I thought, mostly because saving a new item on a has_one relation deletes the old one, which is behaviour I'm not happy about. We need a relation though because otherwise Ransack won't work.

houndci-bot · 2016-03-23T16:52:43Z

core/spec/models/spree/product_spec.rb

-        it "should not change updated_at" do
-          expect { subject }.not_to change{ product.updated_at }
+        it "should not change the cache key" do
+          expect { subject }.not_to change{ product.cache_key }


Space missing to the left of {.

mamhoff · 2016-03-29T18:16:33Z

@gmacdougall @jhawthorn This is done, as far as I'm concerned. Would you have a look? :)

adammathys · 2016-03-29T23:28:31Z

core/db/migrate/20160318130914_migrate_default_to_current_prices.rb

@@ -0,0 +1,34 @@
+class MigrateDefaultToCurrentPrices < ActiveRecord::Migration
+  def up
+    say_with_time 'Setting default prices to be valid from now' do


I don't think the say_with_time is necessary when all it's wrapping is an execute. But it doesn't really hurt to have it.

tvdeyen · 2016-03-30T22:37:44Z

core/app/models/spree/price.rb

+    scope :valid_before,
+          -> (date) { latest_valid_from_first.where("#{Spree::Price.table_name}.valid_from <= ?", date) }
+    scope :valid_before_now, -> { valid_before(Time.current) }
+    scope :with_default_currency, -> { where(currency: Spree::Config.currency) }


in_currency(Spree::Config.currency)

tvdeyen · 2016-03-30T22:40:07Z

Perfect. Thanks for adding more docs and considering my feedback.

Someone please have a look into the SQL in the migrations as I'm not able to review it. @jhawthorn @gmacdougall

👍

The idea here is to introduce a valid_from column to prices that determines which price is taken if there is more prices that fit the given search scope. For example, if you have two prices for currency JPY: ``` a = Spree::Price(currency: "JPY", amount: 13, valid_from: 2015-12-31) b = Spree::Price(currency: "JPY", amount: 13, valid_from: 2014-12-31) c = Spree::Price(currency: "JPY", amount: 13, valid_from: 2016-12-31) ``` And you want to find the price currently valid on Feb 2nd, 2016, you would obtain price b - the latest price that is not valid in the future. If you wanted to find out which price *was* valid on Feb 2nd, 2015, that would be price a. You can even find out a valid price far in the future, although that's more brittle, as is the case with most telling the future things.

Spree::Price.valid_before(date) will return all prices valid before certain date. Spree::Price.valid_before_now will return all prices before the current date. Spree::Price.latest_valid_from_first will return the prices sorted by valid_from, in descending order. Also makes sure that a price is always initialized with the current date and time as a valid from date, if none is given.

Modifies the scope in the `default_price` `has_one` association such that it returns the prices that are valid before the current time.

Without this commit, the product's cache key method would not expire the cache for the product when the currently valid prices change. This attaches the currently valid price ids to the end of the cache key, making it know when to invalidate the cache. Introduces a class method on `Spree::Price` to generate a cache key for a collection of prices that never changes in length.

Set the timestamp for `valid_from` on default prices (not in the future, most recent) to true. This migration respects time zones as well as different ways of saying "true" or "false" in your database dialect.

Solidus re-implements `autosave: true` for prices, even though it already tells ActiveRecord to autosave.

We want prices to be essentially immutable, and we want a history of prices for our variant. This commits makes it such that the present default price is replaced instead of modified. Unfortunately, the old price is also deleted in the process. ActiveRecord does that, and there does not seem to be any way of stopping it! Fixed spec setup for product helper spec. Do not set currency in product duplicator (the master price will take care of that itself) Do not set variants default price currency, as the price object will handle that. Fix another spec so that the validation for price (which isn't tested) does not barf. Use the correct scope instead of unscoping `default_price` `acts_as_paranoid` offers a `with_deleted` scope for paranoid models. This commit uses that scope in the `scope` argument of the `has_one` declaration instead of using `unscoped` and overriding the generated method.

It does not make any sense to set a currency on a variant - if it's the default currency, it does not change what prices do by themselves, and if it's not, you're changing the default price to something that can not be the default as it would have the wrong currency. However, there was a number of places in the old code that still tried to do that, so I expect custom code to also have that code. This commit re-introduces the setter with a Deprecation warning.

Before this commit, the concern would always create an association that has `:variant` as its inverse. This commit changes that for an association that has the symbolized underscored class name as inverse. When using this concern, be aware that you'll need a `your_model_id` column on prices.

I've been a bit stumped about what `default_price` actually does, so here goes some documentation. Also moved the scope for a default price from the default price module into the price class.

We want to know all prices for a variant. When building a new `default_price` as we do every time `Spree::Variant#price=` is called, ActiveRecord will silently `destroy` the old price. `Acts_as_paranoid` mitigates this behaviour, and the `with_deleted` scope here appears like it never happened. I briefly considered overwriting `Spree::Price#destroy` to be a noop, but that would be an introduction of where uncommon behaviour for a model.

mamhoff · 2016-05-16T15:18:59Z

This is now very out-of-date. Much of it has been taken in in other PRs. I'm closing this for the time being.

houndci-bot reviewed Mar 16, 2016
View reviewed changes

mamhoff changed the title ~~WIP/RfC Superseding historical prices~~ Superseding historical prices Mar 18, 2016

gmacdougall reviewed Mar 21, 2016
View reviewed changes

houndci-bot reviewed Mar 23, 2016
View reviewed changes

adammathys reviewed Mar 29, 2016
View reviewed changes

tvdeyen reviewed Mar 30, 2016
View reviewed changes

This was referenced Mar 31, 2016

Solidus VAT Roadmap #989

Closed

WIP Add vat_country relation to prices #968

Closed

mamhoff added 12 commits April 2, 2016 15:20

Remove code that relates to is_default boolean

5f97fb7

Modifies the scope in the `default_price` `has_one` association such that it returns the prices that are valid before the current time.

Migrate default to current prices

b89a909

Set the timestamp for `valid_from` on default prices (not in the future, most recent) to true. This migration respects time zones as well as different ways of saying "true" or "false" in your database dialect.

Remove useless price callback

4adc30c

Solidus re-implements `autosave: true` for prices, even though it already tells ActiveRecord to autosave.

Add documentation to Default Price concern

622020c

I've been a bit stumped about what `default_price` actually does, so here goes some documentation. Also moved the scope for a default price from the default price module into the price class.

Add Changelog entry for superseding prices

76d16ac

mamhoff changed the title ~~Superseding historical prices~~ WIP Superseding historical prices Apr 13, 2016

jhawthorn added the WIP label Apr 13, 2016

mamhoff closed this May 16, 2016

mamhoff deleted the superseding-historical-prices branch May 24, 2016 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Superseding historical prices #987

WIP Superseding historical prices #987

mamhoff commented Mar 16, 2016

houndci-bot Mar 16, 2016

gmacdougall commented Mar 17, 2016

mamhoff commented Mar 17, 2016

mamhoff commented Mar 17, 2016

mamhoff commented Mar 18, 2016

gmacdougall Mar 21, 2016

mamhoff Mar 21, 2016

gmacdougall Mar 21, 2016

mamhoff Mar 21, 2016

mamhoff Mar 21, 2016

mamhoff commented Mar 23, 2016

houndci-bot Mar 23, 2016

mamhoff commented Mar 29, 2016

adammathys Mar 29, 2016

tvdeyen Mar 30, 2016

tvdeyen commented Mar 30, 2016

mamhoff commented May 16, 2016

WIP Superseding historical prices #987

WIP Superseding historical prices #987

Conversation

mamhoff commented Mar 16, 2016

Choose a reason for hiding this comment

gmacdougall commented Mar 17, 2016

mamhoff commented Mar 17, 2016

mamhoff commented Mar 17, 2016

mamhoff commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff commented Mar 23, 2016

Choose a reason for hiding this comment

mamhoff commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Mar 30, 2016

mamhoff commented May 16, 2016