Reedy has uploaded a new change for review. https://gerrit.wikimedia.org/r/175768
Change subject: Filter logged-in status on client ...................................................................... Filter logged-in status on client Filter logged-in status on the client when choosing a banner to display. This is necessary since on production, RL modules don't vary for logged-in vs. anonymous users. Bug: T75812 Change-Id: Ib4d23f2a588f58ef3abcbd8b0b500ad8534723cd --- M api/ApiCentralNoticeBannerChoiceData.php M i18n/en.json M i18n/qqq.json M includes/BannerAllocationCalculator.php M includes/BannerChoiceDataProvider.php M includes/CNBannerChoiceDataResourceLoaderModule.php M modules/ext.centralNotice.bannerController/bannerController.js M modules/ext.centralNotice.bannerController/bannerController.lib.js M special/SpecialBannerAllocation.php M tests/ApiCentralNoticeBannerChoiceDataTest.php M tests/BannerAllocationCalculatorTest.php M tests/BannerChoiceDataProviderTest.php M tests/data/AllocationsFixtures.json M tests/qunit/ext.centralNotice.bannerController.lib/bannerController.lib.tests.js 14 files changed, 130 insertions(+), 119 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice refs/changes/68/175768/1 diff --git a/api/ApiCentralNoticeBannerChoiceData.php b/api/ApiCentralNoticeBannerChoiceData.php index d9402d7..b73d1c5 100644 --- a/api/ApiCentralNoticeBannerChoiceData.php +++ b/api/ApiCentralNoticeBannerChoiceData.php @@ -29,23 +29,8 @@ parent::LANG_FILTER ); - $status = parent::sanitizeText( - $params['status'], - self::STATUS_FILTER - ); - - if ( $status === 'loggedin' ) { - $status = BannerChoiceDataProvider::LOGGED_IN; - } else if ( $status === 'anonymous' ) { - $status = BannerChoiceDataProvider::ANONYMOUS; - } else { - $this->dieUsage( - 'Invalid status value: must be "loggedin" or "anonymous".', - 'invalid-status'); - } - $choicesProvider = new BannerChoiceDataProvider( - $project, $lang, $status, BannerChoiceDataProvider::USE_DEFAULT_DB ); + $project, $lang, BannerChoiceDataProvider::USE_DEFAULT_DB ); $choices = $choicesProvider->getChoices(); @@ -68,17 +53,13 @@ 'language' => array( ApiBase::PARAM_TYPE => 'string', ApiBase::PARAM_REQUIRED => true - ), - 'status' => array( - ApiBase::PARAM_TYPE => 'string', - ApiBase::PARAM_REQUIRED => true ) ); } protected function getExamplesMessages() { return array( - 'action=centralnoticebannerchoicedata&project=wikpedia&language=en&status=anonymous' + 'action=centralnoticebannerchoicedata&project=wikpedia&language=en' => 'apihelp-centralnoticebannerchoicedata-example-1' ); } diff --git a/i18n/en.json b/i18n/en.json index a2ffcc9..7fe9feb 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -246,8 +246,7 @@ "apihelp-centralnoticebannerchoicedata-description": "Get data needed to choose a banner for a given project and language", "apihelp-centralnoticebannerchoicedata-param-project": "The project to get banner choice data for.", "apihelp-centralnoticebannerchoicedata-param-language": "The language to get banner choice data for.", - "apihelp-centralnoticebannerchoicedata-param-status": "The logged-in status to get banner choice data for (loggedin|anonymous).", - "apihelp-centralnoticebannerchoicedata-example-1": "Get the data for choosing a banner for English, Wikipedia, and anonymous users.", + "apihelp-centralnoticebannerchoicedata-example-1": "Get the data for choosing a banner for English Wikipedia users.", "apihelp-query+centralnoticelogs-description": "Get a log of campaign configuration changes.", "apihelp-query+centralnoticelogs-param-campaign": "Campaign name (optional). Separate multiple values with a \"|\" (vertical bar).", "apihelp-query+centralnoticelogs-param-user": "Username (optional).", diff --git a/i18n/qqq.json b/i18n/qqq.json index 79ba918..75f2dba 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -269,7 +269,6 @@ "apihelp-centralnoticebannerchoicedata-description": "{{doc-apihelp-description|centralnoticebannerchoicedata}}", "apihelp-centralnoticebannerchoicedata-param-project": "{{doc-apihelp-param|centralnoticebannerchoicedata|project}}", "apihelp-centralnoticebannerchoicedata-param-language": "{{doc-apihelp-param|centralnoticebannerchoicedata|language}}", - "apihelp-centralnoticebannerchoicedata-param-status": "{{doc-apihelp-param|centralnoticebannerchoicedata|status}}", "apihelp-centralnoticebannerchoicedata-example-1": "{{doc-apihelp-example|centralnoticebannerchoicedata}}", "apihelp-query+centralnoticelogs-description": "{{doc-apihelp-description|query+centralnoticelogs}}", "apihelp-query+centralnoticelogs-param-campaign": "{{doc-apihelp-param|query+centralnoticelogs|campaign}}", diff --git a/includes/BannerAllocationCalculator.php b/includes/BannerAllocationCalculator.php index 7fa88f9..112f040 100644 --- a/includes/BannerAllocationCalculator.php +++ b/includes/BannerAllocationCalculator.php @@ -20,6 +20,10 @@ * Calculates banner allocation percentages */ class BannerAllocationCalculator { + + const LOGGED_IN = 0; + const ANONYMOUS = 1; + /** * Calculate banner allocations given a list of banners filtered to a single * device and bucket @@ -93,19 +97,46 @@ /** * Allocation helper. Maps an array of campaigns with banners to a flattened - * list of banners, omitting those not in the specified device and bucket. + * list of banners, omitting those not available for the specified logged-in + * status, device and bucket. * * @param array $campaigns campaigns with banners as returned by * @see BannerChoiceDataProvider::getChoicesForCountry + * + * @param integer $status A status constant defined by this class (i.e., + * BannerAllocationCalculator::ANONYMOUS or + * BannerAllocationCalculator::LOGGED_IN). + * * @param string $device target device code * @param integer $bucket target bucket number + * * @return array banners with properties suitable for * @see BannerAllocationCalculator::calculateAllocations */ - static function filterAndTransformBanners( $campaigns, $device, $bucket ) { + static function filterAndTransformBanners( + $campaigns, $status, $device, $bucket ) { + + // Set which property we need to check to filter logged-in status + switch ( $status ) { + case self::ANONYMOUS: + $status_prop = 'display_anon'; + break; + + case self::LOGGED_IN: + $status_prop = 'display_account'; + break; + + default: + throw new MWException( $this->status . 'is not a valid status ' + . 'for BannerAllocationsCalculator.' ); + } + $banners = array(); foreach( $campaigns as $campaign ) { foreach ( $campaign['banners'] as $banner ) { + if ( !$banner[$status_prop] ) { + continue; + } if ( !in_array( $device, $banner['devices'] ) ) { continue; } diff --git a/includes/BannerChoiceDataProvider.php b/includes/BannerChoiceDataProvider.php index eb986ae..9ec10d1 100644 --- a/includes/BannerChoiceDataProvider.php +++ b/includes/BannerChoiceDataProvider.php @@ -2,7 +2,7 @@ /*** * Provides a set of campaign and banner choices based on allocations for a - * given project, language and anonymous/logged-in status. + * given project and language combination. */ class BannerChoiceDataProvider { @@ -17,27 +17,19 @@ */ const USE_INFRASTRUCTURE_DB = 1; - const LOGGED_IN = 0; - const ANONYMOUS = 1; - protected $project; protected $language; - protected $status; protected $whichDb; /** * @param string $project The project to get choices for * @param string $language The language to get choices for - * @param int $status Anonymous/logged-in status to get choices for. Can be - * BannerChoiceDataProvider::LOGGED_IN or - * BannerChoiceDataProvider::ANONYMOUS. */ - public function __construct( $project, $language, $status, + public function __construct( $project, $language, $whichDb=self::USE_DEFAULT_DB ) { $this->project = $project; $this->language = $language; - $this->status = $status; $this->whichDb = $whichDb; } @@ -84,21 +76,6 @@ 'notice_languages.nl_language' => $this->language ); - // Set the user status condition - switch ( $this->status ) { - case self::LOGGED_IN: - $conds['templates.tmp_display_account'] = 1; - break; - - case self::ANONYMOUS: - $conds['templates.tmp_display_anon'] = 1; - break; - - default: - throw new MWException( $this->status . 'is not a valid status ' - . 'for BannerChoiceDataProvider.' ); - } - // Query campaigns and banners at once $dbRows = $dbr->select( array( @@ -121,6 +98,8 @@ 'assignments.asn_bucket', 'templates.tmp_id', 'templates.tmp_name', + 'templates.tmp_display_anon', + 'templates.tmp_display_account', 'templates.tmp_category' ), $conds, @@ -183,6 +162,8 @@ 'bucket' => intval( $bucket ), 'weight' => intval( $dbRow->tmp_weight ), 'category' => $dbRow->tmp_category, + 'display_anon' => (bool) $dbRow->tmp_display_anon, + 'display_account' => (bool) $dbRow->tmp_display_account, 'devices' => array() // To be filled by the last query ); diff --git a/includes/CNBannerChoiceDataResourceLoaderModule.php b/includes/CNBannerChoiceDataResourceLoaderModule.php index a364f37..d32906f 100644 --- a/includes/CNBannerChoiceDataResourceLoaderModule.php +++ b/includes/CNBannerChoiceDataResourceLoaderModule.php @@ -33,20 +33,17 @@ $project = $wgNoticeProject; $language = $context->getLanguage(); - // TODO Find out what's up with $context->getUser() - $status = ( $wgUser->isAnon() ) ? 'anonymous' : 'loggedin'; - // Fetch the data via the DB or the API. Decide which to use based // on whether the appropriate global variables are set. // If something's amiss, we warn and return an empty array, but don't // bring everything to a standstill. if ( $wgCentralDBname ) { - $choices = $this->getFromDb( $project, $language, $status ); + $choices = $this->getFromDb( $project, $language ); } else if ( $wgCentralNoticeApiUrl ) { - $choices = $this->getFromApi( $project, $language, $status ); + $choices = $this->getFromApi( $project, $language ); if ( !$choices ) { wfLogWarning( 'Couldn\'t fetch banner choice data via API. ' . @@ -70,16 +67,11 @@ * * @param string $project * @param string $language - * @param string $status Can be 'loggedin' or 'anonymous' */ - protected function getFromDb( $project, $language, $status ) { - - $status = ( $status === 'loggedin' ) ? - BannerChoiceDataProvider::LOGGED_IN : - BannerChoiceDataProvider::ANONYMOUS; + protected function getFromDb( $project, $language ) { $choicesProvider = new BannerChoiceDataProvider( - $project, $language, $status, + $project, $language, BannerChoiceDataProvider::USE_INFRASTRUCTURE_DB ); return $choicesProvider->getChoices(); @@ -91,11 +83,10 @@ * * @param string $project * @param string $language - * @param string $status Can be 'loggedin' or 'anonymous' * * @return array|boolean */ - protected function getFromApi( $project, $language, $status ) { + protected function getFromApi( $project, $language ) { global $wgCentralNoticeApiUrl; // Make the URl @@ -103,7 +94,6 @@ 'action' => 'centralnoticebannerchoicedata', 'project' => $project, 'language' => $language, - 'status' => $status, 'format' => 'json' ); diff --git a/modules/ext.centralNotice.bannerController/bannerController.js b/modules/ext.centralNotice.bannerController/bannerController.js index 5918cf2..1ed85af 100644 --- a/modules/ext.centralNotice.bannerController/bannerController.js +++ b/modules/ext.centralNotice.bannerController/bannerController.js @@ -175,8 +175,8 @@ // Filter choiceData on country and device. Only campaigns that // target the user's country and have at least one banner for - // the user's device pass this filter. - mw.cnBannerControllerLib.filterChoiceDataCountriesAndDevices(); + // the user's logged-in status and device pass this filter. + mw.cnBannerControllerLib.filterChoiceData(); // Do all things bucket. Retrieve or generate buckets for all // the campaigns remaining in choiceData. Then update expiry @@ -184,8 +184,9 @@ mw.cnBannerControllerLib.processBuckets(); // Create a flat list of possible banners available for the - // user's buckets and device, and calculate allocations. - mw.cnBannerControllerLib.makePossibleBannersForBucketsAndDevice(); + // user's buckets, logged-in status and device, and calculate + // allocations. + mw.cnBannerControllerLib.makePossibleBanners(); mw.cnBannerControllerLib.calculateBannerAllocations(); // Get a random seed or use the random= parameter from the URL, @@ -282,7 +283,8 @@ // === Attempt to load parameters from the query string === mw.centralNotice.loadQueryStringVariables(); - // === Initialize things that don't come from MW itself === + // === Initialize some data === + mw.centralNotice.data.anonymous = ( mw.config.get( 'wgUserName' ) === null ); mw.centralNotice.data.country = mw.centralNotice.data.getVars.country || window.Geo.country || 'XX'; mw.centralNotice.data.addressFamily = ( window.Geo.IPv6 || window.Geo.af === 'v6' ) ? 'IPv6' : 'IPv4'; mw.centralNotice.isPreviewFrame = (mw.config.get( 'wgCanonicalSpecialPageName' ) === 'BannerPreview'); @@ -359,7 +361,7 @@ uselang: mw.config.get( 'wgUserLanguage' ), project: mw.config.get( 'wgNoticeProject' ), db: mw.config.get( 'wgDBname' ), - anonymous: mw.config.get( 'wgUserName' ) === null, + anonymous: mw.centralNotice.data.anonymous, device: mw.centralNotice.data.device }; diff --git a/modules/ext.centralNotice.bannerController/bannerController.lib.js b/modules/ext.centralNotice.bannerController/bannerController.lib.js index 5af4941..8548f70 100644 --- a/modules/ext.centralNotice.bannerController/bannerController.lib.js +++ b/modules/ext.centralNotice.bannerController/bannerController.lib.js @@ -215,13 +215,14 @@ }, /** - * Filter choiceData on the user's country and device. Campaigns that - * don't target the user's country or have no banners for their - * device will be removed. We operate on this.choiceData. + * Filter choiceData on the user's country, logged-in status and device. + * Campaigns that don't target the user's country or have no banners for + * their logged-in status and device will be removed. We operate on + * this.choiceData. */ - filterChoiceDataCountriesAndDevices: function() { + filterChoiceData: function() { - var i, campaign, j, keepCampaign, + var i, campaign, j, banner, keepCampaign, filteredChoiceData = []; for ( i = 0; i < this.choiceData.length; i++ ) { @@ -237,21 +238,31 @@ continue; } - // Now filter by banner device. - // To make buckets work consistently even in for strangely + // Now filter by banner logged-in status and device. + // To make buckets work consistently even for strangely // configured campaigns, we won't chose buckets yet, so we'll // filter on them a little later. for ( j = 0; j < campaign.banners.length; j++ ) { + banner = campaign.banners[j]; + + // Logged-in status + if ( mw.centralNotice.data.anonymous && !banner.display_anon ) { + continue; + } + if ( !mw.centralNotice.data.anonymous && !banner.display_account ) { + continue; + } // Device if ( $.inArray( mw.centralNotice.data.device, - campaign.banners[j].devices ) === -1 ) { + banner.devices ) === -1 ) { continue; } - // We get here if the campaign targets the user's country - // and has at least one banner for the user's device. + // We get here if the campaign targets the user's country, + // and has at least one banner for the user's logged-in status + // and device. keepCampaign = true; } @@ -264,11 +275,11 @@ }, /** - * Filter the choice data on the user's device and per-campaign buckets - * (some banners that are not for the user's device may remain following - * previous filters) and create a flat list of possible banners to chose - * from. Add some extra data on to each banner entry. The result is - * placed in possibleBanners. + * Filter the choice data on the user's logged-in status, device and + * per-campaign buckets (some banners that are not for the user's status + * or device may remain following previous filters) and create a flat + * list of possible banners to chose from. Add some extra data on to + * each banner entry. The result is placed in possibleBanners. * * Note that if the same actual banner is assigned to more than one * campaign it can have more than one entry in that list. That's the @@ -281,7 +292,7 @@ * FIXME Re-organize code in all those places to make it easier to * understand. */ - makePossibleBannersForBucketsAndDevice: function() { + makePossibleBanners: function() { var i, campaign, campaignName, j, banner; this.possibleBanners = []; @@ -299,6 +310,16 @@ continue; } + // Filter for logged-in status + if ( mw.centralNotice.data.anonymous && + ( banner.display_anon != 1 ) ) { + continue; + } + if ( !mw.centralNotice.data.anonymous && + ( banner.display_account != 1 ) ) { + continue; + } + // Filter for device if ( $.inArray( mw.centralNotice.data.device, banner.devices ) === -1 ) { diff --git a/special/SpecialBannerAllocation.php b/special/SpecialBannerAllocation.php index 74e3330..0337125 100644 --- a/special/SpecialBannerAllocation.php +++ b/special/SpecialBannerAllocation.php @@ -214,17 +214,11 @@ 'id' => 'algorithm-selector' ) ); - // Given our project and language combination, get banner choice data - // for logged in and anonymous users. - $login_statuses = array( - BannerChoiceDataProvider::ANONYMOUS, - BannerChoiceDataProvider::LOGGED_IN - ); - foreach ( $login_statuses as $provider_status ) { - $provider = new BannerChoiceDataProvider( $project, $language, $provider_status ); - $choice_data[$provider_status] = $provider->getChoicesForCountry( $country ); - } + // Given our project and language combination, get banner choice data, + // then filter on country + $provider = new BannerChoiceDataProvider( $project, $language ); + $choice_data = $provider->getChoicesForCountry( $country ); // Iterate through each possible device type and get allocation information $devices = CNDeviceTarget::getAvailableDevices(); @@ -276,16 +270,17 @@ ); if ( $target['anonymous'] === 'true' ) { $label = $this->msg( 'centralnotice-banner-anonymous' )->text(); - $provider_campaigns = $choice_data[BannerChoiceDataProvider::ANONYMOUS]; + $status = BannerAllocationCalculator::ANONYMOUS; } else { $label = $this->msg( 'centralnotice-banner-logged-in' )->text(); - $provider_campaigns = $choice_data[BannerChoiceDataProvider::LOGGED_IN]; + $status = BannerAllocationCalculator::LOGGED_IN; } $label .= ' -- ' . $this->msg( 'centralnotice-bucket-letter' )-> rawParams( chr( $target['bucket'] + 65 ) )->text(); $provider_banners = BannerAllocationCalculator::filterAndTransformBanners( - $provider_campaigns, + $choice_data, + $status, $device_data['header'], intval( $target['bucket'] ) ); diff --git a/tests/ApiCentralNoticeBannerChoiceDataTest.php b/tests/ApiCentralNoticeBannerChoiceDataTest.php index 2ee6365..1c3e545 100644 --- a/tests/ApiCentralNoticeBannerChoiceDataTest.php +++ b/tests/ApiCentralNoticeBannerChoiceDataTest.php @@ -29,8 +29,7 @@ $ret = $this->doApiRequest( array( 'action' => 'centralnoticebannerchoicedata', 'project' => CentralNoticeTestFixtures::$defaultCampaign['projects'][0], - 'language' => CentralNoticeTestFixtures::$defaultCampaign['project_languages'][0], - 'status' => 'anonymous', + 'language' => CentralNoticeTestFixtures::$defaultCampaign['project_languages'][0] ) ); $this->assertTrue( ComparisonUtil::assertSuperset( $ret[0]['choices'], $data['choices'] ) ); } diff --git a/tests/BannerAllocationCalculatorTest.php b/tests/BannerAllocationCalculatorTest.php index f20be61..49fbaf3 100644 --- a/tests/BannerAllocationCalculatorTest.php +++ b/tests/BannerAllocationCalculatorTest.php @@ -28,14 +28,14 @@ $allocationsProvider = new BannerChoiceDataProvider( CentralNoticeTestFixtures::getDefaultProject(), - CentralNoticeTestFixtures::getDefaultLanguage(), - BannerChoiceDataProvider::ANONYMOUS + CentralNoticeTestFixtures::getDefaultLanguage() ); $choices = $allocationsProvider->getChoicesForCountry( CentralNoticeTestFixtures::getDefaultCountry() ); $banners = BannerAllocationCalculator::filterAndTransformBanners( $choices, + BannerAllocationCalculator::ANONYMOUS, CentralNoticeTestFixtures::getDefaultDevice(), 0 ); diff --git a/tests/BannerChoiceDataProviderTest.php b/tests/BannerChoiceDataProviderTest.php index ec41dd3..68b54b4 100644 --- a/tests/BannerChoiceDataProviderTest.php +++ b/tests/BannerChoiceDataProviderTest.php @@ -28,8 +28,7 @@ $allocationsProvider = new BannerChoiceDataProvider( CentralNoticeTestFixtures::$defaultCampaign['projects'][0], - CentralNoticeTestFixtures::$defaultCampaign['project_languages'][0], - BannerChoiceDataProvider::ANONYMOUS + CentralNoticeTestFixtures::$defaultCampaign['project_languages'][0] ); $choices = $allocationsProvider->getChoices(); $this->assertTrue( ComparisonUtil::assertSuperset( $choices, $data['choices'] ) ); diff --git a/tests/data/AllocationsFixtures.json b/tests/data/AllocationsFixtures.json index 828d9ca..4606f81 100644 --- a/tests/data/AllocationsFixtures.json +++ b/tests/data/AllocationsFixtures.json @@ -16,7 +16,8 @@ "banners": [ { "name": "b1", - "weight": 5 + "weight": 5, + "display_anon": 1 } ] } @@ -392,6 +393,16 @@ ] }, "choices": [ + { + "banners": [ + { + "name": "b1", + "weight": 5, + "display_anon": false, + "display_account": true + } + ] + } ], "allocations": { } diff --git a/tests/qunit/ext.centralNotice.bannerController.lib/bannerController.lib.tests.js b/tests/qunit/ext.centralNotice.bannerController.lib/bannerController.lib.tests.js index 12dc3e7..fa8c343 100644 --- a/tests/qunit/ext.centralNotice.bannerController.lib/bannerController.lib.tests.js +++ b/tests/qunit/ext.centralNotice.bannerController.lib/bannerController.lib.tests.js @@ -11,13 +11,16 @@ }, defaultBannerData = { bucket: 0, devices: [ 'desktop' ], - weight: 25 + weight: 25, + display_anon: true, + display_account: true }; QUnit.module( 'ext.centralNotice.bannerController.lib', QUnit.newMwEnvironment( { setup: function() { mw.centralNotice.data.country = 'XX'; mw.centralNotice.data.device = 'desktop'; + mw.centralNotice.data.anonymous = true; } } ) ); @@ -42,14 +45,14 @@ // BOOM on priority case choices = $.map( testCase.choices, function( campaign, index ) { return $.extend( - { name: index }, - defaultCampaignData, - campaign, - { - banners: $.map( campaign.banners, function( banner ) { - return $.extend( {}, defaultBannerData, banner ); - } ) - } ); + { name: index }, + defaultCampaignData, + campaign, + { + banners: $.map( campaign.banners, function( banner ) { + return $.extend( {}, defaultBannerData, banner ); + } ) + } ); } ); // Set per-campaign buckets to 0 for all campaigns @@ -63,8 +66,8 @@ // TODO: would like to declare individual tests here, but I // haven't been able to make that work, yet. lib.setChoiceData( choices ); - lib.filterChoiceDataCountriesAndDevices(); - lib.makePossibleBannersForBucketsAndDevice(); + lib.filterChoiceData(); + lib.makePossibleBanners(); lib.calculateBannerAllocations(); // TODO: the errors will not reveal anything useful about -- To view, visit https://gerrit.wikimedia.org/r/175768 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib4d23f2a588f58ef3abcbd8b0b500ad8534723cd Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/CentralNotice Gerrit-Branch: wmf_deploy Gerrit-Owner: Reedy <re...@wikimedia.org> Gerrit-Reviewer: AndyRussG <andrew.green...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits