8000 Add method and URL to Event, add method to EventDispatcher by delikat · Pull Request #1 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]
Skip to content

Add method and URL to Event, add method to EventDispatcher #1

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 29, 2016
Merged

Add method and URL to Event, add method to EventDispatcher #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc9eab2
Duck type EventDispatcher
Aug 25, 2016
a58a4f2
Add method to Events, rename EventBuilder > EventBuilderV1
Aug 25, 2016
440e9e3
fix tests
Aug 25, 2016
b8a7dac
Add method to NoOpEventDispatcher
Aug 25, 2016
4b299b3
Lint
Aug 25, 2016
a93f803
Respond to CR feedback
Aug 29, 2016
f28c165
Respond to CR feedback
Aug 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Respond to CR feedback
  • Loading branch information
Andrew Delikat committed Aug 29, 2016
commit f28c16544993c02ee60e00345383b7f57be7a3dc
4 changes: 2 additions & 2 deletions lib/optimizely.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def activate(experiment_key, user_id, attributes = nil)
@logger.log(Logger::INFO,
'Dispatching impression event to URL %s with params %s.' % [impression_event.url,
impression_event.params])
@event_dispatcher.dispatch_event(impression_event.url, impression_event.params, impression_event.http_verb)
@event_dispatcher.dispatch_event(impression_event)

@config.get_variation_key_from_id(experiment_key, variation_id)
end
Expand Down Expand Up @@ -136,7 +136,7 @@ def track(event_key, user_id, attributes = nil, event_value = nil)
@logger.log(Logger::INFO,
'Dispatching conversion event to URL %s with params %s.' % [conversion_event.url,
conversion_event.params])
@event_dispatcher.dispatch_event(conversion_event.url, conversion_event.params, conversion_event.http_verb)
@event_dispatcher.dispatch_event(conversion_event)
end

private
Expand Down
5 changes: 5 additions & 0 deletions lib/optimizely/event_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ def initialize(http_verb, url, params)
@url = url
@params = params
end

# Override equality operator to make two events with the same contents equal for testing purposes
def ==(event)
@http_verb == event.http_verb && @url == event.url && @params == event.params
end
end

class EventBuilderV1
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating two different event builder classes, why don't we have one event builder that can build V1 events and V2 events. That way you don't have to worry about handling the instantiation of two different builder classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because when we eventually remove V1 support it will be easier to simply remove the EventBuilderV1 class than to rip out the V1 logic from a larger omnibus EventBuilder.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree because when we rip out V1 support, we would just be changing the internals of this class and the API references. If we keep two classes around there will also be some forking logic throughout the codebase that we need to kill. Providing the event builder as some sort of facade/factory will make it much easier to deal with.

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 it is way better to pick the right EventBuilder at object invocation and not have logic all over. The model here allows that.

Copy link
Contributor
@mikeproeng37 mikeproeng37 Aug 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

How about this, since the EventBuilder is already initialized with the project config (which tells us whether we are in V1 or V2), why don't we just have the event builder determine whether to return a V1 Event or V2 event when we call eventBuilder.getConversionEvent and eventBuilder.getImpressionEvent. That event can be passed into event dispatcher, which determines based on the event whether to get or post. This way, the optimizely instance calling the event builder doesn't even have to make the decision to use EventBuilder1 or EventBuilder2

Expand Down
12 changes: 5 additions & 7 deletions lib/optimizely/event_dispatcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,21 @@ module Optimizely
class NoOpEventDispatcher
# Class providing dispatch_event method which does nothing.

def dispatch_event(_url, _params, _http_verb)
def dispatch_event(event)
end
end

class EventDispatcher
REQUEST_TIMEOUT = 10

def dispatch_event(url, params, http_verb = :get)
def dispatch_event(event)
# Dispatch the event being represented by the Event object.
#
# http_verb - HTTP verb with which to send the event.
# url - URL to send impression/conversion event to.
# params - Params to be sent to the impression/conversion event.
# event - Event object

if http_verb == :get
if event.http_verb == :get
begin
HTTParty.get(url, query: params, timeout: REQUEST_TIMEOUT)
HTTParty.get(event.url, query: event.params, timeout: REQUEST_TIMEOUT)
rescue Timeout::Error => e
return e
end
Expand Down
4 changes: 3 additions & 1 deletion spec/event_dispatcher_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'spec_helper'
require 'webmock'
require 'optimizely/event_builder'
require 'optimizely/event_dispatcher'

describe Optimizely::EventDispatcher do
Expand All @@ -19,7 +20,8 @@

it 'should fire off GET request with provided URL and params' do
stub_request(:get, @url).with(:query => @params)
@event_dispatcher.dispatch_event(@url, @params, :get)
event = Optimizely::Event.new(:get, @url, @params)
@event_dispatcher.dispatch_event(event)

expect(a_request(:get, @url).with(:query => @params)).to have_been_made.once
end
Expand Down
24 changes: 12 additions & 12 deletions spec/project_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,15 @@ class InvalidErrorHandler; end
}

allow(project_instance.bucketer).to receive(:bucket).and_return('111128')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
.with('test_experiment')
.and_return([])

stub_request(:get, log_url).with(:query => params)

expect(project_instance.activate('test_experiment', 'test_user')).to eq('control')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params, :get).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:get, log_url, params)).once
expect(project_instance.bucketer).to have_received(:bucket).once
end

Expand All @@ -121,11 +121,11 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}
allow(project_instance.bucketer).to receive(:bucket).and_return('122228')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))

expect(project_instance.activate('test_experiment_with_audience', 'test_user', 'browser_type' => 'firefox'))
.to eq('control_with_audience')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params, :get).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:get, log_url, params)).once
expect(project_instance.bucketer).to have_received(:bucket).once
end

Expand Down Expand Up @@ -160,7 +160,7 @@ class InvalidErrorHandler; end
}

allow(project_instance.bucketer).to receive(:bucket).and_return('111128')
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
.with('test_experiment')
.and_return([])
Expand Down Expand Up @@ -192,9 +192,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
project_instance.track('test_event', 'test_user')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params, :get).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:get, log_url, params)).once
end

it 'should properly track an event by calling dispatch_event with right params with revenue provided' do
Expand All @@ -210,9 +210,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
project_instance.track('test_event', 'test_user', nil, 42)
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params, :get).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:get, log_url, params)).once
end

it 'should properly track an event by calling dispatch_event with right params with attributes provided' do
Expand All @@ -228,9 +228,9 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
project_instance.track('test_event_with_audience', 'test_user', 'browser_type' => 'firefox')
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(log_url, params, :get).once
expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:get, log_url, params)).once
end

it 'should not call dispatch_event when tracking an event for which audience conditions do not match' do
Expand Down Expand Up @@ -258,7 +258,7 @@ class InvalidErrorHandler; end
'time' => time_now.strftime('%s').to_i
}

allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(log_url, params, :get)
allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
project_instance.track('test_event', 'test_user', nil, 42)
expect(spy_logger).to have_received(:log).once.with(Logger::INFO, include("Dispatching conversion event to" \
" URL #{log_url} with params"))
Expand Down
0