Couple completed_at touch with complete state on orders

[seand7565]

Right now it's possible - particularly when using an extension that alters the order#finalize method (looking at you, solidus_subscriptions) - to have an order pass the complete! step, then fail in a completion after hook.

Functionally, this means that the order#finalize method will not run. (Or if it breaks mid-way through finalize, only a portion of it will run).

Since completed_at is only touched as a last step in finalize, this means that it's possible for an order to be in the complete state without a completed_at date.

That's a problem by itself, but made worse because the orders complete scope only looks for orders that have a completed_at date.

This can lead to issues that go undiagnosed for some time. Since these orders are not in the complete scope, they won't show up on the admin orders panel, and if your error reporting didn't catch the initial error (or you didn't delve into it deep enough), you can wind up with paid orders that sit unshipped forever.

Potential solutions:

• Touch completed_at as a step during completion instead of after, so there is no more discrepancy between complete and completed_at (if an order fails to complete, completed_at should also roll back to nil)
• Adjust the orders complete scope to look for orders with a state of complete - I think that is a less surprising way to search for completed orders. (It's really hard to understand at a glance why an order with the state of complete is not considered complete)

[kennyadsl]

@seand7565 thanks for opening this discussion.

Touch completed_at as a step during completion instead of after, so there is no more discrepancy between complete and completed_at (if an order fails to complete, completed_at should also roll back to nil)

Using this solution, if an exception makes the order completion fail, wouldn't be the order considered completed even if it's not?

Did you already evaluate moving the touch :completed_at after the completion of the order? In this case, it would be consistent with the state of the order without marking order that are not correctly finalized as completed. What do you think?

[seand7565]

Maybe I'm wrong, but I thought everything in a state_machine transition occurred in a transaction? So if complete! failed, the completed_at touch would roll back.

But anyway, touch :completed_at should work as a first action in finalize after complete to mitigate this partially, but I think that's undesirable for two reasons:

• completed_at is still detached from setting the complete state, meaning things can still go wrong if you customize the wrong way in your app (in a similar way to point number 2)
• Specifically, solidus_subscriptions would still have this issue (barring any change in logic on that extension, which should also probably happen!) because it prepends the finalize method, creates a subscription, then supers - so completed_at is even further decoupled from complete

[kennyadsl]

At the moment, transactions for the order state machine are turned off. Maybe this is the root of the problem here?

[seand7565]

Ahh, got it! That makes sense, since completion can deal with capturing payments. It would be bad to charge a payment then roll it back.

In that light, I think your suggestion of performing the touch as a first step after the transition makes sense; preferably in a separate method from finalize to avoid the issue with solidus_subscriptions (or I guess we could just fix solidus_subscriptions 😛). It doesn't seem like a strange use case to want to update both a state and a timestamp at the same time, but I can't find any indication that that's possible in the state machine documentation, so setting it directly after the transition seems like the next best thing.

I'm not sure how well this would work, but one other option could be:

• Touch the completed_at date as the last callback before the transition
• Set completed_at to nil in after_transition_failure

[kennyadsl]

I think they are both valid approaches. I can't recall having ever used after_transition_failure in the past, but it might be a good option as well.

[FrancescoAiello01]

I like the idea of putting it in after_transition_failure. It makes sense to me that the completed_at would be reset if the transition fails. As a slight aside, I created an issue on solidus_subscriptions to stop prepending to finalize: solidusio/solidus_subscriptions#260

[elia]

Adding to the confusion we also have Order#complete? checking the state and being used alongside Order#completed? liberally throughout the codebase. Would be very nice if at least we could better discriminate between the two, e.g. with a prefix or suffix (complete_state? / completion_date? / …), even if the deprecation cycle will be long it seems a worthy improvement.