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

Support person data in rollbar.js #602

Merged
merged 5 commits into from
Jun 15, 2017

Conversation

and-megan
Copy link
Contributor

No description provided.

@and-megan and-megan requested a review from jondeandres June 13, 2017 18:17
@and-megan
Copy link
Contributor Author

@jondeandres This works but it's automatic, ie you don't need to list it in the config.js_options, - should this be optional or is it ok as is?

@and-megan
Copy link
Contributor Author

and-megan commented Jun 13, 2017

I made it optional (config.js_options.payload.person_tracking: true), but can always take it out.

@@ -103,7 +107,12 @@ def close_old_response(response)
end

def config_js_tag(env)
script_tag("var _rollbarConfig = #{config[:options].to_json};", env)
js_config = config[:options].dup
if js_config[:payload][:person_tracking]
Copy link
Contributor

Choose a reason for hiding this comment

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

If :payload does not exist in the config then this throws

js_config = config[:options].dup
if js_config[:payload][:person_tracking]
person_data = extract_person_data_from_controller(env)
js_config[:payload][:person] = person_data if person_data
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@and-megan
Copy link
Contributor Author

@rokob I think it's fixed now

@and-megan and-megan requested a review from rokob June 13, 2017 20:32
Copy link
Contributor

@jondeandres jondeandres left a comment

Choose a reason for hiding this comment

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

Hey @and-megan, thanks for continuing with this. I just left a comment in the middleware logic. I didn't understand the :person_tracking.

Also, just a tip, try is a method that exists in Rails but not in Ruby itself, so we try to avoid the use of stuff that is only Rails related, so we build compatible things for everybody 😄.

We would need to add a test for this, if you want take a look at https://github.com/rollbar/rollbar-gem/blob/master/spec/rollbar/middleware/js_spec.rb.

I can write it if you don't find time for it.

@@ -103,7 +107,12 @@ def close_old_response(response)
end

def config_js_tag(env)
script_tag("var _rollbarConfig = #{config[:options].to_json};", env)
js_config = config[:options].dup
if js_config.try(:[], :payload).try(:[], :person_tracking)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just need to do:

person_data = extract_person_data_from_controller(env)
if person_data
  js_config[:payload] ||= {} # We initialize the `payload` dictionary just with `{}` if it doesn't exist
  js_config[:payload][:person] = person_data
end

@jondeandres jondeandres force-pushed the support-person-data-in-rollbar.js branch from 07b728d to a453239 Compare June 14, 2017 15:22
@jondeandres jondeandres force-pushed the support-person-data-in-rollbar.js branch from a453239 to e1e26af Compare June 14, 2017 16:04
@and-megan
Copy link
Contributor Author

and-megan commented Jun 14, 2017

@jondeandres Looks good. I had :person_tracking so that someone could turn rollbar.js person tracking on or off, rather than have it be automatic.

Also thanks for the note on Ruby vs Rails! I'll take that into account.

@jondeandres
Copy link
Contributor

Mmmm @and-megan, we even don't have that option in the gem itself, so I think it will be ok sending that information automatically. Using an external option would be the way to do it.

@jondeandres jondeandres merged commit f28de87 into master Jun 15, 2017
@jondeandres jondeandres deleted the support-person-data-in-rollbar.js branch June 15, 2017 11:40
@kylefox kylefox mentioned this pull request Sep 12, 2017
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