r99160 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r99159‎ | r99160 | r99161 >
Date:22:51, 6 October 2011
Author:kaldari
Status:resolved (Comments)
Tags:fundraising 
Comment:
separating fundraising specific features into 2 checkboxes - one for the cookie group, one for using landingcheck
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.db.php (modified) (history)
  • /trunk/extensions/CentralNotice/CentralNotice.i18n.php (modified) (history)
  • /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/CentralNoticeBannerLogPager.php (modified) (history)
  • /trunk/extensions/CentralNotice/centralnotice.js (modified) (history)
  • /trunk/extensions/CentralNotice/patches/patch-template_landingcheck.sql (added) (history)
  • /trunk/extensions/CentralNotice/special/SpecialBannerController.php (modified) (history)
  • /trunk/extensions/CentralNotice/special/SpecialBannerLoader.php (modified) (history)
  • /trunk/extensions/CentralNotice/special/SpecialNoticeTemplate.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/CentralNotice.db.php
——@@ -214,6 +214,7 @@
215215 'tmp_display_anon',
216216 'tmp_display_account',
217217 'tmp_fundraising',
 218+ 'tmp_landingcheck',
218219 'tmp_landing_pages',
219220 'not_name'
220221 ),
——@@ -232,6 +233,7 @@
233234 'display_anon' => intval( $row->tmp_display_anon ), // display to anonymous users?
234235 'display_account' => intval( $row->tmp_display_account ), // display to logged in users?
235236 'fundraising' => intval( $row->tmp_fundraising ), // fundraising banner?
 237+ 'landingcheck' => intval( $row->tmp_landingcheck ), // use LandingCheck?
236238 'landing_pages' => $row->tmp_landing_pages, // landing pages to link to
237239 'campaign' => $row->not_name // campaign the banner is assigned to
238240 );
——@@ -263,6 +265,7 @@
264266 'tmp_display_anon',
265267 'tmp_display_account',
266268 'tmp_fundraising',
 269+ 'tmp_landingcheck',
267270 'tmp_landing_pages'
268271 ),
269272 array( 'tmp_name' => $bannerName ),
——@@ -274,6 +277,7 @@
275278 'anon' => $row->tmp_display_anon,
276279 'account' => $row->tmp_display_account,
277280 'fundraising' => $row->tmp_fundraising,
 281+ 'landingcheck' => $row->tmp_landingcheck,
278282 'landingpages' => $row->tmp_landing_pages
279283 );
280284 }
Index: trunk/extensions/CentralNotice/centralnotice.js
——@@ -95,9 +95,10 @@
9696 }
9797 return true;
9898 }
99 -// Handle revealing the geoMultiSelector when the geotargetted checkbox is checked
 99+
