Skip to content

google global: better empty tracker handling#142

Merged
DonSchado merged 8 commits intorailslove:masterfrom
glaszig:fix/google-global-empty-trackers
Dec 8, 2019
Merged

google global: better empty tracker handling#142
DonSchado merged 8 commits intorailslove:masterfrom
glaszig:fix/google-global-empty-trackers

Conversation

@glaszig
Copy link
Copy Markdown
Contributor

@glaszig glaszig commented Aug 15, 2019

i'm using the following code:

use Rack::Tracker do
  handler :google_global, trackers: [ { id: ENV["GA_ID"] } ], anonymize_ip: true
end

which throws up the following way if GA_ID is empty:

NoMethodError:
  undefined method `[]' for nil:NilClass
# ./lib/rack/tracker/google_global/template/google_global.erb:2:in `block in singleton class'
# ./lib/rack/tracker/google_global/template/google_global.erb:-2:in `instance_eval'
# ./lib/rack/tracker/google_global/template/google_global.erb:-2:in `singleton class'
# ./lib/rack/tracker/google_global/template/google_global.erb:-5:in `__tilt_70324565778440'
# ./lib/rack/tracker/handler.rb:37:in `render'
# ./lib/rack/tracker/handler.rb:50:in `block in inject'
# ./lib/rack/tracker/handler.rb:49:in `inject'
# ./lib/rack/tracker.rb:67:in `block in inject'
# ./lib/rack/tracker.rb:100:in `each'
# ./lib/rack/tracker.rb:100:in `each'
# ./lib/rack/tracker.rb:66:in `inject'
# ./lib/rack/tracker.rb:54:in `block in call'
# ./lib/rack/tracker.rb:54:in `call'
# ./spec/integration/google_global_integration_spec.rb:36:in `block (3 levels) in <top (required)>'

this patch properly handles the case of empty trackers in the templates and deals with a few other cases which should be considered "empty". also caches collected trackers in an ivar to save a few iterations and cleans up the integration test a little.

Copy link
Copy Markdown
Contributor

@bumi bumi left a comment

Choose a reason for hiding this comment

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

utACK
@DonSchado please merge, if you agree.

end
end

def invalid_tracker? tracker
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think in the rest of the codebase we use parentheses.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@bumi
Copy link
Copy Markdown
Contributor

bumi commented Aug 17, 2019

thanks for the PR! 👍

end
end

def invalid_tracker?(tracker)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that's a very good idea!
But shouldn't we use this chance to give a warning, instead of silently ignoring the tracker? :)

@DonSchado
Copy link
Copy Markdown
Collaborator

ping @glaszig :) is this still relevant?

@glaszig glaszig force-pushed the fix/google-global-empty-trackers branch from cf6ad4d to 8412a7e Compare November 8, 2019 09:36
@glaszig
Copy link
Copy Markdown
Contributor Author

glaszig commented Nov 8, 2019

just added the warning.

@glaszig
Copy link
Copy Markdown
Contributor Author

glaszig commented Dec 5, 2019

still a blocker? needs improvements? shall i squash the commits?

@DonSchado DonSchado merged commit 42b8e51 into railslove:master Dec 8, 2019
@DonSchado
Copy link
Copy Markdown
Collaborator

thanks for the reminder! :)

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.

3 participants