Skip to content
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

ActiveSupport notifications for Events Handling #3081

Merged
merged 27 commits into from Apr 15, 2019

Conversation

@spaghetticode
Copy link
Member

spaghetticode commented Feb 6, 2019

This is the code resulted from some experiments I did with ActiveSupport::Notifications after this issue.

I think using ActiveSupport Notifications may simplify the handling of events, as most of the code is already implemented, if the Solidus team decides eventually to go that way.

A bonus of this implementation is that native ActiveSupport code is wrapped inside the Spree::Event module, which should be easy to customize in order to use another event library, if that may be the case... something like a custom event bus or an external gem like rails_event_store.

Differently, a configuration settings may be introduced in order to allow to use an entirely different class instead of Spree::Event.

I think the code is pretty simple and straightforward to understand, it can be surely improved but my primary goal was to provide a POC to be discussed.

I provided a couple of usage examples related to sending emails, as this is a hot topic for this feature.

The first one relates to the order finalization process, and now courtesy of Spree::Event::MailerProcessor the email is sent via event subscription. Removing the subscription is easy and can be done in an initializer, for example:

Spree::Event.unsubscribe Spree::Event::MailerProcessor.order_finalize_subscription

All subscriptions from MailerProcessor can be removed with this line:

Spree::Event::MailerProcessor.unregister!

Some specs show the usage with code.

Adding more events should be quite straightforward by following the examples in the code.

core/spec/models/spree/order_spec.rb Outdated Show resolved Hide resolved
core/lib/spree/event/mailer_processor.rb Outdated Show resolved Hide resolved
core/lib/spree/event/mailer_processor.rb Outdated Show resolved Hide resolved
core/lib/spree/event/mailer_processor.rb Outdated Show resolved Hide resolved
core/lib/spree/event/mailer_processor.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Show resolved Hide resolved
core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/app/models/spree/reimbursement.rb Outdated Show resolved Hide resolved
core/app/models/spree/reimbursement.rb Outdated Show resolved Hide resolved
core/app/models/spree/reimbursement.rb Outdated Show resolved Hide resolved
core/app/models/spree/reimbursement.rb Outdated Show resolved Hide resolved
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch 2 times, most recently Feb 6, 2019
@mdesantis

This comment has been minimized.

Copy link
Contributor

mdesantis commented Feb 7, 2019

@spaghetticode , what about fully embracing the JavaScript metaphore and replace publish, subscribe and unsubscribe respectively with trigger, on and off?

@spaghetticode

This comment has been minimized.

Copy link
Member Author

spaghetticode commented Feb 7, 2019

@mdesantis I'm not sure about that. I feel that this is Ruby so there's no particular benefit in adhering to an interface/naming that it's not strictly ours. Also, on off trigger are mainly jQuery AFAIK .

Another reason why I'm not much into this change is that while I like easy to type (short) and remember methods, I learned in the past that they can be a PITA to search in a codebase, especially when they're as generic as on and off can be.

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved
core/lib/spree/event/adapters/active_support_notifications.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Outdated Show resolved Hide resolved
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch Feb 11, 2019
@spaghetticode

This comment has been minimized.

Copy link
Member Author

spaghetticode commented Feb 11, 2019

I've been working a bit more on this implementation. The main improvements are:

  • Spree::Event now delegates the bulk of its methods to the adapter, which defaults to Spree::Event::Adapter::ActiveSupportNotifications. This will make super-easy switching to a different implementation by simply changing the adapter:
    Spree::Event.adapter = Spree::EventBus

  • Spree::Event.listeners returns a hash with all Solidus related subscriptions, grouped by event name. The implementation is again delegated to the adapter, as ActiveSupport::Notifications already has a nice way for managing this

  • Spree::Event.unsubscribe now accepts a subscription object (it will unsubscribe only that subscription) or an event name (it will unsubscribe all subscriptions to that event)

  • some specs for Spree::Event module in order to improve coverage

@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch to f4022e9 Feb 12, 2019
Copy link
Member

kennyadsl left a comment

This is great @spaghetticode ! Just left some comments, let me know!

core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Outdated Show resolved Hide resolved
core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/lib/spree/event/processors/mailer_processor.rb Outdated Show resolved Hide resolved
core/lib/spree/event.rb Show resolved Hide resolved
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch 2 times, most recently to d8d92c6 Feb 14, 2019
@spaghetticode spaghetticode mentioned this pull request Feb 14, 2019
0 of 5 tasks complete
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch from d8d92c6 Feb 14, 2019
@kennyadsl kennyadsl changed the title Activesupport notifications ActiveSupport notifications for Events Handling Feb 25, 2019
core/lib/spree/event.rb Show resolved Hide resolved
module Spree
module Event
module Adapters
module ActiveSupportNotifications