100100 ( function( $ ) {
101101 $(document).ready(function() {
 102+ // Reveal the geoMultiSelector when the geotargetted checkbox is checked
102103 $("#geotargeted").click(function () {
103104 if ($('#geotargeted:checked').val() !== undefined) {
104105 $("#geoMultiSelector").fadeIn('fast');
——@@ -105,11 +106,12 @@
106107 $("#geoMultiSelector").fadeOut('fast');
107108 }
108109 });
109 - $("#fundraising").click(function () {
110 - if ($('#fundraising:checked').val() !== undefined) {
111 - $("#fundraisingInterface").fadeIn('fast');
 110+ // Reveal the LandingCheck interface when the LandingCheck checkbox is checked
 111+ $("#landingCheck").click(function () {
 112+ if ($('#landingCheck:checked').val() !== undefined) {
 113+ $("#landingCheckInterface").fadeIn('fast');
112114 } else {
113 - $("#fundraisingInterface").fadeOut('fast');
 115+ $("#landingCheckInterface").fadeOut('fast');
114116 }
115117 });
116118 });
Index: trunk/extensions/CentralNotice/special/SpecialBannerController.php
——@@ -161,7 +161,7 @@
162162 $script = <<<JAVASCRIPT
163163 function insertBanner(bannerJson) {
164164 jQuery( 'div#centralNotice' ).prepend( bannerJson.bannerHtml );
165 - if ( bannerJson.fundraising ) {
 165+ if ( bannerJson.landingCheck ) {
166166 JAVASCRIPT;
167167 $script .= "\n\t\tvar url = '" .
168168 Xml::escapeJsString( $wgNoticeFundraisingUrl ) . "';\n";
——@@ -176,7 +176,7 @@
177177 'utm_source': bannerJson.bannerName, 'language': wgUserLanguage,
178178 'country': Geo.country
179179 } );
180 - jQuery( '#cn_fundraising_link' ).attr( 'href', url );
 180+ jQuery( '#cn-landingcheck-link' ).attr( 'href', url );
181181 }
182182 }
183183 }
Index: trunk/extensions/CentralNotice/special/SpecialBannerLoader.php
——@@ -75,6 +75,7 @@
7676 'bannerHtml' => $bannerHtml,
7777 'campaign' => $this->campaign,
7878 'fundraising' => $this->getFundraising( $bannerName ),
 79+ 'landingCheck' => $this->getLandingCheck( $bannerName ),
7980 'landingPages' => $this->getLandingPages( $bannerName )
8081 );
8182 $bannerJs = 'insertBanner('.FormatJson::encode( $bannerArray ).');';
——@@ -203,6 +204,14 @@
204205 return $row->tmp_fundraising;
205206 }
206207
 208+ function getLandingCheck( $bannerName ) {
 209+ global $wgCentralDBname;
 210+ $dbr = wfGetDB( DB_SLAVE, array(), $wgCentralDBname );
 211+ $eBannerName = htmlspecialchars( $bannerName );
 212+ $row = $dbr->selectRow( 'cn_templates', 'tmp_landingcheck', array( 'tmp_name' => $eBannerName ) );
 213+ return $row->tmp_landingcheck;
 214+ }
 215+
207216 function getLandingPages( $bannerName ) {
208217 global $wgCentralDBname;
209218 $dbr = wfGetDB( DB_SLAVE, array(), $wgCentralDBname );
Index: trunk/extensions/CentralNotice/special/SpecialNoticeTemplate.php
——@@ -90,6 +90,7 @@
9191 $wgRequest->getBool( 'displayAnon' ),
9292 $wgRequest->getBool( 'displayAccount' ),
9393 $wgRequest->getBool( 'fundraising' ),
 94+ $wgRequest->getBool( 'landingCheck' ),
9495 $wgRequest->getVal( 'landingPages' )
9596 );
9697 $sub = 'view';
——@@ -106,6 +107,7 @@
107108 $wgRequest->getBool( 'displayAnon' ),
108109 $wgRequest->getBool( 'displayAccount' ),
109110 $wgRequest->getBool( 'fundraising' ),
 111+ $wgRequest->getBool( 'landingCheck' ),
110112 $wgRequest->getVal( 'landingPages' )
111113 );
112114 $sub = 'view';
——@@ -238,6 +240,7 @@
239241 $displayAnon = $wgRequest->getCheck( 'displayAnon' );
240242 $displayAccount = $wgRequest->getCheck( 'displayAccount' );
241243 $fundraising = $wgRequest->getCheck( 'fundraising' );
 244+ $landingCheck = $wgRequest->getCheck( 'landingCheck' );
242245 $landingPages = $wgRequest->getVal( 'landingPages' );
243246 $body = $wgRequest->getVal( 'templateBody' );
244247 } else { // Use default values
——@@ -245,6 +248,7 @@
246249 $displayAnon = true;
247250 $displayAccount = true;
248251 $fundraising = false;
 252+ $landingCheck = false;
249253 $landingPages = '';
250254 $body = '';
251255 }
——@@ -268,13 +272,24 @@
269273
270274 // Fundraising settings
271275 if ( $wgNoticeEnableFundraising ) {
 276+
 277+ // Checkbox for indicating if it is a fundraising banner
272278 $htmlOut .= Html::openElement( 'p', null );
273279 $htmlOut .= Xml::check( 'fundraising', $fundraising, array( 'id' => 'fundraising' ) );
274280 $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-fundraising' ), 'fundraising' );
275281 $htmlOut .= Html::closeElement( 'p' );
 282+
 283+ // Checkbox for whether or not to use the LandingCheck extension
 284+ $htmlOut .= Html::openElement( 'p', null );
 285+ $htmlOut .= Xml::check( 'landingCheck', $landingCheck, array( 'id' => 'landingCheck' ) );
 286+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-landingcheck' ), 'landingCheck' );
 287+ $htmlOut .= Html::closeElement( 'p' );
 288+
 289+ // Interface for setting the landing pages
276290 $htmlOut .= Html::openElement( 'div',
277 - array( 'id' => 'fundraisingInterface', 'style' => 'display: none;' ) );
278 - $htmlOut .= Xml::tags( 'p', array(), wfMsg( 'centralnotice-banner-fundraising-help' ) );
 291+ array( 'id' => 'landingCheckInterface', 'style' => 'display: none;' ) );
 292+ $htmlOut .= Xml::tags( 'p', array(),
 293+ wfMsg( 'centralnotice-banner-landingcheck-help', 'id="cn-landingcheck-link"', 'JimmyAppeal01' ) );
279294 $htmlOut .= Xml::tags( 'p', array(),
280295 Xml::inputLabel(
281296 wfMsg( 'centralnotice-banner-landing-pages' ),
——@@ -548,12 +563,14 @@
549564 $displayAnon = $wgRequest->getCheck( 'displayAnon' );
550565 $displayAccount = $wgRequest->getCheck( 'displayAccount' );
551566 $fundraising = $wgRequest->getCheck( 'fundraising' );
 567+ $landingCheck = $wgRequest->getCheck( 'landingCheck' );
552568 $landingPages = $wgRequest->getVal( 'landingPages' );
553569 $body = $wgRequest->getVal( 'templateBody', $body );
554570 } else { // Use previously stored values
555571 $displayAnon = ( $bannerSettings['anon'] == 1 );
556572 $displayAccount = ( $bannerSettings['account'] == 1 );
557573 $fundraising = ( $bannerSettings['fundraising'] == 1 );
 574+ $landingCheck = ( $bannerSettings['landingcheck'] == 1 );
558575 $landingPages = $bannerSettings['landingpages'];
559576 // $body default is defined prior to message interface code
560577 }
——@@ -572,20 +589,32 @@
573590
574591 // Fundraising settings
575592 if ( $wgNoticeEnableFundraising ) {
 593+
 594+ // Checkbox for indicating if it is a fundraising banner
576595 $htmlOut .= Html::openElement( 'p', null );
577596 $htmlOut .= Xml::check( 'fundraising', $fundraising,
578597 wfArrayMerge( $disabled, array( 'id' => 'fundraising' ) ) );
579598 $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-fundraising' ),
580599 'fundraising' );
581600 $htmlOut .= Html::closeElement( 'p' );
582 - if ( $fundraising ) {
583 - $htmlOut .= Html::openElement( 'div', array( 'id'=>'fundraisingInterface' ) );
 601+
 602+ // Checkbox for whether or not to use the LandingCheck extension
 603+ $htmlOut .= Html::openElement( 'p', null );
 604+ $htmlOut .= Xml::check( 'landingCheck', $landingCheck,
 605+ wfArrayMerge( $disabled, array( 'id' => 'landingCheck' ) ) );
 606+ $htmlOut .= Xml::label( wfMsg( 'centralnotice-banner-landingcheck' ),
 607+ 'landingCheck' );
 608+ $htmlOut .= Html::closeElement( 'p' );
 609+
 610+ // Interface for setting the landing pages
 611+ if ( $landingCheck ) {
 612+ $htmlOut .= Html::openElement( 'div', array( 'id'=>'landingCheckInterface' ) );
584613 } else {
585614 $htmlOut .= Html::openElement( 'div',
586 - array( 'id'=>'fundraisingInterface', 'style'=>'display:none;' ) );
 615+ array( 'id'=>'landingCheckInterface', 'style'=>'display:none;' ) );
587616 }
588617 $htmlOut .= Xml::tags( 'p', array(),
589 - wfMsg( 'centralnotice-banner-fundraising-help' ) );
 618+ wfMsg( 'centralnotice-banner-landingcheck-help', 'id="cn-landingcheck-link"', 'JimmyAppeal01' ) );
