-
Notifications
You must be signed in to change notification settings - Fork 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
add support for google analytics userId #15
Changes from 1 commit
28d59f2
621731f
17000d3
e1b0f9e
af64923
ba3af18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,19 @@ def tracker | |
options[:tracker].respond_to?(:call) ? options[:tracker].call(env) : options[:tracker] | ||
end | ||
|
||
def tracker_options | ||
@tracker_options ||= begin | ||
tracker_options = {} | ||
|
||
tracker_options[:cookieDomain] = options[:cookie_domain] if options[:cookie_domain] | ||
|
||
user_id = options[:user_id].call(env) if options[:user_id] | ||
tracker_options[:userId] = "#{user_id}" if user_id.present? | ||
|
||
tracker_options | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this maybe more generic? always camelize the keys, support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point and useful for the cookie_domain with an i18n multihost setup 17000d3 |
||
|
||
def render | ||
Tilt.new( File.join( File.dirname(__FILE__), 'template', 'google_analytics.erb') ).render(self) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
module Rack | ||
class Tracker | ||
VERSION = '0.3.0' | ||
VERSION = '0.3.1' | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,34 @@ def env | |
end | ||
end | ||
|
||
describe '#tracker_options' do | ||
describe 'with cookie_domain option' do | ||
subject { described_class.new(env, { cookie_domain: 'railslabs.com' }) } | ||
|
||
it 'returns hash with cookieDomain' do | ||
expect(subject.tracker_options).to eql ({ cookieDomain: 'railslabs.com' }) | ||
end | ||
end | ||
|
||
describe 'with user_id option' do | ||
context 'returning a value' do | ||
subject { described_class.new(env, { user_id: ->(env){ '123' } }) } | ||
|
||
it 'returns hash with userId' do | ||
expect(subject.tracker_options).to eql ({ userId: '123' }) | ||
end | ||
end | ||
|
||
context 'returning nil' do | ||
subject { described_class.new(env, { user_id: ->(env){ nil } }) } | ||
|
||
it 'returns hash without userId' do | ||
expect(subject.tracker_options).to eql ({ }) | ||
end | ||
end | ||
end | ||
end | ||
|
||
describe "with events" do | ||
describe "default" do | ||
def env | ||
|
@@ -93,6 +121,15 @@ def env | |
end | ||
end | ||
|
||
describe "with user_id tracking" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't that something that should only be tested in the integration spec? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the cookieDomain thingy is tested here too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok i'm just a bit confused as to what i should test where |
||
subject { described_class.new(env, tracker: 'somebody', user_id: ->(env){ '123' } ).render } | ||
|
||
it "will show asyncronous tracker with userId" do | ||
expect(subject).to match(%r{ga\('create', 'somebody', {\"userId\":\"123\"}\)}) | ||
expect(subject).to match(%r{ga\('send', 'pageview'\)}) | ||
end | ||
end | ||
|
||
describe "with enhanced_link_attribution" do | ||
subject { described_class.new(env, tracker: 'happy', enhanced_link_attribution: true).render } | ||
|
||
|
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.
i know its a matter of taste, but personally i don't like these temporary variables,
tap
is your fried to get rid of 'em