jenkins-bot has submitted this change and it was merged. Change subject: Merge mw.popups.experiment into mw.popups.core ......................................................................
Merge mw.popups.experiment into mw.popups.core ext.popups.experiment depends on .core as it initialized the mw.popups namespace and .core depends on .experiments for mw.popups#getEnabledState. By merging the experiment module into core, we can eliminate any circular dependencies. Changes: * Move ext.popups.experiment.js code into ext.popups.core.js * Remove mw.popups.experiment module and any references to it Note: ext.popups.experiment.test.js was left in its own file for cleaner QUnit module setups and easier removal later. I'm not entirely happy with doing it this way, but I'm not sure changing the mw.config within the mw.popups.core QUnit module is worth merging the files. Bug: T146035 Change-Id: I1f024567010acaa61c1d613c6e59c998198a5976 --- M Popups.hooks.php M extension.json M resources/ext.popups.core.js D resources/ext.popups.experiment.js M tests/qunit/ext.popups.core.test.js M tests/qunit/ext.popups.experiment.test.js 6 files changed, 96 insertions(+), 118 deletions(-) Approvals: Jdlrobson: Looks good to me, approved Bmansurov: Looks good to me, but someone else must approve jenkins-bot: Verified diff --git a/Popups.hooks.php b/Popups.hooks.php index 196c742..a0466f0 100644 --- a/Popups.hooks.php +++ b/Popups.hooks.php @@ -238,7 +238,6 @@ ), 'dependencies' => array( 'ext.popups.desktop', - 'ext.popups.experiment', 'ext.popups.schemaPopups.utils' ), 'localBasePath' => __DIR__, diff --git a/extension.json b/extension.json index ca47486..f355eb6 100644 --- a/extension.json +++ b/extension.json @@ -68,7 +68,9 @@ "mediawiki.Title", "mediawiki.Uri", "mediawiki.RegExp", - "mediawiki.storage" + "mediawiki.storage", + "mediawiki.user", + "mediawiki.experiments" ], "targets": [ "desktop", @@ -86,8 +88,7 @@ "mediawiki.storage", "jquery.client", "ext.popups.core", - "ext.popups.renderer.desktopRenderer", - "ext.popups.experiment" + "ext.popups.renderer.desktopRenderer" ], "targets": [ "desktop" ] }, @@ -105,20 +106,6 @@ ], "dependencies": [ "ext.popups.core" - ] - }, - "ext.popups.experiment": { - "scripts": [ - "resources/ext.popups.experiment.js" - ], - "dependencies": [ - "ext.popups.core", - "mediawiki.user", - "mediawiki.storage", - "mediawiki.experiments" - ], - "targets": [ - "desktop" ] }, "ext.popups.schemaPopups.utils": { diff --git a/resources/ext.popups.core.js b/resources/ext.popups.core.js index 28a941b..20a2054 100644 --- a/resources/ext.popups.core.js +++ b/resources/ext.popups.core.js @@ -237,7 +237,90 @@ if ( !mw.config.get( 'wgPopupsExperiment', false ) ) { return mw.storage.get( popupsEnabledStorageKey ) !== '0'; } else { - return mw.popups.experiment.isUserInCondition(); + return this.isUserInCondition(); + } + }; + + /** + * @ignore + */ + function getToken() { + var key = 'PopupsExperimentID', + id = mw.storage.get( key ); + + if ( !id ) { + id = mw.user.generateRandomSessionId(); + + mw.storage.set( key, id ); + } + + return id; + } + + /** + * Has the user previously enabled Popups by clicking "Enable previews" in the + * footer menu? + * + * @return {boolean} + * @ignore + */ + function hasUserEnabledFeature() { + var value = mw.storage.get( 'mwe-popups-enabled' ); + + return Boolean( value ) && value !== '0'; + } + + /** + * Has the user previously disabled Popups by clicking "Disable previews" in the settings + * overlay? + * + * @return {boolean} + * @ignore + */ + function hasUserDisabledFeature() { + return mw.storage.get( 'mwe-popups-enabled' ) === '0'; + } + + /** + * Gets whether or not the user has Popups enabled, i.e. whether they are in the experiment + * condition. + * + * The user is in the experiment condition if: + * * they've enabled Popups by click "Enable previews" in the footer menu, or + * * they've enabled Popups as a beta feature, or + * * they aren't in the control bucket of the experiment + * + * N.B. that the user isn't entered into the experiment, i.e. they aren't assigned or a bucket, + * if the experiment isn't configured. + * + * @return {boolean} + * @ignore + */ + mw.popups.isUserInCondition = function isUserInCondition() { + var config = mw.config.get( 'wgPopupsExperimentConfig' ); + + // The first two tests deal with whether the user has /explicitly/ enable or disabled via its + // settings. + if ( hasUserEnabledFeature() ) { + return true; + } + + if ( hasUserDisabledFeature() ) { + return false; + } + + if ( mw.user.isAnon() ) { + if ( !config ) { + return false; + } + + // FIXME: mw.experiments should expose the CONTROL_BUCKET constant, e.g. + // `mw.experiments.CONTROL_BUCKET`. + return mw.experiments.getBucket( config, getToken() ) !== 'control'; + } else { + // Logged in users are in condition depending on the beta feature flag + // instead of bucketing + return mw.config.get( 'wgPopupsExperimentIsBetaFeatureEnabled', false ); } }; diff --git a/resources/ext.popups.experiment.js b/resources/ext.popups.experiment.js deleted file mode 100644 index 990fa17..0000000 --- a/resources/ext.popups.experiment.js +++ /dev/null @@ -1,91 +0,0 @@ -( function ( mw ) { - - /** - * @ignore - */ - function getToken() { - var key = 'PopupsExperimentID', - id = mw.storage.get( key ); - - if ( !id ) { - id = mw.user.generateRandomSessionId(); - - mw.storage.set( key, id ); - } - - return id; - } - - /** - * Has the user previously enabled Popups by clicking "Enable previews" in the - * footer menu? - * - * @return {boolean} - * @ignore - */ - function hasUserEnabledFeature() { - var value = mw.storage.get( 'mwe-popups-enabled' ); - - return Boolean( value ) && value !== '0'; - } - - /** - * Has the user previously disabled Popups by clicking "Disable previews" in the settings - * overlay? - * - * @return {boolean} - * @ignore - */ - function hasUserDisabledFeature() { - return mw.storage.get( 'mwe-popups-enabled' ) === '0'; - } - - /** - * @class mw.popups.experiment - * @singleton - */ - mw.popups.experiment = {}; - - /** - * Gets whether or not the user has Popups enabled, i.e. whether they are in the experiment - * condition. - * - * The user is in the experiment condition if: - * * they've enabled Popups by click "Enable previews" in the footer menu, or - * * they've enabled Popups as a beta feature, or - * * they aren't in the control bucket of the experiment - * - * N.B. that the user isn't entered into the experiment, i.e. they aren't assigned or a bucket, - * if the experiment isn't configured. - * - * @return {boolean} - */ - mw.popups.experiment.isUserInCondition = function isUserInCondition() { - var config = mw.config.get( 'wgPopupsExperimentConfig' ); - - // The first two tests deal with whether the user has /explicitly/ enable or disabled via its - // settings. - if ( hasUserEnabledFeature() ) { - return true; - } - - if ( hasUserDisabledFeature() ) { - return false; - } - - if ( mw.user.isAnon() ) { - if ( !config ) { - return false; - } - - // FIXME: mw.experiments should expose the CONTROL_BUCKET constant, e.g. - // `mw.experiments.CONTROL_BUCKET`. - return mw.experiments.getBucket( config, getToken() ) !== 'control'; - } else { - // Logged in users are in condition depending on the beta feature flag - // instead of bucketing - return mw.config.get( 'wgPopupsExperimentIsBetaFeatureEnabled', false ); - } - }; - -}( mediaWiki ) ); diff --git a/tests/qunit/ext.popups.core.test.js b/tests/qunit/ext.popups.core.test.js index 159b74c..360c2ee 100644 --- a/tests/qunit/ext.popups.core.test.js +++ b/tests/qunit/ext.popups.core.test.js @@ -312,7 +312,7 @@ .withArgs( 'wgPopupsExperiment' ), deviceStorageStub = this.sandbox.stub( mw.storage, 'get' ) .withArgs( storageKey ), - experimentStub = this.sandbox.stub( mw.popups.experiment, + experimentStub = this.sandbox.stub( mw.popups, 'isUserInCondition' ); QUnit.expect( 7 ); diff --git a/tests/qunit/ext.popups.experiment.test.js b/tests/qunit/ext.popups.experiment.test.js index 2e52b81..c298b9b 100644 --- a/tests/qunit/ext.popups.experiment.test.js +++ b/tests/qunit/ext.popups.experiment.test.js @@ -1,6 +1,6 @@ ( function ( mw ) { - QUnit.module( 'ext.popups.experiment', QUnit.newMwEnvironment( { + QUnit.module( 'ext.popups.core:experiment', QUnit.newMwEnvironment( { config: { wgPopupsExperimentConfig: { name: 'Popups A/B Test - May, 2016', @@ -31,7 +31,7 @@ mw.config.set( 'wgPopupsExperimentIsBetaFeatureEnabled', true ); assert.strictEqual( - mw.popups.experiment.isUserInCondition(), + mw.popups.isUserInCondition(), true, 'If the user has the beta feature enabled, then they aren\'t in the condition.' ); @@ -43,7 +43,7 @@ result, firstCallArgs; - result = mw.popups.experiment.isUserInCondition(); + result = mw.popups.isUserInCondition(); firstCallArgs = getBucketSpy.firstCall.args; @@ -66,7 +66,7 @@ this.sandbox.stub( mw.user, 'generateRandomSessionId' ).returns( token ); - mw.popups.experiment.isUserInCondition(); + mw.popups.isUserInCondition(); assert.deepEqual( setSpy.firstCall.args[ 1 ], @@ -79,7 +79,7 @@ mw.config.set( 'wgPopupsExperimentConfig', null ); assert.strictEqual( - mw.popups.experiment.isUserInCondition(), + mw.popups.isUserInCondition(), false, 'If the experiment isn\'t configured, then the user isn\'t in the condition.' ); @@ -89,7 +89,7 @@ mw.storage.set( 'mwe-popups-enabled', '1' ); assert.strictEqual( - mw.popups.experiment.isUserInCondition(), + mw.popups.isUserInCondition(), true, 'If the experiment has enabled the feature, then the user is in the condition.' ); @@ -102,7 +102,7 @@ mw.storage.set( 'mwe-popups-enabled', '0' ); assert.strictEqual( - mw.popups.experiment.isUserInCondition(), + mw.popups.isUserInCondition(), false, 'If the experiment has enabled the feature, then the user is in the condition.' ); -- To view, visit https://gerrit.wikimedia.org/r/311977 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1f024567010acaa61c1d613c6e59c998198a5976 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/Popups Gerrit-Branch: master Gerrit-Owner: Phuedx <g...@samsmith.io> Gerrit-Reviewer: Bmansurov <bmansu...@wikimedia.org> Gerrit-Reviewer: Jdlrobson <jrob...@wikimedia.org> Gerrit-Reviewer: Jhobs <jhob...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits