AndyRussG has uploaded a new change for review. https://gerrit.wikimedia.org/r/320317
Change subject: Handle banner loader errors on client ...................................................................... Handle banner loader errors on client Bug: T149107 Change-Id: Iab80d0462becf1e7a7a2d9f2df662c7a68aed8bf --- M resources/subscribing/ext.centralNotice.display.js M resources/subscribing/ext.centralNotice.display.state.js M special/SpecialBannerLoader.php M tests/qunit/subscribing/ext.centralNotice.display.tests.js 4 files changed, 59 insertions(+), 11 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice refs/changes/17/320317/1 diff --git a/resources/subscribing/ext.centralNotice.display.js b/resources/subscribing/ext.centralNotice.display.js index 5536e05..ba4777d 100644 --- a/resources/subscribing/ext.centralNotice.display.js +++ b/resources/subscribing/ext.centralNotice.display.js @@ -153,10 +153,14 @@ ); // The returned javascript will call mw.centralNotice.insertBanner() + // or mw.centralNotice.handleBannerLoaderError() (if an error was + // handled on the server). $.ajax( { url: url.toString(), dataType: 'script', cache: true + } ).fail( function ( jqXHR, status, error ) { + cn.handleBannerLoaderError( status + ': ' + error ); } ); } @@ -309,6 +313,20 @@ } /** + * Stuff we have to do following the call to fetch a banner (successful + * or not) + */ + function processAfterBannerFetch() { + + // If we're testing a banner, don't call Special:RecordImpression or + // run mixin hooks. + if ( !cn.internal.state.getData().testingBanner ) { + runPostBannerMixinHooks(); + recordImpression(); + } + } + + /** * CentralNotice base public object, exposed as mw.centralNotice. Note: * other CN modules may add properties to this object, and we add some * dynamically. These additional properties are: @@ -402,12 +420,7 @@ state.setBannerShown(); } - // If we're testing a banner, don't call Special:RecordImpression or - // run mixin hooks. - if ( !state.getData().testingBanner ) { - runPostBannerMixinHooks(); - recordImpression(); - } + processAfterBannerFetch(); }, /** @@ -531,6 +544,16 @@ } ); }, + /** + * Handle a banner loader error, with an optional message + * @param {string} [msg] + */ + handleBannerLoaderError: function ( msg ) { + cn.internal.state.setBannerLoaderError( msg ); + bannerLoadedDeferredObj.reject( cn.internal.state.getData() ); + processAfterBannerFetch(); + }, + hideBannerWithCloseButton: function () { // Hide the banner element $( '#centralNotice' ).hide(); diff --git a/resources/subscribing/ext.centralNotice.display.state.js b/resources/subscribing/ext.centralNotice.display.state.js index a2b11fb..2d18e1f 100644 --- a/resources/subscribing/ext.centralNotice.display.state.js +++ b/resources/subscribing/ext.centralNotice.display.state.js @@ -31,7 +31,8 @@ NO_BANNER_AVAILABLE: new Status( 'no_banner_available', 3 ), BANNER_CHOSEN: new Status( 'banner_chosen', 4 ), BANNER_LOADED_BUT_HIDDEN: new Status( 'banner_loaded_but_hidden', 5 ), - BANNER_SHOWN: new Status( 'banner_shown', 6 ) + BANNER_SHOWN: new Status( 'banner_shown', 6 ), + BANNER_LOADER_ERROR: new Status( 'banner_loader_error', 7 ) }, // Until T114078 is closed, we minify banner history logs. This lookup @@ -363,6 +364,17 @@ }, /** + * Set a banner loader error, with an optional message + * @param {string} [msg] + */ + setBannerLoaderError: function ( msg ) { + if ( msg ) { + state.data.errorMsg = msg; + } + setStatus( STATUSES.BANNER_LOADER_ERROR ); + }, + + /** * Register that the current page view is included in a test. * * @param {string} identifier A string to identify the test. Should not contain diff --git a/special/SpecialBannerLoader.php b/special/SpecialBannerLoader.php index 3c4dc39..dfaeb61 100644 --- a/special/SpecialBannerLoader.php +++ b/special/SpecialBannerLoader.php @@ -29,13 +29,20 @@ try { $this->getParams(); - echo $this->getJsNotice( $this->bannerName ); + $out = $this->getJsNotice( $this->bannerName ); + } catch ( EmptyBannerException $e ) { - echo "mw.centralNotice.insertBanner( false );"; + $out = "mw.centralNotice.handleBannerLoaderError( 'Empty banner' );"; + } catch ( Exception $e ) { - wfDebugLog( 'CentralNotice', $e->getMessage() ); - echo "mw.centralNotice.insertBanner( false /* due to internal exception */ );"; + $msg = $e->getMessage(); + $msgParamStr = $msg ? " '{$msg}' " : ''; + $out = "mw.centralNotice.handleBannerLoaderError({$msgParamStr});"; + + wfDebugLog( 'CentralNotice', $msg ); } + + echo $out; } function getParams() { diff --git a/tests/qunit/subscribing/ext.centralNotice.display.tests.js b/tests/qunit/subscribing/ext.centralNotice.display.tests.js index 9bfa2c3..08ba6b7 100644 --- a/tests/qunit/subscribing/ext.centralNotice.display.tests.js +++ b/tests/qunit/subscribing/ext.centralNotice.display.tests.js @@ -181,6 +181,7 @@ mw.centralNotice.internal.state.urlParams.banner = 'test_banner'; $.ajax = function ( params ) { assert.ok( params.url.match( /Special(?:[:]|%3A)BannerLoader.*[?&]banner=test_banner/ ) ); + return $.Deferred(); }; mw.centralNotice.displayTestingBanner(); @@ -196,6 +197,7 @@ $.ajax = function ( params ) { assert.ok( params.url.match( /Special(?:[:]|%3A)BannerLoader.*[?&]banner=banner1/ ) ); + return $.Deferred(); }; mw.centralNotice.chooseAndMaybeDisplay(); @@ -204,6 +206,7 @@ $.ajax = function ( params ) { assert.ok( params.url.match( /Special(?:[:]|%3A)BannerLoader.*[?&]banner=banner2/ ) ); + return $.Deferred(); }; mw.centralNotice.chooseAndMaybeDisplay(); } ); @@ -216,6 +219,7 @@ $.ajax = function ( params ) { assert.ok( params.url.match( /Special(?:[:]|%3A)BannerLoader.*[?&]banner=banner1/ ) ); + return $.Deferred(); }; mw.centralNotice.chooseAndMaybeDisplay(); @@ -224,6 +228,7 @@ $.ajax = function ( params ) { assert.ok( params.url.match( /Special(?:[:]|%3A)BannerLoader.*[?&]banner=banner2/ ) ); + return $.Deferred(); }; mw.centralNotice.chooseAndMaybeDisplay(); } ); @@ -232,6 +237,7 @@ var mixin = new mw.centralNotice.Mixin( 'testMixin' ); $.ajax = function () { mw.centralNotice.reallyInsertBanner( bannerData ); + return $.Deferred(); }; mixin.setPreBannerHandler( -- To view, visit https://gerrit.wikimedia.org/r/320317 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iab80d0462becf1e7a7a2d9f2df662c7a68aed8bf Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/CentralNotice Gerrit-Branch: wmf_deploy Gerrit-Owner: AndyRussG <andrew.green...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits