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

Add opt-in usage tracking for blocks added to post content #2140

Merged
merged 11 commits into from Aug 3, 2017

Conversation

Projects
None yet
4 participants
@nylen
Member

nylen commented Aug 1, 2017

This PR asks users for permission to enable usage tracking:

2017-08-01t18 15 11-0400

When the user clicks "Yes" or "No", the preference is stored using the existing WordPress method of saving user preferences (the setUserSetting function used elsewhere in wp-admin; props @swissspidy for telling us about this function in #1948). This preference is saved as a cookie and eventually persisted as a user option.

If the user clicks "Yes", tracking events will be recorded using existing WP.com infrastructure every time a block is added to the editor. The approach taken here is very similar to Calypso's event tracking code.

We can use the data added in this PR to inform various decisions such as default order for blocks and whether some blocks are less suitable for core, and more generally this is a very useful technique to collect user experience data.

isToggled,
className,
disabled,
...additionalProps, // eslint-disable-line comma-dangle

This comment has been minimized.

@nylen

nylen Aug 1, 2017

Member

Without the eslint-disable-line here, eslint reports this line as an error. This seems like a bug.

@nylen

nylen Aug 1, 2017

Member

Without the eslint-disable-line here, eslint reports this line as an error. This seems like a bug.

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

Found quite a bit of prior discussion, which consensus seeming as though the specification does not allow a trailing comma in these cases:

@aduth

aduth Aug 4, 2017

Member

Found quite a bit of prior discussion, which consensus seeming as though the specification does not allow a trailing comma in these cases:

This comment has been minimized.

@nylen

nylen Aug 4, 2017

Member

I guess that sort of makes sense, though it would be nice to have a better error message. I'll incorporate this change into #2205.

@nylen

nylen Aug 4, 2017

Member

I guess that sort of makes sense, though it would be nice to have a better error message. I'll incorporate this change into #2205.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 1, 2017

Codecov Report

Merging #2140 into master will increase coverage by 0.68%.
The diff coverage is 75.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2140      +/-   ##
==========================================
+ Coverage    22.3%   22.98%   +0.68%     
==========================================
  Files         137      141       +4     
  Lines        4291     4415     +124     
  Branches      722      743      +21     
==========================================
+ Hits          957     1015      +58     
- Misses       2815     2874      +59     
- Partials      519      526       +7
Impacted Files Coverage Δ
editor/index.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/enable-tracking-prompt/index.js 100% <100%> (ø)
components/button/index.js 90.9% <100%> (ø) ⬆️
editor/actions.js 39.28% <100%> (+2.24%) ⬆️
editor/utils/tracking.js 91.3% <91.3%> (ø)
blocks/library/button/index.js 26.66% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/index.js 0% <0%> (ø) ⬆️
... and 7 more

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 a1b1886...d10b59c. Read the comment docs.

codecov bot commented Aug 1, 2017

Codecov Report

Merging #2140 into master will increase coverage by 0.68%.
The diff coverage is 75.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2140      +/-   ##
==========================================
+ Coverage    22.3%   22.98%   +0.68%     
==========================================
  Files         137      141       +4     
  Lines        4291     4415     +124     
  Branches      722      743      +21     
==========================================
+ Hits          957     1015      +58     
- Misses       2815     2874      +59     
- Partials      519      526       +7
Impacted Files Coverage Δ
editor/index.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/enable-tracking-prompt/index.js 100% <100%> (ø)
components/button/index.js 90.9% <100%> (ø) ⬆️
editor/actions.js 39.28% <100%> (+2.24%) ⬆️
editor/utils/tracking.js 91.3% <91.3%> (ø)
blocks/library/button/index.js 26.66% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/index.js 0% <0%> (ø) ⬆️
... and 7 more

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 a1b1886...d10b59c. Read the comment docs.

@nylen

This comment has been minimized.

Show comment
Hide comment
@nylen

nylen Aug 1, 2017

Member

To test this PR, you can use the following commands in the browser console:

  • window.getUserSetting( 'gutenberg_tracking' ) - Returns the current value of the gutenberg_tracking user setting. Should be '' if unset or deleted; on immediately after clicking "Yes" and on subsequent page loads; and off immediately after clicking "No" and on subsequent page loads.
  • window.setUserSetting( 'gutenberg_tracking', '', true ); - Deletes the saved value of the gutenberg_tracking user setting. Should immediately stop all tracking events/messages and should cause the prompt to be shown again on the next page load.
Member

nylen commented Aug 1, 2017

To test this PR, you can use the following commands in the browser console:

  • window.getUserSetting( 'gutenberg_tracking' ) - Returns the current value of the gutenberg_tracking user setting. Should be '' if unset or deleted; on immediately after clicking "Yes" and on subsequent page loads; and off immediately after clicking "No" and on subsequent page loads.
  • window.setUserSetting( 'gutenberg_tracking', '', true ); - Deletes the saved value of the gutenberg_tracking user setting. Should immediately stop all tracking events/messages and should cause the prompt to be shown again on the next page load.

@nylen nylen added this to the Beta 0.7.0 milestone Aug 1, 2017

@@ -0,0 +1,61 @@
/* eslint-disable no-console */

This comment has been minimized.

@youknowriad

youknowriad Aug 2, 2017

Contributor

Maybe the tracking could be its own module, it could be useful outside of the editor (and other WP feature plugins later)

@youknowriad

youknowriad Aug 2, 2017

Contributor

Maybe the tracking could be its own module, it could be useful outside of the editor (and other WP feature plugins later)

This comment has been minimized.

@nylen

nylen Aug 2, 2017

Member

Do we have a path forward for making this work? (Working build system, publishing to npm)

@nylen

nylen Aug 2, 2017

Member

Do we have a path forward for making this work? (Working build system, publishing to npm)

This comment has been minimized.

@youknowriad

youknowriad Aug 2, 2017

Contributor

I was thinking, just moving this to a root folder in gutenberg for now. And once we have a WP.org infrastructure we could move to the packages repository

@youknowriad

youknowriad Aug 2, 2017

Contributor

I was thinking, just moving this to a root folder in gutenberg for now. And once we have a WP.org infrastructure we could move to the packages repository

This comment has been minimized.

@nylen

nylen Aug 2, 2017

Member

I don't think this belongs in a new root folder. Maybe as part of utils, but the prompt component should stay in editor.

@nylen

nylen Aug 2, 2017

Member

I don't think this belongs in a new root folder. Maybe as part of utils, but the prompt component should stay in editor.

This comment has been minimized.

@nylen

nylen Aug 2, 2017

Member

Then later we can move it over to https://github.com/WordPress/packages (I thought that was what you meant).

@nylen

nylen Aug 2, 2017

Member

Then later we can move it over to https://github.com/WordPress/packages (I thought that was what you meant).

This comment has been minimized.

@youknowriad

youknowriad Aug 2, 2017

Contributor

I don't think this belongs in a new root folder. Maybe as part of utils

For me a root folder is a separate module just like the packages repo, and I don't think we should add a lot of things to the utils module, I prefer smaller separate modules (like the ongoing PR for wordCounter)

@youknowriad

youknowriad Aug 2, 2017

Contributor

I don't think this belongs in a new root folder. Maybe as part of utils

For me a root folder is a separate module just like the packages repo, and I don't think we should add a lot of things to the utils module, I prefer smaller separate modules (like the ongoing PR for wordCounter)

This comment has been minimized.

@nylen

nylen Aug 3, 2017

Member

We should avoid adding a lot of small packages in this repo. This is what WordPress/packages is designed for; I'm fine with moving stats tracking over there once we get the build system working well. We will probably want to make it a bit more generic (remove the gutenberg_ prefix and modify the opt-in logic).

@nylen

nylen Aug 3, 2017

Member

We should avoid adding a lot of small packages in this repo. This is what WordPress/packages is designed for; I'm fine with moving stats tracking over there once we get the build system working well. We will probably want to make it a bit more generic (remove the gutenberg_ prefix and modify the opt-in logic).

Show outdated Hide outdated editor/enable-tracking-prompt/index.js
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 2, 2017

Contributor

Longer term (or not), WP.org needs its own tracking infrastructure, this could be very useful to enhance WordPress.

Contributor

youknowriad commented Aug 2, 2017

Longer term (or not), WP.org needs its own tracking infrastructure, this could be very useful to enhance WordPress.

@nylen

This comment has been minimized.

Show comment
Hide comment
@nylen

nylen Aug 3, 2017

Member

I plan to rebase and merge this later today. I expect it will provide a lot of good data that we can use to inform decisions.

Making the data available to the public is also doable, but will require some separate work on the WP.com side.

Member

nylen commented Aug 3, 2017

I plan to rebase and merge this later today. I expect it will provide a lot of good data that we can use to inform decisions.

Making the data available to the public is also doable, but will require some separate work on the WP.com side.

+ '=' + encodeURIComponent( name )
+ '&t=' + Math.random();
if ( process.env.NODE_ENV === 'development' ) {

This comment has been minimized.

@swissspidy

swissspidy Aug 3, 2017

Member

Seems like this should be unified with the process.env.NODE_ENV !== 'production' check below.

Especially since the console message says "non-production build".

@swissspidy

swissspidy Aug 3, 2017

Member

Seems like this should be unified with the process.env.NODE_ENV !== 'production' check below.

Especially since the console message says "non-production build".

This comment has been minimized.

@nylen

nylen Aug 3, 2017

Member

Right, good point.

I wrote it this way so the URL-building logic can be unit-tested (NODE_ENV === 'test'). However, we only want to display the URL message in development. I'll add those tests and change the message to say development build as well.

@nylen

nylen Aug 3, 2017

Member

Right, good point.

I wrote it this way so the URL-building logic can be unit-tested (NODE_ENV === 'test'). However, we only want to display the URL message in development. I'll add those tests and change the message to say development build as well.

// There are a couple of pieces of the URL where we don't care about
// testing the specific value. Replace them with placeholders.
const urlMatch = url
.replace( /^[a-z]+:/, 'PROTOCOL:' )

This comment has been minimized.

@nylen

nylen Aug 3, 2017

Member

In actual usage this will either be http: or https:. During test suite runs it is about:. This is difficult to change (jsdom/jsdom#1700) and it doesn't seem worth testing for a specific value.

@nylen

nylen Aug 3, 2017

Member

In actual usage this will either be http: or https:. During test suite runs it is about:. This is difficult to change (jsdom/jsdom#1700) and it doesn't seem worth testing for a specific value.

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

Alternatively, we could run assertions against individual pieces from url.parse.

@aduth

aduth Aug 4, 2017

Member

Alternatively, we could run assertions against individual pieces from url.parse.

This comment has been minimized.

@nylen

nylen Aug 4, 2017

Member

I've incorporated this change into #2205.

@nylen

nylen Aug 4, 2017

Member

I've incorporated this change into #2205.

@nylen nylen merged commit 51466b2 into master Aug 3, 2017

3 checks passed

codecov/project 22.98% (+0.68%) compared to a1b1886
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nylen nylen deleted the add/opt-in-usage-tracking branch Aug 3, 2017

nylen added a commit that referenced this pull request Aug 3, 2017

nylen added a commit that referenced this pull request Aug 3, 2017

/**
* External dependencies
*/
import { mount } from 'enzyme';

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

In this case I think we could have used shallow rendering which tends to be more performant (doesn't mount into DOM, doesn't render descendants).

@aduth

aduth Aug 4, 2017

Member

In this case I think we could have used shallow rendering which tends to be more performant (doesn't mount into DOM, doesn't render descendants).

This comment has been minimized.

@nylen

nylen Aug 4, 2017

Member

shallow doesn't work here because we reach inside a Button element and grab its text. Open to other suggestions.

@nylen

nylen Aug 4, 2017

Member

shallow doesn't work here because we reach inside a Button element and grab its text. Open to other suggestions.

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

shallow doesn't work here because we reach inside a Button element and grab its text. Open to other suggestions.

Remarked at #2140 (comment). It wasn't entirely as simple, but close:

diff --git a/editor/enable-tracking-prompt/test/index.js b/editor/enable-tracking-prompt/test/index.js
index 686fa0ec..098b95e0 100644
--- a/editor/enable-tracking-prompt/test/index.js
+++ b/editor/enable-tracking-prompt/test/index.js
@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { mount } from 'enzyme';
+import { shallow } from 'enzyme';
 
 /**
  * Internal dependencies
@@ -29,13 +29,13 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should render a prompt with Yes and No buttons', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt />
 		);
-		const buttons = prompt.find( '.button' );
+		const buttons = prompt.find( 'Button' );
 		expect( buttons.length ).toBe( 2 );
-		expect( buttons.at( 0 ).text() ).toBe( 'Yes' );
-		expect( buttons.at( 1 ).text() ).toBe( 'No' );
+		expect( buttons.at( 0 ).children().text() ).toBe( 'Yes' );
+		expect( buttons.at( 1 ).children().text() ).toBe( 'No' );
 
 		expect( window.setUserSetting )
 			.not.toHaveBeenCalled();
@@ -46,11 +46,11 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should enable tracking when clicking Yes', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt removeNotice={ removeNotice } />
 		);
-		const buttonYes = prompt.find( '.button' )
-			.filterWhere( node => node.text() === 'Yes' );
+		const buttonYes = prompt.find( 'Button' )
+			.filterWhere( node => node.children().text() === 'Yes' );
 		buttonYes.simulate( 'click' );
 
 		expect( window.setUserSetting )
@@ -62,11 +62,11 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should disable tracking when clicking No', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt removeNotice={ removeNotice } />
 		);
-		const buttonNo = prompt.find( '.button' )
-			.filterWhere( node => node.text() === 'No' );
+		const buttonNo = prompt.find( 'Button' )
+			.filterWhere( node => node.children().text() === 'No' );
 		buttonNo.simulate( 'click' );
 
 		expect( window.setUserSetting )
@aduth

aduth Aug 4, 2017

Member

shallow doesn't work here because we reach inside a Button element and grab its text. Open to other suggestions.

Remarked at #2140 (comment). It wasn't entirely as simple, but close:

diff --git a/editor/enable-tracking-prompt/test/index.js b/editor/enable-tracking-prompt/test/index.js
index 686fa0ec..098b95e0 100644
--- a/editor/enable-tracking-prompt/test/index.js
+++ b/editor/enable-tracking-prompt/test/index.js
@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { mount } from 'enzyme';
+import { shallow } from 'enzyme';
 
 /**
  * Internal dependencies
@@ -29,13 +29,13 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should render a prompt with Yes and No buttons', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt />
 		);
-		const buttons = prompt.find( '.button' );
+		const buttons = prompt.find( 'Button' );
 		expect( buttons.length ).toBe( 2 );
-		expect( buttons.at( 0 ).text() ).toBe( 'Yes' );
-		expect( buttons.at( 1 ).text() ).toBe( 'No' );
+		expect( buttons.at( 0 ).children().text() ).toBe( 'Yes' );
+		expect( buttons.at( 1 ).children().text() ).toBe( 'No' );
 
 		expect( window.setUserSetting )
 			.not.toHaveBeenCalled();
@@ -46,11 +46,11 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should enable tracking when clicking Yes', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt removeNotice={ removeNotice } />
 		);
-		const buttonYes = prompt.find( '.button' )
-			.filterWhere( node => node.text() === 'Yes' );
+		const buttonYes = prompt.find( 'Button' )
+			.filterWhere( node => node.children().text() === 'Yes' );
 		buttonYes.simulate( 'click' );
 
 		expect( window.setUserSetting )
@@ -62,11 +62,11 @@ describe( 'EnableTrackingPrompt', () => {
 	} );
 
 	it( 'should disable tracking when clicking No', () => {
-		const prompt = mount(
+		const prompt = shallow(
 			<EnableTrackingPrompt removeNotice={ removeNotice } />
 		);
-		const buttonNo = prompt.find( '.button' )
-			.filterWhere( node => node.text() === 'No' );
+		const buttonNo = prompt.find( 'Button' )
+			.filterWhere( node => node.children().text() === 'No' );
 		buttonNo.simulate( 'click' );
 
 		expect( window.setUserSetting )

This comment has been minimized.

@nylen

nylen Aug 7, 2017

Member

I am still having problems getting this to work, specifically with the it( 'should show and hide a popover when clicking More info' ) test not covered in the above diff. See 861218d.

@nylen

nylen Aug 7, 2017

Member

I am still having problems getting this to work, specifically with the it( 'should show and hide a popover when clicking More info' ) test not covered in the above diff. See 861218d.

const prompt = mount(
<EnableTrackingPrompt removeNotice={ removeNotice } />
);
const buttonNo = prompt.find( '.button' )

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

To point above, this selector would not work with shallow rendering, but then again, it is not the place of the EnableTrackingPrompt to know that an element of class .button is rendered by the Button component. Replacing this with 'Button' should work just as well.

@aduth

aduth Aug 4, 2017

Member

To point above, this selector would not work with shallow rendering, but then again, it is not the place of the EnableTrackingPrompt to know that an element of class .button is rendered by the Button component. Replacing this with 'Button' should work just as well.

This comment has been minimized.

@nylen

nylen Aug 4, 2017

Member

I found that we couldn't look inside a shallow-rendered Button element to find its text:

console.log( { text: prompt.find( 'Button' ).at( 0 ).text() } );

{ text: '<Button />' }
@nylen

nylen Aug 4, 2017

Member

I found that we couldn't look inside a shallow-rendered Button element to find its text:

console.log( { text: prompt.find( 'Button' ).at( 0 ).text() } );

{ text: '<Button />' }

This comment has been minimized.

@nylen

nylen Aug 4, 2017

Member

I've incorporated the change to Button instead of .button into #2205.

@nylen

nylen Aug 4, 2017

Member

I've incorporated the change to Button instead of .button into #2205.

// There are a couple of pieces of the URL where we don't care about
// testing the specific value. Replace them with placeholders.
const urlMatch = url
.replace( /^[a-z]+:/, 'PROTOCOL:' )

This comment has been minimized.

@aduth

aduth Aug 4, 2017

Member

Alternatively, we could run assertions against individual pieces from url.parse.

@aduth

aduth Aug 4, 2017

Member

Alternatively, we could run assertions against individual pieces from url.parse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment