-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the order updater when shipping shipments, or dealing with payments. #389
Conversation
shipment_state: new_state, | ||
updated_at: Time.now, | ||
) | ||
@order.update! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the point of this being a one-liner, should we inline it above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
👍 for using |
We don't need to do this ourselves. Additionally a user may define hooks, which are run by the order updater that wouldn't get triggered whenever shipments are shipped, which is something worth knowing about. We also don't necessarily know that updating the shipment state shouldn't update other states or parts of the order.
While we keep the intention of only updating the order when the order or payment is in a certain state, we shouldn't be doing this manually. The order updater exposes a method that's responsible for the total update of an order, a payment shouldn't know that shipments need to be updated when the payment is saved. Additionally, this bypasses the order updater hooks, which run as part of the update process, preventing users who rely on these hooks for information about when state changes may occur from using the hooks to accomplish that goal.
Take that, 4d652a7! |
👍 This is also about responsibility and knowledge. A payment should not know that it needs to update the shipment state and the shipment should not know how to update the order. Very happy with this change. |
👍 |
Agree with the sentiments above. Nice work Richard. |
Use the order updater when shipping shipments, or dealing with payments.
Each commit describes the reasons for their respective changes.
At a high level, none of these objects should need to know how to update an order after performing their responsibilities. It is fair to say that they could know that the order should be updated, but the logic for doing so should be encapsulated in the order updater.
Every time we don't use the updater in situations were we should, our codebase becomes less cohesive. A problem that was recently discovered was how difficult it was to be notified when the order was updated to a new state, or set of states. The updater provides hooks for this, but they're not used as classes outside of the updater have been updating the order themselves.