[Spree Upgrade] Remove variant.count_on_hand (keep variant.on_hand only) #3513
Conversation
variant.on_hand will not return infinity any longer if variant.on_demand is true. Clients of these methods will have to handle the combinations between on_hand and on_demand values
…ed on the server side, the UI can make the decision to show on_demand on the on_hand column if on_demand is true
luisramos0
changed the title
luisramos0
force-pushed
the
2-0-remove-variant-count-on-hand
branch
from
f78dd21 to
ffbe790
Compare
| @@ -16,7 +16,8 @@ | |||
| %input{ 'ng-model' => 'variant.price', 'ofn-decimal' => :true, :name => 'variant_price', 'ofn-track-variant' => 'price', :type => 'text' } | |||
| %td{ 'ng-show' => 'columns.on_hand.visible' } | |||
| %input.field{ 'ng-model' => 'variant.on_hand', 'ng-change' => 'updateOnHand(product)', :name => 'variant_on_hand', 'ng-if' => '!variant.on_demand', 'ofn-track-variant' => 'on_hand', :type => 'number' } | |||
| %span{ 'ng-bind' => 'variant.on_hand', :name => 'variant_on_hand', 'ng-if' => 'variant.on_demand' } | |||
There was a problem hiding this comment.
why is the ng-bind not needed now?
There was a problem hiding this comment.
we don't want "on demand" to be bound to the on_hand value, ng-bind would do that. the binding is only when variant is not on_demand, the ng-ifs are controlling that.
| scope.variants.push li.variant | ||
| if li.variant.on_hand == 0 && li.max_quantity > li.variant.on_hand | ||
| li.max_quantity = li.variant.on_hand | ||
| scope.variants.push(li.variant) unless li.variant in scope.variants |
There was a problem hiding this comment.
can we add some blank lines in between? It's feel like a wall 😱
There was a problem hiding this comment.
I have added some line breaks... I dont want to change this logic but it sure is calling for some improvement...
There was a problem hiding this comment.
not changing logic but a bit of spacing would ease the reading...
There was a problem hiding this comment.
Ive added some spaces, I am not sure if you are ok with it now or if you are asking for more spaces? :-D
luisramos0
force-pushed
the
2-0-remove-variant-count-on-hand
branch
from
ffbe790 to
8ee14cc
Compare
| # | ||
| # @return [Float|Integer] | ||
| def on_hand | ||
| warn_deprecation(__method__, '#total_on_hand') | ||
|
|
||
| if on_demand | ||
| Float::INFINITY |
There was a problem hiding this comment.
This is a step backwards. We changed this because Spree introduced this logic of returning infinity. This change would make us less compatible with Spree again. I would prefer to adjust the cart logic or the serializer to deal with infinity. The 9999 value in the cart logic is just a workaround for not dealing with infinity properly.
There was a problem hiding this comment.
I think this is a step forward Maikel. I will explain.
Returning Infinity is poor, it's magic, the client should get specific on_hand and on_demand values all the time and the client should decide what to do, no magic.
Also, one important detail for this logic is that in spree, and soon in ofn, on_demand means backorderable: that means that even if the item is backorderable (which will be the on_demand field), the on_hand value will still be used. Basically stock can go to negative.
So, this is a move in the direction of making on_demand equal to backorderable and also it removes magic.
did I convince you? :-D
There was a problem hiding this comment.
"The 9999 value in the cart logic is just a workaround"
I am not too worried with client side code here (although I have tested it and it works), we can improve it later.
Here we are changing OFN core behaviour, and I think that it is important to be very clear and simple, i.e., not mixing on_hand and on_demand in the core. Leaving that up to the client.
There was a problem hiding this comment.
I really liked the infinity solution as it seemed mathematically correct and solving all the problems. But you convinced me with the different use of on_hand. There are two different uses that Spree started to separate:
- How many can I order? Infinity is right here.
- How many are in stock? That's a finite number of the stock item.
And on_hand sounds a bit more like the answer to the second question.
|
Conflicts resolved. |
What? Why?
In this PR I am proposing replacing all calls to variant.count_on_hand with variant.on_hand.
I dont think we should make major refactoring work now before we go live with v2, but this one was clearly needed to keep variant stock management at acceptable levels of complexity :-)
The only two changes required were:
I recommend the review per commit because one of the commits is a find/replace exercise.
What should we test?
Build should be green.
I think this is the right time to put this PR in as we will be able to validate the full v2 version very soon.