This comment has been minimized.

Copy link
@skukx

skukx Feb 25, 2019

Contributor

This is more of a question and less of a request. Would it be worth implementing more of an interface or strategy pattern? So instead of defining a module, have a base abstract class which all adapters should inherit from an therefore expected to implement instrument, subscribe, etc.

I know I'm splitting hairs here but I love to learn

This comment has been minimized.

Copy link
@spaghetticode

spaghetticode Feb 25, 2019

Author Member

I guess it mostly depends on specific situations. The main advantage of using an adapter is that you can easily change the adapter at runtime and still call the same interface and get the work done.

If you are thinking about any specific implementation that may work better in this situation then please share it, all improvements are welcome :)

This comment has been minimized.

Copy link
@skukx

skukx Feb 25, 2019

Contributor

I think this is great, I say splitting hairs because they all accomplish the same exact thing. Some are just more strictly enforced than others

core/app/models/spree/order.rb Outdated
updater.update_shipment_state
save!
updater.run_hooks
Spree::Event.instrument 'order_finalize', order: self do

This comment has been minimized.

Copy link
@skukx

skukx Feb 25, 2019

Contributor

I think we should have a constant for each event topic.

Spree::EventTopics::ORDER_FINALIZE
#or 
Spree::EventTopics::Order::FINALIZE

This comment has been minimized.

Copy link
@spaghetticode

spaghetticode Feb 25, 2019

Author Member

Yes, maybe. The benefit I can see for using constants is that we can kind of document all the events at the top of each class. It would be easier to spot all of them. Do you have other benefits in mind?

There's no hard constraint that would force devs to use constants, so the rule can be easily bypassed. A rule easily bypassable is usually not worth much.

AFAIK Rails codebase does not use any constant with ActiveSupport notifications, so maybe we can use the same direct approach without loosing much? Still pondering our options...

This comment has been minimized.

Copy link
@skukx

skukx Feb 25, 2019

Contributor

I think you're right, we don't lose much by not having constants. However thinking from an extension stand point it would be easier to manage if constants were available.

Lets say I have many subscribers in my codebase defined with the static string 'order_finalize' and one day down the road that topic name changes. When I run bundle update, all of sudden my code breaks. If I were to define my subscribers with the constant, running bundle update wouldn't break a thing. In general I always think it's best to avoid magic values when possible

This comment has been minimized.

Copy link
@spaghetticode

spaghetticode Feb 26, 2019

Author Member

👍 I agree constants are stronger than simple strings, but constant renaming and behavior changes may anyway break things in the future. I'm still thinking about other ways to make this more robust. Let's wait for some other feedback on this, thank you!

This comment has been minimized.

Copy link
@kennyadsl

kennyadsl Feb 28, 2019

Member

Lets say I have many subscribers in my codebase defined with the static string 'order_finalize' and one day down the road that topic name changes. When I run bundle update, all of sudden my code breaks.

That's true, even if no one ensures us that someone just skipped the usage of the constant and used its content (the string) instead to subscribe to events. In that case, those stores would break in the same way. I imagine a way to change an event name could be:

Spree::Event.instrument 'new_event_name', order: self do
  # Do stuff

  # Instrument the old event as well, so old subscribers will be happy
  Spree::Event.instrument 'old_event_name', order: self
end

We should also find a way in the Spree::Event#subscribe to deprecate events so subscribers to old events would be notified that it's time to change the name of the event.

My point is that if we decide to use constants but we still need to implement something similar cause we can't be sure devs used strings or constants while subscribing to event, I can't see a real advantage introducing them.

This comment has been minimized.

Copy link
@skukx

skukx Feb 28, 2019

Contributor

I agree, I've come to a similar conclusion. It would make more sense for the extension that may create many subscribers to define the constant themselves at their discretion since they'd be the ones repeating that string throughout the codebase

@skukx

This comment has been minimized.

Copy link
Contributor

skukx commented Feb 25, 2019

I'm really excited for this!

Copy link
Contributor

aitbw left a comment

Fantastic work, @spaghetticode 🎉 I've left some comments as further improvements ☺️

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/spree/event_spec.rb Show resolved Hide resolved
core/spec/models/spree/reimbursement_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

tvdeyen left a comment

I appreciate the work on this. It’s greatly done.

