Skip to content

Bring back ahoy gem to track visits #803

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
brendansudol merged 1 commit into from
Dec 2, 2016
Merged

Bring back ahoy gem to track visits #803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ git_source(:github) { |repo_name| "https://github.com/#{repo_name}.git" }

gem 'rails', '~> 4.2.6'

gem 'redis-session-store'
gem 'ahoy_matey'
gem 'american_date'
gem 'aws-sdk-core'
gem 'browserify-rails'
Expand All @@ -26,6 +26,7 @@ gem 'proofer', github: '18F/identity-proofer-gem', branch: 'master'
gem 'valid_email'
gem 'rack-attack'
gem 'readthis'
gem 'redis-session-store'
gem 'rqrcode'
gem 'ruby-saml'
gem 'saml_idp', git: 'https://github.com/18F/saml_idp.git', branch: 'master'
Expand Down
21 changes: 21 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ GEM
tzinfo (~> 1.1)
addressable (2.5.0)
public_suffix (~> 2.0, >= 2.0.2)
ahoy_matey (1.5.3)
addressable
browser (~> 2.0)
geocoder
rack-attack (< 6)
railties
referer-parser (>= 0.3.0)
request_store
safely_block (>= 0.1.1)
user_agent_parser
uuidtools
airbrussh (1.1.1)
sshkit (>= 1.6.1, != 1.7.0)
akami (1.3.1)
Expand Down Expand Up @@ -106,6 +117,7 @@ GEM
binding_of_caller (0.7.2)
debug_inspector (>= 0.0.1)
brakeman (3.4.1)
browser (2.3.0)
browserify-rails (3.4.0)
addressable (>= 2.4.0)
railties (>= 4.0.0, < 5.1)
Expand Down Expand Up @@ -206,6 +218,7 @@ GEM
mail (~> 2.6.3)
encryptor (3.0.0)
equalizer (0.0.11)
errbase (0.0.3)
erubis (2.7.0)
eventmachine (1.0.9.1)
execjs (2.7.0)
Expand All @@ -224,6 +237,7 @@ GEM
thor (~> 0.14)
formatador (0.2.5)
foundation_emails (2.2.1.0)
geocoder (1.4.0)
get_process_mem (0.2.0)
globalid (0.3.7)
activesupport (>= 4.1.0)
Expand Down Expand Up @@ -412,6 +426,8 @@ GEM
codeclimate-engine-rb (~> 0.4.0)
parser (~> 2.3.1, >= 2.3.1.2)
rainbow (~> 2.0)
referer-parser (0.3.0)
request_store (1.3.1)
responders (2.2.0)
railties (>= 4.2.0, < 5.1)
rotp (3.2.0)
Expand Down Expand Up @@ -450,6 +466,8 @@ GEM
nokogiri (>= 1.5.10)
ruby_dep (1.5.0)
safe_yaml (1.0.4)
safely_block (0.1.1)
errbase
sass (3.4.22)
sass-rails (5.0.6)
railties (>= 4.0.0, < 6)
Expand Down Expand Up @@ -552,9 +570,11 @@ GEM
execjs (>= 0.3.0, < 3)
unicode-display_width (1.1.1)
uniform_notifier (1.10.0)
user_agent_parser (2.3.0)
useragent (0.16.8)
uuid (2.3.8)
macaddr (~> 1.0)
uuidtools (2.1.5)
valid_email (0.0.13)
activemodel
mail (~> 2.6.1)
Expand Down Expand Up @@ -595,6 +615,7 @@ PLATFORMS
ruby

DEPENDENCIES
ahoy_matey
american_date
aws-sdk-core
axe-matchers
Expand Down
9 changes: 6 additions & 3 deletions app/services/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,21 @@ def initialize(user, request)

def track_event(event, attributes = {})
analytics_hash = {
event: event,
properties: attributes.except(:user_id),
event_properties: attributes.except(:user_id),
user_id: attributes[:user_id] || uuid
}

ANALYTICS_LOGGER.info(analytics_hash.merge!(request_attributes))
ahoy.track(event, analytics_hash.merge!(request_attributes))
end

private

attr_reader :user, :request

