[WIP] Import count_on_hand methods for Spree 2 #2417
Conversation
The added methods will be gone in Spree 2. I imported them and added logic to work with new and old Spree.
|
I really like this approach. It could potentially allow the Spree upgrade to get finished a lot quicker, whilst also giving more breathing room in terms of decoupling existing code from Spree. |
|
this seems awesome! maybe spree upgrade is possible . . this century :)
… On 16 Jul 2018, at 9:11 AM, Matt-Yorkley ***@***.***> wrote:
I really like this approach. It could potentially allow the Spree upgrade to get finished a lot quicker, whilst also giving more breathing room in terms of decoupling existing code from Spree.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#2417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACxryNZUnkWUZMfLSYlalKU288DlrE8mks5uG8whgaJpZM4U8lkA>.
|
|
If we could use similar techniques to get us up to the point where Solidus was forked we might be able to avoid reinventing the wheel with our own set of hooks throughout the Spree codebase. |
|
@sauloperez What's you opinion on this solution? ^ |
| error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false' | ||
| raise error unless Spree::Config[:track_inventory_levels] | ||
|
|
||
| self.count_on_hand = new_level unless on_demand |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, that's where I saw this code before! Thank you. So this pull request would override Frank's change.
|
I think this is worth discussing in the next Spree upgrade catch up. See my comment in https://openfoodnetwork.slack.com/archives/C4NDJT3FY/p1531749287000116. While it makes it upgrade shorter as we won't refactor all the calls to old methods, it also brings an extra considerable level of complexity we will have to deal with. |
|
|
||
| # Overriding `Spree::Variant.count_on_hand=` in old Spree versions. | ||
| # It doesn't exist in newer Spree versions. | ||
| def count_on_hand=(new_level) |
There was a problem hiding this comment.
Are these methods necessary? Spree::Variant doesn't have count_on_hand or count_on_hand=methods, it uses the on_hand methods as getters and setters.
There was a problem hiding this comment.
Or is this related to Variant Overrides?
There was a problem hiding this comment.
In Spree 1.3 the variants table has a count_on_hand column. So ActiveRecord adds these methods automatically and we have a few calls to these methods in our code base. In order to keep the old API alive, we need these methods.
|
Also related to #2450 |
|
We decided to keep the old Spree API ( |
The added methods will be gone in Spree 2. I imported them and added
logic to work with new and old Spree.
What? Why?
Helps with #2014.
This is a proposal for discussion, possibly at the next Spree update call. It adds logic to deal with current and new Spree code and is therefore a new approach to the Spree upgrade.
When thinking about refactoring our uses of
count_on_hand, I saw a lot of work. But all this work could be obsolete when we come to the network feature and get rid of VariantOverride. That's why I propose to add compatibility code instead of refactoring everything now.I think that this a very quick and easy solution to Spree moving the stock logic into another model. It also allows us to merge this change now. That should avoid merge conflicts. More importantly, it makes every developer working with the current code aware of changes in this area and that they should not use
count_on_handin SQL queries or as direct attribute assignment. After the Spree upgrade, we can and should simplify that code. That's the downside. It also needs testing with the new Spree version.What should we test?
Everywhere where variants are used and their stock is shown. Examples:
Release notes
Added more compatibility to Spree 2.0.
How is this related to the Spree upgrade?
This is to avoid conflicts with the Spree upgrade.