[Spree Upgrade] Fix order_updater_spec by using create (instead of build) in shipment #2785
Conversation
|
I have checked the build and spec/models/order_updater_spec.rb has no errors now. |
luisramos0
force-pushed
the
2-0-order-updater-fix
branch
from
3a39b50 to
f03a2e0
Compare
| selected: true | ||
| ) | ||
| end | ||
| let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } |
There was a problem hiding this comment.
Isn't the use of the :shipping_method trait a bit redundant here? As we're passing a shipping method in this would be equivalent of the simpler create(:shipment, shipping_method: shipping_method). Am I missing something?
There was a problem hiding this comment.
I dont think so.
The spree shipment factory does create(:shipping_method)
but the new ofn shipment_with factory has the same create(:shipping_method) but inside a transient block, which means that if shipping_method is provided, it will get used.
and in this case we are providing different shipping_methods, once as pickup and then as delivery.
Makes sense?
There was a problem hiding this comment.
So, for me to understand, if no shipping method is provided none will be created, am I right?
There was a problem hiding this comment.
no, if no shipping_method is provided the code inside transient is used, in this specific shipment_with factory, the default :shipping_method factory will be used.
| selected: true | ||
| ) | ||
| end | ||
| context 'when shipping method is pickup' do |
There was a problem hiding this comment.
thanks for sticking to the more meaningful pickup/delivery naming 👏
| ) | ||
| end | ||
| context 'when shipping method is pickup' do | ||
| let(:shipping_method) { create(:shipping_method_with, :pickup) } |
There was a problem hiding this comment.
is there any reason to have a separate shipping method factory other than better readability when using traits?
There was a problem hiding this comment.
yeah, I could have extended the spree shipping_method factory, but I think it's better if we keep it with a different name. so, not just readability but also separation from spree code (I think it's similar to extending the class instead of using class_eval).
|
@sauloperez Can you give this another review? |
|
lets wait for the merge from master before we merge this one. |
…. Also simplified spec by using prebuild shipment_with and shipping_method factories
luisramos0
force-pushed
the
2-0-order-updater-fix
branch
from
f03a2e0 to
5700164
Compare
|
Although there were no conflicts, this spec was a difficult merge on #2863 |
Also simplified spec by using prebuild shipment_with and shipping_method factories
What? Why?
Closes #2784
What should we test?
Order Updater spec should be green