Add state_machines related events

[nirebu]

Description

Ref #3684

This PR wants to address #3684 by adding an event emission for every transition change in the listed state machines.

I wanted to retain the events closer to the core as possible since every app built on Solidus could use such events with different semantic meaning. The strategy chosen for the event names is:

"order_transitioned_to_#{transition.to}"

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)
• I have attached screenshots to this PR for visual changes (if needed)

[aldesantis]

@nirebu I think the main problem is that the names of the events don't make much sense. I'm thinking something like order_transitioned_to_address would be more expressive, although it would also be much longer than our usual events.

An alternative would be firing an order_transitioned event and then including the transition in the event's payload, so that the user can inspect the transition themselves and filter by previous state, current state and transition name. This also gives user more granular control over which transitions they want to react to, so perhaps it should be explored further.

The same considerations extend to shipments and payments, btw.

[kennyadsl]

What about order_state_changed (with the state in the payload)? If we go with the payload, maybe another useful information is the state the order transitioned from, WDYT?

[aldesantis]

@kennyadsl my idea was to pass the entire transition object, which responds to #from, #to and #event. That way you have all the info you may need!

[kennyadsl]

@kennyadsl my idea was to pass the entire transition object, which responds to #from, #to and #event. That way you have all the info you may need!

@aldesantis makes sense. I was also thinking to move the logic executed in the after_transition block in an event subscriber. If I recall correctly, we did something similar when we introduced the other events. Did you already discuss this topic?

[aldesantis]

@kennyadsl we did not, but I agree — that makes sense.

[kennyadsl]

If we take this route, I'd also implement an order transition failure event with this block in a subscriber for consistency:

after_failure do |order, transition|
  order.logger.debug "Order #{order.number} halted transition on event #{transition.event} state #{transition.from}: #{order.errors.full_messages.join}"
end

[nirebu]

I like where this is going. Thanks @aldesantis and @kennyadsl for the suggestions!

I was also thinking to move the logic executed in the after_transition block in an event subscriber. If I recall correctly, we did something similar when we introduced the other events.

@kennyadsl could you point me in the right direction on where to find such an example? I seem to able to find only MailerSubscriber

[kennyadsl]

I was thinking to MailerSubscriber actually. The code that it currently executes was in Spree::Order#finalize! before. You can take a look at the PR that introduced the change with a git blame. My suggestion is to implement something like OrderStateChangeSubscriber (up to you for the name of this thing) that, in its basic version, just logs things exactly as we are doing with:

order.logger.debug "Order #{order.number} transitioned from #{transition.from} to #{transition.to} via #{transition.event}"

Let me know if it makes sense for you as well.

[spaghetticode]

Also, it may be worth exploring the idea of having events before transitions as well, with something like:

      base.state_machine do
        before_transition do |order, transition|
          Spree::Event.fire "order_before_#{transition.to_name}", order: order
        end

Regarding the granularity of event names:

An alternative would be firing an order_transitioned event and then including the transition in the event's payload, so that the user can inspect the transition themselves and filter by previous state, current state and transition name. This also gives user more granular control over which transitions they want to react to, so perhaps it should be explored further.

I am not sure what would work better here. By having events such as order_transitioned_to_address one can subscribe directly to the event they need (multiple ones, if they need more than one).

By having a single order_transitioned event then the dev needs to resort to a case or if in the event callback in order to reduce the scope of execution, when needed.

[nirebu]

Thanks @spaghetticode for adding to this, and also to have better worded what I was trying to put here in the meantime.

By emitting the full transition event and making the user check at every transition if the code should react or not via a combination of transition.to and transition.from, in my opinion, would somewhat duplicate the distinctions already made in the state machine configuration.

I'd prefer to go down the road of order_transitioned_to_XYZ, even if it means having longer event names.

Let me know what you think about this.

[kennyadsl]

@nirebu is this ready for another review or are you still working here?

[nirebu]

@kennyadsl This Friday I've updated the branch with the longer event names and updated the first comment to explain them. If these are OK I can continue also with adding the relevant logging functions.

[kennyadsl]

I'm ok with the latest changes even if I can anticipate that the code will slightly change again. 😛

[nirebu]

