Revert "Generalize settings code"

This reverts commit 6924a89b07.

Reason for revert: Breaks persistence of setting
for anonymous users.

Change-Id: I3efc20f44281c1c68c4162584388e33bb38c4848
This commit is contained in:
Jon Robson 2023-10-19 14:12:09 -07:00
parent f504232535
commit a6a65204c6
20 changed files with 210 additions and 195 deletions

View file

@ -14,7 +14,7 @@
"//": "Set the coverage percentage by category thresholds.",
"statements": 80,
"branches": 68,
"branches": 70,
"functions": 80,
"lines": 90,

Binary file not shown.

Binary file not shown.

View file

@ -5,7 +5,6 @@
export default {
BOOT: 'BOOT',
LINK_DWELL: 'LINK_DWELL',
REGISTER_SETTING: 'REGISTER_SETTING',
ABANDON_START: 'ABANDON_START',
ABANDON_END: 'ABANDON_END',
LINK_CLICK: 'LINK_CLICK',

View file

@ -72,22 +72,6 @@ export function boot(
};
}
/**
* Registers a page preview setting for anonymous users.
*
* @param {string} name setting name which is used for storage and deriving associated
* messages.
* @param {boolean} enabled is the feature enabled by default?
* @return {Object}
*/
export function registerSetting( name, enabled ) {
return {
type: types.REGISTER_SETTING,
name,
enabled
};
}
/**
* Represents Page Previews fetching data via the gateway.
*
@ -229,6 +213,7 @@ export function linkDwell( title, el, measures, gateway, generateToken, type ) {
return promise.then( () => {
const previewState = getState().preview;
const enabledValue = previewState.enabled[ type ];
// Note: Only reference previews and default previews can be disabled at this point.
// If there is no UI the enabledValue is always true.
const isEnabled = typeof enabledValue === 'undefined' ? true : enabledValue;

View file

@ -4,6 +4,7 @@ import pageviews from './pageviews';
import render from './render';
import settings from './settings';
import statsv from './statsv';
import syncUserSettings from './syncUserSettings';
export default {
footerLink,
@ -11,5 +12,6 @@ export default {
pageviews,
render,
settings,
statsv
statsv,
syncUserSettings
};

View file

@ -13,19 +13,12 @@ export default function settings( boundActions, render ) {
// Nothing to do on initialization
return;
}
if (
settingsObj &&
Object.keys( oldState.settings.keyValues ).length !== Object.keys( newState.settings.keyValues ).length
) {
// the number of settings changed so force it to be repainted.
settingsObj.refresh( newState.settings.keyValues );
}
// Update global modal visibility
if ( oldState.settings.shouldShow === false && newState.settings.shouldShow ) {
// Lazily instantiate the settings UI
if ( !settingsObj ) {
settingsObj = render( boundActions, newState.settings.keyValues );
settingsObj = render( boundActions );
settingsObj.appendTo( document.body );
}

View file

@ -0,0 +1,67 @@
/**
* @module changeListeners/syncUserSettings
*/
import { previewTypes } from '../preview/model';
/**
* Creates an instance of the user settings sync change listener.
*
* This change listener syncs certain parts of the state tree to user
* settings when they change.
*
* Used for:
*
* * Enabled state: If the previews are enabled or disabled.
* * Preview count: When the user dwells on a link for long enough that
* a preview is shown, then their preview count will be incremented (see
* `reducers/eventLogging.js`, and is persisted to local storage.
*
* @param {ext.popups.UserSettings} userSettings
* @return {ext.popups.ChangeListener}
*/
export default function syncUserSettings( userSettings ) {
return ( oldState, newState ) => {
syncIfChanged(
oldState, newState, 'preview.enabled.' + previewTypes.TYPE_PAGE,
userSettings.storePagePreviewsEnabled
);
syncIfChanged(
oldState, newState, 'preview.enabled.' + previewTypes.TYPE_REFERENCE,
userSettings.storeReferencePreviewsEnabled
);
};
}
/**
* Given a state tree, reducer and property, safely return the value of the
* property if the reducer and property exist
*
* @param {Object} state tree
* @param {string} path dot-separated path in the state tree
* @return {*}
*/
function get( state, path ) {
return path.split( '.' ).reduce(
( element, key ) => element && element[ key ],
state
);
}
/**
* Calls a sync function if the property prop on the property reducer on
* the state trees has changed value.
*
* @param {Object} oldState
* @param {Object} newState
* @param {string} path dot-separated path in the state tree
* @param {Function} sync function to be called with the newest value if
* changed
* @return {void}
*/
function syncIfChanged( oldState, newState, path, sync ) {
const current = get( newState, path );
if ( oldState && ( get( oldState, path ) !== current ) ) {
sync( current );
}
}

View file

@ -118,6 +118,8 @@ function registerChangeListeners(
registerChangeListener( store, changeListeners.render( previewBehavior ) );
registerChangeListener(
store, changeListeners.statsv( registerActions, statsvTracker ) );
registerChangeListener(
store, changeListeners.syncUserSettings( userSettings ) );
registerChangeListener(
store, changeListeners.settings( registerActions, settingsDialog ) );
registerChangeListener( store,
@ -190,11 +192,15 @@ function handleDOMEventIfEligible( handler ) {
referenceGateway = createReferenceGateway(),
userSettings = createUserSettings( mw.storage ),
referencePreviewsState = isReferencePreviewsEnabled( mw.user, userSettings, mw.config ),
settingsDialog = createSettingsDialogRenderer(),
settingsDialog = createSettingsDialogRenderer( referencePreviewsState !== null ),
experiments = createExperiments( mw.experiments ),
statsvTracker = getStatsvTracker( mw.user, mw.config, experiments ),
pageviewTracker = getPageviewTracker( mw.config ),
pagePreviewState = createIsPagePreviewsEnabled( mw.user, userSettings, mw.config );
initiallyEnabled = {
[ previewTypes.TYPE_PAGE ]:
createIsPagePreviewsEnabled( mw.user, userSettings, mw.config ),
[ previewTypes.TYPE_REFERENCE ]: referencePreviewsState
};
// If debug mode is enabled, then enable Redux DevTools.
if ( mw.config.get( 'debug' ) ||
@ -219,7 +225,7 @@ function handleDOMEventIfEligible( handler ) {
);
boundActions.boot(
{},
initiallyEnabled,
mw.user,
userSettings,
mw.config,
@ -231,15 +237,13 @@ function handleDOMEventIfEligible( handler ) {
* extensions can query it (T171287)
*/
mw.popups = createMediaWikiPopupsObject(
store, registerModel, registerPreviewUI, registerGatewayForPreviewType,
boundActions.registerSetting
store, registerModel, registerPreviewUI, registerGatewayForPreviewType
);
if ( pagePreviewState !== null ) {
if ( initiallyEnabled[ previewTypes.TYPE_PAGE ] !== null ) {
const excludedLinksSelector = EXCLUDED_LINK_SELECTORS.join( ', ' );
// Register default preview type
mw.popups.register( {
enabled: pagePreviewState,
type: previewTypes.TYPE_PAGE,
selector: `#mw-content-text a[href][title]:not(${excludedLinksSelector})`,
delay: FETCH_COMPLETE_TARGET_DELAY - FETCH_START_DELAY,
@ -253,10 +257,9 @@ function handleDOMEventIfEligible( handler ) {
]
} );
}
if ( referencePreviewsState !== null ) {
if ( initiallyEnabled[ previewTypes.TYPE_REFERENCE ] !== null ) {
// Register the reference preview type
mw.popups.register( {
enabled: referencePreviewsState,
type: previewTypes.TYPE_REFERENCE,
selector: '#mw-content-text .reference a[ href*="#" ]',
delay: FETCH_DELAY_REFERENCE_TYPE,

View file

@ -12,12 +12,9 @@ import { previewTypes } from '../preview/model';
* @param {Function} registerModel allows extensions to register custom preview handlers.
* @param {Function} registerPreviewUI allows extensions to register custom preview renderers.
* @param {Function} registerGatewayForPreviewType allows extensions to register gateways for preview types.
* @param {Function} registerSetting
* @return {Object} external Popups interface
*/
export default function createMwPopups( store, registerModel, registerPreviewUI,
registerGatewayForPreviewType, registerSetting
) {
export default function createMwPopups( store, registerModel, registerPreviewUI, registerGatewayForPreviewType ) {
return {
/**
* @return {boolean} If Page Previews are currently active
@ -62,7 +59,7 @@ export default function createMwPopups( store, registerModel, registerPreviewUI,
* @param {PopupModule} module
*/
register: function ( module ) {
const { type, selector, gateway, renderFn, subTypes, delay, init, enabled } = module;
const { type, selector, gateway, renderFn, subTypes, delay, init } = module;
if ( !type || !selector || !gateway ) {
throw new Error(
`Registration of Popups custom preview type "${type}" failed: You must specify a type, a selector, and a gateway.`
@ -71,16 +68,6 @@ export default function createMwPopups( store, registerModel, registerPreviewUI,
registerModel( type, selector, delay );
registerGatewayForPreviewType( type, gateway );
registerPreviewUI( type, renderFn );
// Only show if doesn't exist.
if ( mw.message( `popups-settings-option-${type}` ).exists() ) {
registerSetting( type, enabled === undefined ? true : enabled );
/* global process */
} else if ( process.env.NODE_ENV !== 'production' ) {
mw.log.warn(
`[Popups] No setting for ${type} registered.
Please create message with key "popups-settings-option-${type}" if this is a mistake.`
);
}
if ( subTypes ) {
subTypes.forEach( function ( subTypePreview ) {
registerPreviewUI( subTypePreview.type, subTypePreview.renderFn );

View file

@ -28,12 +28,7 @@ export default function preview( state, action ) {
return nextState( state, {
enabled: action.initiallyEnabled
} );
case actionTypes.REGISTER_SETTING:
return nextState( state, {
enabled: Object.assign( {}, state.enabled, {
[ action.name ]: action.enabled
} )
} );
case actionTypes.SETTINGS_CHANGE: {
return nextState( state, {
enabled: action.newValue

View file

@ -12,7 +12,6 @@ export default function settings( state, action ) {
if ( state === undefined ) {
state = {
shouldShow: false,
keyValues: {},
showHelp: false,
shouldShowFooterLink: false
};
@ -58,23 +57,12 @@ export default function settings( state, action ) {
shouldShowFooterLink: anyDisabled
} );
}
case actionTypes.REGISTER_SETTING: {
const keyValues = Object.assign( {}, state.keyValues, {
[ action.name ]: action.enabled
} );
return nextState( state, {
keyValues,
shouldShowFooterLink: state.shouldShowFooterLink || !action.enabled
} );
}
case actionTypes.BOOT: {
// Warning, when the state is null the user can't re-enable this popup type!
const anyDisabled = Object.keys( action.initiallyEnabled )
.some( ( type ) => action.initiallyEnabled[ type ] === false );
const keyValues = Object.assign( {}, action.initiallyEnabled );
return nextState( state, {
keyValues,
shouldShowFooterLink: action.user.isAnon && anyDisabled
} );
}

View file

@ -3,28 +3,33 @@
*/
import { renderSettingsDialog } from './templates/settingsDialog/settingsDialog';
import { previewTypes } from '../preview/model';
/**
* Create the settings dialog shown to anonymous users.
*
* @param {Object} keyValues
* @param {boolean} referencePreviewsAvaliable
* @return {HTMLElement} settings dialog
*/
export function createSettingsDialog( keyValues ) {
const choices = Object.keys( keyValues ).map( ( id ) => (
export function createSettingsDialog( referencePreviewsAvaliable ) {
const choices = [
{
id,
// This can produce:
// * popups-settings-option-preview
// * popups-settings-option-reference
name: mw.msg( `popups-settings-option-${id}` ),
// This can produce:
// * popups-settings-option-preview-description
// * popups-settings-option-reference-description
description: mw.msg( `popups-settings-option-${id}-description` ),
isChecked: keyValues[ id ]
id: previewTypes.TYPE_PAGE,
name: mw.msg( 'popups-settings-option-page' ),
description: mw.msg( 'popups-settings-option-page-description' )
},
{
id: previewTypes.TYPE_REFERENCE,
name: mw.msg( 'popups-settings-option-reference' ),
description: mw.msg( 'popups-settings-option-reference-description' )
}
) );
];
// TODO: Remove when not in Beta any more
if ( !referencePreviewsAvaliable ) {
// Anonymous users can't access reference previews as long as they are in beta
choices.splice( 1, 1 );
}
return renderSettingsDialog( {
heading: mw.msg( 'popups-settings-title' ),

View file

@ -4,34 +4,14 @@
import { createSettingsDialog } from './settingsDialog';
const initDialog = ( boundActions, keyValues ) => {
const dialog = createSettingsDialog( keyValues );
// Setup event bindings
dialog.querySelector( '.save' ).addEventListener( 'click', () => {
boundActions.saveSettings(
Array.from( dialog.querySelectorAll( 'input' ) ).reduce(
( enabled, el ) => {
enabled[ el.value ] = el.matches( ':checked' );
return enabled;
},
{}
)
);
} );
dialog.querySelector( '.okay' ).addEventListener( 'click', boundActions.hideSettings );
dialog.querySelector( '.close' ).addEventListener( 'click', boundActions.hideSettings );
return dialog;
};
/**
* Creates a render function that will create the settings dialog and return
* a set of methods to operate on it
*
* @param {boolean} referencePreviewsAvaliable
* @return {Function} render function
*/
export default function createSettingsDialogRenderer() {
export default function createSettingsDialogRenderer( referencePreviewsAvaliable ) {
/**
* Cached settings dialog
*
@ -49,32 +29,28 @@ export default function createSettingsDialogRenderer() {
* Renders the relevant form and labels in the settings dialog
*
* @param {Object} boundActions
* @param {Object} keyValues
* @return {Object} object with methods to affect the rendered UI
*/
return ( boundActions, keyValues ) => {
return ( boundActions ) => {
if ( !dialog ) {
dialog = createSettingsDialog( referencePreviewsAvaliable );
overlay = document.createElement( 'div' );
overlay.classList.add( 'mwe-popups-overlay' );
dialog = initDialog( boundActions, keyValues );
// Setup event bindings
dialog.querySelector( '.save' ).addEventListener( 'click', () => {
const enabled = {};
Array.prototype.forEach.call( dialog.querySelectorAll( 'input' ), ( el ) => {
enabled[ el.value ] = el.matches( ':checked' );
} );
boundActions.saveSettings( enabled );
} );
dialog.querySelector( '.close' ).addEventListener( 'click', boundActions.hideSettings );
dialog.querySelector( '.okay' ).addEventListener( 'click', boundActions.hideSettings );
}
return {
/**
* Re-initialize the dialog when the available settings have changed.
*
* @param {Object} keyValuesNew updated key value pairs
*/
refresh( keyValuesNew ) {
if ( dialog ) {
const parent = dialog.parentNode;
dialog.remove();
dialog = initDialog( boundActions, keyValuesNew );
if ( parent ) {
dialog.appendTo( parent );
}
}
},
/**
* Append the dialog and overlay to a DOM element
*

View file

@ -60,22 +60,6 @@ QUnit.test( '#boot', ( assert ) => {
);
} );
QUnit.test( '#registerSetting', ( assert ) => {
const action = actions.registerSetting(
'foo',
false
);
assert.deepEqual(
action,
{
type: actionTypes.REGISTER_SETTING,
name: 'foo',
enabled: false
},
'Setting action'
);
} );
/**
* Stubs `wait.js` and adds the deferred and its promise as properties
* of the module.

View file

@ -10,24 +10,21 @@ QUnit.module( 'ext.popups/changeListeners/settings', {
toggleHelp: this.sandbox.spy(),
setEnabled: this.sandbox.spy()
};
const keyValues = {};
this.render.withArgs( 'actions' ).returns( this.rendered );
this.defaultState = { settings: { shouldShow: false, keyValues } };
this.defaultState = { settings: { shouldShow: false } };
this.showState = {
settings: { shouldShow: true, keyValues },
settings: { shouldShow: true },
preview: { enabled: { page: true, reference: true } }
};
this.showHelpState = {
settings: {
keyValues,
shouldShow: true,
showHelp: true
}
};
this.hideHelpState = {
settings: {
keyValues,
shouldShow: true,
showHelp: false
}

View file

@ -0,0 +1,72 @@
import syncUserSettings from '../../../src/changeListeners/syncUserSettings';
QUnit.module( 'ext.popups/changeListeners/syncUserSettings', {
beforeEach() {
this.userSettings = {
storePagePreviewsEnabled: this.sandbox.spy(),
storeReferencePreviewsEnabled: this.sandbox.spy()
};
this.changeListener = syncUserSettings( this.userSettings );
}
} );
QUnit.test(
'it shouldn\'t update the storage if the enabled state hasn\'t changed',
function ( assert ) {
const oldState = { preview: { enabled: { page: true } } },
newState = { preview: { enabled: { page: true } } };
this.changeListener( undefined, newState );
this.changeListener( oldState, newState );
assert.false(
this.userSettings.storePagePreviewsEnabled.called,
'The user setting is unchanged.'
);
}
);
QUnit.test( 'it should update the storage if the enabled flag has changed', function ( assert ) {
const oldState = { preview: { enabled: { page: true } } },
newState = { preview: { enabled: { page: false } } };
this.changeListener( oldState, newState );
assert.true(
this.userSettings.storePagePreviewsEnabled.calledWith( false ),
'The user setting is disabled.'
);
} );
QUnit.test(
'it shouldn\'t update the storage if the reference preview state hasn\'t changed',
function ( assert ) {
const oldState = { preview: { enabled: { reference: true } } },
newState = { preview: { enabled: { reference: true } } };
this.changeListener( undefined, newState );
this.changeListener( oldState, newState );
assert.false(
this.userSettings.storeReferencePreviewsEnabled.called,
'Reference previews are unchanged.'
);
}
);
QUnit.test( 'it should update the storage if the reference preview state has changed', function ( assert ) {
const oldState = { preview: { enabled: { reference: true } } },
newState = { preview: { enabled: { reference: false } } };
this.changeListener( oldState, newState );
assert.false(
this.userSettings.storePagePreviewsEnabled.called,
'Page previews are unchanged.'
);
assert.true(
this.userSettings.storeReferencePreviewsEnabled.calledWith( false ),
'Reference previews opt-out is stored.'
);
} );

View file

@ -10,7 +10,6 @@ QUnit.test( '@@INIT', ( assert ) => {
state,
{
shouldShow: false,
keyValues: {},
showHelp: false,
shouldShowFooterLink: false
},
@ -25,15 +24,15 @@ QUnit.test( 'BOOT with a single disabled popup type', ( assert ) => {
user: { isAnon: true }
};
assert.deepEqual(
settings( {}, action ).shouldShowFooterLink,
true,
settings( {}, action ),
{ shouldShowFooterLink: true },
'The boot state shows a footer link.'
);
action.user.isAnon = false;
assert.deepEqual(
settings( {}, action ).shouldShowFooterLink,
false,
settings( {}, action ),
{ shouldShowFooterLink: false },
'If the user is logged in, then it doesn\'t signal that the footer link should be shown.'
);
} );
@ -45,62 +44,26 @@ QUnit.test( 'BOOT with multiple popup types', ( assert ) => {
user: { isAnon: true }
};
assert.deepEqual(
settings( {}, action ).shouldShowFooterLink,
false,
settings( {}, action ),
{ shouldShowFooterLink: false },
'Footer link ignores unavailable popup types.'
);
action.initiallyEnabled.reference = true;
assert.deepEqual(
settings( {}, action ).shouldShowFooterLink,
false,
settings( {}, action ),
{ shouldShowFooterLink: false },
'Footer link is pointless when there is nothing to enable.'
);
action.initiallyEnabled.reference = false;
assert.deepEqual(
settings( {}, action ).shouldShowFooterLink,
true,
settings( {}, action ),
{ shouldShowFooterLink: true },
'Footer link appears when at least one popup type is disabled.'
);
} );
QUnit.test( 'REGISTER_SETTING that is disabled by default reveals footer link', ( assert ) => {
const REGISTER_SETTING_FOO_DISABLED = {
type: actionTypes.REGISTER_SETTING,
name: 'foo',
enabled: false
};
const newState = settings( {
keyValues: {},
shouldShowFooterLink: false
}, REGISTER_SETTING_FOO_DISABLED );
assert.deepEqual(
newState.shouldShowFooterLink,
true,
'if one setting is registered as disabled, then the footer link is revealed.'
);
} );
QUnit.test( 'REGISTER_SETTING that is enabled by default should not show footer link', ( assert ) => {
const REGISTER_SETTING_FOO_ENABLED = {
type: actionTypes.REGISTER_SETTING,
name: 'foo',
enabled: true
};
const newState = settings( {
keyValues: {},
shouldShowFooterLink: false
}, REGISTER_SETTING_FOO_ENABLED );
assert.deepEqual(
newState.keyValues.foo,
true,
'keyValues is updated'
);
} );
QUnit.test( 'SETTINGS_SHOW', ( assert ) => {
assert.deepEqual(
settings( {}, { type: actionTypes.SETTINGS_SHOW } ),

View file

@ -43,14 +43,13 @@ QUnit.test( '#render', ( assert ) => {
hideSettings() {}
},
expected = {
refresh() {},
appendTo() {},
show() {},
hide() {},
toggleHelp() {},
setEnabled() {}
},
result = createSettingsDialogRenderer( mw.config )( boundActions, {} );
result = createSettingsDialogRenderer( mw.config )( boundActions );
// Specifically NOT a deep equal. Only structure.
assert.propEqual(

View file

@ -118,8 +118,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: 47.1 * 1024,
maxEntrypointSize: 47.1 * 1024,
maxAssetSize: 46.9 * 1024,
maxEntrypointSize: 46.9 * 1024,
// The default filter excludes map files but we rename ours.
assetFilter: ( filename ) => !filename.endsWith( srcMapExt )