preserve shipping method when shipments change

[gvaughn]

https://bonobos.atlassian.net/browse/ENG-3511

The main impetus of this is Tulip in which the guides have a much more
non-linear approach to creating an order, but the same problem exists
through the web client too.

Other approaches have been tried, but lets try this within the
Estimator itself. An extra side-effect of this is that we don't
delete shipments in Order#ensure_updated_shipments so that's poorly
named now, but it also raises a question of why it's called when it
is. This means a shipment will stick with an order if created before we
reach the "delivery" state of the order. Upon transition into "delivery"
then #created_proposed_shipments gets called to ensure they're correct.
The new logic in the Estimator now will re-select any prior shipping
method/rate selection the customer had previously made.

[gvaughn]

This is the new test case that tests for the error. Updating an order followed by an advance which does order.next lost all concept of the user's prior selection of a shipping method. If this test is testing the wrong thing, then PLEASE let me know because all the fixes I'm focused on depend upon this.

[gvaughn]

I'm a bit nervous about these changes to the factories, but they were the simplest way to get tests passing

[athal7]

I'm less concerned about these after chatting through them. and there's a potential performance benefit!

[gvaughn]

Great. Your insight helped my confidence in this change too.

[gvaughn]

With that "fix broken api spec" commit all tests in core and api are passing locally. backend fails different tests each time I run it and they seem quite unrelated, so I'm not focused on those now.

[athal7]

i generally find more comfort in the spec if we have an absolute value here rather than a calculated one

[gvaughn]

I debated that point. As a newcomer to the code I first found the numerical constant mysterious and had to break down in my head where it came from. My other option for this spec is to ensure the order itself is created without a selected shipping method, then I won't need to change the body of this test. I'll work on that as a compromise.

(I could just replace 1010 with 1110 to get it to pass, but that feels like cheating)

[athal7]

what about both? use the number in the spec but comments to explain how that number was calculated?

[gvaughn]

Yes, I think for expediency in this case, I'll do that. Though in general is still prefer not to hard code absolute values.

[athal7]

Ah ok, if that is your preference then don't let me deter you. We can have a philosophical debate separately :)

[gvaughn]

@athal7 The open question I want to talk about is whether Order#ensure_updated_shipments really should call #create_proposed_shipments or not. Every edit to the order (via OrderContents#update_cart) calls #ensure_updated_shipments. Do we want to be creating and destroying a shipment every time the order is modified? The other consideration is that once #create_proposed_shipments is called the first time, then we will have a selected shipping_rate on the shipment, even though the user did not choose it. I'm not sure it's a problem beyond assumptions in unit tests, but from a certain perspective, no, there hasn't really been a "chosen" shipping method/rate yet, but the system will act as if there has been.

In the bigger picture, I'm not sure what value we get out of having a shipment on an order before the "delivery" state (besides a place to hold the user's selected shipping method/rate -- which I'd argue implies a design smell). As long as #create_proposed_shipments is called when we transition to "delivery" then the shipment info should be just fine. It's a matter of whether we need them updated each time the order is modified in the intermediate steps or not. Is shipment info ever presented to the customer before the "delivery" stage?

(We plan to have a hangout in the morning to discuss these issues, but rather than just a private email, I wanted to put my questions out here in case others have input.)

[athal7]

@gvaughn I agree there is a lot of complexity there, would it make sense to leave it as it was?

[gvaughn]

OK. So I think the remaining TODO list with this is to ensure flatiron doesn't have any test failures. solidus/core and solidus/api all test cleanly. solidus/backend has changing failures each time I run it. Should I track that down? I'm pretty sure solidus/frontend tests are not important to this, but I can run them anyway.

I also believe @philbirt has updated the nov_9_... branch so I'll need to rebase.

[gvaughn]

This is ready for final review.

core and api specs pass cleanly. backend and frontend have sporadic failures, which also happens on the parent branch. We will circle back to get that code into CI and fixup failing specs as a separate task.

[athal7]

👍 from me. @jordan-brough if you care to take a look as well your eyes would be appreciated, but I'm also as confident as I could be with this given the state of previously-written related code

[jordan-brough]

The Spree shipping code makes my head explode. :( I've been looking at this PR for an hour and I'm still having trouble wrapping my brain around all the previous code and the implications of these changes. I just don't understand it and this well enough and I'm not sure how much time you guys want me to spend diving in as deep as you have already.

My gut reaction is that this doesn't seem to really move the code to a much cleaner place overall. If we could find a way to instead start side-stepping all this crazy shipping & inventory code that we don't use that would be really nice. I'm guessing you've probably already tried something like that and have come back to this.

So, no thumbs-up or thumbs-down from me (unless you want me to jump in a lot more), just ¯\_(ツ)_/¯.

[gvaughn]

Thanks for your time @jordan-brough Yes, I know what you mean. I would like to take this farther; it represents only a baby step at this point.