But I have some questions/comments:

  1. Why do we have the concept of adapters (for what I don’t see a necessity for, tbh), but the same time just mimicking ActiveSupport::Notifications? Why wrap the code in a block for instance? Active support uses this to instrument the code, but we don’t have to. This makes the whole implementation very complex and hard to reason about when a event is actually fired. Before order update? Or after order update? That leads me to
  2. We need to have more events than just one when something updates. We need to know when an event fired. “Before order update” and “after order update”. We do not need to introduce this in this PR, but we should at least name the first event correctly then. In this case it seems to be “after order updates”
  3. Spree::Event.instrument does not seem like a good name for dispatching events. fire, commit, dispatch seem better candidates IMO

I like the whole idea of an event bus, but as this is the foundation for lots of events in the future we need to agree on a reasonable API.

Thanks for working on this 👌

@skukx

This comment has been minimized.

Copy link
Contributor

skukx commented Mar 1, 2019

@tvdeyen I can't answer all but maybe a few of the issues you raised.

Why do we have the concept of adapters

This is what I actually like most about this implementation. Using the adapter pattern allows developers to not be locked into one event store. For instance if I wanted to use Redis as in event store, I could write an adapter and plug it in to use that instead.

What I don't know, is that if I already have redis setup for my ActiveSupport caching, is it essentially accomplishing the same thing?

Spree::Event.instrument does not seem like a good name for dispatching events. fire, commit, dispatch seem better candidates IMO

I believe the reason instrument is used is because it mimics the rails api for issuing notifications:
https://guides.rubyonrails.org/active_support_instrumentation.html#creating-custom-events

@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch to 1acad92 Mar 5, 2019
@spaghetticode

This comment has been minimized.

Copy link
Member Author

spaghetticode commented Mar 5, 2019

@skukx thanks for your answer to @tvdeyen questions, what you wrote is much similar to what floats in mind 👍

Still I'd like to expand a bit on the questions:

Why wrap the code in a block for instance? Active support uses this to instrument the code, but we don’t have to
I'm not 100% sure what you mean here... if are you referring for example to the block enclosed in this:

Spree::Event.instrument 'order_finalize', order: self do
  all_adjustments.each(&:finalize!)
  # ...
end

then yes, there's no real need to wrap the code inside a block. We could move the event instrumentation code at the bottom of the method, but I personally prefer the other way for some reasons:

  • the block clarifies what actions are part of the order finalization process (you may argue that in this specific example the method is already named finalize!, so there's no need to add another wrapper, but this is just a basic example... if I had to stick to a convention I'd wrap rather than not, at the cost of being sometimes redundant)
  • you get the ability to do what Rails does, for example calculating the time required to perform the action (the code inside the block)

Anyway, I think both ways can be used depending on the situation

We need to have more events than just one when something updates.

The examples I provided are just examples that show how to change some previous code to something that is still very similar but can emit events we can leverage on. The order finalization event was a low-hanging fruit, as the method was already there, and it made sense being able to subscribe to an event about order finalization.

Of course I agree this is just a step in the process, so there's much more that can be done, but I personally don't feel comfortable in modifying the existing processes to that extent, at least not now.
Maybe someone more experienced than me with Solidus and its processes can come up with better examples that go in that direction.

Spree::Event.instrument does not seem like a good name for dispatching events. fire, commit, dispatch seem better candidates IMO

I also rather prefer a different naming, but I'd stick with ActiveSupport names for these reasons:

  • we're all Rails programmers, so it makes totally sense to me using the same interface (many of us are possibly already accustomed to it)
  • ATM the only provided adapter is for ActiveSupport::Notification, so to me it makes sense sticking with its interface
  • I don't know how successful the ActiveSupport::Notification adapter is going to be, but if it is going to be the default one, I think choosing the same interface makes sense
@mdesantis

This comment has been minimized.

Copy link
Contributor

mdesantis commented Mar 6, 2019

Regarding instrument with or without block: I think that if the reported timings are not used the block is redundant and even misleading: looking at it it tells me that there is a reasoning behind it and that it is required for something.

