Bring back ahoy gem to track visits#803
Conversation
|
Is there a github issue associated with this PR? (I understand what this does but not why we are adding it now) |
|
We are adding it back because it was a mistake to remove it. I thought that we could get by with just using logging to publish events, but being able to track visits is important, and Ahoy is the easiest way to do that currently. As a follow up, I will see if it makes sense to extract just the visit tracking code from Ahoy and implement it in our app. |
**Why**: To be able to associate visits with users, so we can track them from unauthenticated to authenticated state, and to make it easier to count unique visits.
monfresh
force-pushed
the
mb-track-visits
branch
from
599f966 to
23984ed
Compare
|
|
||
| def ahoy | ||
| @ahoy ||= Rails.env.test? ? FakeAhoyTracker.new : Ahoy::Tracker.new(request: request) | ||
| end |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Yeah that seems fair enough, would we have to implement all the methods or just a no-op initialize?
There was a problem hiding this comment.
Alternatively, we could do a double().as_null_object? But up to you!
|
|
||
| let(:ahoy) { instance_double(FakeAhoyTracker) } | ||
|
|
||
| before { allow(FakeAhoyTracker).to receive(:new).and_return(ahoy) } |
There was a problem hiding this comment.
if we stubbed Ahoy::Tracker instead of FakeAhoyTracker here, would that make it possible to remove the env check in Analytics ?
| @@ -0,0 +1,5 @@ | |||
| class FakeAhoyTracker | |||
There was a problem hiding this comment.
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?
|
woops just reviewed after this was merged accidentally. Feel free to consider or ignore my comments as you see fit. |
**Why**: To be able to associate visits with users, so we can track them from unauthenticated to authenticated state, and to make it easier to count unique visits.
**Why**: To be able to associate visits with users, so we can track them from unauthenticated to authenticated state, and to make it easier to count unique visits.
4 participants
Why: To be able to associate visits with users, so we can track
them from unauthenticated to authenticated state, and to make it easier
to count unique visits.