[Subscriptions] (v1) Fix assertions in specs for OrderSyncer #3605
Conversation
This fixes some assertions from using the same subscription shipping and billing addresses, and distributor address. One of the specs also pass because the subscription shipping address matches the distributor address. This commit makes that scenario clearer.
This is to check that we are using the correct address data in assertions. This also clarifies the scenario for one of the specs.
The shipping methods are updated to their target settings after the subscription order has been created, so the order is created with the "require_ship_address" factory default which is "true". The shipping method setting at time of order creation has implications on whether a shipment is set up for the order or not.
kristinalim
force-pushed
the
fix/2788-improve_specs_for_order_syncer
branch
from
9e4c213 to
3ab00d8
Compare
| expect(order.ship_address.address1).to eq "123 abc st" | ||
| expect(order.ship_address.phone).to eq "1123581321" | ||
| end | ||
| end |
There was a problem hiding this comment.
this is not simple but I get it now :-)
the spec helps a lot!
| @@ -146,8 +164,8 @@ | |||
| expect(order.bill_address.address1).to eq "123 abc st" | |||
| expect(order.bill_address.phone).to eq "1123581321" | |||
| expect(order.ship_address.firstname).to eq "Bill" | |||
| expect(order.ship_address.lastname).to eq ship_address_attrs["lastname"] | |||
| expect(order.ship_address.address1).to eq ship_address_attrs["address1"] | |||
| expect(order.ship_address.lastname).to eq bill_address_attrs["lastname"] | |||
There was a problem hiding this comment.
why not using the address' object rather than its hash of attributes? I don't see any benefit on using the hash. Just more code.
There was a problem hiding this comment.
Also, why are we switching from ship to bill address? Sorry, I find all this file very hard to follow.
There was a problem hiding this comment.
@sauloperez This is using _attrs to keep the original ship and bill attributes after the order is initialized. Syncing modifies the order object including sometimes the original_ship_address and original_bill_address.
There was a problem hiding this comment.
Also, why are we switching from ship to bill address? Sorry, I find all this file very hard to follow.
When the shipping method is for a pickup (require_ship_address == false), syncing the order with the subscription disregards whatever ship address you have in the subscription. It forces the ship address of the order to be the distributor's address (because it is a pickup) and to use the orders bill contact details.
This is existing behaviour. I didn't actually change the app code here - it just so happened that the previous assertions passed because order.ship_address, order.bill_address, and distributor.address used the same attributes (create(:address)).
There was a problem hiding this comment.
Yes, I remember fixing that logic for v2. Thanks for the explanation!
This forces us to write correct assertions when adding specs affected by subscription addresses.
Randomizing all addresses generated will be explored separately.
This also fixes some phone references in OrderSyncer specs.
|
I made follow-up changes related to @luisramos0's recommendation to move the setup code for subscription addresses to a factory. I checked the feasibility of randomizing bill and ship addresses for all sample subscriptions. As mentioned, so far, they use the address factory of Spree which generates addresses with the same attributes. POC PR for this shows that only So I've appended to this PR:
Requesting reviews again, because you might have issue with the |
|
PR no test, it means this can be merged @sauloperez right? |
|
Sorry, I missed this one |
What? Why?
Relates to #2696
There are some wrong assertions in specs for
OrderSyncer, caused by addresses (subscription shipping, subscription billing, distributor) having the same attributes.This shows how it can be dangerous to use static data in factories.
What should we test?
This only affects specs. No manual tests are needed.
Release notes
OrderSyncerspecs.Changelog Category: Fixed
How is this related to the Spree upgrade?
This is in preparation for some fixes for subscriptions in v2. This is best to merge in v1 so we can be sure that the behaviour that we wish to achieve in v2 is indeed the behaviour in v1.