Replace Shipment#determine_state - #6316
Conversation
github-actions
Bot
added
the
changelog:solidus_core
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6316 +/- ##
=======================================
Coverage 89.35% 89.35%
=======================================
Files 961 961
Lines 20180 20192 +12
=======================================
+ Hits 18031 18043 +12
Misses 2149 2149 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| else | ||
| 'pending' | ||
| end | ||
| def recalculate_state |
There was a problem hiding this comment.
I'm not sure I fully understand the benefits of assigning the value to the instance of the object, if we are using this method only when we cannot use the instance as is (just calling save). It might look cleaner, but I have the feeling we are just moving the complexity into update_state. Let me know your thoughts (non-blocking comment).
There was a problem hiding this comment.
The benefit of assigning the value is a bit abstract at this point, but it's in service of the in-memory order updater (#5872). Using an assignment is great in situations where we don't actually want to persist changes to an order but want to ready or preview for a future state. Currently, we would have to use #determine_state to do this, but that doesn't let us actually re-use that value in memory for the current shipment.
It might look cleaner, but I have the feeling we are just moving the complexity into
update_state.
I am not sure I understand what you mean. I don't think the functionality of #update_state will be changing at all as a result of this.
There was a problem hiding this comment.
We also feel that the method signature for #determine_state isn't helpful and is worth deprecating. There isn't a very good reason to ever call #determine_state with another_order... only the order associated with the current record. We think that #recalculate_state is also an improvement in this sense.
|
Just making a note that we intend to go back and deprecate |
d33555c to
4f23f98
Compare
|
We've now deprecated |
4f23f98 to
8df9532
Compare
|
@benjaminwil can I kindly ask to rebase this with latest |
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
I really like this change. It makes much more sense 👍🏻
We don't want our in-memory order updater to have to persist shipment state. We also discovered that a portion of the existing `Shipment#update_state` method has conditionals that could never be hit. So we're adding a new method to help with the recalculation of the shipment state because we don't want to modify any of the existing methods because they're public. Co-authored-by: Jared Norman <jared@super.gd> Co-authored-by: Noah Silvera <noah@super.gd>
While `#determine_state` is technically a public method, it is only used within the shipment model. We also checked all repos on the `solidusio` and `solidusio-contrib` GitHub organizations and only found it used once. So in this commit we remove the method entirely and replace its single usage with the more useful `Shipment#recalculate_state` method. Using `#recalculate_state` is beneficial because it assigns the new state to the shipment in memory. This gives developers more flexibility to actually do something with the new state information. For example, when simulating order updates without persisting them. In some other upcoming work on an in-memory order updater (solidusio#5872) we use the new `Shipment#recalculate_state` method as a public method, within the new order updater, which we why we think this is a good candidate for a public method. Co-authored-by: Chris Todorov <chris@super.gd> Co-authored-by: Senem Soy <senem@super.gd>
8df9532 to
86202cf
Compare
|
@tvdeyen Conflicts resolved! |
5 participants
Summary
While working on a new in-memory order updater (#5872), we identified that having a
Shipment#recalculate_statemethod that assigns but does not persist a new shipment state is advantageous for application developers.After making this change, it became clear that the
Shipment#determine_statemethod doesn't provide a lot of value. So we have decided to replace it entirely and instead use#recalculate_stateeverywhere.Checklist