def ahoy
@ahoy ||= Rails.env.test? ? FakeAhoyTracker.new : Ahoy::Tracker.new(request: request)
end
Copy link
Contributor Author

@monfresh monfresh Dec 2, 2016

Choose a reason for hiding this comment

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

@zachmargolis I know you had made a comment about this technique in a previous PR, where you suggested to use stub_const in tests, but it caused the tests to fail and caused me some head scratching due to an error message that didn't make sense and included the Ahoy gem in the stack trace.

The problem was that I was stubbing FakeAhoyTracker for Ahoy::Tracker, and because I didn't have an initialize method defined in FakeAhoyTracker, it complained about wrong number of arguments, pointing to Ahoy's controller initializer. It said 1 argument was passed, but 0 were expected. That didn't make sense because Ahoy::Tracker clearly accepts arguments. Finally it dawned on me that the tests were using FakeAhoyTracker, and stubbing the const meant that I had to implement all the methods called by the real class in my fake class.

The whole point of the fake class is so that the tests use that class and only that class, and it shouldn't be necessary to implement all the methods from the real class in the fake class. I don't want the tests to touch the real class at all, which is why I prefer this way of doing it.

Copy link
Contributor

@zachmargolis zachmargolis Dec 2, 2016

Choose a reason for hiding this comment

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

Yeah that seems fair enough, would we have to implement all the methods or just a no-op initialize?

Copy link
Contributor

@zachmargolis zachmargolis Dec 2, 2016

Choose a reason for hiding this comment

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

Alternatively, we could do a double().as_null_object? But up to you!


def request_attributes
{
user_ip: request.remote_ip,
Expand Down
9 changes: 9 additions & 0 deletions config/initializers/ahoy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Ahoy.mount = false
Ahoy.throttle = false
# Period of inactivity before a new visit is created
Ahoy.visit_duration = 30.minutes

module Ahoy
class Store < Ahoy::Stores::LogStore
end
end
8 changes: 0 additions & 8 deletions config/initializers/analytics_logger.rb
View file
Open in desktop

This file was deleted.

18 changes: 10 additions & 8 deletions spec/services/analytics_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,23 @@
}
end

let(:ahoy) { instance_double(FakeAhoyTracker) }

before { allow(FakeAhoyTracker).to receive(:new).and_return(ahoy) }
Copy link
Contributor

@jessieay jessieay Dec 2, 2016

Choose a reason for hiding this comment

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

if we stubbed Ahoy::Tracker instead of FakeAhoyTracker here, would that make it possible to remove the env check in Analytics ?


describe '#track_event' do
it 'identifies the user and sends the event to the backend' do
user = build_stubbed(:user, uuid: '123')

analytics = Analytics.new(user, FakeRequest.new)

analytics_hash = {
event: 'Trackable Event',
properties: {},
event_properties: {},
user_id: user.uuid
}

expect(ANALYTICS_LOGGER).to receive(:info).
with(analytics_hash.merge(request_attributes))
expect(ahoy).to receive(:track).
with('Trackable Event', analytics_hash.merge(request_attributes))

analytics.track_event('Trackable Event')
end
Expand All @@ -34,13 +37,12 @@
analytics = Analytics.new(current_user, FakeRequest.new)

analytics_hash = {
event: 'Trackable Event',
properties: {},
event_properties: {},
user_id: tracked_user.uuid
}

expect(ANALYTICS_LOGGER).to receive(:info).
with(analytics_hash.merge(request_attributes))
expect(ahoy).to receive(:track).
with('Trackable Event', analytics_hash.merge(request_attributes))

analytics.track_event('Trackable Event', user_id: tracked_user.uuid)
end
Expand Down
5 changes: 5 additions & 0 deletions spec/support/fake_ahoy_tracker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class FakeAhoyTracker
Copy link
Contributor

@jessieay jessieay Dec 2, 2016

Choose a reason for hiding this comment

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

I am not opposed to this strategy, but it probably makes sense to have a test that confirms that FakeAhoyTracker and Ahoy::Tracker have the same interface.

As is, I can imagine a scenario in which the ahoy lib requires new/different args for initialize, for example, but the specs still pass without a change. Am I making sense?

def track(_name, _properties = {}, _options = {})
# no-op
end
end