r71004 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r71003‎ \| r71004 \| r71005 >
Date:	01:52, 13 August 2010
Author:	kaldari
Status:	resolved (Comments)
Tags:
Comment:	allow control of whether banners are displayed to anon and/or logged in users
Modified paths:	/trunk/extensions/CentralNotice/CentralNotice.db.php (modified) (history) /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history) /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history) /trunk/extensions/CentralNotice/SpecialNoticeTemplate.php (modified) (history) /trunk/extensions/CentralNotice/SpecialNoticeText.php (modified) (history) /trunk/extensions/CentralNotice/patches/patch-notice_preferred.sql (modified) (history) /trunk/extensions/CentralNotice/patches/patch-template_settings.sql (added) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/CentralNotice.php
—	—	@@ -158,6 +158,7 @@
159	159	$wgExtNewTables[] = array( 'cn_notices', $base . '/CentralNotice.sql' );
160	160	$wgExtNewFields[] = array( 'cn_notices', 'not_preferred', $base . '/patches/patch-notice_preferred.sql' );
161	161	$wgExtNewTables[] = array( 'cn_notice_languages', $base . '/patches/patch-notice_languages.sql' );
	162	+ $wgExtNewFields[] = array( 'cn_templates', 'tmp_display_anon', $base . '/patches/patch-template_settings.sql' );
162	163	}
163	164	return true;
164	165	}
Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
—	—	@@ -1013,6 +1013,7 @@
1014	1014
1015	1015	/**
1016	1016	* Build a list of all the banners assigned to a campaign
	1017	+ * @return An array of template names
1017	1018	*/
1018	1019	function selectTemplatesAssigned ( $notice ) {
1019	1020	$dbr = wfGetDB( DB_SLAVE );
—	—	@@ -1041,8 +1042,10 @@
1042	1043	}
1043	1044
1044	1045	/**
1045		~~- * Lookup function for active campaigns under a given language and project~~
1046		~~- * @return An array of running campaign names with associated banner weights~~
	1046	+ * Lookup function for active banners under a given language and project. This function is
	1047	+ * called by SpecialNoticeText::getJsOutput() in order to build the static Javascript files for
	1048	+ * each project.
	1049	+ * @return A 2D array of running banners with associated weights and settings
1047	1050	*/
1048	1051	static function selectNoticeTemplates( $project, $language ) {
1049	1052	$dbr = wfGetDB( DB_SLAVE );
—	—	@@ -1056,7 +1059,9 @@
1057	1060	),
1058	1061	array(
1059	1062	'tmp_name',
1060		~~- 'SUM(tmp_weight) AS total_weight'~~
	1063	+ 'SUM(tmp_weight) AS total_weight',
	1064	+ 'tmp_display_anon',
	1065	+ 'tmp_display_account'
1061	1066	),
1062	1067	array (
1063	1068	"not_start <= $encTimestamp",
—	—	@@ -1073,13 +1078,16 @@
1074	1079	'GROUP BY' => 'tmp_name'
1075	1080	)
1076	1081	);
1077		~~- $templateWeights = array();~~
	1082	+ $templates = array();
1078	1083	foreach ( $res as $row ) {
1079		~~- $name = $row->tmp_name;~~
1080		~~- $weight = intval( $row->total_weight );~~
1081		~~- $templateWeights[$name] = $weight;~~
	1084	+ $template = array();
	1085	+ $template['name'] = $row->tmp_name;
	1086	+ $template['weight'] = intval( $row->total_weight );
	1087	+ $template['display_anon'] = intval( $row->tmp_display_anon );
	1088	+ $template['display_account'] = intval( $row->tmp_display_account );
	1089	+ $templates[] = $template;
1082	1090	}
1083		~~- return $templateWeights;~~
	1091	+ return $templates;
