Skip to content

Commit

Permalink
Centralize legacy event bus deprecation and test legacy on CI
Browse files Browse the repository at this point in the history
This commit adds a new job on CI to perform the test suite with the
legacy adapter configured.

Besides that, it extracts the deprecation logic into its module to avoid
having it scattered through different parts of the system.

Relates to discussion:

solidusio#4130 (comment)
  • Loading branch information
waiting-for-dev authored and cpfergus1 committed Aug 25, 2022
1 parent 3130803 commit 85dae08
Show file tree
Hide file tree
Showing 9 changed files with 234 additions and 61 deletions.
10 changes: 10 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ jobs:
- setup
- test

event_bus_legacy_adapter:
executor: postgres
parallelism: *parallelism
environment:
CI_LEGACY_EVENT_BUS_ADAPTER: '1'
steps:
- setup
- test

workflows:
build:
jobs:
Expand All @@ -141,6 +150,7 @@ workflows:
- mysql
- postgres_rails60
- postgres_rails52
- event_bus_legacy_adapter
- stoplight/push:
context: "Solidus Core Team"
project: solidus/solidus-api
Expand Down
17 changes: 9 additions & 8 deletions core/lib/spree/event.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
# frozen_string_literal: true

require_relative 'event/adapters/active_support_notifications'
require_relative 'event/subscriber_registry'
require_relative 'event/adapters/deprecation_handler'
require_relative 'event/configuration'
require_relative 'event/listener'
require_relative 'event/subscriber_registry'
require_relative 'event/subscriber'

module Spree
Expand Down Expand Up @@ -99,7 +100,7 @@ def fire(event_name, opts = {}, &block)
def subscribe(event_name, adapter: default_adapter, &block)
event_name = normalize_name(event_name)
adapter.subscribe(event_name, &block).tap do
if legacy_adapter?(adapter)
if deprecation_handler.legacy_adapter?(adapter)
listener_names << adapter.normalize_name(event_name)
end
end
Expand Down Expand Up @@ -154,7 +155,7 @@ def unsubscribe(subscriber_or_event_name, adapter: default_adapter)
# # => {"order_finalized"=> [#<Spree::Event::Listener...>],
# "reimbursement_reimbursed"=> [#<Spree::Event::Listener...>]}
def listeners(adapter: default_adapter)
if legacy_adapter?(adapter)
if deprecation_handler.legacy_adapter?(adapter)
adapter.listeners_for(listener_names)
else
init = Hash.new { |h, k| h[k] = [] }
Expand Down Expand Up @@ -224,8 +225,8 @@ def handle_block_on_fire(block, opts, adapter)
order.do_something
Spree::Event.fire 'event_name', order: order
MSG
if legacy_adapter?(adapter)
Spree::Deprecation.warn <<~MSG
if deprecation_handler.legacy_adapter?(adapter)
Spree::Deprecation.warn <<~MSG if deprecation_handler.render_deprecation_message?(adapter)
Blocks on `Spree::Event.fire` are ignored in the new adapter
`Spree::Event::Adapters::Default`, and your current adapter
(`Spree::Event::Adapters::ActiveSupportNotifications`) is deprecated.
Expand All @@ -247,8 +248,8 @@ def handle_block_on_fire(block, opts, adapter)
end
end

def legacy_adapter?(adapter)
adapter == Adapters::ActiveSupportNotifications
def deprecation_handler
Adapters::DeprecationHandler
end
end
end
29 changes: 29 additions & 0 deletions core/lib/spree/event/adapters/deprecation_handler.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# frozen_string_literal: true

require 'spree/event/adapters/active_support_notifications'
require 'spree/deprecation'

module Spree
module Event
module Adapters
# @api private
module DeprecationHandler
LEGACY_ADAPTER = ActiveSupportNotifications

CI_LEGACY_ADAPTER_ENV_KEY = 'CI_LEGACY_EVENT_BUS_ADAPTER'

def self.legacy_adapter?(adapter)
adapter == LEGACY_ADAPTER
end

def self.legacy_adapter_set_by_env
return LEGACY_ADAPTER if ENV[CI_LEGACY_ADAPTER_ENV_KEY].present?
end

