Ensure cartons find soft deleted shipping methods

[pelargir]

Description

Ensure cartons can find soft deleted shipping methods they belong to. If they can't, the #tracking_url method blows up when it tries to call #build_tracking_url on a nil shipping method.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)

[jacobherrington]

Thanks for making this PR @pelargir! 🚀

I think this would work, but I'd like to see if anyone else objects.

[tvdeyen]

Makes total sense to me. Thanks, Matt :)

[kennyadsl]

I'm not sure we should do this. Every carton will now also return discarded records as well by default but I still can't figure out how to make this kind of things work with discard.

[jacobherrington]

@kennyadsl that was my concern, but I'm not sure if that is bad or acceptable.

[jarednorman]

Currently shipping methods have both paranoia and discard on them (both using the deleted_at column.)

I think a better solution might to be remove paranoia from the model, making the extra scope unnecessary. I'm not 100% on what else this might affect, though.

@jarednorman That is the problem. I'm going to recommend not merging this until we can talk about it at a core team meeting.

[jacobherrington]

Oops @tvdeyen I dismissed your review. 🤦‍♂️

I shouldn't look at PRs before having coffee. ☕️

I'm going to recommend not merging this until we can talk about it at a core team meeting.

[pelargir]

Rebased from master to resolve conflict.

[ericsaupe]

Ideally it would only show the soft deleted shipping_method if it's already attached but not list it if choosing another, right?

[kennyadsl]

Yes, but my concern is if we are using (in core, extensions or users' stores) some code to check if there's a shipping method attached to the carton.

if carton.shipping_method
  # do something dangerous
end

The current behavior is returning false if the shipping method has been soft-deleted, if we merge this PR it would return true. I'm not sure if this is actually critical and we should considered this to be a breaking change. We have a single occurrence of this in core though. 🤔

In terms of functionality, do we really want to still build tracking urls for deleted shipping methods? Maybe not generating the tracking at all is the best thing as default behavior here, so probably a check of the shipping method presence in the tracking_url method is enough to fix the reported bug?

[pelargir]

@kennyadsl I see your point. If a carton's shipping method was soft deleted, wouldn't you still want to display it as part of the historical record for that carton?

If it's important to indicate when a carton's shipping method has been deleted, the shipping method itself could be checked for its deletion status and an appropriate message displayed.

I'm having trouble imagining a scenario where a dangerous operation would be performed based on the presence or absence of a carton's shipping method.

[kennyadsl]

@pelargir I'm sure it's the right thing to do and I'm also having trouble imagining a scenario when this could cause issues, maybe just some recurring task that makes sure that all cartons have a shipping method associated for consistency but... 🤷‍♂️

Let me talk with the rest of the core team during the next meeting about this and I'll let you know. I left a comment in the meantime, we should use discard scopes rather than paranoia ones.

[kennyadsl]

Can you also take a look at why specs are not running, I think it's something with your CircleCI account. Let me know if I can be of help here.

[pelargir]

@kennyadsl I've been attempting to fix the CircleCI build but I keep encountering the error in #3374.

[pelargir]

@kennyadsl the build is passing now. What else can I do to help get this approved?

[spaghetticode]

Also, not strictly related to the issue with #tracking_url but I think it helps in getting more context, we have a validation on Spree::Carton that requires the presence of a shipping_method, so all cartons with a soft-deleted shipping method are invalid.

[kennyadsl]

Thanks, @pelargir!

[kennyadsl]

@solidusio/core-team can we have a second review here?

[tvdeyen]

Nice. Thanks, Matt 👍

[kennyadsl]

Thanks, @pelargir!