Remove Spree::Shipment#address association.

[metade]

We found that if an order address was updated after checkout through the admin, the shipment address was not updated.

It turns out that the Spree::Shipment#address was not actually being used for anything in particular, so it feels better to remove the association and delegate to the Spree::Order#ship_address instead.

We'll want to remove Spree::Shipment#address in the long term, so adding a deprecation message and removing any specs that refer to it.

[mamhoff]

This would remove the possibility of shipping two line items to different addresses within the same order. This possibility is arguable only there at the model level. Anyone have that case? Using it?

[jordan-brough]

@mamhoff that is true and we had a quick chat with @jhawthorn about that. We should definitely think this over very carefully, but we're thinking this might be the right choice because:

• Spree::Order can only have 1 "ship_address", so it's confusing and error prone to support shipments with multiple addresses in a single order
• We have Spree::Carton in Solidus now, which can still record the fact that you've actually shipped things to different locations, even if the original intention (e.g. the Order.ship_address) was a single address.

This change makes me nervous about breaking things for people but we're already broken (like @metade said, updating the ship address in the admin doesn't actually change the shipment if the order is complete) so it feels like we're in a bad place right now also...

It does make sense to allow multiple shipping destinations in a single order, but it feels like there are more changes we need to make to really support that properly, so we might be better off doing this for the time being.

Thoughts?

[alexstoick]

Had a brief talk with @metade today (cc @jordan-brough), and I have a suggestion about doing this. We can rename the address field on the shipment to optional_address, so as not to break anyone's existing setup, and also enable us to do interesting split shipments scenarios, for the people that need them.

So then the address function on Spree::Shipment could look like this:

def address
  shipment.optional_address || order.ship_address
end

More thoughts on this?

[metade]

I think that Shipment#address gives the illusion that we might be able to support shipping to different addresses but in practice this isn't the case.

For example, with an order that has 2 shipments with different addresses, if you edit the Order#ship_address how do you track which of the 2 Shipment#address should also be changed?

On top of that, there appear to be lots of bugs (e.g. updating an order ship address and #1141) that indicate that Shipment#address isn't being used consistently.

So this is why I feel it's redundant and can be removed.

We could think of an approach that supports shipments to multiple locations (e.g. based on @alexstoick's suggestion), but we should probably wait until someone wants to support that and implement it properly.

But I agree that it's not a decision that should be taken lightly and we should consult the community to see if anyone is actually shipping to multiple destinations.

[jhawthorn]

There's definitely downsides to removing the shipments.address column. Though we don't have a builtin way to support it (yet?) we should be able to represent shipments being sent to multiple locations. It's a not uncommon feature of online stores (though obviously non-trivial).

However, if editing the shipping address in the admin doesn't update the address we are shipping to, that is obviously broken. For that reason I'm in favour of removing the column (but disappointed we have to).

I really like @alexstoick's suggestion of a rename. It would allow us to still represent the concept of multiple ship addresses, but fix the functionality easily right now. I'd suggest maybe something more descriptive than optional_address, maybe address_override.

[metade]

I just spoke to @jordan-brough about this, and we agreed that instead of removing the spee_shipments.address_id column we should rename it.

Whilst we do want to support the feature in the future there may be stores using the column in unexpected ways (e.g. in SQL for analytics etc, or potentially updating the column).

Renaming the column would mean that these stores would realise that they need to change their application when upgrading, and we'd still be able to support the feature in the future.

However, we shouldn't attempt to implement multiple ship addresses at this point.

Instead we should wait until someone has this requirement and can design, implement and test the feature properly within their own store and then upstream the work into core when it's been tried and tested.

Let me know if you agree with this approach and I'll rename the column to spree_shipments.address_override_id.

[jordan-brough]

👍 on @metade's comment.

I'm thinking it would be good to also remove the field from the migrations so that new apps don't have it. Otherwise it feels a bit confusing and possibly problematic in the future if people add new functionality against it since we really don't support it in the code. And if we remove it from the migration we'd want to make the "rename column" migration smart enough to only rename the field if it already exists.

[mamhoff]

I don't like the idea of two Solidus stores having different schemas. Do the rename, remove the functionality, all that is perfectly fine with me (I just wanted the discussion to happen, and am happy with the outcome) but IMO the column should not be deleted.

[jordan-brough]

@mamhoff I don't think the idea is to have multiple Solidus stores with different schemas. Rather it's to deprecate a column that we aren't going to support anymore (and never really did).

I think we're saying this:
This column never really worked correctly and we're not going to support it anymore so we're removing it from Solidus. We don't want to delete any of your data but we do want to make sure you realize that we've removed it from Solidus (in case you're still using it somewhere) so we're going to rename it rather than drop it.

It doesn't seem right to keep a "mystery" column around in Solidus that isn't used or tested anywhere does it?

What if we renamed the column to deprecated_address_id instead of address_override_id?

[jasonfb]

FWIW, I rename all my 'dropped' columns to _DEPRECATED (all-caps for nice readability), leave them as _DEPRECATED for about 2 months, then go through routinely and actually drop the deprecated fields.

(Yes I do this on all my columns, even from Spree migrations, which means I have to go through migration-by-migration and modify them to do this)

I just scanned my own codebase and found 2 small places in which we are relying on the address association on the shipments, but this will be easy for me to fix (when the time comes), as I could simply change it to .order.ship_address

[metade]

I like the idea of deprecated_address_id as it makes the intention clear - we no longer use this column.

When we come back to implement shipments to multiple addresses this can potentially be renamed appropriately - but it will depend on the implementation.

Unless there are any objections, I'll add a migration to do this.

[metade]

I've added the migration (1678847) and rebased from master.

[mamhoff]

I'm fine with this. For what it's worth, the Spree people are adding that functionality to the backend now:
spree/spree#7350. Would you mind adding a changelog entry?

[jordan-brough]

👍 (ditto on a changelog entry)

[jordan-brough]

@metade a couple more references to Shipment#address that I noticed: Lostmyname#1

[metade]

I've removed the extra references to Spree::Shipment#address that @jordan-brough picked up, and added an entry to the changelog as suggested by @mamhoff.

[jordan-brough]

👍

[jasonfb]

What do you do if you want an exchange shipment to go to a different address? (For example, when the original address was wrong and the package was returned to sender).

We use the Shipment's address for this in the context of exchange shipments only.

My feeling is that the Order's ship_address is the one the customer put the order in as, and the Shipment's address is the place the package actually got shipped to (which may have been changed by Customer Service). Seems like, although duplicitous, each has a function as its own data point

[jordan-brough]

@jasonfb I chatted w/ the core team and we want to figure out how to support that kind of thing, but right now shipping address being different from order address just isn't really supported in Solidus (and has at least one bug -- if you update the shipping address for an order after it is complete it will not update the address for the shipments. This started after the immutable-address work (v1.2 I think?)). You can certainly continue to use the deprecated_address_id field for that scenario in your store, or add your own db overrides to allow for that sort of thing in the meantime.

We very much do want to come back to this and figure out how to support the type of thing that you mention and other scenarios as well, but I think we need to give ourselves a bit of a clean slate first by fixing this.

[mamhoff]

👍

[jasonfb]

@jordan-brough Makes sense. I think the impact to us will be minimal --- differing address is an edge case.

cc: @chrishawk -- for our app we can probably get away with following their lead and delegating up to the order; with the possibility of future support for differing addresses as a planned extension