-
Notifications
You must be signed in to change notification settings - Fork 28
fix(track): Send decisions for all experiments using an event when using track #120
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
Conversation
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes mostly look good. Just need to clean up the unit tests
spec/event_builder_spec.rb
Outdated
@@ -253,6 +253,18 @@ | |||
expect(conversion_event.http_verb).to eq(:post) | |||
end | |||
|
|||
it 'should create_conversion_event when Event is used in multiple experiments' do | |||
@expected_conversion_params[:visitors][0][:snapshots][0][:decisions].push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup of this test is not very clear from just looking at this. Can you split this out into its own scenario with its own setup? Specifically, the scenario testing event tracking with multiple experiments.
lib/optimizely/event_builder.rb
Outdated
|
||
conversion_event_params.push(single_snapshot) | ||
event_object[:tags] = event_tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a check if event_tags has any item in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.