@kennyadsl I've updated the PR content also with the after_failure logging as suggested. Let me know what you think.

[kennyadsl]

@nirebu 👍 tbh I'd move them in a separate commit with a description that explains why we are adding them (consistency). Also, in my understanding, the failures should fire an event as well, isn't it?

[nirebu]

I've tried to compact the commit history separating the changes between success and failure events. And I also added the failure events with the following format:

"order_failed_transition_to_#{transition.to}"

I was also thinking whether it would be a good idea to move the logger calls to event subscribers or not. In my opinion they should stay in the state_machines definitions, but I'm also open to discussion on this one 😄

[kennyadsl]

I was also thinking whether it would be a good idea to move the logger calls to event subscribers or not. In my opinion they should stay in the state_machines definitions, but I'm also open to discussion on this one 😄

I'm also not completely sure about this one. We can move those actions to subscribers later with another PR if needed.

[kennyadsl]

What about order_failed_transitioning_to_...?

[nirebu]

It could also work no problem, I have no strong opinions on transition vs transitioning.

[kennyadsl]

Let's wait for the opinion of @spaghetticode, which has more experience with events' names

[spaghetticode]

Personally, I guess I am slightly partial to transitioned_to, as it's a bit shorter.

In general, I think we should strive to make these event names descriptive and easily mappable to the state machine transition that triggered them. One extra bonus may be having names with consistent/predictable format, so if after_failure spawns the event order_failed_transition_to_completed, then for symmetry the name fo after_transition may be changed to order_successful_transition_to_completed. Unfortunately, it's a bit too long 🤷.

Maybe we can spend a little more time in searching the best names, also considering that changing them later will require a deprecation cycle.

[jarednorman]

How should we test events here? Using events as extension points means we need them to be stable and consistent so I want to make sure we've got good test coverage around how and when events are fired.

[kennyadsl]

@aldesantis I know you've been working with testing events in the past, do you have any advice for the legit @jarednorman comment about testing these events?

[aldesantis]

Testing event emission

Our approach on client projects has always been fairly straightforward for testing event emission:

stub_const('Spree::Event', class_spy(Spree::Event))

# run whatever code is supposed to fire the event

expect(Spree::Event).to have_received(:fire).with('...')

An even better solution would be to have a "test" event adapter, similar to what ActiveJob provides.

Testing subscriber behavior

As for testing the actual subscribers, that's a bit trickier, because you need to make sure you're testing them in isolation — i.e., when you run the subscriber, you need to make sure there are no other subscribers that could interfere with your test.

Our approach has been the following:

• By default, event subscribers are only enabled in integration tests (i.e., system/request specs), and disabled everywhere else. This encourages developers to consistently test things at the right level.
• We have a perform_subscribers helper that works similarly to ActiveJob's perform_enqueued_jobs: you pass it a block and a list of event subscribers that you want to execute in the context of that block, and the subscribers will only be enable for the duration of the block.

So our subscriber tests usually look like this:

RSpec.describe MyEventSubscriber do
  describe 'event_name' do
    it 'does something' do
      # set up the test here
      # ...

      perform_subscribers(only: described_class) do
        Spree::Event.fire 'event_name'
      end

      # verify the side-effects of the subscriber here
      # ...
    end
  end
end

This pattern has worked very well for us so far in apps that rely very heavily on the event bus.

I was already planning to contribute it upstream, so I can accelerate that if we all agree it's a good idea. 🙂

[waiting-for-dev]

Thanks for your work on this @nirebu ❤️ The event system got a massive revamp, and it'll be available from Solidus v3.2 (at this point, it's only on master). All the event-driven architecture has been extracted to a standalone library: omnes.

The new event system is much more powerful, and we aim to integrate all it provides on Solidus. For instance, events can be serialized to run handlers in the background. Because of that, we should move away from passing active record instances as the event payload.

Besides, having the publication of the events so closely attached to the code in the state machine is not a good idea. Events should be handled outside the main database transaction, as they're a way to run code that is not coupled with the main flow (disclaimer: at this point, the state machine transactions don't run within a database transaction, but it should be considered as a flaw we need to fix).

Because all of that, I think it's best to close this PR for now. Adding more events into core is defined in the roadmap, so I hope we'll get there soon!!