Skip to content
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 :custom_vars option #20

Merged
merged 1 commit into from
Mar 9, 2014
Merged

Add :custom_vars option #20

merged 1 commit into from
Mar 9, 2014

Conversation

yonda
Copy link
Collaborator

@yonda yonda commented Nov 14, 2013

Hello!

In the case of using analytics_init method, if you add additional events by add_events option, it is set after that _trackPageview request.

However, for example, as is described in the Google Analytics Documents, we need to set "_trackPageview()" request after setting a custom variable.

In certain cases this might not be possible, 
and you will need to set another _trackPageview() request after setting a custom variable.

(In my applications, _setCustomVar option has not been reflected in the Google Analytics.)

So, I changed the order in which additional events setting.
After change the order, _setCustomVar option has been reflected in the Google Analytics.

What do you think?

Thanks!

@paracycle
Copy link
Collaborator

Hi there, we recently ran into this issue as well. It seems both _setCustomVar and _setAllowLinker need to be set before the _trackPageview is generated. So more fine-grained control over how the additional events are processed would be great.

@bgarret Do you have any intentions to accept this pull request and make a release? Do you need any help maintaining the library? Can I help at all?

@bgarret
Copy link
Owner

bgarret commented Mar 2, 2014

@yonda, @paracycle,

I wont merge this as-is because (as @paracycle said) some events need to bet set before and some need to be set after _trackPageView. I'm really picky about backwards compatibility and this PR is likely to break a whole lot of scenarios.

A better solution in this case would be to add a new option (say :before_events, I'm not really sure about this name but it's the first one that popped into my mind) for those events that have to be before the page track.

Regarding the maintenance of the library, I have indeed not touched it for a long long time. I'm not using Rails anymore and I don't really follow the developments of Google Analytics. If you want to lend a hand, I have given you full access to the repo. For the releases, I'll need a rubygems username to be able to add you as an owner.

Thanks for the patience.

@paracycle
Copy link
Collaborator

@bgarret Thank you, I will start working on the current issues and make a release. My rubygems username is "ufuk".

@yonda Could you update your pull-request so that before events and after events are properly accounted for in backwards compatible manner? If you can't, please let me know, so that I can start working on it myself. Thanks.

@bgarret
Copy link
Owner

bgarret commented Mar 3, 2014

@paracycle Done, you should be able to push releases on rubygems.

I'm using the following (rough) release process :

  • Increase the version number in lib/google-analytics/version.rb
  • Update the changelog
  • Commit
  • Run rake release

I'm not particularly attached to the process but I find it simple and efficient. Feel free to make changes as you see fit.

@yonda
Copy link
Collaborator Author

yonda commented Mar 3, 2014

@paracycle I will be able to update my PR in a few days so that before events and after events are properly accounted for in backwards compatible manner. Please wait until this PR is merged if possible.

@bgarret Thank you, my rubygems username is "yonda". Can you add to the owner ?

@paracycle
Copy link
Collaborator

@yonda Sure, I'll be waiting on an update from you.

@bgarret
Copy link
Owner

bgarret commented Mar 3, 2014

@yonda I've added you as an owner on rubygems and a collaborator on github. Let me know if I can do anything.

@yonda
Copy link
Collaborator Author

yonda commented Mar 4, 2014

@paracycle @bgarret
In this case, it seems (as @paracycle said) to need to be set before the _trackPageView event is only _setCustomVar .
So, in the PR, I don't add a new option for this issue, I'll modify to set _setCustomVar before _trackPageview like _setAllowLinker.

What do you think? I am glad if I get your advice.

@paracycle
Copy link
Collaborator

@yonda I guess it would be best to just turn this PR into a change that adds a :custom_vars option addition, instead of a full reorder of parameters. Keep in mind that users of the library might want to set multiple custom variables.

This is what I have in mind:

# application.html.erb
...
    <%= analytics_init custom_vars: [
                  GA::Events::SetCustomVar.new(1, 'test_group', 'black', 1),
                  GA::Events::SetCustomVar.new(2, 'cohort', '2014-02', 1)
                ]
    %>
...

# lib/google-analytics/rails/view_helpers.rb
...
       events.unshift GA::Events::TrackPageview.new(options[:page])

       (options[:custom_vars] || []).each do |custom_var|
         events.unshift custom_var
       end
...

What do you think?

@bgarret
Copy link
Owner

bgarret commented Mar 4, 2014

@paracycle 👍 for this, I like the fact that it preserves the GA semantics and does not force the implementer to guess whether the call should go before or after _trackPageView. This was, after all, my main motivation in starting this library.

@mcclaskc
Copy link

mcclaskc commented Mar 7, 2014

Hi guys, how soon do you think the :custom_vars PR will be added? This is currently the exact problem that is keeping me from using this gem. I'm happy to do it if @yonda or @paracycle don't.

@yonda
Copy link
Collaborator Author

yonda commented Mar 8, 2014

@paracycle @bgarret Thank you for your advice !
I guess it would be best to just turn this PR into a change that adds a :custom_vars option addition too .
I updated this RP , please review this PR .

@mcclaskc
Copy link

mcclaskc commented Mar 8, 2014

@yonda 👍

@paracycle
Copy link
Collaborator

@yonda This seems great, thanks.

The Travis build does not pass but that is related to a problem with the Rubinius build, so I am merging this.

paracycle added a commit that referenced this pull request Mar 9, 2014
Add `:custom_vars` option
@paracycle paracycle merged commit 18f6dd7 into bgarret:master Mar 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants