Rename many functions and files for clarity

The main motivation here is to dramatically reduce the number
of places that use the same property name "enabled" for values
on different objects (e.g. "state", "actions", and "updates"
are all different things) with slightly different meanings. I
tried hard to come up with names that reflect better what each
meaning is.

Bug: T277639
Change-Id: Ie766259793f716262e3d4622ca55156d11f4842c

README.md

@@ -65,6 +65,8 @@ Popups works with a local copy of the [Mobile Content Service] too:
   functionality. In production builds, this same functionality can be
   enabled by setting a `debug=true` query. E.g.,
   `https://en.wikipedia.org/wiki/Popup?debug=true`.
+* When a QUnit test fails but you can't see why, open package.json and
+  temporarily remove the snippet `| tap-mocha-reporter dot`.
 
 ## Storybook.js Component Library
 

src/actions.js

@@ -57,16 +57,16 @@ function timedAction( baseAction ) {
  * [`mw.requestIdleCallback`](https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback))
  * so as not to impact latency-critical events.
  *
- * @param {boolean} isEnabled See `isEnabled.js`
+ * @param {boolean} initiallyEnabled Disables all popup types while still showing the footer link
  * @param {mw.user} user
  * @param {ext.popups.UserSettings} userSettings
  * @param {mw.Map} config The config of the MediaWiki client-side application,
  *  i.e. `mw.config`
- * @param {string} url url
+ * @param {string} url
  * @return {Object}
  */
 export function boot(
-	isEnabled,
+	initiallyEnabled,
 	user,
 	userSettings,
 	config,
@@ -77,7 +77,8 @@ export function boot(
 
 	return {
 		type: types.BOOT,
-		isEnabled,
+		initiallyEnabled,
+		// This is only used for logging
 		isNavPopupsEnabled: config.get( 'wgPopupsConflictsWithNavPopupGadget' ),
 		sessionToken: user.sessionId(),
 		pageToken: user.getPageviewToken(),
@@ -252,6 +253,9 @@ export function linkDwell( title, el, measures, gateway, generateToken, type ) {
 		return promise.then( () => {
 			const previewState = getState().preview;
 
+			// The `enabled` flag allows to disable all popup types while still showing the footer
+			// link. This comes from the boot() action (called `initiallyEnabled` there) and the
+			// preview() reducer.
 			if ( previewState.enabled && isNewInteraction() ) {
 				return dispatch( fetch( gateway, title, el, token, type ) );
 			}
@@ -411,7 +415,7 @@ export function hideSettings() {
  * N.B. This action returns a Redux.Thunk not because it needs to perform
  * asynchronous work, but because it needs to query the global state for the
  * current enabled state. In order to keep the enabled state in a single
- * place (the preview reducer), we query it and dispatch it as `wasEnabled`
+ * place (the preview reducer), we query it and dispatch it as `oldValue`
  * so that other reducers (like settings) can act on it without having to
  * duplicate the `enabled` state locally.
  * See docs/adr/0003-keep-enabled-state-only-in-preview-reducer.md for more
@@ -424,8 +428,8 @@ export function saveSettings( enabled ) {
 	return ( dispatch, getState ) => {
 		dispatch( {
 			type: types.SETTINGS_CHANGE,
-			wasEnabled: getState().preview.enabled,
-			enabled
+			oldValue: getState().preview.enabled,
+			newValue: enabled
 		} );
 	};
 }

src/changeListeners/syncUserSettings.js

@@ -22,13 +22,12 @@ export default function syncUserSettings( userSettings ) {
 	return ( prevState, state ) => {
 		syncIfChanged(
 			prevState, state, 'eventLogging.previewCount',
-			userSettings.setPreviewCount
+			userSettings.storePreviewCount
 		);
 		syncIfChanged(
 			prevState, state, 'preview.enabled',
-			userSettings.setIsEnabled
+			userSettings.storePagePreviewsEnabled
 		);
-
 	};
 }
 

src/index.js

@@ -11,13 +11,12 @@ import createUserSettings from './userSettings';
 import createPreviewBehavior from './previewBehavior';
 import createSettingsDialogRenderer from './ui/settingsDialogRenderer';
 import registerChangeListener from './changeListener';
-import createIsEnabled from './isEnabled';
+import createIsPagePreviewsEnabled from './isPagePreviewsEnabled';
 import { fromElement as titleFromElement } from './title';
 import { init as rendererInit } from './ui/renderer';
 import createExperiments from './experiments';
 import { isEnabled as isStatsvEnabled } from './instrumentation/statsv';
-import { isEnabled as isEventLoggingEnabled }
-	from './instrumentation/eventLogging';
+import { isEnabled as isEventLoggingEnabled } from './instrumentation/eventLogging';
 import changeListeners from './changeListeners';
 import * as actions from './actions';
 import reducers from './reducers';
@@ -185,7 +184,7 @@ function registerChangeListeners(
 			mw.config,
 			window
 		),
-		isEnabled = createIsEnabled( mw.user, userSettings, mw.config );
+		isPagePreviewsEnabled = createIsPagePreviewsEnabled( mw.user, userSettings, mw.config );
 
 	// If debug mode is enabled, then enable Redux DevTools.
 	if ( mw.config.get( 'debug' ) === true ||
@@ -212,7 +211,8 @@ function registerChangeListeners(
 	);
 
 	boundActions.boot(
-		isEnabled,
+		// FIXME: Currently this disables all popup types (for anonymous users).
+		isPagePreviewsEnabled,
 		mw.user,
 		userSettings,
 		mw.config,

src/integrations/mwpopups.js

@@ -11,6 +11,8 @@
  */
 export default function createMwPopups( store ) {
 	return {
+		// FIXME: This is underspecified. It's meant to be for PagePreviews, but might be false when
+		// another or all popup types are disabled.
 		isEnabled: function isEnabled() {
 			return store.getState().preview.enabled;
 		}

src/isPagePreviewsEnabled.js

@@ -1,5 +1,5 @@
 /**
- * @module isEnabled
+ * @module isPagePreviewsEnabled
  */
 
 /**
@@ -18,23 +18,23 @@
  *
  * @return {boolean}
  */
-export default function isEnabled( user, userSettings, config ) {
+export default function isPagePreviewsEnabled( user, userSettings, config ) {
 	if ( config.get( 'wgPopupsConflictsWithNavPopupGadget' ) ) {
 		return false;
 	}
 
-	if ( !user.isAnon() ) {
-		// For logged-in users, enablement is equal to the
-		// loading of this very code.
-		return true;
-	} else {
+	if ( user.isAnon() ) {
 		// For anonymous users, the code loads always, but the feature
 		// can be toggled at run-time via local storage.
 		if ( !userSettings.hasIsEnabled() ) {
 			// Default when no setting is stored.
 			return true;
 		} else {
-			return userSettings.getIsEnabled();
+			return userSettings.isPagePreviewsEnabled();
 		}
 	}
+
+	// For logged-in users, this very code loads only when PagePreviews are enabled.
+	// FIXME: This changes when there are more popup types.
+	return true;
 }

src/reducers/eventLogging.js

@@ -18,7 +18,7 @@ function getBaseData( bootAction ) {
 		namespaceIdSource: bootAction.page.namespaceId,
 		pageIdSource: bootAction.page.id,
 		isAnon: bootAction.user.isAnon,
-		popupEnabled: bootAction.isEnabled,
+		popupEnabled: bootAction.initiallyEnabled,
 		pageToken: bootAction.pageToken,
 		sessionToken: bootAction.sessionToken,
 		previewCountBucket: counts.getPreviewCountBucket(
@@ -296,7 +296,7 @@ export default function eventLogging( state, action ) {
 			} );
 
 		case actionTypes.SETTINGS_CHANGE:
-			if ( action.wasEnabled && !action.enabled ) {
+			if ( action.oldValue && !action.newValue ) {
 				return nextState( state, {
 					event: {
 						action: 'disabled',

src/reducers/preview.js

@@ -25,12 +25,12 @@ export default function preview( state, action ) {
 	switch ( action.type ) {
 		case actionTypes.BOOT:
 			return nextState( state, {
-				enabled: action.isEnabled
+				enabled: action.initiallyEnabled
 			} );
 
 		case actionTypes.SETTINGS_CHANGE:
 			return nextState( state, {
-				enabled: action.enabled
+				enabled: action.newValue
 			} );
 
 		case actionTypes.LINK_DWELL:

src/reducers/settings.js

@@ -29,7 +29,7 @@ export default function settings( state, action ) {
 				showHelp: false
 			} );
 		case actionTypes.SETTINGS_CHANGE:
-			return action.wasEnabled === action.enabled ?
+			return action.oldValue === action.newValue ?
 				// If the setting is the same, just hide the dialogs
 				nextState( state, {
 					shouldShow: false
@@ -38,17 +38,17 @@ export default function settings( state, action ) {
 				nextState( state, {
 					// If we enabled, we just hide directly, no help
 					// If we disabled, keep it showing and let the ui show the help.
-					shouldShow: !action.enabled,
-					showHelp: !action.enabled,
+					shouldShow: !action.newValue,
+					showHelp: !action.newValue,
 
 					// Since the footer link is only ever shown to anonymous users (see
 					// the BOOT case below), state.userIsAnon is always truthy here.
-					shouldShowFooterLink: !action.enabled
+					shouldShowFooterLink: !action.newValue
 				} );
 
 		case actionTypes.BOOT:
 			return nextState( state, {
-				shouldShowFooterLink: action.user.isAnon && !action.isEnabled
+				shouldShowFooterLink: action.user.isAnon && !action.initiallyEnabled
 			} );
 		default:
 			return state;

src/userSettings.js

@@ -8,14 +8,14 @@
  * @global
  */
 
-const IS_ENABLED_KEY = 'mwe-popups-enabled',
+const PAGE_PREVIEWS_ENABLED_KEY = 'mwe-popups-enabled',
 	PREVIEW_COUNT_KEY = 'ext.popups.core.previewCount';
 
 /**
  * Creates an object whose methods encapsulate all interactions with the UA's
  * storage.
  *
- * @param {Object} storage The `mw.storage` singleton instance
+ * @param {mw.storage} storage The `mw.storage` singleton instance
  *
  * @return {UserSettings}
  */
@@ -25,27 +25,27 @@ export default function createUserSettings( storage ) {
 		 * Gets whether the user has previously enabled Page Previews.
 		 *
 		 * N.B. that if the user hasn't previously enabled or disabled Page
-		 * Previews, i.e. userSettings.setIsEnabled(true), then they are treated as
+		 * Previews, i.e. userSettings.storePagePreviewsEnabled(true), then they are treated as
 		 * if they have enabled them.
 		 *
 		 * @method
-		 * @name UserSettings#getIsEnabled
+		 * @name UserSettings#isPagePreviewsEnabled
 		 * @return {boolean}
 		 */
-		getIsEnabled() {
-			return storage.get( IS_ENABLED_KEY ) !== '0';
+		isPagePreviewsEnabled() {
+			return storage.get( PAGE_PREVIEWS_ENABLED_KEY ) !== '0';
 		},
 
 		/**
-		 * Sets whether the user has enabled Page Previews.
+		 * Permanently persists (typically in localStorage) whether the user has enabled Page
+		 * Previews.
 		 *
 		 * @method
-		 * @name UserSettings#setIsEnabled
-		 * @param {boolean} isEnabled
-		 * @return {void}
+		 * @name UserSettings#storePagePreviewsEnabled
+		 * @param {boolean} enabled
 		 */
-		setIsEnabled( isEnabled ) {
-			storage.set( IS_ENABLED_KEY, isEnabled ? '1' : '0' );
+		storePagePreviewsEnabled( enabled ) {
+			storage.set( PAGE_PREVIEWS_ENABLED_KEY, enabled ? '1' : '0' );
 		},
 
 		/**
@@ -57,7 +57,7 @@ export default function createUserSettings( storage ) {
 		 * @return {boolean}
 		 */
 		hasIsEnabled() {
-			const value = storage.get( IS_ENABLED_KEY );
+			const value = storage.get( PAGE_PREVIEWS_ENABLED_KEY );
 
 			return Boolean( value ) !== false;
 		},
@@ -86,7 +86,7 @@ export default function createUserSettings( storage ) {
 			// stored number is not a zero, override it to zero and store new value
 			if ( isNaN( count ) ) {
 				count = 0;
-				this.setPreviewCount( count );
+				this.storePreviewCount( count );
 			}
 			return count;
 		},
@@ -95,11 +95,10 @@ export default function createUserSettings( storage ) {
 		 * Sets the number of previews that the user has seen.
 		 *
 		 * @method
-		 * @name UserSettings#setPreviewCount
+		 * @name UserSettings#storePreviewCount
 		 * @param {number} count
-		 * @return {void}
 		 */
-		setPreviewCount( count ) {
+		storePreviewCount( count ) {
 			storage.set( PREVIEW_COUNT_KEY, count.toString() );
 		}
 	};

tests/node-qunit/actions.test.js

@@ -41,7 +41,7 @@ QUnit.test( '#boot', ( assert ) => {
 		action,
 		{
 			type: actionTypes.BOOT,
-			isEnabled: false,
+			initiallyEnabled: false,
 			isNavPopupsEnabled: true,
 			sessionToken: '0123456789',
 			pageToken: '9876543210',
@@ -504,8 +504,8 @@ QUnit.test( 'it should dispatch an action with previous and current enabled stat
 	assert.ok(
 		dispatch.calledWith( {
 			type: actionTypes.SETTINGS_CHANGE,
-			wasEnabled: false,
-			enabled: true
+			oldValue: false,
+			newValue: true
 		} ),
 		'it should dispatch the action with the previous and next enabled state'
 	);

tests/node-qunit/changeListeners/syncUserSettings.test.js

@@ -3,8 +3,8 @@ import syncUserSettings from '../../../src/changeListeners/syncUserSettings';
 QUnit.module( 'ext.popups/changeListeners/syncUserSettings', {
 	beforeEach() {
 		this.userSettings = {
-			setPreviewCount: this.sandbox.spy(),
-			setIsEnabled: this.sandbox.spy()
+			storePreviewCount: this.sandbox.spy(),
+			storePagePreviewsEnabled: this.sandbox.spy()
 		};
 
 		this.changeListener = syncUserSettings( this.userSettings );
@@ -29,7 +29,7 @@ QUnit.test(
 		this.changeListener( prevState, state );
 
 		assert.notOk(
-			this.userSettings.setPreviewCount.called,
+			this.userSettings.storePreviewCount.called,
 			'The preview count is unchanged.'
 		);
 	}
@@ -48,7 +48,7 @@ QUnit.test( 'it should update the storage if the previewCount has changed', func
 	this.changeListener( prevState, state );
 
 	assert.ok(
-		this.userSettings.setPreviewCount.calledWith( 223 ),
+		this.userSettings.storePreviewCount.calledWith( 223 ),
 		'The preview count is updated.'
 	);
 } );
@@ -71,7 +71,7 @@ QUnit.test(
 		this.changeListener( prevState, state );
 
 		assert.notOk(
-			this.userSettings.setIsEnabled.called,
+			this.userSettings.storePagePreviewsEnabled.called,
 			'The user setting is unchanged.'
 		);
 	}
@@ -90,7 +90,7 @@ QUnit.test( 'it should update the storage if the enabled flag has changed', func
 	this.changeListener( prevState, state );
 
 	assert.ok(
-		this.userSettings.setIsEnabled.calledWith( false ),
+		this.userSettings.storePagePreviewsEnabled.calledWith( false ),
 		'The user setting is disabled.'
 	);
 } );

tests/node-qunit/integration.test.js

@@ -88,7 +88,7 @@ QUnit.module( 'ext.popups preview @integration', {
 		this.el = $( '<a>' ).attr( 'href', '/wiki/Foo' ).get( 0 );
 
 		this.actions.boot(
-			/* isEnabled: */
+			/* initiallyEnabled: */
 			constant( true ),
 			/* user */
 			stubs.createStubUser( true ),

tests/node-qunit/isPagePreviewsEnabled.test.js

@@ -1,18 +1,18 @@
 import * as stubs from './stubs';
-import isEnabled from '../../src/isEnabled';
+import isPagePreviewsEnabled from '../../src/isPagePreviewsEnabled';
 
 function createStubUserSettings( expectEnabled ) {
 	return {
 		hasIsEnabled() {
 			return expectEnabled !== undefined;
 		},
-		getIsEnabled() {
+		isPagePreviewsEnabled() {
 			return Boolean( expectEnabled );
 		}
 	};
 }
 
-QUnit.module( 'ext.popups#isEnabled (logged out)', {
+QUnit.module( 'ext.popups#isPagePreviewsEnabled (logged out)', {
 	beforeEach() {
 		this.user = stubs.createStubUser( /* isAnon = */ true );
 	}
@@ -25,7 +25,7 @@ QUnit.test( 'is should handle logged out users', ( assert ) => {
 	const cases = [
 		/*
 		[
-			<isAnon>, <expected value of isEnabled>, <test description>
+			<isAnon>, <expected value of isPagePreviewsEnabled>, <test description>
 		]
 		*/
 		[ undefined, true, 'When the user hasn\'t enabled or disabled' +
@@ -40,7 +40,7 @@ QUnit.test( 'is should handle logged out users', ( assert ) => {
 		userSettings = createStubUserSettings( testCase[ 0 ] );
 
 		assert.strictEqual(
-			isEnabled( user, userSettings, config ),
+			isPagePreviewsEnabled( user, userSettings, config ),
 			testCase[ 1 ],
 			testCase[ 2 ]
 		);
@@ -53,7 +53,7 @@ QUnit.test( 'it should handle logged in users', ( assert ) => {
 		config = new Map();
 
 	assert.ok(
-		isEnabled( user, userSettings, config ),
+		isPagePreviewsEnabled( user, userSettings, config ),
 		'If the user is logged in and the user is in the on group, then it\'s enabled.'
 	);
 } );
@@ -66,7 +66,7 @@ QUnit.test( 'it should handle the conflict with the Navigation Popups Gadget', (
 	config.set( 'wgPopupsConflictsWithNavPopupGadget', true );
 
 	assert.notOk(
-		isEnabled( user, userSettings, config ),
+		isPagePreviewsEnabled( user, userSettings, config ),
 		'Page Previews is disabled when it conflicts with the Navigation Popups Gadget.'
 	);
 

tests/node-qunit/reducers/eventLogging.test.js

@@ -27,7 +27,7 @@ QUnit.test( '@@INIT', function ( assert ) {
 QUnit.test( 'BOOT', function ( assert ) {
 	const action = {
 		type: actionTypes.BOOT,
-		isEnabled: true,
+		initiallyEnabled: true,
 		isNavPopupsEnabled: false,
 		sessionToken: '0123456789',
 		pageToken: '9876543210',
@@ -59,7 +59,7 @@ QUnit.test( 'BOOT', function ( assert ) {
 				namespaceIdSource: action.page.namespaceId,
 				pageIdSource: action.page.id,
 				isAnon: action.user.isAnon,
-				popupEnabled: action.isEnabled,
+				popupEnabled: action.initiallyEnabled,
 				pageToken: action.pageToken,
 				sessionToken: action.sessionToken,
 				editCountBucket: expectedEditCountBucket,
@@ -628,8 +628,8 @@ QUnit.test( 'SETTINGS_SHOW should enqueue a "tapped settings cog" event', functi
 QUnit.test( 'SETTINGS_CHANGE should enqueue disabled event', ( assert ) => {
 	let state = eventLogging( undefined, {
 		type: actionTypes.SETTINGS_CHANGE,
-		wasEnabled: false,
-		enabled: false
+		oldValue: false,
+		newValue: false
 	} );
 
 	assert.strictEqual(
@@ -640,8 +640,8 @@ QUnit.test( 'SETTINGS_CHANGE should enqueue disabled event', ( assert ) => {
 
 	state = eventLogging( state, {
 		type: actionTypes.SETTINGS_CHANGE,
-		wasEnabled: true,
-		enabled: false
+		oldValue: true,
+		newValue: false
 	} );
 
 	assert.deepEqual(
@@ -656,8 +656,8 @@ QUnit.test( 'SETTINGS_CHANGE should enqueue disabled event', ( assert ) => {
 	delete state.event;
 	state = eventLogging( state, {
 		type: actionTypes.SETTINGS_CHANGE,
-		wasEnabled: false,
-		enabled: true
+		oldValue: false,
+		newValue: true
 	} );
 
 	assert.strictEqual(

tests/node-qunit/reducers/preview.test.js

@@ -31,7 +31,7 @@ QUnit.test( '@@INIT', ( assert ) => {
 QUnit.test( 'BOOT', ( assert ) => {
 	const action = {
 		type: actionTypes.BOOT,
-		isEnabled: true
+		initiallyEnabled: true
 	};
 
 	assert.expect( 1, 'All assertions are executed.' );
@@ -48,7 +48,7 @@ QUnit.test( 'BOOT', ( assert ) => {
 QUnit.test( 'SETTINGS_CHANGE', ( assert ) => {
 	const action = {
 		type: actionTypes.SETTINGS_CHANGE,
-		enabled: true
+		newValue: true
 	};
 
 	assert.expect( 1, 'All assertions are executed.' );

tests/node-qunit/reducers/settings.test.js

@@ -20,7 +20,7 @@ QUnit.test( '@@INIT', ( assert ) => {
 QUnit.test( 'BOOT', ( assert ) => {
 	const action = {
 		type: actionTypes.BOOT,
-		isEnabled: false,
+		initiallyEnabled: false,
 		user: {
 			isAnon: true
 		}
@@ -75,11 +75,11 @@ QUnit.test( 'SETTINGS_HIDE', ( assert ) => {
 } );
 
 QUnit.test( 'SETTINGS_CHANGE', ( assert ) => {
-	const action = ( wasEnabled, enabled ) => {
+	const action = ( oldValue, newValue ) => {
 		return {
 			type: actionTypes.SETTINGS_CHANGE,
-			wasEnabled,
-			enabled
+			oldValue,
+			newValue
 		};
 	};
 

tests/node-qunit/userSettings.test.js

@@ -8,21 +8,21 @@ QUnit.module( 'ext.popups/userSettings', {
 	}
 } );
 
-QUnit.test( '#getIsEnabled should return false if Page Previews have been disabled', function ( assert ) {
-	this.userSettings.setIsEnabled( false );
+QUnit.test( '#isPagePreviewsEnabled should return false if Page Previews have been disabled', function ( assert ) {
+	this.userSettings.storePagePreviewsEnabled( false );
 
 	assert.notOk(
-		this.userSettings.getIsEnabled(),
+		this.userSettings.isPagePreviewsEnabled(),
 		'The user has disabled Page Previews.'
 	);
 
 	// ---
 
-	this.userSettings.setIsEnabled( true );
+	this.userSettings.storePagePreviewsEnabled( true );
 
 	assert.ok(
-		this.userSettings.getIsEnabled(),
-		'#getIsEnabled should return true if Page Previews have been enabled'
+		this.userSettings.isPagePreviewsEnabled(),
+		'#isPagePreviewsEnabled should return true if Page Previews have been enabled'
 	);
 } );
 
@@ -34,7 +34,7 @@ QUnit.test( '#hasIsEnabled', function ( assert ) {
 
 	// ---
 
-	this.userSettings.setIsEnabled( false );
+	this.userSettings.storePagePreviewsEnabled( false );
 
 	assert.ok(
 		this.userSettings.hasIsEnabled(),
@@ -81,8 +81,8 @@ QUnit.test( '#getPreviewCount should return the count as a number', function ( a
 	);
 } );
 
-QUnit.test( '#setPreviewCount should store the count as a string', function ( assert ) {
-	this.userSettings.setPreviewCount( 222 );
+QUnit.test( '#storePreviewCount should store the count as a string', function ( assert ) {
+	this.userSettings.storePreviewCount( 222 );
 
 	assert.strictEqual(
 		this.storage.get( 'ext.popups.core.previewCount' ),

webpack.config.js

@@ -112,8 +112,8 @@ module.exports = ( env, argv ) => ( {
 		// Minified uncompressed size limits for chunks / assets and entrypoints. Keep these numbers
 		// up-to-date and rounded to the nearest 10th of a kibibyte so that code sizing costs are
 		// well understood. Related to bundlesize minified, gzipped compressed file size tests.
-		maxAssetSize: 43.4 * 1024,
-		maxEntrypointSize: 43.4 * 1024,
+		maxAssetSize: 43.5 * 1024,
+		maxEntrypointSize: 43.5 * 1024,
 
 		// The default filter excludes map files but we rename ours.
 		assetFilter: ( filename ) => !filename.endsWith( srcMapExt )