the block clarifies what actions are part of the order finalization process (you may argue that in this specific example the method is already named finalize!, so there's no need to add another wrapper, but this is just a basic example...

Exactly, there's the method scope for that

if I had to stick to a convention I'd wrap rather than not, at the cost of being sometimes redundant

if I had to stick to a convention I'd not wrap rather than yes, because we are using ActiveSupport::Notification as event dispatcher, we aren't using it as timings reporter, so the blocks are redundant

you get the ability to do what Rails does, for example calculating the time required to perform the action (the code inside the block)

But since does timings are not used it just adds unneeded complexity; also, even if you need those timings, the code wrapped by the block should accidentally correspond with the code that you need to measure, so it's pretty unreliable to me

@tvdeyen

This comment has been minimized.

Copy link
Member

tvdeyen commented Mar 6, 2019

@spaghetticode I agree with @mdesantis here.

Only because the ActiveSupport::Notifications is our current implementation we should not use its interface to design ours and not tightly couple us to it. Rather abstract the interface into something that can easily be used in our code case and is adapter agnostic. All the integration work should be done in the adapter instead.

I am in favor of

  • Renaming Spree::Event.instrument into Spree::Event.fire
  • Make it a simple call that does not wrap code in a block

So the only change we need to make is add a single line into the order finalize! method:

Spree::Event.fire('order_finalize', order: self)
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch Mar 11, 2019
spaghetticode added 20 commits Feb 6, 2019
Rails guides suggest to:

You should follow Rails conventions when defining your own events. The format
is: event.library. If your application is sending Tweets, you should create an
event named tweet.twitter.

Here are some event names from the Rails codebase:
perform_action.action_cable
transmit_subscription_confirmation.action_cable
ead_fragment.action_mailer
process_action.action_controller
`Extract Spree::Event#adapter` can now be configured in order to easily use a
custom event adapter.

The default adapter is `Spree::Event::Adapters::ActiveSupportNotifications`.
This new method returns a hash with a list of all `.spree` namespaced event names and their subscriptions.
Besides unsubscribing from a single subscription object, it is now possible to
unsubscribe from all subscriptions bound to an event name.
This helps keeping the name interface consistent with the default event adapter
`ActiveSupport::Notifications`.
This can be changed easily with, for example:
`Spree::Config.event_adapter_class_name = "Spree::EventBus"`
The module can be used to store all the `Spree::Event` configuration details.

The configuration is available at `Spree::Config.events`, at the moment the
only available setting is `Spree::Config.events.adapter`
`suffix` is more used than `postfix`, so the remaning seems reasonable.

Also, the constant that stored the value has been removed in favor of another
entry in `Spree::Event::Configuration`.
…namespace

Since there can be multiple event processors, it makes sense having a dedicated
namespace for them.

`SUBSCRIPTIONS` names should be ordered alphabetically and one per line for
clarity and readability.
The name `fire` is more suitable than `instrument` for our Event library.
No need for redundant blocks to explicitly wrap the code logically bound to the
event. The method definitions are quite self-explaining themseleves.
Event names should be in the past, as events are relevant only after they
happen.

Also, the new event "reimbursement_errored" was added besides the event
"reimbursement_reimbursed".
Some documentation has been added in order to describe briefly how events work
in Solidus.
`Spree::Event` must be loaded before loading `Spree::Core::Engine` as the
latter includes some event subscriptions.
The module `Spree::Event::Subscriber` can be used to easily subscribe to
events.

The new interface allows to add event_actions that match the event name:

`event_action :order_finalized`

or event actions with a different name than the event name they subscribe:

`event_action :send_reimbursement_email, event_name: :reimbursement_reimbursed`
@spaghetticode spaghetticode force-pushed the spaghetticode:activesupport-notifications branch to 505460f Apr 3, 2019
@jacobherrington jacobherrington self-requested a review Apr 3, 2019
Copy link
Member

kennyadsl left a comment

This PR looks fantastic, thanks @spaghetticode !

@kennyadsl kennyadsl merged commit 3e6de0c into solidusio:master Apr 15, 2019
15 checks passed
15 checks passed
Header rules - solidus-docs No header rules processed
Details
Header rules - solidus-guides No header rules processed
Details
Pages changed - solidus-docs 301 new files uploaded
Details
Pages changed - solidus-guides 1 new file uploaded
Details
Redirect rules - solidus-docs No redirect rules processed
Details
Redirect rules - solidus-guides No redirect rules processed
Details
Hound No violations found. Woof!
Mixed content - solidus-docs No mixed content detected
Details
Mixed content - solidus-guides No mixed content detected
Details
ci/circleci: mysql Your tests passed on CircleCI!
Details
ci/circleci: mysql_rails51 Your tests passed on CircleCI!
Details
ci/circleci: postgres Your tests passed on CircleCI!
Details
ci/circleci: postgres_rails51 Your tests passed on CircleCI!
Details
netlify/solidus-docs/deploy-preview Deploy preview ready!
Details
netlify/solidus-guides/deploy-preview Deploy preview ready!
Details
This was referenced Apr 15, 2019
elia added a commit to nebulab/solidus that referenced this pull request Jan 23, 2020
Was removed as part of solidusio#3081,
but not being private it's better to keep it around with a deprecation
for a while.
kennyadsl added a commit to nebulab/solidus that referenced this pull request Feb 4, 2020
Was removed as part of solidusio#3081,
but not being private it's better to keep it around with a deprecation
for a while.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.