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.

Sign up for GitHub

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

Jump to bottom

Help: Track page views while users are chatting with olark operators. #1720

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Help: Track page views while users are chatting with olark operators. #1720

merged 1 commit into from
Jan 14, 2016

Conversation

omarjackman
Copy link
Contributor

This pull request allows Happiness Engineers to see the URL of the page being viewed by the user they are chatting with.

Olark operators will now see something similar to this:

screen shot 2015-12-17 at 5 56 51 am

Fixes #1506

@omarjackman omarjackman added [Status] In Progress Live Chat [Feature Group] Support All things related to WordPress.com customer support. labels Dec 17, 2015
@omarjackman omarjackman self-assigned this Dec 17, 2015
@omarjackman omarjackman force-pushed the update/olark-track-page-views branch from c52b64c to 19fa557 Compare December 17, 2015 10:55
@omarjackman omarjackman changed the title Track page views while users are chatting with olark operators. Help: Track page views while users are chatting with olark operators. Dec 17, 2015
@omarjackman omarjackman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 17, 2015
mtias
mtias reviewed Dec 17, 2015
View reviewed changes
client/lib/olark/index.js
@@ -373,3 +377,13 @@ const olark = {

emitter( olark );
olark.initialize();

export function trackPageViews( context, next ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this notifyLocation or something like that instead. Reading it in boot it made me think of stats instead.

@omarjackman omarjackman force-pushed the update/olark-track-page-views branch 2 times, most recently from 39e09e9 to fd02577 Compare December 17, 2015 17:30
@omarjackman omarjackman added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 17, 2015
@omarjackman
Copy link
Contributor Author

I found some issues here after doing some more testing. I'll submit for review again after its fixed.

@omarjackman omarjackman force-pushed the update/olark-track-page-views branch 2 times, most recently from 18fed93 to 3283d17 Compare December 17, 2015 18:47
@omarjackman omarjackman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 17, 2015
@omarjackman
Copy link
Contributor Author

I'd call this notifyLocation or something like that instead. Reading it in boot it made me think of stats instead.

@mtias I renamed the function and fixed an issue with notifying with the wrong url. Please review again

@omarjackman
Copy link
Contributor Author

@mtias @dllh @dmsnell any objections to shipping this?

dmsnell
dmsnell reviewed Dec 18, 2015
View reviewed changes
client/lib/olark/index.js
@@ -177,6 +178,9 @@ const olark = {
debug( 'Nickname: ' + visitorNickname );
debug( 'Group: ' + group );

// system.give_location_to_operator doesn't work well with our single page app so we do
// it our selves using page context in the notifyLocation function
olarkApi.configure( 'system.give_location_to_operator', false );
Copy link
Member

Choose a reason for hiding this comment

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

thank you for this comment - this is a great comment!

@dmsnell
Copy link
Member

dmsnell commented Dec 18, 2015

@omarjackman looks like a great feature - I'm sure the HEs will love it! Anyway, check out my note on the URL piece, that seems a bit risky the way it's written (not like it would break anything if a bug appeared though)

@dmsnell dmsnell added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 18, 2015
@omarjackman omarjackman force-pushed the update/olark-track-page-views branch from 3283d17 to 73b1fdc Compare January 12, 2016 02:07
@omarjackman omarjackman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jan 12, 2016
@omarjackman
Copy link
Contributor Author

@dmsnell I made the suggested changes.

@omarjackman
Copy link
Contributor Author

I'll be merging this tomorrow AM if there are no objections by then

@dllh
Copy link
Member

dllh commented Jan 14, 2016

The change seems reasonable to me. 🚢

omarjackman added a commit that referenced this pull request Jan 14, 2016
@omarjackman
Merge pull request #1720 from Automattic/update/olark-track-page-views
1e64b61 
Help: Track page views while users are chatting with olark operators.
@omarjackman omarjackman merged commit 1e64b61 into master Jan 14, 2016
@omarjackman omarjackman deleted the update/olark-track-page-views branch January 14, 2016 01:28
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 14, 2016
@omarjackman omarjackman restored the update/olark-track-page-views branch January 14, 2016 02:02
@omarjackman omarjackman mentioned this pull request Jan 14, 2016
Merged
@omarjackman
Copy link
Contributor Author

Had to revert this because of a missing ! that went missing during my last rebase

@lancewillett lancewillett deleted the update/olark-track-page-views branch January 14, 2016 16:48
This was referenced Jul 27, 2020
Open
Open
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Aug 21, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@omarjackman omarjackman

Labels
[Feature Group] Support All things related to WordPress.com customer support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@omarjackman @dmsnell @dllh @mtias @scruffian