rayshan · 2014-09-18T08:01:07Z

This may not be the most elegant approach but it does fix tracking issue.

rayshan · 2014-09-19T05:21:12Z

I've been thinking about what the default should be for the case of "if no specified value, no stored value, and can't prompt for one". This is most likely in a CI environment (e.g. all Travis CI's test runs), and we probably don't want this case to pollute our data. I changed the default to false and updated the PR.

sheerun · 2014-09-19T09:07:05Z

change test name?

good catch, thx

rayshan · 2014-09-19T20:38:18Z

@sheerun since you mentioned calling commands in programmatic API, I realized that we could have a non-CI scenario that's also non-interactive. I plan to change the default sessionAnalytics back to true and check for process.env.CI in analytics.js.

sheerun · 2014-09-19T22:16:20Z

Just please add the tests

sheerun · 2014-09-19T22:18:27Z

Are you sure you want to check for CI in analytics.js? Isn't check for { interactive: true } enough?

rayshan · 2014-09-19T22:21:28Z

How do tools like Webstorm integrate bower? I assume that would be an environment where process.env.CI = null and interactive = false?

8000

sheerun · 2014-09-19T22:29:55Z

I think the environment should be irrelevant when calling bower via programmatic API.

Also, currently analytics is enabled when interactive = false. We probably want to track WebStorm?

rayshan · 2014-09-19T23:04:12Z

I think the environment should be irrelevant when calling bower via programmatic API.

Yes you're right. Let's not check for CI in analytics.js.

Also, currently analytics is enabled when interactive = false. We probably want to track WebStorm?

Yes latest bower version enables analytics when interactive = false. This should enable analytics in WebStorm unless manually disabled. But this also enables analytics for CI environments. Do you think if it'll be an issue?

sheerun · 2014-09-19T23:15:26Z

Well, we can't distinguish WebStorm from programmatic API on CI without checking CI env variable.

I don't know what to do. It's getting more and more complicated.

rayshan · 2014-09-20T19:22:12Z

I addressed your inline concerns. This still doesn't address cases when someone pass conflicting values and pass different values at different times, however that wasn't addressed previously either. I'd like to get this patch in because we're loosing valuable data.

To address the last concerns, I propose we introduce a new config option in Insight called sessionOptOut. We just use Insight to deal with any analytics-related configs, and implement a bridge in bower's config.js. That way we don't have to think about keeping track of conflicting config values in 2 modules. I'll get a patch in Insight then submit another patch to Bower.

sheerun · 2014-09-23T18:15:37Z

I think we should leave analytics only for interactive sessions. It solves a lot of issues.

rayshan · 2014-09-23T18:43:12Z

Agreed, this is the case now that we defaulted sessionAnalytics to false right? Unless user manually set config.analytics to true. Is this PR ready or would you recommend further edits?

sheerun · 2014-09-23T18:45:27Z

I'll fix this PR and tests soon.

sheerun · 2014-09-23T18:45:56Z

@rayshan Mind to log in to IRC?

sheerun · 2014-09-23T23:05:50Z

@rayshan Please review

rayshan · 2014-09-24T05:04:33Z

would it be more clear if we check for undefined since that's what the value actually is if not set? i know this works, just for clarification

sheerun · 2014-09-26T15:19:30Z

@rayshan Could you review my code?

rayshan · 2014-09-26T18:54:39Z

@sheerun hi, I did already, please see inline comments. I think the biggest issue is checking for null vs. undefined, which makes this patch not work. Let me know what other feedback you'd like.

sheerun · 2014-09-26T20:35:15Z

@rayshan I fixed your commit again. Sorry I didn't see your feedback.

rayshan · 2014-09-27T04:53:00Z

@sheerun no prob, it's properly working now, ready to merge. Thanks for all the help on this. I'll let you merge in in the order you prefer.

fix broken analytics tracking introduced in #1507 & changed default

rayshan · 2014-09-28T20:03:37Z

Working:

rayshan force-pushed the analytics_fix branch from 0c4c9e5 to d10f0ef Compare September 19, 2014 05:19

rayshan changed the title ~~fix broken analytics tracking introduced in #1507~~ fix broken analytics tracking introduced in #1507 & changed default Sep 19, 2014

sheerun reviewed Sep 19, 2014
View reviewed changes

rayshan force-pushed the analytics_fix branch from d10f0ef to e802b87 Compare September 20, 2014 19:16

sheerun force-pushed the analytics_fix branch from e802b87 to 48fe9be Compare September 23, 2014 23:04

rayshan reviewed Sep 24, 2014
View reviewed changes

rayshan closed this Sep 24, 2014

rayshan reopened this Sep 24, 2014

fix broken analytics tracking introduced in #1507

4656021

sheerun force-pushed the analytics_fix branch from 48fe9be to 4656021 Compare September 26, 2014 20:34

sheerun added a commit that referenced this pull request Sep 27, 2014

Merge pull request #1529 from bower/analytics_fix

4aa0f56

fix broken analytics tracking introduced in #1507 & changed default

sheerun merged commit 4aa0f56 into master Sep 27, 2014

sheerun deleted the analytics_fix branch November 27, 2015 16:05

Navigation Menu

Uh oh!

Uh oh!

fix broken analytics tracking introduced in #1507 & changed default - #1529