solidus

solidus copied to clipboard

Description

The naming of this method is confusing and it may induce developers to believe it refreshes shipments while it actually only destroys them on a standard Solidus store.

The method definition is thoroughly tested in the codebase, and the wanted behavior is to actually remove shipments without recreating them back.

The old name is still available but deprecated with a message that suggests using the new one.

FYI if the original order's state machine is patched for example by removing the address state, then shipments will be actually recreated since the order will be nexted to the delivery state and a callback will create them again.

I'm not 100% satisfied with the new name clean_shipments_and_restart_checkout, if anybody has a better one I'll be happy to change it.

Checklist:

• [ ] I have followed Pull Request guidelines
• [ ] I have added a detailed description into each commit message
• [ ] I have added tests to cover this change (if needed)

I'm not 100% satisfied with the new name clean_shipments_and_restart_checkout, if anybody has a better one I'll be happy to change it.

Actually, "Cleaning shipments" makes me think of some sort of moving them in a cleaner state.

What about empty_shipments_and_restart_checkout? "Empty" can be misleading as well, maybe; it could mean that the shipments are still there but their contents are removed.

Another alternative could be remove_shipments_and_restart_checkout, WDYT?

@kennyadsl I didn't want to use a term that strongly conveys the meaning of absolutely destroying/removing shipments, because this doesn't happen always (i.e. when order is completed it doesn't) and the final outcome also depends on what does later restart_checkout_flow. That's the reason for my naming 😅

Hey! This one is ready to be merged, but the naming is holding it back :slightly_smiling_face:

What about check_shipments_and_restart_checkout?

@waiting-for-dev looks good to me, I'm going to change the name according to your suggestion 👍

Hey @spaghetticode, sorry for the hassle, but, can you rebase from master so that specs are green?

@spaghetticode last rebase, I swear it will be merged this time! ❤️

@spaghetticode I think something is messed with the reabase, maybe it's because of the master -> main rename?

Codecov Report

Merging #4173 (5228b00) into main (a03f443) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4173   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files         563      563           
  Lines       13882    13884    +2     
=======================================
+ Hits        12307    12309    +2     
  Misses       1575     1575           

Impacted Files	Coverage Δ
core/app/models/spree/order.rb	94.65% <100.00%> (+0.02%)	:arrow_up:
core/app/models/spree/order_contents.rb	96.72% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kennyadsl yes, that was the issue! Finally it's now green 💚

I swear it will be merged this time!

Promise kept! :trollface: