-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
441b0de
2cab1fa
b725515
16b882b
32d3e31
88d24cd
d12951a
07755f6
e0b18e2
be2bd49
d10b59c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { connect } from 'react-redux'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { Button } from '@wordpress/components'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import './style.scss'; | ||
import { bumpStat } from '../utils/tracking'; | ||
import { removeNotice } from '../actions'; | ||
|
||
export const TRACKING_PROMPT_NOTICE_ID = 'notice:enable-tracking-prompt'; | ||
|
||
export function EnableTrackingPrompt( props ) { | ||
function dismissTrackingPrompt( enableTracking ) { | ||
window.setUserSetting( | ||
'gutenberg_tracking', | ||
enableTracking ? 'on' : 'off' | ||
); | ||
if ( enableTracking ) { | ||
bumpStat( 'tracking', 'opt-in' ); | ||
} | ||
props.removeNotice( TRACKING_PROMPT_NOTICE_ID ); | ||
} | ||
|
||
return ( | ||
<div className="enable-tracking-prompt"> | ||
<div className="enable-tracking-prompt__message"> | ||
{ __( 'Can Gutenberg collect data about your usage of the editor?' ) } | ||
<div className="enable-tracking-prompt__buttons"> | ||
<Button | ||
isPrimary | ||
isSmall | ||
onClick={ () => dismissTrackingPrompt( true ) } | ||
> | ||
{ __( 'Yes' ) } | ||
</Button> | ||
<Button | ||
isSecondary | ||
isSmall | ||
onClick={ () => dismissTrackingPrompt( false ) } | ||
> | ||
{ __( 'No' ) } | ||
</Button> | ||
</div> | ||
</div> | ||
<div className="enable-tracking-prompt__clarification"> | ||
{ __( 'Usage data is completely anonymous, does not include your post content, and will only be used to improve the editor.' ) } | ||
</div> | ||
</div> | ||
); | ||
} | ||
|
||
export default connect( | ||
undefined, | ||
{ removeNotice } | ||
)( EnableTrackingPrompt ); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
.enable-tracking-prompt { | ||
.enable-tracking-prompt__message { | ||
font-weight: bold; | ||
margin-top: 8px; | ||
|
||
.enable-tracking-prompt__buttons { | ||
float: right; | ||
|
||
.button { | ||
margin-left: 6px; | ||
} | ||
} | ||
} | ||
|
||
.enable-tracking-prompt__clarification { | ||
clear: both; | ||
margin-bottom: 8px; | ||
font-size: 90%; | ||
font-style: italic; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { mount } from 'enzyme'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I think we could have used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still having problems getting this to work, specifically with the |
||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { | ||
EnableTrackingPrompt, | ||
TRACKING_PROMPT_NOTICE_ID, | ||
} from '../'; | ||
|
||
describe( 'EnableTrackingPrompt', () => { | ||
const tracking = require( '../../utils/tracking' ); // no default export | ||
const originalSetUserSetting = window.setUserSetting; | ||
const originalBumpStat = tracking.bumpStat; | ||
let removeNotice; | ||
|
||
beforeEach( () => { | ||
window.setUserSetting = jest.fn(); | ||
tracking.bumpStat = jest.fn(); | ||
removeNotice = jest.fn(); | ||
} ); | ||
|
||
afterEach( () => { | ||
window.setUserSetting = originalSetUserSetting; | ||
tracking.bumpStat = originalBumpStat; | ||
} ); | ||
|
||
it( 'should render a prompt with Yes and No buttons', () => { | ||
const prompt = mount( | ||
<EnableTrackingPrompt /> | ||
); | ||
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( window.setUserSetting ) | ||
.not.toHaveBeenCalled(); | ||
expect( tracking.bumpStat ) | ||
.not.toHaveBeenCalled(); | ||
expect( removeNotice ) | ||
.not.toHaveBeenCalled(); | ||
} ); | ||
|
||
it( 'should enable tracking when clicking Yes', () => { | ||
const prompt = mount( | ||
<EnableTrackingPrompt removeNotice={ removeNotice } /> | ||
); | ||
const buttonYes = prompt.find( '.button' ) | ||
.filterWhere( node => node.text() === 'Yes' ); | ||
buttonYes.simulate( 'click' ); | ||
|
||
expect( window.setUserSetting ) | ||
.toHaveBeenCalledWith( 'gutenberg_tracking', 'on' ); | ||
expect( tracking.bumpStat ) | ||
.toHaveBeenCalledWith( 'tracking', 'opt-in' ); | ||
expect( removeNotice ) | ||
.toHaveBeenCalledWith( TRACKING_PROMPT_NOTICE_ID ); | ||
} ); | ||
|
||
it( 'should disable tracking when clicking No', () => { | ||
const prompt = mount( | ||
<EnableTrackingPrompt removeNotice={ removeNotice } /> | ||
); | ||
const buttonNo = prompt.find( '.button' ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To point above, this selector would not work with shallow rendering, but then again, it is not the place of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found that we couldn't look inside a shallow-rendered console.log( { text: prompt.find( 'Button' ).at( 0 ).text() } );
{ text: '<Button />' } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've incorporated the change to |
||
.filterWhere( node => node.text() === 'No' ); | ||
buttonNo.simulate( 'click' ); | ||
|
||
expect( window.setUserSetting ) | ||
.toHaveBeenCalledWith( 'gutenberg_tracking', 'off' ); | ||
expect( tracking.bumpStat ) | ||
.not.toHaveBeenCalled(); | ||
expect( removeNotice ) | ||
.toHaveBeenCalledWith( TRACKING_PROMPT_NOTICE_ID ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/* eslint-disable no-console */ | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { bumpStat } from '../tracking'; | ||
|
||
describe( 'bumpStat', () => { | ||
const originalConsoleError = console.error; | ||
const originalGetUserSetting = window.getUserSetting; | ||
|
||
beforeEach( () => { | ||
console.error = jest.fn(); | ||
window.getUserSetting = () => 'off'; | ||
} ); | ||
|
||
afterEach( () => { | ||
console.error = originalConsoleError; | ||
window.getUserSetting = originalGetUserSetting; | ||
} ); | ||
|
||
it( 'should reject non-string stat groups', () => { | ||
expect( bumpStat( 42, 'valid-name' ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat group names and stat names must be strings.' | ||
); | ||
} ); | ||
|
||
it( 'should reject non-string stat names', () => { | ||
expect( bumpStat( 'valid_group', 42 ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat group names and stat names must be strings.' | ||
); | ||
} ); | ||
|
||
it( 'should reject group names with invalid characters', () => { | ||
expect( bumpStat( 'invalid-group', 'valid-name' ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat group names must consist of letters, numbers, and underscores.' | ||
); | ||
} ); | ||
|
||
it( 'should reject group names longer than 22 chars', () => { | ||
expect( bumpStat( Array( 23 + 1 ).join( 'x' ), 'valid-name' ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat group names cannot be longer than 22 characters.' | ||
); | ||
} ); | ||
|
||
it( 'should reject stat names with invalid characters', () => { | ||
expect( bumpStat( 'group', 'invalidName' ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat names must consist of letters, numbers, underscores, and dashes.' | ||
); | ||
} ); | ||
|
||
it( 'should reject stat names longer than 32 chars', () => { | ||
expect( bumpStat( 'name', Array( 33 + 1 ).join( 'x' ) ) ).toBeUndefined(); | ||
expect( console.error ).toHaveBeenCalledWith( | ||
'Stat names cannot be longer than 32 characters.' | ||
); | ||
} ); | ||
|
||
it( 'should do nothing if the user has not opted in', () => { | ||
expect( bumpStat( 'valid_group', 'valid-name' ) ).toBeUndefined(); | ||
expect( console.error ).not.toHaveBeenCalled(); | ||
} ); | ||
|
||
it( 'should bump valid stats', () => { | ||
window.getUserSetting = () => 'on'; | ||
const url = bumpStat( 'valid_group', 'valid-name' ); | ||
// 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:' ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In actual usage this will either be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, we could run assertions against individual pieces from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've incorporated this change into #2205. |
||
.replace( /t=[0-9.]+$/, 't=NUMBER' ); | ||
expect( urlMatch ).toBe( | ||
'PROTOCOL://pixel.wp.com/g.gif?v=wpcom-no-pv' | ||
+ '&x_gutenberg_valid_group=valid-name' | ||
+ '&t=NUMBER' | ||
); | ||
} ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the
eslint-disable-line
here,eslint
reports this line as an error. This seems like a bug.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found quite a bit of prior discussion, which consensus seeming as though the specification does not allow a trailing comma in these cases:
comma-dangle
incorrectly requires commas after rest properties eslint/eslint#7297There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.