def self.render_deprecation_message?(adapter)
legacy_adapter?(adapter) && legacy_adapter_set_by_env.nil?
end
end
end
end
end
83 changes: 37 additions & 46 deletions core/lib/spree/event/configuration.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
# frozen_string_literal: true

require 'spree/core/versioned_value'
require 'spree/event/adapters/deprecation_handler'
require 'spree/event/adapters/active_support_notifications'
require 'spree/event/adapters/default'

module Spree
module Event
Expand All @@ -17,64 +16,56 @@ def autoload_subscribers
@autoload_subscribers.nil? ? true : !!@autoload_subscribers
end

# TODO: Update to Adapters::Default.new on Solidus 4
def adapter
@adapter ||= Spree::Core::VersionedValue.new(
Spree::Event::Adapters::ActiveSupportNotifications,
"4.0.0.alpha" => Spree::Event::Adapters::Default.new
).call.tap do |value|
deprecate_if_legacy_adapter(value)
end
end

# Only used by {Spree::Event::Adapters::ActiveSupportNotifications}.
def suffix
@suffix ||= '.spree'
end

private

def deprecate_if_legacy_adapter(adapter)
Spree::Deprecation.warn <<~MSG if adapter == Spree::Event::Adapters::ActiveSupportNotifications
`Spree::Event::Adapters::ActiveSupportNotifications` adapter is
deprecated. Please, take your time to update it to an instance of
`Spree::Event::Adapters::Default`. I.e., in your `spree.rb`:
@adapter ||= Adapters::ActiveSupportNotifications.tap do |adapter|
Spree::Deprecation.warn <<~MSG if Adapters::DeprecationHandler.render_deprecation_message?(adapter)
`Spree::Event::Adapters::ActiveSupportNotifications` adapter is
deprecated. Please, take your time to update it to an instance of
`Spree::Event::Adapters::Default`. I.e., in your `spree.rb`:
require 'spree/event/adapters/default'
Spree.config do |config|
# ...
config.events.adapter = Spree::Event::Adapters.Default.new
# ...
end
require 'spree/event/adapters/default'
That will be the new default on Solidus 4.
Spree.config do |config|
# ...
config.events.adapter = Spree::Event::Adapters.Default.new
# ...
end
Take into account there're two critical changes in behavior in the new adapter:
That will be the new default on Solidus 4.
- Event names are no longer automatically suffixed with `.spree`, as
they're no longer in the same bucket that Rails's ones. So, for
instance, if you were relying on a global subscription to all Solidus
events with:
Take into account there're two critical changes in behavior in the new adapter:
Spree::Event.subscribe /.*\.spree$/
- Event names are no longer automatically suffixed with `.spree`, as
they're no longer in the same bucket that Rails's ones. So, for
instance, if you were relying on a global subscription to all Solidus
events with:
You should change it to:
Spree::Event.subscribe /.*\.spree$/
Spree::Event.subscribe /.*/
You should change it to:
- Providing a block to `Spree::Event.fire` is no longer supported. If
you're doing something like:
Spree::Event.subscribe /.*/
Spree::Event.fire 'event_name', order: order do
order.do_something
end
- Providing a block to `Spree::Event.fire` is no longer supported. If
you're doing something like:
You now need to change it to:
Spree::Event.fire 'event_name', order: order do
order.do_something
end
Spree::Event.fire 'event_name', order: order
You now need to change it to:
order.do_something
Spree::Event.fire 'event_name', order: order
MSG
end
end

MSG
# Only used by {Spree::Event::Adapters::ActiveSupportNotifications}.
def suffix
@suffix ||= '.spree'
end
end
end
Expand Down
7 changes: 5 additions & 2 deletions core/lib/spree/testing_support/dummy_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,11 @@ class Application < ::Rails::Application
Spree.config do |config|
config.mails_from = "[email protected]"
# TODO: Remove on Solidus 4.0 as it'll be the default
require 'spree/event/adapters/default'
config.events.adapter = Spree::Event::Adapters::Default.new
require 'spree/event/adapters/deprecation_handler'
if Spree::Event::Adapters::DeprecationHandler.legacy_adapter_set_by_env.nil?
require 'spree/event/adapters/default'
config.events.adapter = Spree::Event::Adapters::Default.new
end

if ENV['DISABLE_ACTIVE_STORAGE']
config.image_attachment_module = 'Spree::Image::PaperclipAttachment'
Expand Down
4 changes: 2 additions & 2 deletions core/spec/lib/spree/app_configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class DummyClass; end;
expect(prefs.admin_vat_location.country_id).to eq(nil)
end

it 'can access event adapter' do
expect(prefs.events.adapter).to be_an_instance_of(Spree::Event::Adapters::Default)
it 'can access event configuration' do
expect(prefs.events).to be_an_instance_of(Spree::Event::Configuration)
end
end
85 changes: 85 additions & 0 deletions core/spec/lib/spree/event/adapters/deprecation_handler_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
# frozen_string_literal: true

require 'spec_helper'
require 'spree/event/adapters/deprecation_handler'
require 'spree/event/adapters/default'

module Spree
module Event
module Adapters
RSpec.describe DeprecationHandler do
let(:env_key) { described_class::CI_LEGACY_ADAPTER_ENV_KEY }

let(:reset_env_key) do
lambda do |example|
old_value = ENV[env_key]
example.run
ENV[env_key] = old_value
end
end

describe '#legacy_adapter?' do
context 'when the events adapter is the legacy' do
it 'returns true' do
expect(described_class.legacy_adapter?(ActiveSupportNotifications)).to be(true)
end
end

context 'when the events adapter is not the legacy' do
it 'returns false' do
expect(described_class.legacy_adapter?(Default.new)).to be(false)
end
end
end

describe '#legacy_adapter_set_by_env' do
around { |example| reset_env_key.(example) }

context 'when env var is set' do
it 'returns the legacy adapter' do
ENV[env_key] = '1'

expect(described_class.legacy_adapter_set_by_env).to be(ActiveSupportNotifications)
end
end

context 'when env var is not set' do
it 'returns nil' do
ENV[env_key] = nil

expect(described_class.legacy_adapter_set_by_env).to be(nil)
end
end
end

describe '#render_deprecation_message?' do
context 'when adapter is legacy and is not set by env' do
around { |example| reset_env_key.(example) }

it 'returns true' do
ENV[env_key] = nil

expect(described_class.render_deprecation_message?(ActiveSupportNotifications)).to be(true)
end
end

context 'when adapter is legacy and is set by env' do
around { |example| reset_env_key.(example) }

it 'returns true' do
ENV[env_key] = '1'

expect(described_class.render_deprecation_message?(ActiveSupportNotifications)).to be(false)
end
end

context 'when adapter is not legacy' do
it 'returns false' do
expect(described_class.render_deprecation_message?(Default.new)).to be(false)
end
end
end
end
end
end
end
49 changes: 49 additions & 0 deletions core/spec/lib/spree/event/configuration_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true

require 'spec_helper'
require 'spree/event/configuration'

RSpec.describe Spree::Event::Configuration do
describe '#adapter' do
context "when it hasn't been explicitly set" do
let(:supress_warning_env_key) { Spree::Event::Adapters::DeprecationHandler::CI_LEGACY_ADAPTER_ENV_KEY }

it 'is ActiveSupportNotifications adapter' do
allow(Spree::Deprecation).to receive(:warn)

expect(described_class.new.adapter).to be(Spree::Event::Adapters::ActiveSupportNotifications)
end

it 'renders a deprecation message when no supressed via env' do
old_value = ENV[supress_warning_env_key]
ENV[supress_warning_env_key] = nil
expect(Spree::Deprecation).to receive(:warn).with(/adapter is.*deprecated/m)

described_class.new.adapter
ensure
ENV[supress_warning_env_key] = old_value
end

it "doesn't render a deprecation message when supressed via env" do
old_value = ENV[supress_warning_env_key]
ENV[supress_warning_env_key] = '1'
expect(Spree::Deprecation).not_to receive(:warn).with(/adapter is.*deprecated/m)

described_class.new.adapter
ensure
ENV[supress_warning_env_key] = old_value
end
end

context "when it has been explicitly set" do
it "doesn't render a deprecation message" do
expect(Spree::Deprecation).not_to receive(:warn)

config = described_class.new
config.adapter = Spree::Event::Adapters::ActiveSupportNotifications

config.adapter
end
end
end
end
Loading

0 comments on commit 85dae08

Please sign in to comment.