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

Add event bus #2431

Closed
wants to merge 10 commits into from
Closed

Add event bus #2431

wants to merge 10 commits into from

Conversation

swcraig
Copy link
Contributor

@swcraig swcraig commented Dec 7, 2017

Related to #2430, this is an "event bus" implementation that @derrickp and I have been working on.

Spree::MailProcessor subscribes to Spree::EventBus which publishes events.

Spree::MailProcessor can be used as a template for how to create new processors (and how to subscribe to events). The processor implements a #register! method defining its subscriptions. This class will presumably be ripped out into its own extension after this PR is merged.

This PR does not handle hierarchical events or asynchronous/distributed processing.

@swcraig swcraig mentioned this pull request Dec 7, 2017
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort on this, I think this is close to something that we should seriously consider to merge in core.

This seems to be perfect for mail processor or other notifications stuff but it's not immediately clear to me how an extension or a store should structure their code to subscribe to these events, probably to make unexpected things which are not related to sending notifications.

I think it could be a good idea to play a bit and propose a solid path for these scenarios, maybe it will also make us figure out something better in terms of naming modules/classes or filesystem structure.

Thanks again!

deliver_order_confirmation_email unless confirmation_delivered?
Spree.event_bus.publish(
:order_confirmed,
Spree::Events::OrderConfirmedEvent.new(order: self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about using configurable classes instead? Spree::Config.order_confirmed_event_class.new(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to ensure we have a strong API, so anyone that wishes to count on these events through their app or extensions can expect events to look a certain way.
If someone wanted to write their own event class (that, for instance, supported more things), they could catch this one and re-broadcast a different one, or override the code that triggers the event.

@@ -39,6 +39,10 @@ class Engine < ::Rails::Engine
Migrations.new(config, engine_name).check
end

initializer 'spree.core.register_mail_processor' do |_app|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about a more generic initializer 'spree.core.register_event_processors'?

Spree::Config.event_bus.subscribe(:order_cancelled, ->(event) { send_cancel_email(event) })
Spree::Config.event_bus.subscribe(:carton_shipped, ->(event) { send_carton_shipped_emails(event) })
Spree::Config.event_bus.subscribe(:reimbursement_processed, ->(event) { send_reimbursement_email(event) })
Spree::Config.event_bus.subscribe(:order_inventory_cancelled, ->(event) { send_inventory_cancellation_email(event) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible (easy) for stores or extensions to add other subscriptions to this list? One way could be creating a custom MailProcessor class that inherits from this one, but we should probably use something like

Spree::Config.mail_processor_class.register!

instead of

Spree::Events::Processors::MailProcessor.register!

into core/engine.rb

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is to eventually pull this mail processor out entirely, and move it to an solidus_action_mailer extension, eventually, so a Config for mail_processor_class doesn't need to be supported by core at all.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only time you'd need to modify this list specifically was if you wanted to change the MailProcessor list, which as @cbrunsdon mentioned eventually might be pulled out entirely.

However, if all you wanted to do was subscribe to the same event, any other object can call Spree::Config.event_bus.subscribe(:order_cancelled, OTHER_PROC) and it will not change the existing subscriptions. You can have any number of subscribers to an event, so a store can fairly easily call that function to add themselves to the list if they had some functionality they wanted to made sure happened in response to the event. The only concern then is that they call subscribe early enough in the startup of the app so as to be able to get the events when they happen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favour of removing the class out of core and move this into an action mailer extension instead. Then provide an easy (as in not class_eval) configuration for people wanting to enable/disable certain event handlers. But this can be discussed in that new extension instead of core.

class MailProcessor
class << self
def register!
Spree::Config.event_bus.subscribe(:order_confirmed, ->(event) { send_confirm_email(event) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this, but maybe method(:send_confirm_email) is the same here.

@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
@solidusio solidusio deleted a comment from houndci-bot Dec 7, 2017
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastically clean code and looks like it will be extendable for many use cases.

In the first commit message, @derrickp is given credit, but his Github username there might generate unwanted notifications.

@aaronlifton
Copy link

I really like this idea - the only problems I see are not being able to easily modify or delete the default spree subscriptions.

@fylooi
Copy link
Contributor

fylooi commented Dec 11, 2017

This is a very extensible approach, but I feel that care needs to be taken to maintain accessibility for Solidus beginners and/or stores which don't need to do much customization.

My experience replacing the Order mailers with a Mailchimp service call as a first time Solidus user:

  • Figuring out where the mailer gets triggered was a challenge.
  • Had to follow the order flow through solidus_frontend into solidus_core through Order and `Checkout.
  • Monkey patched Spree::OrderMailer.confirm_email using an OrderMailerDecorator.

With this approach, I would need to understand the event bus design pattern & implementation in addition to Solidus's core modeling and checkout structure. Some documentation in the code would go a long way.

@swcraig
Copy link
Contributor Author

swcraig commented Dec 12, 2017

Thank you for the feedback @fylooi and @aaronlifton2.

Would the sort of documentation I've added to Spree::EventBus and Spree::MailProcessor be of value?

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like where this is going 🤗.

We should not ship the action mailer processor within core, but in an extension instead.

Also, I prefer Spree.events or Spree.notifications as a name over Spree.event_bus. But this is just my 2 cents. If everybody likes the name, go for it 😏

send_shipment_emails(carton) if stock_location.fulfillable? && !suppress_mailer # e.g. digital gift cards that aren't actually shipped
Spree.event_bus.publish(
:carton_shipped,
Spree::Events::CartonShippedEvent.new(carton: carton, suppress_mailer: suppress_mailer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like our event depend on suppress_mailer here. Can't we just pass the order instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or actually pass the carton - it holds a reference to the order(s).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already passing the carton and as the carton only nows it's orders we do not know the order object that sends this event, though.

Copy link
Contributor Author

@swcraig swcraig Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is not ideal. I've removed suppress_mailer from the event object, and have made a (possibly presumptuous) decision within the MailProcessor:

return if event.carton.shipments.any?(&:suppress_mailer)

The suppress_mailer flag is defined on a Shipment. Having the carton and order in the event does not give the necessary context of which shipment has triggered the CartonShippedEvent.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not send the event if suppress_mailer is true? This is it’s whole purpose right? “Do not send” as in “do not notify the user”, whatever the method of notification is.

Copy link
Contributor Author

@swcraig swcraig Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for customer-related notifications, but couldn't there be other processors that don't care about the suppress_mailer flag? For example, a processor that sends event information to a service like statsd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. You are right we should not suppress events ever. But then we need a way for stores to be able to alter already registered event handlers.

# To override/modify which notifications are sent, add a decorator for
# this file to define your own `#register!` method. For example:
#
# Spree::Events::Processors::MailProcessor.class_eval do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we plan to move this class into an action mailer extension anyway and people are able to write their own mail processors we do not need to imply class_eval is a good idea to use the event bus. This violates the whole purpose of an event bus, no?

Spree::Config.event_bus.subscribe(:order_cancelled, ->(event) { send_cancel_email(event) })
Spree::Config.event_bus.subscribe(:carton_shipped, ->(event) { send_carton_shipped_emails(event) })
Spree::Config.event_bus.subscribe(:reimbursement_processed, ->(event) { send_reimbursement_email(event) })
Spree::Config.event_bus.subscribe(:order_inventory_cancelled, ->(event) { send_inventory_cancellation_email(event) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favour of removing the class out of core and move this into an action mailer extension instead. Then provide an easy (as in not class_eval) configuration for people wanting to enable/disable certain event handlers. But this can be discussed in that new extension instead of core.

end

def send_carton_shipped_emails(event)
return if event.suppress_mailer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we pass the order instead of surpress_mailer

return if event.order.suppress_mailer

@@ -389,6 +389,10 @@ def stock
@stock_configuration ||= Spree::Core::StockConfiguration.new
end

def event_bus
@event_bus_class ||= Spree::EventBus.instance
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should live in Spree::Config. The class is not configurable, but the name implies it is.

Defining this in Spree.event_bus should be sufficient.

@@ -42,6 +42,11 @@ def self.config(&_block)
yield(Spree::Config)
end

# Convenience method to access the configured event bus
def self.event_bus
Spree::Config.event_bus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the Spree::Config namespace? Could we use

def self.event_bus
  @event_bus_class ||= Spree::EventBus.instance
end

instead?

@@ -51,10 +51,12 @@ class IncompleteReimbursementError < StandardError; end
# These are called if the call to "reimburse!" succeeds.
class_attribute :reimbursement_success_hooks
self.reimbursement_success_hooks = []
deprecate :reimbursement_success_hooks, 'Add a processor for the :reimbursement_processed event instead.', deprecator: Spree::Deprecation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecating implies that this will be removed in a future version, but as far as I can tell the changes later in this file have made this entirely non-functional.


def subscribe(name, proc_to_call)
subscription = @subscriptions[name]
already_subbed = subscription.any? { |sub_proc| sub_proc == proc_to_call }
Copy link
Contributor

@jhawthorn jhawthorn Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could subscription.include? work here?

@jhawthorn
Copy link
Contributor

I like a lot of where this is going.

My big concern is still: at what time and in what context do the event callbacks run?

The current implementation has a lot of behaviour implied:

  • Callbacks might run inside a transaction from the "publishing" code
    • An exception in the callback will roll back the transaction 💣
    • The transaction might rolled back after the callback is run (either by a later callback or by containing code) 💣
  • An exception in a callback prevents all further callbacks
  • Callbacks are (maybe?) run within a request
    • I18n.locale will be set correctly for the user 👍
    • An exception in the callback will cause the request to return an error respones (500, 404, etc. Depending on exception)
  • The callback is passed a live AR object
    • there may be unsaved changes from the "publishing" code (we could be careful to avoid this)
    • unsaved attribute changes from the callback may be saved by the containing code 💣
      • or a subsequent callback 💣
    • There may be stale associations (we've worked hard to fix associations becoming stale, but it can probably still happen, especially in areas involving inventory_units)

Since we are currently only using this for emails, these aren't issues, thanks to ActiveJob, but as soon as we intend these extensions to be used by users or extensions, some behaviours here are going to cause problems and others are going to be relied upon.

I don't know the best solution, but I think we should consider running all callbacks through ActiveJob, which solves most of these problems.

@jhawthorn jhawthorn added this to the 2.5.0 milestone Dec 22, 2017
@jhawthorn jhawthorn removed this from the 2.5.0 milestone Feb 14, 2018
@swcraig swcraig changed the title [RFC] Add event bus Add event bus Feb 14, 2018
@fastjames
Copy link
Contributor

Every time I read this PR I get a little bit giddy, as it would make the job of extension authors (like me) much easier.

@jhawthorn I understand what you are saying about a callback not getting a consistent environment.
I would be fine with events always running through ActiveJob as I think that enforces the right kind of decoupling. If someone then wanted a synchronous event, it feels like "publish event and wait for some sort of resolution" could be built as a feature atop the first part.

I would love to talk more about this feature and what, if anything, we can do to help it see the light of day.

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead.
(obsolete configuration found in .rubocop.yml, please update it)

@@ -0,0 +1,82 @@
require 'rails_helper'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.


RSpec.describe Spree::EventBus do
class SomeEventClass; end
let(:event_bus) { Spree::EventBus.instance }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/ExtraSpacing: Unnecessary spacing detected.

@@ -0,0 +1,39 @@
require 'rails_helper'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@@ -0,0 +1,11 @@
module Spree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@@ -0,0 +1,11 @@
module Spree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@@ -0,0 +1,44 @@
module Spree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

else
errored!
reimbursement_failure_hooks.each { |h| h.call self }
Spree.event_bus.publish(
Spree::Events::ReimbursementFailedEvent.new(reimbursement_id: self.id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/RedundantSelf: Redundant self detected.

@@ -108,10 +108,15 @@ def perform!
if unpaid_amount_within_tolerance?
reimbursed!
reimbursement_success_hooks.each { |h| h.call self }
send_reimbursement_email
Spree.event_bus.publish(
Spree::Events::ReimbursementProcessedEvent.new(reimbursement_id: self.id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/RedundantSelf: Redundant self detected.

@@ -885,14 +882,12 @@ def after_cancel
payments.completed.each { |payment| payment.cancel! unless payment.fully_refunded? }
payments.store_credits.pending.each(&:void_transaction!)

send_cancel_email
Spree.event_bus.publish(
Spree::Events::OrderCancelledEvent.new(order_id: self.id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/RedundantSelf: Redundant self detected.

@@ -438,7 +438,9 @@ def finalize!

touch :completed_at

deliver_order_confirmation_email unless confirmation_delivered?
Spree.event_bus.publish(
Spree::Events::OrderConfirmedEvent.new(order_id: self.id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/RedundantSelf: Redundant self detected.

Spree.event_bus.subscribe(Spree::Events::OrderCancelledEvent, ->(event) { send_cancel_email(event) })
Spree.event_bus.subscribe(Spree::Events::CartonShippedEvent, ->(event) { send_carton_shipped_emails(event) })
Spree.event_bus.subscribe(Spree::Events::ReimbursementProcessedEvent, ->(event) { send_reimbursement_email(event) })
Spree.event_bus.subscribe(Spree::Events::OrderInventoryCancelledEvent, ->(event) { send_inventory_cancellation_email(event) })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/ExtraSpacing: Unnecessary spacing detected.

@@ -40,7 +40,11 @@ def short_ship(inventory_units, whodunnit: nil)
end

update_shipped_shipments(inventory_units)
Spree::OrderMailer.inventory_cancellation_email(@order, inventory_units.to_a).deliver_later if Spree::OrderCancellations.send_cancellation_mailer
Spree.event_bus.publish(
event = Spree::Events::OrderInventoryCancelledEvent.new(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/UselessAssignment: Useless assignment to variable - event.

@swcraig
Copy link
Contributor Author

swcraig commented Apr 19, 2018

Thank you for the encouraging words @fastjames.

I've gone ahead and stopped sending live AR objects to the processors. I have also moved the publishing of events to be after our transactions have completed. Thank you @jhawthorn for the feedback.

I think that it should be up to specific processors to make the decision whether to run their code in-process or through ActiveJob. This PR is not intended to implement an asynchronous message queue. The event bus (we are open to changing the nomenclature) provides a better mechanism for stores to add their own custom callbacks to run given the firing of an event. In my opinion, if one of my callbacks (subscribers) throws an exception I want my system to blow up. I don't want my application to continue execution in a possibly compromised state. If I want to write a subscriber that runs asynchronously, or does aggressive exception handling, or sends things to SQS for example, I am still free to do that.

This implementation does not stop someone from writing asynchronous event subscribers, and I think defaulting to ActiveJob makes it more difficult to write in-process handlers, rather than vice-versa.

If this still isn't what we are going for, or we have misunderstood the concerns about this, we are very open to continue making changes and get this into a merge-able state.

@askl56
Copy link

askl56 commented May 21, 2018

Is there any update on this being merged/where it fits into the roadmap?

@kennyadsl kennyadsl added this to the 3.0 milestone Jun 13, 2018
We need an easier way to hook into events such as an order being
confirmed/cancelled, a carton getting shipped, et cetera.

The EventBus delegates events to appropriate processors (classes that
run procs, see the Spree::Events::Processors::MailProcessor as an
example). This is an in-process publish/subscribe model. To add
behaviour to run after an event, a developer should create a processor
and subscribe it to the appropriate event class, such as
`Spree::Events::OrderConfirmedEvent`.

See PR 2431 for an example of how this works in practice.
This class subscribes to events (via the Spree::EventBus) and runs its
defined methods when appropriate. Doing this decouples specific
ActionMailer calls from "events" (at the moment mostly Rails callbacks)
in Solidus. Developers can follow this class as an example on how to
create a processor for the Spree::EventBus.
These are the events that are currently in Solidus. Representing them as
classes makes them self documenting and gives a good template for other
stores that wish to add their own custom events.
Instead of calling ActionMailer methods directly from the Order code, we
can decouple it via publishing to the EventBus. This way stores can
subscribe to these events and create their own custom processors to
programmatically send/not send emails when different things happen in
Solidus.

See PR 2431 as a working example of using the EventBus.
Instead of calling ActionMailer methods directly from the cancellations
code, we can decouple it via publishing to the EventBus. This way stores
can subscribe to these events and create their own customer processors
to programmatically send/not send emails when different things happen in
Solidus.

See PR 2431 as a working example of using the EventBus.
Instead of calling ActionMailer methods directly from the shipping code,
we can decouple it via publishing to the EventBus. This way stores can
subscribe to these events and create their own customer processors to
programmatically send/not send emails when different things happen in
Solidus.

See PR 2431 as a working example of using the EventBus.
Instead of calling ActionMailer methods directly from the reimbursement
code, we can decouple it via publishing to the EventBus. This way stores
can subscribe to these events and create their own customer processors
to programmatically send/not send emails when different things happen in
Solidus.

See PR 2431 as a working example of using the EventBus.
Just a little nice-to-have for devs to reference the EventBus in the
Spree namespace.
This is how to hook a processor and subscribe it to events in the
system. The `register!` method is called in an initializer so that it is
hooked up _before_ any events happen in the system.
Since our events are not given live AR objects there is no way for us to
determine the value of the `suppress_mailer` option that was selected
when a carton was shipped.

We didn't want to persist this flag on a `Shipment`, and conditionally
using ActionMailer in the API controller is exactly what PR 2431 is
working to avoid.
@swcraig
Copy link
Contributor Author

swcraig commented Jun 15, 2018

This is good for a merge when we are ready. Thank you everyone for the feedback.

@fastjames
Copy link
Contributor

Still burning a 🕯️ for this one. To blend with v2.7 it needs to be modified slightly, pointing to Spree::Config.order_mailer_class and Spree::Config.reimbursement_mailer_class rather than calling the classes directly.

@swcraig
Copy link
Contributor Author

swcraig commented Oct 9, 2018

Thanks for pointing out the stale code @fastjames. I think the community is still unsure whether they want to see this change in Solidus. Once we get the green light I will rebase and update the branch with the changes.

@dhonig
Copy link

dhonig commented Oct 9, 2018 via email

@@ -884,12 +881,11 @@ def after_cancel
payments.completed.each { |payment| payment.cancel! unless payment.fully_refunded? }
payments.store_credits.pending.each(&:void_transaction!)

send_cancel_email
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a situation recently where the removal of this method caused problems with an extension that tried to override it. Would it be possible to leave the send_foo_email methods in place and have them call the publish method instead? That would allow extension authors to update their tricks before things stop working.

@kennyadsl
Copy link
Member

Closing in favor of #3081, which has been now merged, thanks a lot for this work anyway!

@kennyadsl kennyadsl closed this Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet