Generalize settings code

A new REGISTER_SETTING action replaces the BOOT action for
registering settings. This allows custom preview types to be
associated with a setting. They do this by adding the enabled
property to the module they provide to mw.popups.register

Every time the new action is called, we refresh the settings
dialog UI with the new settings.

Previously the settings dialog was hardcoded, but now it is generated
from the registered preview types by deriving associated messages
and checking they exist, so by default custom types will not show
up in the settings.

Benefits:
* This change empowers us to add a setting for Math previews to allow
them to be enabled or disabled.
* Allows us to separate references as its own module

Additional notes:
* The syncUserSettings.js changeListener is no longer needed as the logic
for this is handled inside the "userSettings" change listener in response to
the "settings" reducer which is responding to
SETTINGS_CHANGE and REGISTER_SETTING actions.

Upon merging:
* https://www.mediawiki.org/wiki/Extension:Popups#Extensibility will be
updated to detail how a setting can be registered.

Bug: T334261
Bug: T326692
Change-Id: Ie11057052fb9035944f2b79a17fb486f97102994
This commit is contained in:
Jon Robson 2023-09-27 08:46:39 -07:00 committed by Jdlrobson
parent 9cfd3358aa
commit 6924a89b07
20 changed files with 195 additions and 210 deletions

View file

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

Binary file not shown.

Binary file not shown.

View file

@ -5,6 +5,7 @@
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,6 +72,22 @@ 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.
*
@ -213,7 +229,6 @@ 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,7 +4,6 @@ import pageviews from './pageviews';
import render from './render';
import settings from './settings';
import statsv from './statsv';
import syncUserSettings from './syncUserSettings';
export default {
footerLink,
@ -12,6 +11,5 @@ export default {
pageviews,
render,
settings,
statsv,
syncUserSettings
statsv
};

View file

@ -13,12 +13,19 @@ 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 );
settingsObj = render( boundActions, newState.settings.keyValues );
settingsObj.appendTo( document.body );
}

View file

@ -1,67 +0,0 @@
/**
* @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,8 +118,6 @@ 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,
@ -192,15 +190,11 @@ function handleDOMEventIfEligible( handler ) {
referenceGateway = createReferenceGateway(),
userSettings = createUserSettings( mw.storage ),
referencePreviewsState = isReferencePreviewsEnabled( mw.user, userSettings, mw.config ),
settingsDialog = createSettingsDialogRenderer( referencePreviewsState !== null ),
settingsDialog = createSettingsDialogRenderer(),
experiments = createExperiments( mw.experiments ),
statsvTracker = getStatsvTracker( mw.user, mw.config, experiments ),
pageviewTracker = getPageviewTracker( mw.config ),
initiallyEnabled = {
[ previewTypes.TYPE_PAGE ]:
createIsPagePreviewsEnabled( mw.user, userSettings, mw.config ),
[ previewTypes.TYPE_REFERENCE ]: referencePreviewsState
};
pagePreviewState = createIsPagePreviewsEnabled( mw.user, userSettings, mw.config );
// If debug mode is enabled, then enable Redux DevTools.
if ( mw.config.get( 'debug' ) ||
@ -225,7 +219,7 @@ function handleDOMEventIfEligible( handler ) {
);
boundActions.boot(
initiallyEnabled,
{},
mw.user,
userSettings,
mw.config,
@ -237,13 +231,15 @@ function handleDOMEventIfEligible( handler ) {
* extensions can query it (T171287)
*/
mw.popups = createMediaWikiPopupsObject(
store, registerModel, registerPreviewUI, registerGatewayForPreviewType
store, registerModel, registerPreviewUI, registerGatewayForPreviewType,
boundActions.registerSetting
);
if ( initiallyEnabled[ previewTypes.TYPE_PAGE ] !== null ) {
if ( pagePreviewState !== 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,
@ -257,9 +253,10 @@ function handleDOMEventIfEligible( handler ) {
]
} );
}
if ( initiallyEnabled[ previewTypes.TYPE_REFERENCE ] !== null ) {
if ( referencePreviewsState !== 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,9 +12,12 @@ 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 ) {
export default function createMwPopups( store, registerModel, registerPreviewUI,
registerGatewayForPreviewType, registerSetting
) {
return {
/**
* @return {boolean} If Page Previews are currently active
@ -59,7 +62,7 @@ export default function createMwPopups( store, registerModel, registerPreviewUI,
* @param {PopupModule} module
*/
register: function ( module ) {
const { type, selector, gateway, renderFn, subTypes, delay, init } = module;
const { type, selector, gateway, renderFn, subTypes, delay, init, enabled } = 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.`
@ -68,6 +71,16 @@ 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,7 +28,12 @@ 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,6 +12,7 @@ export default function settings( state, action ) {
if ( state === undefined ) {
state = {
shouldShow: false,
keyValues: {},
showHelp: false,
shouldShowFooterLink: false
};
@ -57,12 +58,23 @@ 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,33 +3,28 @@
*/
import { renderSettingsDialog } from './templates/settingsDialog/settingsDialog';
import { previewTypes } from '../preview/model';
/**
* Create the settings dialog shown to anonymous users.
*
* @param {boolean} referencePreviewsAvaliable
* @param {Object} keyValues
* @return {HTMLElement} settings dialog
*/
export function createSettingsDialog( referencePreviewsAvaliable ) {
const choices = [
export function createSettingsDialog( keyValues ) {
const choices = Object.keys( keyValues ).map( ( 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' )
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 ]
}
];
// 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,14 +4,34 @@
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( referencePreviewsAvaliable ) {
export default function createSettingsDialogRenderer() {
/**
* Cached settings dialog
*
@ -29,28 +49,32 @@ export default function createSettingsDialogRenderer( referencePreviewsAvaliable
* 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 ) => {
return ( boundActions, keyValues ) => {
if ( !dialog ) {
dialog = createSettingsDialog( referencePreviewsAvaliable );
overlay = document.createElement( 'div' );
overlay.classList.add( 'mwe-popups-overlay' );
// 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 );
dialog = initDialog( boundActions, keyValues );
}
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,6 +60,22 @@ 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,21 +10,24 @@ 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 } };
this.defaultState = { settings: { shouldShow: false, keyValues } };
this.showState = {
settings: { shouldShow: true },
settings: { shouldShow: true, keyValues },
preview: { enabled: { page: true, reference: true } }
};
this.showHelpState = {
settings: {
keyValues,
shouldShow: true,
showHelp: true
}
};
this.hideHelpState = {
settings: {
keyValues,
shouldShow: true,
showHelp: false
}

View file

@ -1,72 +0,0 @@
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,6 +10,7 @@ QUnit.test( '@@INIT', ( assert ) => {
state,
{
shouldShow: false,
keyValues: {},
showHelp: false,
shouldShowFooterLink: false
},
@ -24,15 +25,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.'
);
} );
@ -44,26 +45,62 @@ 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,13 +43,14 @@ 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: 46.9 * 1024,
maxEntrypointSize: 46.9 * 1024,
maxAssetSize: 47.1 * 1024,
maxEntrypointSize: 47.1 * 1024,
// The default filter excludes map files but we rename ours.
assetFilter: ( filename ) => !filename.endsWith( srcMapExt )