segment-integrations / analytics.js-integration-intercom

The Intercom analytics.js integration.
https://segment.com/docs/integrations/intercom/
MIT License
7 stars 12 forks source link

initialize intercom with segment anonymous_id #31

Open RahavLussato opened 6 years ago

RahavLussato commented 6 years ago

fix #30 another thing is that i think segment intercom integration should check if the user is already identified and initialize with the identified user, currently i don't see how to do it since the user_hash is not persistent.

EDIT: after digging inside the lib i can see that we can also solve #6 as part of this. the issue is that the intercom integration is not sync with segment user data on multiple cases:

  1. if the user not identified its not using the same anonymous id as segment.
  2. if the user identified and we open it on another tab without identify again its not using the user data.
  3. when the user data is rested intercom integration not doing shutdown and boot again with the new data.

to solve all of this i added new logic that get the user data on each page call in order to be synced with segment user data and also i've added new boot mode (to solve #6) that should be used like that:

if (window.analytics) {
    window.analytics.user().reset();
    if (window.Intercom) {
        window.Intercom('shutdown');
    }
    window.analytics.page('_boot');
}
RahavLussato commented 6 years ago

the tests failed because of "Uh oh, you've run out of minutes! Please visit https://saucelabs.com/pricing to purchase a subscription."

codecov-io commented 6 years ago

Codecov Report

Merging #31 into master will decrease coverage by 0.74%. The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   99.01%   98.27%   -0.75%     
==========================================
  Files           1        1              
  Lines         102      116      +14     
==========================================
+ Hits          101      114      +13     
- Misses          1        2       +1
Impacted Files Coverage Δ
lib/index.js 98.27% <93.33%> (-0.75%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bac8a65...3ee85b5. Read the comment docs.

Peripheral1994 commented 6 years ago

Hey @RahavLussato - thanks for the PR! Sorry for the delay on my reply here, but I'll be helping with getting this PR out the door! Thanks for adding in tests, I just have a last few questions for clarity to validate this change:

  1. In your edit, you mention adding a new boot mode, but I can't find the associated change. Can you clarify what you mean here? :)
  2. How can I best E2E test this to know that it's working as expected?

Thanks so much, just ping me when you reply and I'll hop right in to help get this done!

RahavLussato commented 6 years ago

Hi @Peripheral1994

  1. you can find the change here, its there for solving #6 and you should use it like that:
    if (window.analytics) {
    window.analytics.user().reset();
    if (window.Intercom) {
        window.Intercom('shutdown');
    }
    window.analytics.page('_boot');
    }

basically, it gives you the ability to reset this.booted on logout.

  1. I think that the best E2E will be to check this scenario : a. calling page with unidentified user - check that it uses segment anonymous id b. call identify c. call page with identified user - check that it uses the same anonymous id as before + all the user data (id, traits ....) d. logout with the new boot mode e. call page to see that it use the same anonymous id as segment
Peripheral1994 commented 6 years ago

Awesome, got it @RahavLussato . I've tested it and it definitely seems to work with the code as-written. My only concern would be that we'd have to require each individual customer to implement this custom behavior manually, which doesn't make for a great user experience. I'm thinking that this should actually be something that lives within the settings in the Segment UI, I'll take a moment to think of a solution we can make on our end that would hopefully make this a much easier solution on the user side!

tobold commented 6 years ago

Any news on this? We've been struggling with the problem of Intercom not logging out on .reset()

RahavLussato commented 6 years ago

hi @Peripheral1994 is there any updates? do you need my help with this?

srthurman commented 6 years ago

Hi @tobold and @RahavLussato - Sara from Segment following up here. Pardon our delay in providing an update.

After some additional internal discussion on this matter, we've concluded we're not yet ready to move on implementing these changes. We'll need to take some extra time to make sure we have a solution that we're confident will work for all users, ideally without requiring custom implementation. I don't have a timeline to share on that just yet, but will keep this thread updated.

Thanks for your input here, and we appreciate your patience!

RahavLussato commented 6 years ago

Hi @srthurman, I understand the needs for seamless change and integration but the part that require from the user to have the custom code is only related to point 3

when the user data is rested intercom integration not doing shutdown and boot again with the new data.

the other 2 fixes another big issue related to the user data that I think it may worth to have it as soon as you can.

RahavLussato commented 5 years ago

hi, @srthurman we are waiting for a long time for this, what is the status for that. meantime it's requiring from me to work with a fork wich much harder to maintain. this PR is fixing 2 critical bugs:

  1. intercom integration is not using the segment anonymous id which leading to inconsistent data.
  2. intercom integration is not logging out when users log out.
srthurman commented 5 years ago

Hi @RahavLussato - thanks for checking in. I still don't have a timeline to share on this update based on other priorities the team is working on at the moment, but will be sure to update here when I do.

SegmentDestinationsBot commented 4 years ago

Hi @RahavLussato, as part of the monorepo migration, this issue has been moved to new issue. Our engineers have been notified and will prioritize and work on it ASAP. Thank you!

For more information, see README.md.