1084	1092	}
1085	1093
1086	1094	function addNotice( $noticeName, $enabled, $start, $project_name, $project_languages ) {
Index: trunk/extensions/CentralNotice/patches/patch-template_settings.sql
—	—	@@ -0,0 +1,7 @@
	2	+-- Update to support controlling whether banners are displayed to anon and/or logged in users.
	3	+-- Two flags are stored in the database for each banner, one indicating whether or not the banner
	4	+-- is displayed to anonymous users, the other indicating whether or not the banner is displayed
	5	+-- to logged in users.
	6	+
	7	+ALTER TABLE /$wgDBprefix/cn_templates ADD `tmp_display_anon` BOOLEAN NOT NULL DEFAULT 1,
	8	+ ADD `tmp_display_account` BOOLEAN NOT NULL DEFAULT 1;
Index: trunk/extensions/CentralNotice/patches/patch-notice_preferred.sql
—	—	@@ -3,4 +3,4 @@
4	4	-- overlap. Use case is to be able to use one all language and projects notice
5	5	-- and have it superceded by a specific one for en wikipedia.
6	6
7		~~-ALTER TABLE cn_notices ADD COLUMN not_preferred bool NOT NULL default '0';~~
	7	+ALTER TABLE /$wgDBprefix/cn_notices ADD COLUMN not_preferred bool NOT NULL default '0';
Index: trunk/extensions/CentralNotice/SpecialNoticeTemplate.php
—	—	@@ -72,16 +72,48 @@
73	73	// Handle adding banner
74	74	// FIXME: getText()? weak comparison
75	75	if ( $wgRequest->getVal( 'wpMethod' ) == 'addTemplate' ) {
	76	+
	77	+ // Handle "Display to anonymous users" checkbox
	78	+ $displayAnon = 0;
	79	+ if ( $wgRequest->getVal( 'displayAnon' ) ) {
	80	+ $displayAnon = $wgRequest->getVal( 'displayAnon' );
	81	+ }
	82	+
	83	+ // Handle "Display to logged in users" checkbox
	84	+ $displayAccount = 0;
	85	+ if ( $wgRequest->getVal( 'displayAccount' ) ) {
	86	+ $displayAccount = $wgRequest->getVal( 'displayAccount' );
	87	+ }
	88	+
76	89	$this->addTemplate(
77	90	$wgRequest->getVal( 'templateName' ),
78		~~- $wgRequest->getVal( 'templateBody' )~~
	91	+ $wgRequest->getVal( 'templateBody' ),
	92	+ $displayAnon,
	93	+ $displayAccount
79	94	);
80	95	$sub = 'view';
81	96	}
	97	+
	98	+ // Handle editing banner
82	99	if ( $wgRequest->getVal( 'wpMethod' ) == 'editTemplate' ) {
	100	+
	101	+ // Handle "Display to anonymous users" checkbox
	102	+ $displayAnon = 0;
	103	+ if ( $wgRequest->getVal( 'displayAnon' ) ) {
	104	+ $displayAnon = $wgRequest->getVal( 'displayAnon' );
	105	+ }
	106	+
	107	+ // Handle "Display to logged in users" checkbox
	108	+ $displayAccount = 0;
	109	+ if ( $wgRequest->getVal( 'displayAccount' ) ) {
	110	+ $displayAccount = $wgRequest->getVal( 'displayAccount' );
	111	+ }
	112	+
83	113	$this->editTemplate(
84	114	$wgRequest->getVal( 'template' ),
85		~~- $wgRequest->getVal( 'templateBody' )~~
	115	+ $wgRequest->getVal( 'templateBody' ),
	116	+ $displayAnon,
	117	+ $displayAccount
86	118	);
87	119	$sub = 'view';
88	120	}
—	—	@@ -194,6 +226,15 @@
195	227	$htmlOut .= Xml::tags( 'p', null,
196	228	Xml::inputLabel( wfMsg( 'centralnotice-banner-name' ) . ":", 'templateName', 'templateName', 25 )
197	229	);
	230	+
	231	+ $htmlOut .= Xml::openElement( 'p', null );
	232	+ $htmlOut .= wfMsg( 'centralnotice-banner-display' );
	233	+ $htmlOut .= Xml::check( 'displayAnon', true, array( 'id' => 'displayAnon' ) );
	234	+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-anonymous' ), 'displayAnon' );
	235	+ $htmlOut .= Xml::check( 'displayAccount', true, array( 'id' => 'displayAccount' ) );
	236	+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-logged-in' ), 'displayAccount' );
	237	+ $htmlOut .= Xml::closeElement( 'p' );
	238	+
198	239	$htmlOut .= Xml::fieldset( wfMsg( 'centralnotice-banner' ) );
199	240	$htmlOut .= wfMsg( 'centralnotice-edit-template-summary' );
200	241	$buttons = array();
—	—	@@ -231,8 +272,10 @@
232	273
233	274	if ( $this->editable ) {
234	275	$readonly = array();
	276	+ $disabled = array();
235	277	} else {
236	278	$readonly = array( 'readonly' => 'readonly' );
	279	+ $disabled = array( 'disabled' => 'disabled' );
237	280	}
238	281
239	282	// Get user's language
—	—	@@ -241,6 +284,17 @@
242	285	// Get current banner
243	286	$currentTemplate = $wgRequest->getText( 'template' );
244	287
	288	+ // Pull banner settings from database
	289	+ $dbr = wfGetDB( DB_SLAVE );
	290	+ $row = $dbr->selectRow( 'cn_templates',
	291	+ array(
	292	+ 'tmp_display_anon',
	293	+ 'tmp_display_account'
	294	+ ),
	295	+ array( 'tmp_name' => $currentTemplate ),
	296	+ __METHOD__
	297	+ );
	298	+
245	299	// Begin building HTML
246	300	$htmlOut = '';
247	301
—	—	@@ -397,6 +451,18 @@
398	452	$htmlOut .= Xml::openElement( 'form', array( 'method' => 'post' ) );
399	453	$htmlOut .= Xml::hidden( 'wpMethod', 'editTemplate' );
400	454	}
	455	+
	456	+ // Show banner settings
	457	+ $htmlOut .= Xml::fieldset( 'Settings' );
	458	+ $htmlOut .= Xml::openElement( 'p', null );
	459	+ $htmlOut .= wfMsg( 'centralnotice-banner-display' );
	460	+ $htmlOut .= Xml::check( 'displayAnon', ( $row->tmp_display_anon == 1 ), wfArrayMerge( $disabled, array( 'id' => 'displayAnon' ) ) );
	461	+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-anonymous' ), 'displayAnon' );
	462	+ $htmlOut .= Xml::check( 'displayAccount', ( $row->tmp_display_account == 1 ), wfArrayMerge( $disabled, array( 'id' => 'displayAccount' ) ) );
	463	+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-logged-in' ), 'displayAccount' );
	464	+ $htmlOut .= Xml::closeElement( 'p' );
	465	+ $htmlOut .= Xml::closeElement( 'fieldset' );
	466	+
401	467	$htmlOut .= Xml::fieldset( wfMsg( 'centralnotice-edit-template' ) );
402	468	$htmlOut .= wfMsg( 'centralnotice-edit-template-summary' );
403	469	$buttons = array();
—	—	@@ -552,7 +618,7 @@
553	619	/**
554	620	* Create a new banner
555	621	*/
556		~~- private function addTemplate ( $name, $body ) {~~
	622	+ private function addTemplate( $name, $body, $displayAnon, $displayAccount ) {
557	623	global $wgOut;
558	624
559	625	if ( $body == '' \|\| $name == '' ) {
—	—	@@ -577,9 +643,12 @@
578	644	} else {
579	645	$dbw = wfGetDB( DB_MASTER );
580	646	$dbw->begin();
581		~~- $res = $dbw->insert(~~
582		~~- 'cn_templates',~~
583		~~- array( 'tmp_name' => $name ),~~
	647	+ $res = $dbw->insert( 'cn_templates',
	648	+ array(
	649	+ 'tmp_name' => $name,
	650	+ 'tmp_display_anon' => $displayAnon,
	651	+ 'tmp_display_account' => $displayAccount
	652	+ ),
584	653	__METHOD__
585	654	);
586	655	$dbw->commit();
—	—	@@ -596,7 +665,7 @@
597	666	/**
598	667	* Update a banner
599	668	*/
600		~~- private function editTemplate ( $name, $body ) {~~
	669	+ private function editTemplate( $name, $body, $displayAnon, $displayAccount ) {
601	670	global $wgOut;
602	671
603	672	if ( $body == '' \|\| $name == '' ) {
—	—	@@ -610,7 +679,18 @@
611	680	__METHOD__
612	681	);
613	682
614		~~- if ( $dbr->numRows( $res ) > 0 ) {~~
	683	+ if ( $dbr->numRows( $res ) == 1 ) {
	684	+ $dbw = wfGetDB( DB_MASTER );
	685	+ $dbw->begin();
	686	+ $res = $dbw->update( 'cn_templates',
	687	+ array(
	688	+ 'tmp_display_anon' => $displayAnon,
	689	+ 'tmp_display_account' => $displayAccount
	690	+ ),
	691	+ array( 'tmp_name' => $name )
	692	+ );
	693	+ $dbw->commit();
	694	+
615	695	// Perhaps these should move into the db as blob
616	696	$article = new Article(
617	697	Title::newFromText( "centralnotice-template-{$name}", NS_MEDIAWIKI )
—	—	@@ -624,19 +704,35 @@
625	705	* Copy all the data from one banner to another
626	706	*/
627	707	public function cloneTemplate( $source, $dest ) {
	708	+
628	709	// Reset the timer as updates on meta take a long time
629	710	set_time_limit( 300 );
	711	+
630	712	// Pull all possible langs
631	713	$langs = $this->getTranslations( $source );
632	714
633	715	// Normalize name
634	716	$dest = ereg_replace( '[^A-Za-z0-9\_]', '', $dest );
	717	+
	718	+ // Pull banner settings from database
	719	+ $dbr = wfGetDB( DB_SLAVE );
	720	+ $row = $dbr->selectRow( 'cn_templates',
	721	+ array(
	722	+ 'tmp_display_anon',
	723	+ 'tmp_display_account'
	724	+ ),
	725	+ array( 'tmp_name' => $source ),
	726	+ __METHOD__
	727	+ );
	728	+ $displayAnon = $row->tmp_display_anon;
	729	+ $displayAccount = $row->tmp_display_account;
635	730
636		~~- // Pull text and respect any inc: markup~~
	731	+ // Pull banner text and respect any inc: markup
637	732	$bodyPage = Title::newFromText( "Centralnotice-template-{$source}", NS_MEDIAWIKI );
638	733	$template_body = Revision::newFromTitle( $bodyPage )->getText();
639	734
640		~~- if ( $this->addTemplate( $dest, $template_body ) ) {~~
	735	+ // Create new banner
	736	+ if ( $this->addTemplate( $dest, $template_body, $displayAnon, $displayAccount ) ) {
641	737
642	738	// Populate the fields
643	739	foreach ( $langs as $lang => $fields ) {
Index: trunk/extensions/CentralNotice/CentralNotice.db.php
—	—	@@ -79,7 +79,7 @@
80	80	}
81	81
82	82	/*
83		~~- * Given a notice return all templates bound to it~~
	83	+ * Given a notice return all banners bound to it
84	84	*/
85	85	public function selectTemplatesAssigned( $notice ) {
86	86	$dbr = wfGetDB( DB_SLAVE );
—	—	@@ -92,8 +92,10 @@
93	93	'cn_templates'
94	94	),
95	95	array(
96		~~- 'cn_templates.tmp_name',~~
	96	+ 'tmp_name',
97	97	'SUM(tmp_weight) AS total_weight',
	98	+ 'tmp_display_anon',
	99	+ 'tmp_display_account'
98	100	),
99	101	array(
100	102	'cn_notices.not_name' => $notice,
—	—	@@ -105,13 +107,16 @@
106	108	'GROUP BY' => 'tmp_name'
107	109	)
108	110	);
109		~~- $templateWeights = array();~~
	111	+ $templates = array();
110	112	foreach ( $res as $row ) {
111		~~- $name = $row->tmp_name;~~
112		~~- $weight = intval( $row->total_weight );~~
113		~~- $templateWeights[$name] = $weight;~~
	113	+ $template = array();
	114	+ $template['name'] = $row->tmp_name;
	115	+ $template['weight'] = intval( $row->total_weight );
	116	+ $template['display_anon'] = intval( $row->tmp_display_anon );
	117	+ $template['display_account'] = intval( $row->tmp_display_account );
	118	+ $templates[] = $template;
114	119	}
115		~~- return $templateWeights;~~
	120	+ return $templates;
116	121	}
117	122
118	123	public function updatePreferred( $notice, $preferred ) {
Index: trunk/extensions/CentralNotice/SpecialNoticeText.php
—	—	@@ -1,5 +1,8 @@
2	2	<?php
3	3
	4	+/**
	5	+ * Generates content for static Javascript files
	6	+ */
4	7	class SpecialNoticeText extends NoticePage {
5	8	var $project = 'wikipedia';
6	9	var $language = 'en';
—	—	@@ -21,6 +24,9 @@
22	25	return 86400 * 7;
23	26	}
24	27
	28	+ /**
	29	+ * Given a project key, generate the body for a static Javascript file
	30	+ */
25	31	function getJsOutput( $par ) {
26	32
27	33	// Break $par into separate parameters and assign to $this->project and $this->language
—	—	@@ -60,7 +66,18 @@
61	67	$templates = CentralNotice::selectNoticeTemplates( $this->project, $this->language );
62	68	}
63	69
64		~~- $templateNames = array_keys( $templates );~~
	70	+ // Slice the columns of the $templates array into separate arrays.
	71	+ // This is required due to how pickTemplate() currently works.
	72	+ $templateNames = array();
	73	+ $templateWeights = array();
	74	+ $templateDisplayAnons = array();
	75	+ $templateDisplayAccounts = array();
	76	+ foreach ( $templates as $template ) {
	77	+ $templateNames[] = $template['name'];
	78	+ $templateWeights[] = $template['weight'];
	79	+ $templateDisplayAnons[] = $template['display_anon'];
	80	+ $templateDisplayAccounts[] = $template['display_account'];
	81	+ }
65	82
66	83	$templateTexts = array_map(
67	84	array( $this, 'getHtmlNotice' ),
—	—	@@ -69,23 +86,23 @@
70	87	if ( preg_grep( "/<centralnotice-template-\w{1,}>\z/", $templateTexts ) ) {
71	88	return false; // Bailing out if we have a failed cache lookup
72	89	}
73		-
74		~~- $weights = array_values( $templates );~~
75		-
	90	+
76	91	return
77	92	$this->getScriptFunctions() .
78	93	$this->getToggleScripts() .
79	94	'wgNotice=pickTemplate(' .
80	95	Xml::encodeJsVar( $templateTexts ) .
81	96	"," .
82		~~- Xml::encodeJsVar( $weights ) .~~
	97	+ Xml::encodeJsVar( $templateWeights ) .
	98	+ "," .
	99	+ Xml::encodeJsVar( $templateDisplayAnons ) .
	100	+ "," .
	101	+ Xml::encodeJsVar( $templateDisplayAccounts ) .
83	102	");\n" .
84	103	"if (wgNotice != '')\n" .
85	104	"wgNotice='<div id=\"centralNotice\" class=\"' + " .
86	105	"(wgNoticeToggleState ? 'expanded' : 'collapsed') + " .
87		~~- "' ' + " .~~
88		~~- "(wgUserName ? 'usernotice' : 'anonnotice' ) + " .~~
89		~~- "'\">' + wgNotice+'</div>';\n";~~
	106	+ "'\">' + wgNotice+'</div>';\n";
90	107	}
91	108
92	109	function getHtmlNotice( $noticeName ) {
—	—	@@ -142,25 +159,27 @@
143	160	var work='hidesnmessage='+state+'; expires=' + e.toGMTString() + '; path=/';
144	161	document.cookie = work;
145	162	}
146		~~-function pickTemplate(templates, weights) {~~
	163	+function pickTemplate(templates, weights, displayAnons, displayAccounts) {
147	164	var weightedTemplates = new Array();
148	165	var currentTemplate = 0;
149	166	var totalWeight = 0;
150	167
151	168	if (templates.length == 0)
152	169	return '';
153		-
	170	+
154	171	while (currentTemplate < templates.length) {
155		~~- totalWeight += weights[currentTemplate];~~
156		~~- for (i=0; i<weights[currentTemplate]; i++) {~~
157		~~- weightedTemplates[weightedTemplates.length] = templates[currentTemplate];~~
	172	+ if ((wgUserName && displayAccounts[currentTemplate]) \|\| (!wgUserName && displayAnons[currentTemplate])) {
	173	+ totalWeight += weights[currentTemplate];
	174	+ for (i=0; i<weights[currentTemplate]; i++) {
	175	+ weightedTemplates[weightedTemplates.length] = templates[currentTemplate];
	176	+ }
158	177	}
159	178	currentTemplate++;
160	179	}
161		-
	180	+
162	181	if (totalWeight == 0)
163	182	return '';
164		-
	183	+
165	184	var randomnumber=Math.floor(Math.random()*totalWeight);
166	185	return weightedTemplates[randomnumber];
167	186	}\n\n";

Follow-up revisions

Revision	Commit summary	Author	Date
r71222	forgot to update main SQL file for r71004, cleaning up SQL formatting	kaldari	00:08, 18 August 2010
r71581	unreinventing getBool per r71004 comment	kaldari	20:43, 24 August 2010
r71582	scoping Javascript var per comment at r71004	kaldari	20:50, 24 August 2010
r71586	localizing 'Settings' per comment at r71004	kaldari	21:10, 24 August 2010

Comments

#Comment by Catrope (talk | contribs) 00:54, 24 August 2010

+				$displayAnon = 0;
+				if ( $wgRequest->getVal( 'displayAnon' ) ) {
+					$displayAnon = $wgRequest->getVal( 'displayAnon' );
+				}

You're reinventing $wgRequest->getBool() here.

+		$htmlOut .= Xml::fieldset( 'Settings' );

Needs i18n.

+			for (i=0; i<weights[currentTemplate]; i++) {

Needs to say var i = 0; so as to not pollute the global scope.

#Comment by Kaldari (talk | contribs) 21:11, 24 August 2010

These issues are fixed by r71581, r71586, and r71582 respectively.

#Comment by Tim Starling (talk | contribs) 02:03, 26 August 2010

I think adding two extra parameters to several functions was not the best way to do this, and as a result, the code here is barely passable. The data structure returned by selectTemplatesAssigned() is good, and you could easily have used it to reduce the number of formal parameters in addTemplate(), editTemplate(), and the JavaScript function pickTemplate(). The excuse given in the comment "This is required due to how pickTemplate() currently works" is not particularly convincing since you are substantially changing how pickTemplate() works, and you could have changed this while you were at it.

I'm marking this resolved.

#Comment by Kaldari (talk | contribs) 03:14, 26 August 2010

All of the pickTemplates functionality is going to be redone from scratch next month. Just need to get this feature usable in the meantime (for tomorrow's test campaign). Otherwise, I agree with you completely :)

#Comment by Catrope (talk | contribs) 02:21, 26 August 2010

Yes, the way this currently works is totally weird, but right now we just want to get this deployed by noon on Thursday. Then for phase 2 Ryan is planning to overhaul the JS (or have Adam do so), among a lot of other things.

Status & tagging log

02:03, 26 August 2010 Tim Starling (talk | contribs) changed the status of r71004 [removed: fixme added: resolved]
00:54, 24 August 2010 Catrope (talk | contribs) changed the status of r71004 [removed: new added: fixme]