590619 $htmlOut .= Xml::tags( 'p', array(),
591620 Xml::inputLabel(
592621 wfMsg( 'centralnotice-banner-landing-pages' ),
——@@ -594,6 +623,7 @@
595624 )
596625 );
597626 $htmlOut .= Html::closeElement( 'div' );
 627+
598628 }
599629
600630 // Begin banner body section
——@@ -780,11 +810,12 @@
781811 * @param $displayAnon integer flag for display to anonymous users
782812 * @param $displayAccount integer flag for display to logged in users
783813 * @param $fundraising integer flag for fundraising banner (optional)
 814+ * @param $landingCheck integer flag for using LandingCheck (optional)
784815 * @param $landingPages string list of landing pages (optional)
785816 * @return true or false depending on whether banner was successfully added
786817 */
787818 public function addTemplate( $name, $body, $displayAnon, $displayAccount, $fundraising = 0,
788 - $landingPages = '' ) {
 819+ $landingCheck = 0, $landingPages = '' ) {
789820
790821 if ( $body == '' || $name == '' ) {
791822 $this->showError( 'centralnotice-null-string' );
——@@ -813,6 +844,7 @@
814845 'tmp_display_anon' => $displayAnon,
815846 'tmp_display_account' => $displayAccount,
816847 'tmp_fundraising' => $fundraising,
 848+ 'tmp_landingcheck' => $landingCheck,
817849 'tmp_landing_pages' => $landingPages
818850 ),
819851 __METHOD__
——@@ -831,6 +863,7 @@
832864 'anon' => $displayAnon,
833865 'account' => $displayAccount,
834866 'fundraising' => $fundraising,
 867+ 'landingcheck' => $landingCheck,
835868 'landingpages' => $landingPages
836869 );
837870 $this->logBannerChange( 'created', $bannerId, $beginSettings, $endSettings );
——@@ -843,7 +876,7 @@
844877 * Update a banner
845878 */
846879 private function editTemplate( $name, $body, $displayAnon, $displayAccount, $fundraising,
847 - $landingPages ) {
 880+ $landingCheck, $landingPages ) {
848881
849882 if ( $body == '' || $name == '' ) {
850883 $this->showError( 'centralnotice-null-string' );
——@@ -865,6 +898,7 @@
866899 'tmp_display_anon' => $displayAnon,
867900 'tmp_display_account' => $displayAccount,
868901 'tmp_fundraising' => $fundraising,
 902+ 'tmp_landingcheck' => $landingCheck,
869903 'tmp_landing_pages' => $landingPages
870904 ),
871905 array( 'tmp_name' => $name )
——@@ -911,6 +945,7 @@
912946 'tmp_display_anon',
913947 'tmp_display_account',
914948 'tmp_fundraising',
 949+ 'tmp_landingcheck',
915950 'tmp_landing_pages'
916951 ),
917952 array( 'tmp_name' => $source ),
——@@ -919,6 +954,7 @@
920955 $displayAnon = $row->tmp_display_anon;
921956 $displayAccount = $row->tmp_display_account;
922957 $fundraising = $row->tmp_fundraising;
 958+ $landingCheck = $row->tmp_landingcheck;
923959 $landingPages = $row->tmp_landing_pages;
924960
925961 // Pull banner text and respect any inc: markup
——@@ -927,7 +963,7 @@
928964
929965 // Create new banner
930966 if ( $this->addTemplate( $dest, $template_body, $displayAnon, $displayAccount, $fundraising,
931 - $landingPages ) ) {
 967+ $landingCheck, $landingPages ) ) {
932968
933969 // Populate the fields
934970 foreach ( $langs as $lang => $fields ) {
Index: trunk/extensions/CentralNotice/CentralNotice.php
——@@ -153,6 +153,8 @@
154154 $base . '/patches/patch-template_settings.sql' );
155155 $wgExtNewFields[] = array( 'cn_templates', 'tmp_fundraising',
156156 $base . '/patches/patch-template_fundraising.sql' );
 157+ $wgExtNewFields[] = array( 'cn_templates', 'tmp_landingcheck',
 158+ $base . '/patches/patch-template_landingcheck.sql' );
157159 $wgExtNewTables[] = array( 'cn_notice_countries',
158160 $base . '/patches/patch-notice_countries.sql' );
159161 $wgExtNewTables[] = array( 'cn_notice_projects',
——@@ -174,6 +176,8 @@
175177 $base . '/patches/patch-template_settings.sql', true ) );
176178 $updater->addExtensionUpdate( array( 'addField', 'cn_templates', 'tmp_fundraising',
177179 $base . '/patches/patch-template_fundraising.sql', true ) );
 180+ $updater->addExtensionUpdate( array( 'addField', 'cn_templates', 'tmp_landingcheck',
 181+ $base . '/patches/patch-template_landingcheck.sql', true ) );
178182 $updater->addExtensionUpdate( array( 'addTable', 'cn_notice_countries',
179183 $base . '/patches/patch-notice_countries.sql', true ) );
180184 $updater->addExtensionUpdate( array( 'addTable', 'cn_notice_projects',
——@@ -190,7 +194,7 @@
191195 function efCentralNoticeLoader( $out, $skin ) {
192196 global $wgOut;
193197
194 - // Include '.js' to exempt script from squid cache override
 198+ // Include '.js' to exempt script from squid cache expiration override
195199 $centralLoader = SpecialPage::getTitleFor( 'BannerController' )->getLocalUrl( 'cache=/cn.js' );
196200
197201 // Insert the banner controller Javascript into the <head>
Index: trunk/extensions/CentralNotice/patches/patch-template_landingcheck.sql
——@@ -0,0 +1,8 @@
 2+-- Update to add a separate flag for LandingCheck
 3+
 4+-- Store a flag indicating whether or not this banner uses LandingCheck
 5+ALTER TABLE /*$wgDBprefix*/cn_templates ADD `tmp_landingcheck` bool NOT NULL DEFAULT 0 AFTER `tmp_fundraising`;
 6+
 7+-- Store before and after flag values for logging
 8+ALTER TABLE /*$wgDBprefix*/cn_template_log ADD `tmplog_begin_landingcheck` bool NULL DEFAULT NULL AFTER `tmplog_end_fundraising`;
 9+ALTER TABLE /*$wgDBprefix*/cn_template_log ADD `tmplog_end_landingcheck` bool NULL DEFAULT NULL AFTER `tmplog_begin_landingcheck`;
\ No newline at end of file
Index: trunk/extensions/CentralNotice/CentralNoticeBannerLogPager.php
——@@ -149,6 +149,11 @@
150150 wfMsg ( 'centralnotice-fundraising' ),
151151 ($row->tmplog_end_fundraising ? 'on' : 'off')
152152 )."<br/>";
 153+ $details .= wfMsg (
 154+ 'centralnotice-log-label',
 155+ wfMsg ( 'centralnotice-landingcheck' ),
 156+ ($row->tmplog_end_landingcheck ? 'on' : 'off')
 157+ )."<br/>";
153158 if ( $row->tmplog_end_landingpages ) {
154159 $details .= wfMsg (
155160 'centralnotice-log-label',
——@@ -165,6 +170,7 @@
166171 $details .= $this->testBooleanChange( 'anon', $row );
167172 $details .= $this->testBooleanChange( 'account', $row );
168173 $details .= $this->testBooleanChange( 'fundraising', $row );
 174+ $details .= $this->testBooleanChange( 'landingcheck', $row );
169175 $details .= $this->testTextChange( 'landingpages', $row );
170176 if ( $row->tmplog_content_change ) {
171177 // Show changes to banner content
Index: trunk/extensions/CentralNotice/CentralNotice.i18n.php
——@@ -132,7 +132,8 @@
133133 'centralnotice-banner-hidable' => 'Static/Hidable',
134134 'centralnotice-banner-collapsible' => 'Collapsible',
135135 'centralnotice-banner-fundraising' => 'This is a fundraising banner',
136 - 'centralnotice-banner-fundraising-help' => 'Create an anchor tag in the banner body with id="cn_fundraising_link" and enter one or more landing pages below, for example, "JimmyAppeal01". The href of the link will be constructed automatically.',
 136+ 'centralnotice-banner-landingcheck' => 'Use the LandingCheck extension',
 137+ 'centralnotice-banner-landingcheck-help' => 'Create an anchor tag in the banner body with $1 and enter one or more landing pages below, for example, $2. The link will be constructed automatically.',
137138 'centralnotice-banner-landing-pages' => 'Landing pages (comma-separated):',
138139 'centralnotice-geo' => 'Geotargeted',
139140 'centralnotice-countries' => 'Countries',
——@@ -167,6 +168,7 @@
168169 'centralnotice-anon' => 'Display to anonymous users',
169170 'centralnotice-account' => 'Display to logged in users',
170171 'centralnotice-fundraising' => 'Fundraising',
 172+ 'centralnotice-landingcheck' => 'LandingCheck',
171173 'centralnotice-landingpages' => 'Landing pages',
172174 'centralnotice-banner-content' => 'Banner content',
173175 'centralnotice-banner-content-changed' => 'Changed',
——@@ -258,6 +260,8 @@
259261 'centralnotice-expand-button' => 'This is an action, a verb in imperative form. It refers to "Do expand the centralnotice!"
260262
261263 See also {{msg|Centralnotice-hide-button}}.',
 264+ 'centralnotice-banner-landingcheck' => 'LandingCheck is the name of a MediaWiki extension.',
 265+ 'centralnotice-banner-landingcheck-help' => 'An explanation for how to use the LandingCheck feature. $1 is a bit of HTML, $2 is a title for a page.',
262266 'centralnotice-geo' => 'Used to label a checkbox which activates geotargeting',
263267 'centralnotice-allocation' => 'Tab for sub-page [[m:BannerAllocation|banner allocation]] to central notice special page.',
264268 'centralnotice-view-allocation' => 'Heading of dialog box on [[m:Special:BannerAllocation|banner allocation]] special page.',
——@@ -289,6 +293,7 @@
290294 'centralnotice-banner-settings' => "Label for a radio button
291295
292296 {{msg-mw|Centralnotice-banner-settings}} and {{msg-mw|Centralnotice-banner-content}} are visible at [{{fullurl:meta:Special:CentralNoticeLogs|log=bannersettings}} Special:CentralNoticeLogs] – ''settings'' chooses between logs for campaigns and log for banners, ''content'' is one of the things which could be changed on a banner (see the details of a log entry). So, I would go with “banners in general” for ''settings'' and “a single banner” in ''content''.",
 297+ 'centralnotice-landingcheck' => 'This is a the name of a MediaWiki extension.',
293298 'centralnotice-banner-content' => "{{msg-mw|Centralnotice-banner-settings}} and {{msg-mw|Centralnotice-banner-content}} appear to be visible at [{{fullurl:meta:Special:CentralNoticeLogs|log=bannersettings}} Special:CentralNoticeLogs] – ''settings'' chooses between logs for campaigns and log for banners, ''content'' is one of the things which could be changed on a banner (see the details of a log entry). So, I would go with “banners in general” for ''settings'' and “a single banner” in ''content''.",
294299 'centralnotice-filters' => 'Label for a set of options that control filtering of logs',
295300 'centralnotice-date' => 'Label for a date selection interface',

Follow-up revisions

RevisionCommit summaryAuthorDate
r99168follow-up to r99160, correct load order for new db patch - this needs to load...kaldari23:22, 6 October 2011
r100085follow-up to r99160, indexing banner nameskaldari21:00, 17 October 2011
r100100MFT r92510, r92676, r96496, r97304, r99160, r99165, r99169, r99176, r99178, r...awjrichards23:56, 17 October 2011
r100102picking up i18n changes; MFT r99033, r99145, r99160, r99166, r99176, r99181, ...awjrichards00:03, 18 October 2011

Comments

#Comment by Awjrichards (talk | contribs)   20:58, 17 October 2011

+ $row = $dbr->selectRow( 'cn_templates', 'tmp_landingcheck', array( 'tmp_name' => $eBannerName ) ); 'tmp_name' does not appear to be an indexed column - this will likely have negative performance implications. Please add an index to that field or query on an indexed field (eg tmp_id).

Status & tagging log